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

Implement std::error::Error for lrpar::lex::Lexeme<T> #126

Closed
pablosichert opened this issue Dec 15, 2019 · 4 comments
Closed

Implement std::error::Error for lrpar::lex::Lexeme<T> #126

pablosichert opened this issue Dec 15, 2019 · 4 comments

Comments

@pablosichert
Copy link
Contributor

It would be nice if std::error::Error was implemented for lrpar::lex::Lexeme<T>.

That way, if we're in the context of a -> Result<T, Box<dyn std::error::Error>> function, we can do

let value = $1?;

instead of

let value = $1.map_err(|error| format!("{:?}", error))?;

In context:

Integer -> Result<i32, Box<dyn Error>>:
    "INT" {
        let value = $1.map_err(|error| format!("{:?}", error))?;
        let string = $lexer.lexeme_str(&value);
        Ok(string.parse::<i32>()?)
    }
;

$1 is of type Result<Lexeme<u32>, Lexeme<u32>> in the generated code.

@ltratt
Copy link
Member

ltratt commented Dec 16, 2019

I'm not sure about this: I think it will confuse people, partly because Lexemes aren't very user friendly on their own. For example, my preferred idiom is:

Integer -> Result<i32, ()>:
  "INT" {
    let v = $1.map_err(|_| ())?;
    ...
  }
;

In other words, as soon as I encounter an inserted token for "something that matters", I stop evaluation (but don't bother reporting the reason why to the user, as I find that it's generally obvious from the syntax errors they will also have received).

That said, in your grammar file, could you not implement Error on Lexeme yourself, or is that forbidden because it's in another crate?

@pablosichert
Copy link
Contributor Author

That said, in your grammar file, could you not implement Error on Lexeme yourself, or is that forbidden because it's in another crate?

Correct, that does not work, unfortunately:

error[E0117]: only traits defined in the current crate can be implemented for arbitrary types
     |
     | impl<T> std::error::Error for lrpar::lex::Lexeme<T> {}
     | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^---------------------
     | |                             |
     | |                             `lrpar::lex::Lexeme` is not defined in the current crate
     | impl doesn't use only types from inside the current crate

That being said, I haven't looked at the error handling / recovery of grmtools yet, because currently I get plain unrecoverable panics when I parse something unexpected. (That is, in my wasm32 build.)

I'll try to find out the cause for that, because eventually I also want to report nice errors with locations in the parser input to the user.

I don't have time for that right now, but I'll come back to this when I finished the core part of my project where I use grmtools.

@ltratt
Copy link
Member

ltratt commented Dec 18, 2019

That being said, I haven't looked at the error handling / recovery of grmtools yet, because currently I get plain unrecoverable panics when I parse something unexpected.

That doesn't sound good! If that's a bug in grmtools, please report it back here!

@pablosichert
Copy link
Contributor Author

After reading the Rust guidelines for publishing crates I thought of this issue again.

Due to the orphaning rules for implementing traits, they recommend eagerly implementing std::error::Error: https://rust-lang.github.io/api-guidelines/interoperability.html#c-good-err.

bors bot added a commit that referenced this issue Mar 3, 2020
161: impl Error on Lexeme. r=ptersilie a=ltratt

This can make some idioms in .y files easier.

@pablosichert Does this do what you were asking for in #126 (comment)? I'd like to know for sure that it helps before we consider merging this!

Co-authored-by: Laurence Tratt <laurie@tratt.net>
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

No branches or pull requests

2 participants