Try to recover from unmatched delimiters in the parser #54029

estebank · 2018-09-07T10:49:11Z

Follow up to #53949.

oli-obk · 2018-09-07T11:05:17Z

src/libsyntax/parse/parser.rs

+        mut err: DiagnosticBuilder<'a>,
+    ) -> PResult<'a, ()> {
+        let mut pos = None;
+        let mut tokens: Vec<token::Token> = tokens.iter().map(|t| (*t).clone()).collect();


isn't that just tokens.to_vec()?

oli-obk · 2018-09-07T11:12:20Z

src/libsyntax/parse/mod.rs

+    sess: &ParseSess,
+    override_span: Option<Span>,
+) -> TokenStream {
+    source_file_to_stream(sess, sess.source_map().new_source_file(name, source), override_span).0


does this mean that for rustc invocations that don't read the source from a file but from stdin we don't get these suggestions?

Correct. On the one hand, those will not have any worse output. On the other, this was a bit explorative and want to nail it down before trying to get to to work in all cases.

estebank · 2018-09-11T13:52:39Z

src/test/ui/resolve/token-error-correct-3.rs

+            //~| ERROR expected expression
+            fs::create_dir_all(path.as_ref()).map(|()| true)
+        } else {
+            //~^ ERROR incorrect close delimiter: `}`
            Ok(false);
        }


What would be the ideal output for this file?

I'll get back to this over the weekend to see if I can make the parser recover from missing delimiters and parse the rest correctly, so that we get the type error about fs::create_dir_all back.

src/test/ui/token/issue-10636-2.stderr

estebank · 2018-09-11T14:47:51Z

@nikomatsakis ready for a proper review.

nikomatsakis · 2018-09-12T17:13:59Z

src/libsyntax/parse/parser.rs

+                //      -   -  ^ expected one of `)`, <...>
+                //      |   |
+                //      |   help: ...missing `)` might belong here
+                //      you might have meant to close this...


is this really the right spot? I would have suggested us to suggest adding the ) after the {}, like so {foo(bar {})}, not {foo(bar) {}}, which doesn't look to me like it would parse

Now I'm intrigued on wether the comment is correct, at all...

I'd like to create a bunch of real "broken" code samples to test against, which would give me more confidence that what I'm doing in this PR is beneficial.

That would help me too, since I'm starting my reviews by just looking at the test cases (I haven't really looked at the code at all yet)

nikomatsakis · 2018-09-12T17:24:52Z

src/test/ui/parser-recovery-3.stderr

+LL |     }
+   |     ^ incorrect close delimiter
+
+error: expected one of `!`, `(`, `)`, `+`, `,`, `::`, or `<`, found `{`


it's kind of surprising to me that this advice comes here, in a second error after the "main" error above... why is that?

The lexer first performs the basic matching braces checks and emits the appropriate error. Then the parser takes the tokenstream and tries to parse things. So the first error is about the lexer finding unbalanced delimiters, while the second one is a parse error not finding an expected close delimiter.

There're two options: keeping the un-emitted DiagnosticBuilders from the lexer and emitting them once we are parsing and find an appropriate place for the close delimiter suggestion, or completely stop performing balanced delimiter checks in the lexer and rely entirely on the parser for parse errors. Both felt like too much work for little win. I'd prefer to iterate this PR until we have the recovery nailed down before revisiting the above possibilities.

nikomatsakis · 2018-09-12T17:25:02Z

src/test/ui/parser-recovery-3.stderr

+LL |     }
+   |     ^ expected expression
+
+error[E0401]: can't use type parameters from outer function


still, nice that we recovered here

nikomatsakis · 2018-09-12T17:25:17Z

src/test/ui/parser-recovery-3.stderr

+   |                 |                       help: ...the missing `)` may belong here
+   |                 if you meant to close this...
+
+error: expected expression, found `)`


"found )" is confusing too...?

Mmm... This is surprising. I'm not sure why the parser is finding a ) at all here...

nikomatsakis · 2018-09-12T17:26:25Z

src/test/ui/parser-recovery-4.stderr

+LL | }
+   | ^ incorrect close delimiter
+
+error: expected one of `!`, `)`, `,`, `.`, `::`, `?`, `{`, or an operator, found `bar`


something feels..inelegant to me here. Like, I would like to be suppressing these "fallout" errors, and just picking up parsing from some other point..? Hmm, I'm not sure.

That is an option, to just bail on parsing the entire block from that point on, as we do in other parts of the parser.

The problem with out right bailing until the next closing brace is that we then don't get the nice recovery shown above. Maybe that's a reasonable trade-off...

which is the nice recovery you are referring to?

I tend to think that giving "false errors" is kind of the most confusing thing we can do and we should bias in favor of too few errors and not too many, but opinions vary.

estebank · 2018-09-12T17:30:01Z

src/test/ui/parser-recovery-4.stderr

