Fix last let_chains blocker
#98633
Fix last let_chains blocker
#98633
Conversation
rustbot
added
the
T-compiler
|
(rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
c410-f3r
force-pushed
the
yet-another-let-chain
branch
2 times, most recently
from
5ad965c to
16b3d40
Compare
|
Look like @petrochenkov is not available right not. Perhaps another reviewer? It would be a shame to miss yet another release. r? @rust-lang/compiler |
| // Catches things like `if let Some(_) = _opt && [1, 2, 3][let _ = ()] = 1` | ||
| let is_in_a_let_chains_context_but_nested_in_other_expr = self.let_expr_allowed | ||
| && !matches!( | ||
| self.prev_token.kind, | ||
| TokenKind::AndAnd | ||
| | TokenKind::CloseDelim(Delimiter::Brace) | ||
| | TokenKind::Ident(kw::If, _) | ||
| | TokenKind::Ident(kw::While, _) | ||
| ); |
There was a problem hiding this comment.
As you already mention, this is a bit of a hack, but seems safe enough (although I feel like it might also not trigger with things like let _ = &&let Some(x) = Some(42); where the intent might be &&bool, can we test that?).
Having said that, I'm wondering if you explored adding a visit_pat function to https://github.com/rust-lang/rust/blob/master/compiler/rustc_ast_passes/src/ast_validation.rs to catch the nested pattern cases (like parens around the let expression).
It feels like cases like
https://github.com/rust-lang/rust/pull/98633/files#diff-211c1d6e2f4ff5f2af12400a49c018c5ecebd9e0ec9e8a15049a9e55521692bbR366-R370
in line 368 ([][let () = ()]) should be able to be caught by the current code (but that test has that expression cfgd away, which is likely why it wasn't triggering?).
If the reason for this code is to trigger even in the face of cfgd code, and modifying the visitor to be more exhaustive isn't possible for good reason, then I'm ok with the code as is. It isn't great, but worst case scenario this is overly restrictive, which is better than the alternative.
There was a problem hiding this comment.
As you already mention, this is a bit of a hack, but seems safe enough (although I feel like it might also not trigger with things like let _ = &&let Some(x) = Some(42); where the intent might be &&bool, can we test that?).
Added test in a let-chain context as well as a let statement.
Having said that, I'm wondering if you explored adding a visit_pat function to https://github.com/rust-lang/rust/blob/master/compiler/rustc_ast_passes/src/ast_validation.rs to catch the nested pattern cases (like parens around the let expression).
Nested parenthesis in the pattern? Like?
if let Some(value) = opt && let &[(1)] = value { ... }
if let Some(value) = opt && let Some((1)) = value { ... }If so, they are currently denied.
Parenthesis in nested expressions are also forbidden.
if let Some(value) = opt && let Some(1) = {
if let 1 = 1 && (let 1 = 1) {}
value
} {}rust/compiler/rustc_ast_passes/src/ast_validation.rs
Lines 1856 to 1860 in 049308c
It feels like cases like
https://github.com/rust-lang/rust/pull/98633/files#diff-211c1d6e2f4ff5f2af12400a49c018c5ecebd9e0ec9e8a15049a9e55521692bbR366-R370
in line 368 ([][let () = ()]) should be able to be caught by the current code (but that test has that expression cfgd away, which is likely why it wasn't triggering?).
Couldn't reach the link. Do you mean?
If so, then the invalid code is indeed caught and issued as expected expression, found 'let' statement. If cfg is removed, then ast_validation.rs also warns 'let' expressions are not supported here.
If the reason for this code is to trigger even in the face of cfgd code, and modifying the visitor to be more exhaustive isn't possible for good reason, then I'm ok with the code as is. It isn't great, but worst case scenario this is overly restrictive, which is better than the alternative.
Yes, every invalid let location is already handled in ast_validation.rs but people are worried about cfg usage thus the reason of this PR.
c410-f3r
force-pushed
the
yet-another-let-chain
branch
from
16b3d40 to
32a5417
Compare
c410-f3r
force-pushed
the
yet-another-let-chain
branch
from
32a5417 to
9d2a9d9
Compare
|
FWIW, a lot of the spaghetti code in the parser is mainly about error recovery, the underlying parser should always be "clean" and with strict short lookahead (we break these rules for error recovery, including with spooky action at a distance, because we're dealing with code that is already non-regular). In this case I can make the case that the "N=1 lookback" is just an implementation detail and the Grammar remains "N=1 lookahead". The only case that I am somewhat scared a about is @bors r+ |
bors
added
S-waiting-on-bors
…stebank
Fix last `let_chains` blocker
In order to forbid things like `let x = (let y = 1);` or `if let a = 1 && { let x = let y = 1; } {}`, the parser **HAS** to know the context of `let`.
This context thing is not a surprise in the parser because you can see **a lot** of ad hoc fixes mixing parsing logic with validation logic creating code that looks more like spaghetti with tomato sauce.
To make things even greater, a new ad hoc fix was added to only allow `let`s in a valid `let_chains` context by checking the previously processed token. This was the only solution I could think of and believe me, I thought about it for a long time 👍
In the long term, it should be preferable to segregate different responsibilities or create a more robust and cleaner parser framework.
cc rust-lang#94927
cc rust-lang#53667
…stebank
Fix last `let_chains` blocker
In order to forbid things like `let x = (let y = 1);` or `if let a = 1 && { let x = let y = 1; } {}`, the parser **HAS** to know the context of `let`.
This context thing is not a surprise in the parser because you can see **a lot** of ad hoc fixes mixing parsing logic with validation logic creating code that looks more like spaghetti with tomato sauce.
To make things even greater, a new ad hoc fix was added to only allow `let`s in a valid `let_chains` context by checking the previously processed token. This was the only solution I could think of and believe me, I thought about it for a long time 👍
In the long term, it should be preferable to segregate different responsibilities or create a more robust and cleaner parser framework.
cc rust-lang#94927
cc rust-lang#53667
|
☔ The latest upstream changes (presumably #98482) made this pull request unmergeable. Please resolve the merge conflicts. |
bors
added
S-waiting-on-author
c410-f3r
force-pushed
the
yet-another-let-chain
branch
from
9d2a9d9 to
1c3bab2
Compare
|
@rustbot label +S-waiting-on-review -S-waiting-on-author |
rustbot
added
S-waiting-on-review
|
Further work on reference documentation depends on this PR that was already re-based three days ago due to code changes. It is already approved so I ask anyone to |
|
@bors r=estebank |
bors
added
S-waiting-on-bors
|
Thank you |
…stebank
Fix last `let_chains` blocker
In order to forbid things like `let x = (let y = 1);` or `if let a = 1 && { let x = let y = 1; } {}`, the parser **HAS** to know the context of `let`.
This context thing is not a surprise in the parser because you can see **a lot** of ad hoc fixes mixing parsing logic with validation logic creating code that looks more like spaghetti with tomato sauce.
To make things even greater, a new ad hoc fix was added to only allow `let`s in a valid `let_chains` context by checking the previously processed token. This was the only solution I could think of and believe me, I thought about it for a long time 👍
In the long term, it should be preferable to segregate different responsibilities or create a more robust and cleaner parser framework.
cc rust-lang#94927
cc rust-lang#53667
…stebank
Fix last `let_chains` blocker
In order to forbid things like `let x = (let y = 1);` or `if let a = 1 && { let x = let y = 1; } {}`, the parser **HAS** to know the context of `let`.
This context thing is not a surprise in the parser because you can see **a lot** of ad hoc fixes mixing parsing logic with validation logic creating code that looks more like spaghetti with tomato sauce.
To make things even greater, a new ad hoc fix was added to only allow `let`s in a valid `let_chains` context by checking the previously processed token. This was the only solution I could think of and believe me, I thought about it for a long time 👍
In the long term, it should be preferable to segregate different responsibilities or create a more robust and cleaner parser framework.
cc rust-lang#94927
cc rust-lang#53667
Rollup of 6 pull requests Successful merges: - rust-lang#98622 (rustc_target: Flip the default for `TargetOptions::executables` to true) - rust-lang#98633 (Fix last `let_chains` blocker) - rust-lang#98972 (Suggest adding a missing zero to a floating point number) - rust-lang#99038 (Some more `EarlyBinder` cleanups) - rust-lang#99154 (use PlaceRef::iter_projections to fix old FIXME) - rust-lang#99171 (Put back UI test regex) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
…iler-errors Remove let-chain close brace check. rust-lang#98633 added some checks to forbid let-expressions that aren't in a let chain. This check looks at the preceding token to determine if it is a valid let-chain position. One of those tokens it checks is the close brace `}`. However, to my understanding, it is not possible for a let chain to be preceded by a close brace. This PR removes the check to avoid any confusion. This is a followup to the discussion at rust-lang#98633 (review). It wasn't clear what issues the original PR ran into, but I have run the full set of CI tests and nothing failed. I also can't conceive of a situation where this would be possible. This doesn't reject any valid code, I'm just removing it to avoid confusion to anyone looking at this code in the future.
In order to forbid things like
let x = (let y = 1);orif let a = 1 && { let x = let y = 1; } {}, the parser HAS to know the context oflet.This context thing is not a surprise in the parser because you can see a lot of ad hoc fixes mixing parsing logic with validation logic creating code that looks more like spaghetti with tomato sauce.
To make things even greater, a new ad hoc fix was added to only allow
lets in a validlet_chainscontext by checking the previously processed token. This was the only solution I could think of and believe me, I thought about it for a long time 👍In the long term, it should be preferable to segregate different responsibilities or create a more robust and cleaner parser framework.
cc #94927
cc #53667