Skip to content

Conversation

@ThinkRedstone
Copy link

This PR adds a new type of parser error, SpannedParserError, which includes the span of the token which raised the error.
In terms of simply displaying the error via to_string or Display, nothing has changed, but if the error is handled by a more complicated system it can get the actual span of the error- to for example underline the relevant part of the statement in red or otherwise highlight it.

This new type of error has been implemented for most parser errors, specifically all errors raised via parser_err! plus a few extra.

pub enum ParserError {
TokenizerError(String),
ParserError(String),
SpannedParserError(String, Span),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking introducing a new error type would be confusing since it would duplicate the ParserError variant. Can we instead change the ParserError variant to e.g?

ParserError(ParseError {
    message: String,
    span: Span
})

We can also mention in the documentation of span that it returns [Span::empty] when the information is unknown

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure why a new error type would be confusing. We already have three, one of which can't actually be raised by the parser.

We can change ParserError, but this would require changing all existing errors which don't include a span to include an empty span. This would offer no benefit to a user to the library, and might in fact confuse them- since they would need to know and remember that for some errors, the span might be empty. That a field in your error can't actually be relied upon, despite not being marked as Option, I think is much more confusing.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah there are three distinct error variants today - they can't be confused for each other because they flag different things to the user. That isn't the case with ParserError vs SpannedParserError (e.g. the user can't tell what kinds of errors show up as the former vs the latter, and it wouldn't be nice that an error that was previously thrown under the former suddenly starts being thrown as the latter).

Returning an empty span is fine in this case, as the documentation mentions, the empty span flags that the info is unknown at that point in time. We're gradually working our way through span coverage, please see here some context

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.

2 participants