+LL |     foo(x
+   |          - expected one of `.`, `;`, `?`, `}`, or an operator here
+LL |     bar()
+   |     ^^^ unexpected token


In the ideal end state, this error would not get emitted, instead having the prior one be

error: expected one of `!`, `)`, `,`, `.`, `::`, `?`, `{`, or an operator, found `bar` --> $DIR/parser-recovery-4.rs:17:5 | LL | foo(x | - -- help: you might also be missing a semicolon here: `;` | | | | | expected one of 8 possible tokens here | | help: ...the missing `)` may belong here | if you meant to close this... LL | bar() | ^^^ unexpected token

but I'm not sure how to verify that we don't start suggesting bogus code that wouldn't work...

I'm not sure I understand. But I do think that this example you just gave is kind of cluttered -- I feel like it'd be better to break it apart into multiple sections or something.

nikomatsakis · 2018-09-24T21:27:39Z

@estebank are you planning to make changes here?

estebank · 2018-09-24T21:32:45Z

@nikomatsakis yes. I have a couple of alternative approaches that I'm going to try out once I have spare time. Rough timeline, next 2 weeks at the latest.

nikomatsakis · 2018-09-25T15:38:52Z

OK.

bors · 2018-10-11T09:20:09Z

☔ The latest upstream changes (presumably #54969) made this pull request unmergeable. Please resolve the merge conflicts.

TimNN · 2018-10-16T17:10:54Z

Ping from triage @estebank: What is the status of this PR?

estebank · 2018-10-16T17:31:21Z

@TimNN I'll close to clear the backlog and reopen once I get back to it.

Deduplicate mismatched delimiter errors Delay unmatched delimiter errors until after the parser has run to deduplicate them when parsing and attempt recovering intelligently. Second attempt at #54029, follow up to #53949. Fix #31528.

rust-highfive assigned nikomatsakis Sep 7, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 7, 2018

oli-obk reviewed Sep 7, 2018

View reviewed changes

This comment has been minimized.

Sign in to view

Try to recover from unmatched delimiters in the parser

0036e72

estebank force-pushed the unclosed-delim-the-next-generation branch from 7a222ae to 0036e72 Compare September 11, 2018 13:49

estebank commented Sep 11, 2018

View reviewed changes

src/test/ui/token/issue-10636-2.stderr Outdated Show resolved Hide resolved

estebank added 2 commits September 11, 2018 06:57

address review comment

b5b9299

Prioritize Primary underline for overlapping spans of same length

46ffe9d

This comment has been minimized.

Sign in to view

estebank added 2 commits September 11, 2018 09:08

fix unittest

3942ad4

Continue parsing after recovering from missing close delim

8605d87

estebank force-pushed the unclosed-delim-the-next-generation branch from 2962e35 to 8605d87 Compare September 11, 2018 20:19

nikomatsakis reviewed Sep 12, 2018

View reviewed changes

estebank commented Sep 12, 2018

View reviewed changes

formatting

413912a

estebank mentioned this pull request Sep 14, 2018

idea: consult git diff for better diagnostics / syntax errors #48911

Closed

nikomatsakis added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 25, 2018

estebank closed this Oct 16, 2018

estebank mentioned this pull request Jan 28, 2019

Deduplicate mismatched delimiter errors #57944

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Try to recover from unmatched delimiters in the parser #54029

Try to recover from unmatched delimiters in the parser #54029

estebank commented Sep 7, 2018 •

edited

Loading

oli-obk Sep 7, 2018

oli-obk Sep 7, 2018

estebank Sep 7, 2018

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

estebank Sep 11, 2018

estebank commented Sep 11, 2018

This comment has been minimized.

nikomatsakis Sep 12, 2018

estebank Sep 12, 2018

nikomatsakis Sep 12, 2018

nikomatsakis Sep 12, 2018

estebank Sep 12, 2018

nikomatsakis Sep 12, 2018

nikomatsakis Sep 12, 2018

estebank Sep 12, 2018 •

edited

Loading

nikomatsakis Sep 12, 2018

estebank Sep 12, 2018

estebank Sep 14, 2018

nikomatsakis Sep 18, 2018

estebank Sep 12, 2018 •

edited

Loading

nikomatsakis Sep 18, 2018

nikomatsakis commented Sep 24, 2018

estebank commented Sep 24, 2018

nikomatsakis commented Sep 25, 2018

bors commented Oct 11, 2018

TimNN commented Oct 16, 2018

estebank commented Oct 16, 2018

Try to recover from unmatched delimiters in the parser #54029

Try to recover from unmatched delimiters in the parser #54029

Conversation

estebank commented Sep 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

estebank commented Sep 11, 2018

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

estebank Sep 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

estebank Sep 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikomatsakis commented Sep 24, 2018

estebank commented Sep 24, 2018

nikomatsakis commented Sep 25, 2018

bors commented Oct 11, 2018

TimNN commented Oct 16, 2018

estebank commented Oct 16, 2018

estebank commented Sep 7, 2018 •

edited

Loading

estebank Sep 12, 2018 •

edited

Loading

estebank Sep 12, 2018 •

edited

Loading