Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Custom Error Default trait with extra information from the Lexer as parameters #462

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sokorototo
Copy link

Closes #444, Adjacent to #445.

@sokorototo
Copy link
Author

sokorototo commented Jan 6, 2025

Adds a DefaultLexerError trait, that replaces the Default trait bound on the #[error] attribute. This is very similar to #445, but doesn't alter the macro source code. The DefaultLexerError trait is not strict and can be used for several Lexer<'source, T> types (as Lexer is not passed as an parameter).

Usage is very similar to implementing Default, with extra parameters (minus the macro Derive).

Example:

impl<'source, Extras> DefaultLexerError<'source, str, Extras> for LexerError {
    fn from_lexer<'e>(source: &'source str, span: logos::Span, _: &'e Extras) -> Self {
        LexerError::Other {
            source: source.to_owned(),
            span,
        }
    }
}

@jeertmans
Copy link
Collaborator

Hi @sokorototo, thanks for your PR! I think this might conflict with #445, or at least make it even harder for the compiler to optimize the default case (which is observed to decrease performance). I am still waiting for a better benchmarking tool before merging #445

@sokorototo
Copy link
Author

No, biggie. @jeertmans I just sent this out here as an alternative take on the issue. Also, isn't the compiler simple inlining the Default implementation? I think adding parameters must induce a perfomance loss anyways?

@mysteriouslyseeing
Copy link

mysteriouslyseeing commented Jan 7, 2025

No, biggie. @jeertmans I just sent this out here as an alternative take on the issue. Also, isn't the compiler simple inlining the Default implementation? I think adding parameters must induce a perfomance loss anyways?

The working theory is that the lex.set method borrows lex mutably, and even if the compiler inlines the error_from_lexer implementation, it can't break the reference aliasing rules (because other unsafe code is allowed to depend on it). Thus with the old implementation, the Default can construct it "in place" in the lexer (because the lex reference is only used once), whereas the new implementation has to make the new error before moving it into the lexer.

The way we solve this is by having the method used to create the error actually call lex.set inside it so in the best case (where no information in the lexer is actually used) the compiler can still inline it properly.

If you can figure out a way to move the methods around to remove the reference aliasing issue we can directly compare both implementations.

For reference, I wrote #445

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add option to change error constructor
3 participants