Learn to parse `a as usize < b` by estebank · Pull Request #42578 · rust-lang/rust

estebank · 2017-06-10T03:33:56Z

Parsing a as usize > b always works, but a as usize < b was a
parsing error because the parser would think the < started a generic
type argument for usize. The parser now attempts to parse as before,
and if a DiagnosticError is returned, try to parse again as a type with
no generic arguments. If this fails, return the original
DiagnosticError.

Fix#22644.

rust-highfive · 2017-06-10T03:34:07Z

r? @nikomatsakis

(rust_highfive has picked a reviewer for you, use r? to override)

oli-obk · 2017-06-10T09:10:28Z

Is it possible to use mk _ty here? Or is the DUMMY_NODE_ID intentional?

mk_ty is part of TyCtxt which isn't available yet. This was extracted from parse_ty_common in order to see if this would work at all. I could modify parse_ty_common for this case, but I felt it would muddy the logic of that method.

petrochenkov · 2017-06-10T09:43:52Z

I'd be okay with this if the unbounded lookahead and parser state saving/restoring machinery were used only for reporting better diagnostics, recovery and fix-it suggestions, not for actually accepting the code.

Mark-Simulacrum · 2017-06-11T16:03:05Z

Looks like there are some test failures in Travis: https://travis-ci.org/rust-lang/rust/jobs/241424157#L1853, though addressing @petrochenkov's concern first seems like a good idea.

estebank · 2017-06-11T17:22:02Z

I'd be okay with this if the unbounded lookahead and parser state saving/restoring machinery were used only for reporting better diagnostics, recovery and fix-it suggestions, not for actually accepting the code.

Should a as usize < b be accepted? I understand the concern, and would heavily lean towards your opinion on all other situations, but in this case in particular it is adding a very specific edge case to the grammar which IMO is a big ergonomic win.

Having said that, what would be your preferred output if this were used only for diagnostics? Something like this?

error: expected one of `!`, `(`, `+`, `,`, `::`, `<`, or `>`, found `;`
--> <anon>:6:19
|
6 | a as usize < b;
| ^ expected one of 7 possible tokens here
help: may be you want to compare the casted value?
| (a as usize) < b;

Parsing `a as usize > b` always works, but `a as usize < b` was a parsing error because the parser would think the `<` started a generic type argument for `usize`. The parser now attempts to parse as before, and if a DiagnosticError is returned, try to parse again as a type with no generic arguments. If this fails, return the original `DiagnosticError`.

petrochenkov · 2017-06-11T20:44:14Z

@estebank

Having said that, what would be your preferred output if this were used only for diagnostics?

A specially crafted error would be the best, IMO.

error: `<` is interpreted as a start of generic arguments for `usize`, not comparison
--> <anon>:6:19
|
6 | a as usize < b;
| ^ <some label, not sure what to write here>
help: if you want to compare the casted value then write
| (a as usize) < b;

After this is reported as a non-fatal error, AST is build as if a as usize < b were legal and meant (a as usize) < b, e.g. most likely error recovery is performed.

petrochenkov · 2017-06-11T21:04:51Z

@estebank

it is adding a very specific edge case to the grammar which IMO is a big ergonomic win.

It's an edge case, but it has large effect on properties of the grammar as a whole.
If syntactic disambiguation between types and expressions is going to be done, it should probably be done in more principled fashion, discussed properly and maybe modeled in something like https://github.com/nikomatsakis/rustypop
BTW, the Const generics RFC is related, first it break the assumption this PR does that if b is not a type, then it's the rhs argument for comparison, second it creates more demand for general disambiguation between types and expressions.

petrochenkov · 2017-06-11T21:15:59Z

(a as usize) < b

