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

Make the parser’s ‘expected <foo>, found <bar>’ errors more accurate #19494

Merged
merged 1 commit into from
Dec 5, 2014

Conversation

ftxqxd
Copy link
Contributor

@ftxqxd ftxqxd commented Dec 3, 2014

As an example of what this changes, the following code:

let x: [int ..4];

Currently spits out ‘expected ], found ..’. However, a comma would also be valid there, as would a number of other tokens. This change adjusts the parser to produce more accurate errors, so that that example now produces ‘expected one of (, +, ,, ::, or ], found ..’.

(Thanks to cramer on IRC for pointing out this problem with diagnostics.)

@brson
Copy link
Contributor

brson commented Dec 3, 2014

This looks pretty clever and useful.

Is it incorrect to ever compare equality of Tokens without check or check_operator? If so, it may be prudent to not implement PartialEq/Eq to avoid future errors.

@ftxqxd
Copy link
Contributor Author

ftxqxd commented Dec 3, 2014

No, it’s not incorrect to compare Tokens using ==, and I do keep a number of uses of == in the code here, because we don’t want an error message to say that it expected something which isn’t supported any more (e.g., let x: fn int will say something like ‘expected (, found int’ instead of ‘expected one of ( or <, found int’, because the fn<'a>(…) syntax is deprecated).

That said, I think you’re right in that it’s very easy for someone modifying the parser in the future to write self.token == foo instead of self.check(&foo). Adding a new method which does not add the token to self.expected_tokens would let us remove PartialEq/Eq, but I’m wary about disallowing == for Tokens everywhere, not just in the parser (which is all we really want to disallow). Perhaps Parser.token could use a wrapper around Token that does not implement PartialEq instead of directly using Token?

@brson
Copy link
Contributor

brson commented Dec 3, 2014

@P1start I'm not sure whether it's necessary. The potential bug is relatively minor: that the error fails to suggest a possible token. I'll leave it to your judgement to decide whether such defensive programming is warranted here.

As an example of what this changes, the following code:

    let x: [int ..4];

Currently spits out ‘expected `]`, found `..`’. However, a comma would also be
valid there, as would a number of other tokens. This change adjusts the parser
to produce more accurate errors, so that that example now produces ‘expected one
of `(`, `+`, `,`, `::`, or `]`, found `..`’.
@ftxqxd
Copy link
Contributor Author

ftxqxd commented Dec 4, 2014

I decided not to prevent == on self.token. It’s just not worth the bother, and significantly decreases the ergonomics writing code in the parser.

I did end up removing check_operator, and the code to my eyes looks clearer and makes more sense. I also fixed up a few other special-cased ‘expected…’ messages to print out the actual symbol for the token instead of the internal representation of it.

r? @brson

@alexcrichton alexcrichton merged commit 108bca5 into rust-lang:master Dec 5, 2014
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.

3 participants