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.

Sign up for GitHub

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

Jump to bottom

Improve messages for un-closed delimiter errors #53949

Merged
merged 4 commits into from
Sep 9, 2018
Merged

Improve messages for un-closed delimiter errors #53949

merged 4 commits into from
Sep 9, 2018

Conversation

estebank
Copy link
Contributor

@estebank estebank commented Sep 4, 2018

No description provided.

@rust-highfive
Copy link
Collaborator

r? @nikomatsakis

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

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

This comment has been minimized.

Sign in to view
@rust-highfive

This comment has been minimized.

Sign in to view
nikomatsakis
nikomatsakis approved these changes Sep 4, 2018
View reviewed changes
Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

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

I'm very excited about this. Looks like tests don't quite pass yet, though.

I was wondering whether weirdness could happen because of macros, but I guess not: macro expansions can't introduce unpaired braces, after all. The error would occur when parsing the original file.

r=me once tests pass

src/test/ui/parser/unclosed-braces.stderr Outdated
| - this might be the culprit...
...
LL | }
| - ...as it matches this but it has different indentation
Copy link
Contributor

Choose a reason for hiding this comment

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

omg yes!

src/libsyntax/parse/lexer/tokentrees.rs Outdated
.next() // these are in reverse order as they get inserted on close, but
{ // we want the last open/first close
if d == delim {
err.span_label(*open_sp, "this might be the culprit...");
Copy link
Contributor

Choose a reason for hiding this comment

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

I love this language, but perhaps something a touch more formal...

  • "this delimiter might not be properly closed" ?
  • "this delimiter might be at fault" ?

@nikomatsakis 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 4, 2018
@estebank
Reword un-closed delimiter label
5238b52
@estebank
Provide more context for unenclosed delimiters
008aa5a 
 * When encountering EOF, point at the last opening brace that does not
   have the same indentation level as its close delimiter.
 * When encountering the wrong type of close delimiter, point at the
   likely correct open delimiter to give a better idea of what went
   wrong.
@estebank
Fix existing test
b2d2d83
@estebank
Copy link
Contributor Author

estebank commented Sep 5, 2018

I'm very excited about this. Looks like tests don't quite pass yet, though.

It was a parse-fail test with outdated wording. Fixed.

I was wondering whether weirdness could happen because of macros, but I guess not: macro expansions can't introduce unpaired braces, after all. The error would occur when parsing the original file.

The only thing that worries me is that code with wonky indentation will receive subpar suggestions, but I have banged my head trying to come up with better heuristics and they were all much more complicated than this for marginal benefit.

@nikomatsakis
Copy link
Contributor

The only thing that worries me is that code with wonky indentation will receive subpar suggestions...

I feel like that's ok.

@estebank
Copy link
Contributor Author

estebank commented Sep 5, 2018

@bors r=nikomatsakis

@bors
Copy link
Contributor

bors commented Sep 5, 2018

📌 Commit 3192d3d has been approved by nikomatsakis

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 5, 2018
@estebank estebank mentioned this pull request Sep 7, 2018
Closed
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request Sep 8, 2018
@Mark-Simulacrum
Rollup merge of rust-lang#53949 - estebank:unclosed-delim, r=nikomats…
1e5e014 
…akis

Improve messages for un-closed delimiter errors
@Mark-Simulacrum Mark-Simulacrum mentioned this pull request Sep 8, 2018
Closed
@bors
Copy link
Contributor

bors commented Sep 9, 2018

⌛ Testing commit 3192d3d with merge 004bc5a...

bors added a commit that referenced this pull request Sep 9, 2018
@bors
Auto merge of #53949 - estebank:unclosed-delim, r=nikomatsakis
004bc5a 
Improve messages for un-closed delimiter errors
@bors
Copy link
Contributor

bors commented Sep 9, 2018

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

@bors bors merged commit 3192d3d into rust-lang:master Sep 9, 2018
@nikomatsakis
Copy link
Contributor

Hmm, sadly, this PR seems to be a massive compilation time hit — I think we have to back it out :(

@nikomatsakis nikomatsakis mentioned this pull request Sep 9, 2018
Closed
@estebank
Copy link
Contributor Author

estebank commented Sep 9, 2018

Wow, that's terrible. You can revert with my auto approval, as I'm laptop less until Monday. I'll take a look tonight to see if I can find any way to avoid this penalty.

Is there a way to run those performance tests locally?

@kennytm kennytm mentioned this pull request Sep 9, 2018
Merged
@estebank estebank mentioned this pull request Sep 9, 2018
Merged
bors added a commit that referenced this pull request Sep 9, 2018
@bors
Auto merge of #54092 - estebank:gotta-go-fast, r=<try>
ff274ae 
Don't compute padding of braces unless they are unmatched

Follow up to #53949. Attempt to fix # #54083.

r? @nikomatsakis
@nnethercote
Copy link
Contributor

Benchmarking docs.

bors added a commit that referenced this pull request Sep 11, 2018
@bors
Auto merge of #54092 - estebank:gotta-go-fast, r=nikomatsakis
d6720cc 
Don't compute padding of braces unless they are unmatched

Follow up to #53949. Attempt to fix # #54083.

r? @nikomatsakis
@estebank estebank mentioned this pull request Sep 14, 2018
Closed
@arielb1 arielb1 mentioned this pull request Dec 15, 2018
Closed
@estebank estebank mentioned this pull request Jan 28, 2019
Merged
bors added a commit that referenced this pull request Feb 7, 2019
@bors
Auto merge of #57944 - estebank:unclosed-delim-the-quickening, r=<try>
dbcc2e8 
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.
bors added a commit that referenced this pull request Feb 8, 2019
@bors
Auto merge of #57944 - estebank:unclosed-delim-the-quickening, r=oli-obk
8c2963b 
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.
bors added a commit that referenced this pull request Feb 9, 2019
@bors
Auto merge of #57944 - estebank:unclosed-delim-the-quickening, r=oli-obk
3315728 
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.
@smmalis37 smmalis37 mentioned this pull request Apr 15, 2021
Closed
@estebank estebank deleted the unclosed-delim branch November 9, 2023 05:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nikomatsakis nikomatsakis nikomatsakis approved these changes

Assignees

@nikomatsakis nikomatsakis

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@estebank @rust-highfive @nikomatsakis @bors @nnethercote