Add callbacks and priority for #[logos(skip)] #441

mysteriouslyseeing · 2024-11-29T21:35:56Z

Closes #440.

Syntax is same as #[regex] and #[token]:
#[logos(skip(literal[, callback[ = callback]][, priority = priority]))]

Also added an example with usage:

fn main() {
    let mut lexer = Token::lexer("abc 123\nab( |23\nAbc 123");
    let mut out_tokens = Vec::new();

    while let Some(token_result) = lexer.next() {
        if let Ok(token) = token_result {
            out_tokens.push(token);
        } else {
            // Oh no! There was an error!
            eprintln!("Unrecognised character on line {}", lexer.extras.line_num + 1);
        }
    }
}

use logos::{Lexer, Logos, Skip};

#[derive(Debug, Clone, Copy, Default)]
struct Extras {
    line_num: usize,
}

#[derive(Logos, Debug, PartialEq)]
#[logos(skip r"[ \r]")]
#[logos(skip(r"\n", callback = newline_callback, priority = 3))]
#[logos(extras = Extras)]
enum Token {
    #[regex("[a-z]+")]
    Letters,
    #[regex("[0-9]+")]
    Numbers,
}

fn newline_callback(lexer: &mut Lexer<Token>) -> Skip {
    lexer.extras.line_num += 1;
    Skip
}

A lot of the logic is copied and pasted from Definition - maybe it would be better to refactor them both to use common code? Let me know.

Syntax is same as #[regex] and #[token]: `#[logos(skip(literal, callback = callback, priority = priority))]`

jeertmans · 2024-11-30T10:15:31Z

Hi @mysteriouslyseeing, it looks great, thanks for your contribution!

Before I review this in depth, could you please:

add unit tests to check that all syntax variations are supported;
add documentation to the book so that people will know this feature is supported (you can also link to the example file you wrote)?

Thanks!

Provides type inference for closure argument.

jeertmans

Thanks for adding tests and book docs!

Before I review other parts of the code, please check my first comments about the failing test :-)

jeertmans · 2024-12-01T10:34:45Z

tests/tests/callbacks.rs

+    fn skip_comment<'src>(lexer: &mut Lexer<'src, Token<'src>>) {
+        let end = lexer
+            .remainder()
+            .find("-->")
+            .unwrap_or(lexer.remainder().len());
+        lexer.bump(end + 3);
+    }
+
+    #[test]
+    fn skip_callback_function() {
+        let mut lexer = Token::lexer("<foo> <!-- comment --> <bar>");
+        assert_eq!(lexer.next(), Some(Ok(Token::Tag("foo"))));
+        assert_eq!(lexer.next(), Some(Ok(Token::Tag("bar"))));
+        assert_eq!(lexer.next(), None);
+
+        let mut lexer = Token::lexer("<foo> <!-- unterminated comment");
+        assert_eq!(lexer.next(), Some(Ok(Token::Tag("foo"))));
+        // Errors not allowed from skips
+        assert_eq!(lexer.next(), None);
+    }


This test is failing, I think this is because you cannot bump end + 3 if the comment is unterminated. The callback logic should only pay attention to this edge case (which you hopefully tested here :-)

Whoops, I forgot to run cargo test in ./test. My bad, I'll fix that now.

jeertmans · 2024-12-01T10:35:49Z

tests/tests/callbacks.rs

+        // Errors not allowed from skips
+        assert_eq!(lexer.next(), None);


Does it mean the error type is never returned? I am not sure to understand this comment

That was to do with the other branch, where skips could return errors. I've removed that line

jeertmans · 2024-12-01T10:37:39Z

tests/tests/callbacks.rs

+        let end = lexer
+            .remainder()
+            .find("-->")
+            .unwrap_or(lexer.remainder().len());


In line with other comment, you can probably fix the text by changing this to .map(|id| id + 3).unwrap_or(lexer.remainer().len()); and bump(end).

mysteriouslyseeing · 2024-12-01T11:29:54Z

When I run cargo test in the ./test subdirectory, some doctests fail. They don't appear to have anything to do with my changes. Is that something to do with me?

mysteriouslyseeing · 2024-12-02T06:59:01Z

To clarify, Clippy complains that

pub enum Callback {
    Label(TokenStream),
    Inline(Box<InlineCallback>),
    SkipWithCallback(SkipCallback),
    Skip(Span),
}

the SkipWithCallback variant ends with Callback, which is the type's name.

Now the enum is defined like so:

