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

Identify when a stmt could have been parsed as an expr #60188

Merged
merged 6 commits into from
May 10, 2019
Merged

Identify when a stmt could have been parsed as an expr #60188

merged 6 commits into from
May 10, 2019

Conversation

estebank
Copy link
Contributor

@estebank estebank commented Apr 23, 2019 •
edited
Loading

There are some expressions that can be parsed as a statement without
a trailing semicolon depending on the context, which can lead to
confusing errors due to the same looking code being accepted in some
places and not others. Identify these cases and suggest enclosing in
parenthesis making the parse non-ambiguous without changing the
accepted grammar.

Fix #54186, cc #54482, fix #59975, fix #47287.

@rust-highfive
Copy link
Collaborator

r? @michaelwoerister

(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 Apr 23, 2019
@rust-highfive

This comment has been minimized.

Sign in to view
@rust-highfive

This comment has been minimized.

Sign in to view
@varkor
Copy link
Member

varkor commented Apr 23, 2019

Fix #54186, fix #54482, fix #59975.

I'm not sure it's fair to say it actually fixes these, though it improves the diagnostics. I think it's possible we want to accept the code there (there isn't a reason these are not accepted other than "it's hard to parse").

@estebank
Copy link
Contributor Author

@varkor I thought that we wanted to make sure the parser was unambiguous. Some of these cases can be identified in the parser itself, but some require checks in the type checker or elsewhere. At that point the options are either only accept some of these operations but not others, which could look arbitrary, or have a more advanced parser/evaluator, which aiui is out of the question.

@rust-highfive

This comment has been minimized.

Sign in to view
@rust-highfive

This comment has been minimized.

Sign in to view
@estebank
Copy link
Contributor Author

r? @varkor

@estebank
Identify when a stmt could have been parsed as an expr
f007e6f 
There are some expressions that can be parsed as a statement without
a trailing semicolon depending on the context, which can lead to
confusing errors due to the same looking code being accepted in some
places and not others. Identify these cases and suggest enclosing in
parenthesis making the parse non-ambiguous without changing the
accepted grammar.
@estebank
Add test case for rust-lang#47287
bff0be3
@varkor
Copy link
Member

varkor commented Apr 30, 2019

Sorry for the delay; I will get to this soon.

varkor
varkor reviewed May 2, 2019
View reviewed changes
Copy link
Member

@varkor varkor left a comment

Choose a reason for hiding this comment

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

I made a few comments, but I still need to think about this some more.

src/librustc_typeck/check/mod.rs Outdated Show resolved Hide resolved
src/libsyntax/parse/parser.rs Outdated Show resolved Hide resolved
src/libsyntax/parse/parser.rs Outdated Show resolved Hide resolved
src/libsyntax/parse/parser.rs Outdated Show resolved Hide resolved
src/librustc_typeck/check/mod.rs Outdated Show resolved Hide resolved
src/libsyntax/parse/parser.rs Outdated Show resolved Hide resolved
@varkor
Copy link
Member

varkor commented May 2, 2019

@varkor I thought that we wanted to make sure the parser was unambiguous. Some of these cases can be identified in the parser itself, but some require checks in the type checker or elsewhere.

I agree that the grammar should be unambiguous, but I was under the impression some of these cases were not ambiguous; they were just tricky to detect. Maybe it's reasonable to close these issues as diagnostic issues, but I think there should be an issue somewhere keeping track of this until a deliberate decision has been made.

@estebank
Copy link
Contributor Author

estebank commented May 2, 2019

I agree that the grammar should be unambiguous, but I was under the impression some of these cases were not ambiguous; they were just tricky to detect.

That is indeed true. If you look at https://github.com/rust-lang/rust/pull/60188/files#diff-11be04506fe70f5f173b5fdf7f294ad7 you can see the full list of unambiguous operations, but I wouldn't want to have some constructs work, and some not in an almost arbitrary manner.

@varkor varkor 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 May 6, 2019
@estebank estebank added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 6, 2019
@estebank
Copy link
Contributor Author

estebank commented May 8, 2019

ping @varkor :)

@varkor
Copy link
Member

varkor commented May 9, 2019

@bors r+

@bors
Copy link
Contributor

bors commented May 9, 2019

📌 Commit 54430ad has been approved by varkor

@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 May 9, 2019
Centril added a commit to Centril/rust that referenced this pull request May 9, 2019
@Centril
Rollup merge of rust-lang#60188 - estebank:recover-block, r=varkor
39edc68 
Identify when a stmt could have been parsed as an expr

There are some expressions that can be parsed as a statement without
a trailing semicolon depending on the context, which can lead to
confusing errors due to the same looking code being accepted in some
places and not others. Identify these cases and suggest enclosing in
parenthesis making the parse non-ambiguous without changing the
accepted grammar.

Fix rust-lang#54186, cc rust-lang#54482, fix rust-lang#59975, fix rust-lang#47287.
@Centril Centril mentioned this pull request May 9, 2019
Merged
bors added a commit that referenced this pull request May 10, 2019
@bors
Auto merge of #60683 - Centril:rollup-p05qh5d, r=Centril
03bd2f6 
Rollup of 8 pull requests

Successful merges:

 - #59348 (Clean up and add tests for slice drop shims)
 - #60188 (Identify when a stmt could have been parsed as an expr)
 - #60234 (std: Derive `Default` for `io::Cursor`)
 - #60618 (Comment ext::tt::transcribe)
 - #60648 (Skip codegen for one UI test with long file path)
 - #60671 (remove unneeded `extern crate`s from build tools)
 - #60675 (Remove the old await! macro)
 - #60676 (Fix async desugaring providing wrong input to procedural macros.)

Failed merges:

r? @ghost
@bors
Copy link
Contributor

bors commented May 10, 2019

⌛ Testing commit 54430ad with merge 03bd2f6...

@bors bors merged commit 54430ad into rust-lang:master May 10, 2019
@estebank estebank mentioned this pull request Jun 3, 2019
Closed
@eddyb eddyb mentioned this pull request Jul 11, 2020
Closed
@estebank estebank deleted the recover-block branch November 9, 2023 05:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@varkor varkor varkor left review comments

Assignees

@varkor varkor

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
5 participants
@estebank @rust-highfive @varkor @bors @michaelwoerister