a as (usize) < b works too and is more "local", but arguably more surprising, maybe it can be recommended instead? (I'm not sure)

``` warning: `<` is interpreted as a start of generic arguments for `usize`, not comparison --> $DIR/issue-22644.rs:16:33 | 16 | println!("{}", a as usize < b); | ^ expected one of `!`, `(`, `+`, `,`, `::`, or `>` here | help: if you want to compare the casted value then write | println!("{}", (a as usize) < b); ```

estebank · 2017-06-12T07:08:16Z

It's an edge case, but it has large effect on properties of the grammar as a whole.
If syntactic disambiguation between types and expressions is going to be done, it should probably be done in more principled fashion, discussed properly and maybe modeled in something like https://github.com/nikomatsakis/rustypop

I understand and completely agree.

a as (usize) < b works too and is more "local", but arguably more surprising, maybe it can be recommended instead? (I'm not sure)

I think I prefer the former, as it'd be less surprising for a newcomer.

I got something working which accepts the code as proposed but also emits a warning suggesting a correction.

~~I should probably change the code that emits the warning before we merge, as right now I am modifying the existing error in place, which is probably not what we want.~~

This is how it looks like right now:

warning: `<` is interpreted as a start of generic arguments for `usize`, not comparison
--> $DIR/issue-22644.rs:16:33
|
16 | println!("{}", a as usize < b);
| - ^ interpreted as generic argument
| |
| not interpreted as comparison
|
help: if you want to compare the casted value then write:
| println!("{}", (a as usize) < b);

bjorn3 · 2017-06-12T12:09:25Z

Could you make this a struct with field names?

``` warning: `<` is interpreted as a start of generic arguments for `usize`, not a comparison --> $DIR/issue-22644.rs:16:33 | 16 | println!("{}", a as usize < b); | - ^ interpreted as generic argument | | | not interpreted as comparison | help: if you want to compare the casted value then write: | println!("{}", (a as usize) < b); ```

nikomatsakis · 2017-06-12T21:39:36Z

As long as we're not changing the grammar, I'm happy if @petrochenkov is happy =)

nikomatsakis · 2017-06-12T21:39:46Z

r? @petrochenkov

estebank · 2017-06-12T22:52:10Z

@nikomatsakis just to be clear, the code as is accepts it under the assumption that it is correct and only emits a warn diagnostic, so it does technically change the grammar. If this is the only error, it will generate a valid binary. Is it ok to accept this code relying on people to be annoyed enough by warnings to fix their code? I can change the warn to an error and call it a day :)

bjorn3 · 2017-06-13T06:25:18Z


 // Parse types, optionally.
- let parameters = if self.eat_lt() {
+ let parameters = if self.is_lt() && !dont_parse_generics {


if !dont_parse_generics && self.eat_lt() {
&& doesnt run second arg when first arg is false.

👍
This also makes fn is_lt unnecessary
EDIT: ... and keeps < in the expected set.

nikomatsakis · 2017-06-13T16:19:50Z

@estebank

the code as is accepts it under the assumption that it is correct and only emits a warn diagnostic

Accepts what exactly? And what do you mean by "as is"? You mean, in the PR, or in master?

estebank · 2017-06-13T20:43:09Z

@nikomatsakis The PR accepts a as b < c as valid code if it can parse b as a type without type arguments, showing a warning, not an error to change the code.

petrochenkov · 2017-06-13T21:04:22Z

 /// - `a::b<T,U>::c(V) -> W`
 /// - `a::b<T,U>::c(V)`
- pub fn parse_path_segments_without_colons(&mut self) -> PResult<'a, Vec<PathSegment>> {
+ pub fn parse_path_segments_without_colons(&mut self, dont_parse_generics: bool)


Nit: dont_parse_generics: bool -> parse_generics: bool

petrochenkov · 2017-06-13T21:07:00Z

+ ExprKind::Cast(lhs, rhs), ThinVec::new());
+ }
+ Err(mut err) => {
+ // Rewind to before attempting to parse the type with generics, to get


Could you move the error handling logic into a separate function?

nikomatsakis · 2017-06-14T21:52:50Z

The PR accepts a as b < c as valid code if it can parse b as a type without type arguments, showing a warning, not an error to change the code.

Huh, I'm confused. After all, you can do let x = foo as Box<Bar>. How do we know that b < c should not be parsed as a type?

petrochenkov · 2017-06-15T09:04:32Z

r=me with stray note removed.

- generate error instead of warning - remove `RewindPoint` and just keep a copy of `Parser` to rewind state. - `dont_parse_generics: bool` -> `parse_generics: bool` - remove `eat_lt` - move error handling code to separate method

estebank · 2017-06-15T15:46:22Z

@petrochenkov done

@bors r=petrochenkov

bors · 2017-06-15T15:46:23Z

📌 Commit ad260ff has been approved by petrochenkov

nikomatsakis · 2017-06-15T18:19:22Z

@bors r-

nikomatsakis · 2017-06-15T18:19:39Z

I'd like to know if this is changing the grammar that is accepted. I feel like I still don't understand quite what's happening here. =)

nikomatsakis · 2017-06-15T18:20:01Z

In particular, do we now compile (successfully) more programs than we did before? If so, what set of programs?

estebank · 2017-06-15T18:27:14Z

@nikomatsakis The first commit of this PR changed the accepted grammar and only emitted a warning. I changed it back to not have any changes in the grammar and emit an error. The parser state changes slightly after finding this case in order to continue parsing successfully the rest of the file (before this PR the rest of the file would not get any diagnostics). No new set of programs will be accepted, the new behaviors are:

a more specific error for the case a as b < c
continued parsing of the rest of the file after encountering a as b < c

nikomatsakis · 2017-06-15T18:38:27Z

@estebank perfect =)

nikomatsakis · 2017-06-15T18:38:31Z

@bors r+

bors · 2017-06-15T18:38:31Z

📌 Commit ad260ff has been approved by nikomatsakis

bors · 2017-06-16T03:31:15Z

⌛ Testing commit ad260ff with merge 44eeb21...

Learn to parse `a as usize < b` Parsing `a as usize > b` always works, but `a as usize < b` was a parsing error because the parser would think the `<` started a generic type argument for `usize`. The parser now attempts to parse as before, and if a DiagnosticError is returned, try to parse again as a type with no generic arguments. If this fails, return the original `DiagnosticError`. Fix#22644.

bors · 2017-06-16T05:43:33Z

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing 44eeb21 to master...

@estebank

Add a nicer error message for missing in for loop, fixesrust-lang#40782. As suggested by @estebank in issue rust-lang#40782, this works in the same way as rust-lang#42578: if the in keyword is missing, we continue parsing the expression and if this works correctly an adapted error message is produced. Otherwise we return the old error. A specific test case has also been added. This is my first PR on rust-lang/rust so any feedback is very welcome.

44100hertz · 2017-12-17T06:32:14Z

One last thing: << isn't covered here.

error: `<` is interpreted as a start of generic arguments for `u16`, not a comparison --> /tmp/angle.rs:4:29 | 4 | println!("{}", x asu16 << 4); | -------- ^^ - interpreted as generic arguments | | | | | not interpreted ascomparison | help: try comparing the casted value: `(x asu16)`

petrochenkov · 2017-12-17T10:36:58Z

@Hertz4
<< is fixed on beta/nightly

error: `<` is interpreted as a start of generic arguments for `u16`, not a shift
--> src/main.rs:2:29
|
2 | println!("{}", x as u16 << 4);
| -------- ^^ - interpreted as generic arguments
| | |
| | not interpreted as shift
| help: try shifting the casted value: `(x as u16)`

rust-highfive assigned nikomatsakis Jun 10, 2017

oli-obk reviewed Jun 10, 2017
View reviewed changes

Mark-Simulacrum added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jun 11, 2017

estebank force-pushed the recover-binop branch from 6426f7c to a6d3215Compare June 11, 2017 20:40

estebank force-pushed the recover-binop branch 2 times, most recently from e53c7a1 to 803d323Compare June 12, 2017 07:03

bjorn3 requested changes Jun 12, 2017
View reviewed changes

Comment threadsrc/libsyntax/parse/parser.rs Outdated
bjorn3Jun 12, 2017
Copy link
Copy Markdown
Member
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you make this a struct with field names?

Fix affected tests
5fcfa08

estebank force-pushed the recover-binop branch from 803d323 to 5fcfa08Compare June 12, 2017 14:52

rust-highfive assigned petrochenkov and unassigned nikomatsakis Jun 12, 2017

bjorn3 requested changes Jun 13, 2017
View reviewed changes

petrochenkov reviewed Jun 13, 2017
View reviewed changes

estebank force-pushed the recover-binop branch from 2aa49b7 to 31bf47dCompare June 15, 2017 05:15

Review comments
ad260ff
- generate error instead of warning - remove `RewindPoint` and just keep a copy of `Parser` to rewind state. - `dont_parse_generics: bool` -> `parse_generics: bool` - remove `eat_lt` - move error handling code to separate method

estebank force-pushed the recover-binop branch from 31bf47d to ad260ffCompare June 15, 2017 15:45

bors merged commit ad260ff into rust-lang:masterJun 16, 2017

est31 mentioned this pull request Jun 29, 2017
Rustc crashes when parsing an ambiguous cast and compare #42954
Closed

This was referenced Sep 21, 2017
Improve error message for incorrect 'for' input #40782
Closed
Confusing error message: "can't qualify macro invocation with pub" when a inherent method lacks "fn" #38911
Closed

estebank mentioned this pull request Oct 24, 2017
Detect = -> : typo in let bindings #45452
Merged

LaurentMazare mentioned this pull request Oct 30, 2017
Add a nicer error message for missing in for loop, fixes #40782. #45639
Merged

estebank mentioned this pull request Sep 13, 2018
Undisambiguated generic arguments in expressions rust-lang/rfcs#2527
Closed

matklad mentioned this pull request Feb 11, 2019
ra_syntax: errors on a type cast + comparison rust-lang/rust-analyzer#595
Closed

estebank deleted the recover-binop branch November 9, 2023 05:26

Uh oh!

Uh oh!

Conversation

estebank commented Jun 10, 2017

Uh oh!

rust-highfive commented Jun 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

petrochenkov commented Jun 10, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Mark-Simulacrum commented Jun 11, 2017

Uh oh!

estebank commented Jun 11, 2017

Uh oh!

petrochenkov commented Jun 11, 2017

Uh oh!

petrochenkov commented Jun 11, 2017

Uh oh!

petrochenkov commented Jun 11, 2017

Uh oh!

estebank commented Jun 12, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nikomatsakis commented Jun 12, 2017

Uh oh!

nikomatsakis commented Jun 12, 2017

Uh oh!

estebank commented Jun 12, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

petrochenkovJun 13, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nikomatsakis commented Jun 13, 2017

Uh oh!

estebank commented Jun 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nikomatsakis commented Jun 14, 2017

Uh oh!

petrochenkov commented Jun 15, 2017

Uh oh!

estebank commented Jun 15, 2017

Uh oh!

bors commented Jun 15, 2017

Uh oh!

nikomatsakis commented Jun 15, 2017

Uh oh!

nikomatsakis commented Jun 15, 2017

Uh oh!

nikomatsakis commented Jun 15, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

estebank commented Jun 15, 2017

Uh oh!

nikomatsakis commented Jun 15, 2017

Uh oh!

nikomatsakis commented Jun 15, 2017

Uh oh!

bors commented Jun 15, 2017

Uh oh!

bors commented Jun 16, 2017

Uh oh!

bors commented Jun 16, 2017

petrochenkov commented Jun 10, 2017•
edited
Loading

estebank commented Jun 12, 2017•
edited
Loading

estebank commented Jun 12, 2017•
edited
Loading

petrochenkovJun 13, 2017•
edited
Loading

nikomatsakis commented Jun 15, 2017•
edited
Loading