pub enum Callback {
    Label(TokenStream),
    Inline(Box<InlineCallback>),
    Skip(SkipCallback), // note these are swapped
    SkipEmpty(Span), // note these are swapped
}

which could be confusing to people previously familiar with the codebase. I've committed it like this, but if you'd rather revert to 294a7ab (before any of the switching around of the variants) that's totally fine by me.

jeertmans · 2024-12-02T09:22:08Z

be confusing to people previously familiar with the codebase. I've committed it like this, b

I think this is better to avoid breaking changes when possible (and changing the name of an enum variant is a breaking change), so I prefer reverting to previous name (and IMO it conveys the functionality better), and disable Clippy warning for this variant.

Callbacks and priority for #[logos(skip)] added

bdb5dc7

Syntax is same as #[regex] and #[token]: `#[logos(skip(literal, callback = callback, priority = priority))]`

mysteriouslyseeing changed the title ~~Callbacks and priority for #[logos(skip)] added~~ Add callbacks and priority for #[logos(skip)] Nov 29, 2024

mysteriouslyseeing mentioned this pull request Nov 29, 2024

Add callback to skip #440

Open

jeertmans added enhancement New feature or request book Related to the book labels Nov 30, 2024

mysteriouslyseeing added 9 commits December 1, 2024 18:06

added book documentation

e3982fe

Made unused return warning for skip callback clearer

d4c3ebd

Skip callbacks should now always return unit.

1297f33

Provides type inference for closure argument.

Added unit tests for skip callbacks

829bed3

applied rustfmt for callbacks.rs

3e81628

Reverted type inference fix to allow returning Skip

ee79e7a

Replaced skip_callback.rs with usage in extras.rs

9d13bd1

Fixed type inference for inline closures

932b02e

Updated book's skip callback return type

a6fc08a

jeertmans requested changes Dec 1, 2024

View reviewed changes

Fixed unit test skip_callback_function

be999e3

mysteriouslyseeing added 3 commits December 1, 2024 23:13

Ran rustfmt

294a7ab

Renamed new variant to make clippy happy

8dfd772

Switched around variant naming to make clippy happy

8624fd1

mysteriouslyseeing added 3 commits December 2, 2024 18:05

renamed enum variants without mistaken changes to Error

cabf32d

Merge branch 'skip_callback_without_error_changes' into skip_callback

9a605ae

undid mistaken error changes

a559867

mysteriouslyseeing added 3 commits December 2, 2024 23:11

Reverted to "Ran rustfmt", silenced lint

ec5e687

Silenced lint

6c64103

Ran rustfmt

66ff393

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add callbacks and priority for #[logos(skip)] #441

Add callbacks and priority for #[logos(skip)] #441

mysteriouslyseeing commented Nov 29, 2024 •

edited

Loading

jeertmans commented Nov 30, 2024

jeertmans left a comment

jeertmans Dec 1, 2024

mysteriouslyseeing Dec 1, 2024

jeertmans Dec 1, 2024

mysteriouslyseeing Dec 1, 2024

jeertmans Dec 1, 2024

mysteriouslyseeing Dec 1, 2024

mysteriouslyseeing commented Dec 1, 2024

mysteriouslyseeing commented Dec 2, 2024 •

edited

Loading

jeertmans commented Dec 2, 2024

		// Errors not allowed from skips
		assert_eq!(lexer.next(), None);

Add callbacks and priority for #[logos(skip)] #441

Are you sure you want to change the base?

Add callbacks and priority for #[logos(skip)] #441

Conversation

mysteriouslyseeing commented Nov 29, 2024 • edited Loading

jeertmans commented Nov 30, 2024

jeertmans left a comment

Choose a reason for hiding this comment

jeertmans Dec 1, 2024

Choose a reason for hiding this comment

mysteriouslyseeing Dec 1, 2024

Choose a reason for hiding this comment

jeertmans Dec 1, 2024

Choose a reason for hiding this comment

mysteriouslyseeing Dec 1, 2024

Choose a reason for hiding this comment

jeertmans Dec 1, 2024

Choose a reason for hiding this comment

mysteriouslyseeing Dec 1, 2024

Choose a reason for hiding this comment

mysteriouslyseeing commented Dec 1, 2024

mysteriouslyseeing commented Dec 2, 2024 • edited Loading

jeertmans commented Dec 2, 2024

mysteriouslyseeing commented Nov 29, 2024 •

edited

Loading

mysteriouslyseeing commented Dec 2, 2024 •

edited

Loading