Recover from `if (let ...)` in parsing, instead of lowering #83407

osa1 · 2021-03-23T11:55:50Z

When we recover from if (let ...) in lowering we can't "ungate" the
syntax after generating the error

invalid parentheses around `let` expression in `if let`

as the feature_gate::check_crate pass has already run. This is mentioned
in these comments in the old code

Ideally, we'd remove the feature gating of a let expression since we
are already complaining about it here, but feature_gate::check_crate
has already run by now

We now recover from if (let ...) in parsing, generate the error as
before, and "ungate" the syntax. For example, in this code:

fn main() {
    let x = Some(3);
    if (let Some(y) = x) {}
}

We would previously generate:

error[E0658]: `let` expressions in this position are experimental
 --> test.rs:3:9
  |
3 |     if (let Some(y) = x) {}
  |         ^^^^^^^^^^^^^^^
  |
  = note: see issue #53667 <https://github.com/rust-lang/rust/issues/53667> for more information
  = help: add `#![feature(let_chains)]` to the crate attributes to enable
  = help: you can write `matches!(<expr>, <pattern>)` instead of `let <pattern> = <expr>`

error: invalid parentheses around `let` expression in `if let`
 --> test.rs:3:8
  |
3 |     if (let Some(y) = x) {}
  |        ^               ^
  |
help: `if let` needs to be written without parentheses
  |
3 |     if let Some(y) = x {}
  |       --             --

error: aborting due to 2 previous errors

The first error doesn't make sense. With the "ungating" we now generate
just the second one:

error: invalid parentheses around `let` expression in `if let`
 --> test.rs:3:8
  |
3 |     if (let Some(y) = x) {}
  |        ^               ^
  |
help: `if let` needs to be written without parentheses
  |
3 |     if let Some(y) = x {}
  |       --             --

error: aborting due to previous error

Fixes #83274

rust-highfive · 2021-03-23T11:55:54Z

r? @davidtwco

(rust-highfive has picked a reviewer for you, use r? to override)

petrochenkov · 2021-03-23T12:09:12Z

Looks like there's an intersection with #80357 here.

estebank · 2021-03-25T15:48:21Z

@petrochenkov do you see any reason not to go ahead with this change (beyond the potential merge conflicts inflicted on the other PR, given that it is already in conflict with master)?

petrochenkov · 2021-03-25T21:35:27Z

After a more detailed look, this PR goes exactly 180 degrees away from the right direction.
It removes something that should be reported (a feature gate error), but keeps something that should not be reported - the parentheses error.

#80357 on the other hand does the right thing - keeps the feature gate while moving the implementation of let expressions further, and removes the parentheses error. So, I don't see any reasons to merge this PR or keep it open.

estebank · 2021-03-26T03:12:47Z

It removes something that should be reported (a feature gate error), but keeps something that should not be reported - the parentheses error.

Presenting both errors is pretty terrible user experience. Complaining about the feature gate in a case that is an easy typo to make (surrounding parens around the let expression) can be incredibly frustrating/confusing to a newcomer.

#80357 on the other hand does the right thing - keeps the feature gate while moving the implementation of let expressions further, and removes the parentheses error.

