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.

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

Fix regression 61475 #61500

Merged
merged 2 commits into from
Jun 5, 2019
Merged

Fix regression 61475 #61500

merged 2 commits into from
Jun 5, 2019

Conversation

estebank
Copy link
Contributor

@estebank estebank commented Jun 3, 2019

Addresses #61475.

@estebank estebank added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jun 3, 2019
@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 Jun 3, 2019
@estebank estebank added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jun 3, 2019
@estebank
Copy link
Contributor Author

estebank commented Jun 3, 2019

cc @Mark-Simulacrum

@Mark-Simulacrum
Copy link
Member

r? @petrochenkov perhaps?

@lqd
Copy link
Member

lqd commented Jun 3, 2019

If we also wanted a test here, this seemed small and enough to reproduce on beta/nightly (but accepted on stable):

enum E {
    A, B
}

fn main() {
    match &&E::A {
        &&E::A => {
        }
        &&E::B => {
        }
    };
}

@petrochenkov
Copy link
Contributor

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jun 4, 2019

📌 Commit 5716e26 has been approved by petrochenkov

@petrochenkov petrochenkov 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 Jun 4, 2019
Centril added a commit to Centril/rust that referenced this pull request Jun 4, 2019
pietroalbini added a commit to pietroalbini/rust that referenced this pull request Jun 4, 2019
bors added a commit that referenced this pull request Jun 4, 2019
Rollup of 5 pull requests

Successful merges:

 - #61069 (Make MIR drop terminators borrow the dropped location)
 - #61453 (Remove unneeded feature attr from atomic integers doctests)
 - #61488 (Fix NLL typeck ICEs)
 - #61500 (Fix regression 61475)
 - #61523 (Hide gen_future API from documentation)

Failed merges:

r? @ghost
@bors bors merged commit 5716e26 into rust-lang:master Jun 5, 2019
@pnkfelix
Copy link
Member

pnkfelix commented Jun 6, 2019

discussed at T-compiler meeting; approved for beta-backport

@pnkfelix pnkfelix added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Jun 6, 2019
@pietroalbini pietroalbini removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jun 6, 2019
bors added a commit that referenced this pull request Jun 6, 2019
[beta] Rollup backports

Rolled up:

* [beta] Permit unwinding through FFI by default #61569

Cherry-picked:

* upgrade rustdoc's `pulldown-cmark` to 0.5.2 #60802
* Fix overflowing literal lint in loops #61098
* Revert edition-guide toolstate override #61110
* Turn turbo 🐟 🍨 into an error #61189
* Bump hashbrown to 0.4.0 #61388
* Fix regression 61475 #61500

r? @ghost
bors added a commit that referenced this pull request Jun 7, 2019
[beta] Rollup backports

Rolled up:

* [beta] Permit unwinding through FFI by default #61569

Cherry-picked:

* upgrade rustdoc's `pulldown-cmark` to 0.5.2 #60802
* Fix overflowing literal lint in loops #61098
* Revert edition-guide toolstate override #61110
* Turn turbo 🐟 🍨 into an error #61189
* Bump hashbrown to 0.4.0 #61388
* Fix regression 61475 #61500

r? @ghost
Comment on lines +2852 to +2854
// If the next token is a keyword, then the tokens above *are* unambiguously incorrect:
// `if x { a } else { b } && if y { c } else { d }`
if !self.look_ahead(1, |t| t.is_reserved_ident()) => {
Copy link
Member

Choose a reason for hiding this comment

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

Was the intention here to catch if, match, loop, while and for? Because it also triggers for {} &&false (which would otherwise parse the same way as {} & &false), despite {} &&0 being unaffected.

Copy link
Member

@eddyb eddyb Jul 11, 2020

Choose a reason for hiding this comment

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

Oh, this seems coincidental to #74233, and not actually a bug in itself.

@@ -2847,7 +2847,11 @@ impl<'a> Parser<'a> {
// want to keep their span info to improve diagnostics in these cases in a later stage.
(true, Some(AssocOp::Multiply)) | // `{ 42 } *foo = bar;` or `{ 42 } * 3`
(true, Some(AssocOp::Subtract)) | // `{ 42 } -5`
(true, Some(AssocOp::Add)) => { // `{ 42 } + 42
(true, Some(AssocOp::LAnd)) | // `{ 42 } &&x` (#61475)
(true, Some(AssocOp::Add)) // `{ 42 } + 42
Copy link
Member

Choose a reason for hiding this comment

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

The Add case (although not introduced in this PR) is also a bit strange... surely Rust expressions can't start with +?
That is, Rust has no "unary plus" operator, like other languages do.

Comment on lines 2848 to 2849
(true, Some(AssocOp::Multiply)) | // `{ 42 } *foo = bar;` or `{ 42 } * 3`
(true, Some(AssocOp::Subtract)) | // `{ 42 } -5`
Copy link
Member

Choose a reason for hiding this comment

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

If these are included, why isn't { 42 } & 3? Surely the have the same property?

Copy link
Member

Choose a reason for hiding this comment

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

Ah this seems like a reporting-only heuristic and only the can_continue_expr_unambiguously check is relevant to what I was seeing.

@eddyb
Copy link
Member

eddyb commented Jul 11, 2020

Looking again at this, I don't think this PR was necessary, it seems to have worked around the bug without fully fixing it.
As per #74233, can_continue_expr_unambiguously should've never returned true for LAnd, preventing the regression.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta-accepted Accepted for backporting to the compiler in the beta channel. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants