Skip to content

Fix fn front matter parsing ICE from invalid code. #60132

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

Merged
merged 1 commit into from
Apr 21, 2019

Conversation

davidtwco
Copy link
Member

Fixes #60075.

This PR fixes an "unreachable code" ICE that results from parsing
invalid code where the compiler is expecting the next trait item
declaration in the middle of the previous trait item due to extra
closing braces.

r? @estebank (thanks for the minimized test case)

This commit fixes an "unreachable code" ICE that results from parsing
invalid code where the compiler is expecting the next trait item
declaration in the middle of the previous trait item due to extra
closing braces.
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 20, 2019
Copy link
Contributor

@estebank estebank left a comment

Choose a reason for hiding this comment

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

I would like us to handle this more gracefully but this PR is good. r=me unless you want to address the comment in this PR.

// `self.expected_tokens`, therefore, do not use `self.unexpected()` which doesn't
// account for this.
if !self.expect_one_of(&[], &[])? { unreachable!() }
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Although I think this change will fix the general problem today, I feel we could also address the side of the recovery code as well to not attempt to close the block when encountering an incorrect closing delimiter. It is tricky because what is best to do depends entirely on the type of mistake made and we don't do global analysis...

What do you think? Should we have a follow up PR/ticket with different common cases to test the recovery mechanism?

Copy link
Member Author

Choose a reason for hiding this comment

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

I’d land this as is to fix the ICE, but I think a follow up to try and handle this case more gracefully definitely makes sense.

I spent a bunch of time trying to work out a nice way to recover from this case but I couldn’t see one. If we don’t attempt to close the block when encountering an unclosed delimiter then that’ll regress other cases that we recover from currently, it’s a tough one.

@estebank
Copy link
Contributor

@bors r+ rollup

@bors
Copy link
Collaborator

bors commented Apr 20, 2019

📌 Commit 60c6ed9 has been approved by estebank

@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-review Status: Awaiting review from the assignee but also interested parties. labels Apr 20, 2019
@bors
Copy link
Collaborator

bors commented Apr 21, 2019

⌛ Testing commit 60c6ed9 with merge 4d9c6cd...

bors added a commit that referenced this pull request Apr 21, 2019
Fix fn front matter parsing ICE from invalid code.

Fixes #60075.

This PR fixes an "unreachable code" ICE that results from parsing
invalid code where the compiler is expecting the next trait item
declaration in the middle of the previous trait item due to extra
closing braces.

r? @estebank (thanks for the minimized test case)
@bors
Copy link
Collaborator

bors commented Apr 21, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: estebank
Pushing 4d9c6cd to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 21, 2019
@bors bors merged commit 60c6ed9 into rust-lang:master Apr 21, 2019
@davidtwco davidtwco deleted the issue-60075 branch April 21, 2019 15:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Compiler panic: 'internal error: entered unreachable code' in src/libsyntax/parse/parser.rs
4 participants