I looked at the other PR and it produces no changes in output for src/test/ui/rfc-2497-if-let-chains/feature-gate.rs, so they seem to be somewhat independent of each other in their effects. If we want to unconditionally remove the error for the specific case of if (let pat = expr) { (which looking at it now should only be emitted when the feature is gated), we need to improve the lint to detect this case and provide the same suggestion. Doing so would be more convoluted than the targeted diagnostic.

estebank · 2021-03-26T03:14:21Z

compiler/rustc_parse/src/parser/expr.rs

-        let cond = self.parse_cond_expr()?;
+        let cond = self.parse_cond_expr()?.into_inner();
+
+        let cond = if let ExprKind::Paren(paren) = &cond.kind {


This should only be done if the gate is disabled, otherwise there's no way to accept this syntax ever, which is valid when let chains are enabled.

Hm.. in the current version, we lower if (let ...) to if let unconditionally, so that's what I did here as well.

This should only be done if the gate is disabled

Which gate do you mean?

let_chains. I think you can access it with self.sess.features_untracked().or_patterns. If that expr is true, then we shouldn't emit this error. I think this is a bug introduced by me when I added the targeted error.

If we do this, then parsing of if (let ...) will be different depending on whether let_chains is enabled or not, right? Should feature gates be able to change parsing? I'd expect not.

The feature gates would change what the resulting AST is interpreted as. if (let pat = expr) is valid when the feature gate is enabled. This is why you get the error we're trying to silence. All of this is appropriate behavior, but it makes for a bad experience. Because of this ideally the feature gate error would suggest removing the parens for this case, but that would mean ferrying more information into the linting machinery so that it can know that it was this case. I find it easier to treat it more locally.

I think it's not possible to check for gates in parser. The data is not available in ParseSess.

I think for this information to be available when parsing you would need to first parse the crate root for the attributes, then parse other files. Perhaps the parser is not implemented this way currently? (though I just saw the Edition field, which can be updated with an attribute at the crate root..)

compiler/rustc_parse/src/parser/expr.rs

osa1 · 2021-03-26T07:41:46Z

@petrochenkov

It removes something that should be reported (a feature gate error), but keeps something that should not be reported - the parentheses error.

I think you misunderstand what this PR is doing. When the user writes if (let ...) we assume they meant if let .... This is already the case today, without this PR. The problem is, today, even though we assume if let, which is not feature gated, we generate a feature gate error. This makes no sense, because if let syntax is not feature gated, and we already generated an error suggesting removal of the parentheses.

This PR maintains the same assumption and does not add any new assumptions. Only difference is we no longer generate the invalid feature gate error when user types if (let ...), because we assume they meant if let (as explained), and if let syntax is not feature gated.

Does this make sense now?

davidtwco

After resolving @estebank's comments, this seems fine to me.

davidtwco · 2021-03-26T10:27:15Z

r? @estebank

estebank · 2021-03-26T15:03:41Z

When the user writes if (let ...) we assume they meant if let .... This is already the case today, without this PR.

That is a bug though I introduced. We don't just assume it meant if let, we emit an error and adapt the AST, but we still need to allow it when the gate is opened.

When we recover from `if (let ...)` in lowering we can't "ungate" the syntax after generating the error invalid parentheses around `let` expression in `if let` as the feature_gate::check_crate pass has already run. This is mentioned in these comments in the old code > Ideally, we'd remove the feature gating of a `let` expression since we > are already complaining about it here, but `feature_gate::check_crate` > has already run by now We now recover from `if (let ...)` in parsing, generate the error as before, and "ungate" the syntax. For example, in this code: fn main() { let x = Some(3); if (let Some(y) = x) {} } We would previously generate: error[E0658]: `let` expressions in this position are experimental --> test.rs:3:9 | 3 | if (let Some(y) = x) {} | ^^^^^^^^^^^^^^^ | = note: see issue rust-lang#53667 <rust-lang#53667> for more information = help: add `#![feature(let_chains)]` to the crate attributes to enable = help: you can write `matches!(<expr>, <pattern>)` instead of `let <pattern> = <expr>` error: invalid parentheses around `let` expression in `if let` --> test.rs:3:8 | 3 | if (let Some(y) = x) {} | ^ ^ | help: `if let` needs to be written without parentheses | 3 | if let Some(y) = x {} | -- -- error: aborting due to 2 previous errors The first error doesn't make sense. With the "ungating" we now generate just the second one: error: invalid parentheses around `let` expression in `if let` --> test.rs:3:8 | 3 | if (let Some(y) = x) {} | ^ ^ | help: `if let` needs to be written without parentheses | 3 | if let Some(y) = x {} | -- -- error: aborting due to previous error Fixes rust-lang#83274

estebank · 2021-03-29T16:08:25Z

src/test/ui/rfc-2497-if-let-chains/issue-83274.rs

@@ -0,0 +1,7 @@
+// check-fail


Can we add another copy of this file with // check-pass and the gate enabled to catch any regressions?

estebank · 2021-03-29T16:11:32Z

compiler/rustc_parse/src/parser/expr.rs

-        let cond = self.parse_cond_expr()?;
+        let mut cond = self.parse_cond_expr()?.into_inner();
+
+        if let ExprKind::Paren(paren) = &cond.kind {


We still need the feature gate check we talked about here, right?

That's blocked currently. Have you seen my comment above? #83407 (comment)

Hadn't, looking into it. (The parser is indeed not supposed to be able to use that to avoid diverging parsing based on feature gates.)

estebank · 2021-03-29T16:13:34Z

compiler/rustc_parse/src/parser/expr.rs

@@ -1765,7 +1765,20 @@ impl<'a> Parser<'a> {
    /// Parses an `if` expression (`if` token already eaten).
    fn parse_if_expr(&mut self, attrs: AttrVec) -> PResult<'a, P<Expr>> {
        let lo = self.prev_token.span;
-        let cond = self.parse_cond_expr()?;
+        let mut cond = self.parse_cond_expr()?.into_inner();


Is the into_inner necessary? IIUC, you need that to be able to reassing, but wouldn't changing line 1777 to be cond = P(cond.peel_parens_owned()); work as well and make this "unwrapping/rewrapping" unnecessary in the happy path? I'm just concerned about doing unnecessary work that might marginally slow down compile times.

crlf0710 · 2021-04-17T03:20:01Z

@osa1 Ping from triage, could you address the comments above? Thanks!

osa1 · 2021-04-25T07:49:05Z

@osa1 Ping from triage, could you address the comments above? Thanks!

Sorry for late response. I think this is currently stuck as we don't have the information we need in parse time, see this thread: #83407 (comment)

I'd like to keep this open for a bit more and think about it more before closing.

JohnCSimon · 2021-05-31T05:03:54Z

Ping from triage:
@osa1 I'm closing this as inactive - please feel free to reopen as necessary.

rust-highfive assigned davidtwco Mar 23, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 23, 2021

osa1 force-pushed the issue83274 branch from c72f24d to ba8307b Compare March 23, 2021 11:56

petrochenkov self-assigned this Mar 25, 2021

petrochenkov removed their assignment Mar 25, 2021

estebank reviewed Mar 26, 2021

View reviewed changes

compiler/rustc_parse/src/parser/expr.rs Outdated Show resolved Hide resolved

davidtwco reviewed Mar 26, 2021

View reviewed changes

rust-highfive assigned estebank and unassigned davidtwco Mar 26, 2021

osa1 added 2 commits March 27, 2021 10:38

Refactor updating expr in parse_if_expr

57363f4

osa1 force-pushed the issue83274 branch from ba8307b to 57363f4 Compare March 27, 2021 07:49

estebank reviewed Mar 29, 2021

View reviewed changes

crlf0710 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 Apr 17, 2021

JohnCSimon 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 9, 2021

JohnCSimon added the S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. label May 31, 2021

JohnCSimon closed this May 31, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Recover from `if (let ...)` in parsing, instead of lowering #83407

Recover from `if (let ...)` in parsing, instead of lowering #83407

osa1 commented Mar 23, 2021 •

edited

rust-highfive commented Mar 23, 2021

petrochenkov commented Mar 23, 2021

estebank commented Mar 25, 2021

petrochenkov commented Mar 25, 2021

estebank commented Mar 26, 2021

estebank Mar 26, 2021

osa1 Mar 26, 2021

estebank Mar 26, 2021

osa1 Mar 26, 2021

estebank Mar 26, 2021

osa1 Mar 27, 2021

osa1 commented Mar 26, 2021

davidtwco left a comment

davidtwco commented Mar 26, 2021

estebank commented Mar 26, 2021

estebank Mar 29, 2021

estebank Mar 29, 2021

osa1 Mar 29, 2021

estebank Mar 29, 2021

estebank Mar 29, 2021

crlf0710 commented Apr 17, 2021

osa1 commented Apr 25, 2021

JohnCSimon commented May 31, 2021

Recover from if (let ...) in parsing, instead of lowering #83407

Recover from if (let ...) in parsing, instead of lowering #83407

Conversation

osa1 commented Mar 23, 2021 • edited

rust-highfive commented Mar 23, 2021

petrochenkov commented Mar 23, 2021

estebank commented Mar 25, 2021

petrochenkov commented Mar 25, 2021

estebank commented Mar 26, 2021

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

osa1 commented Mar 26, 2021

davidtwco left a comment

Choose a reason for hiding this comment

davidtwco commented Mar 26, 2021

estebank commented Mar 26, 2021

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

crlf0710 commented Apr 17, 2021

osa1 commented Apr 25, 2021

JohnCSimon commented May 31, 2021

Recover from `if (let ...)` in parsing, instead of lowering #83407

Recover from `if (let ...)` in parsing, instead of lowering #83407

osa1 commented Mar 23, 2021 •

edited