Fix last let_chains blocker

[c410-f3r]

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 lets 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 #94927
cc #53667

[rust-highfive]

r? @petrochenkov

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

[c410-f3r]

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

[estebank]

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.

[c410-f3r]

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

	/// A let chain with invalid parentheses
	///
	/// For exemple, `let 1 = 1 && (expr && expr)` is allowed
	/// but `(let 1 = 1 && (let 1 = 1 && (let 1 = 1))) && let a = 1` is not
	NotSupportedParentheses(Span),

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?

https://github.com/rust-lang/rust/blob/16b3d404d2267972d770e239c6e7ac68628f7a4a/src/test/ui/rfc-2497-if-let-chains/disallowed-positions.rs#L368

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.

#94927 (comment)
#94927 (comment)

[estebank]

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 }, but due to this being in the worst case overly restrictive, I think we can land this until we refactor this codepath.

@bors r+

[bors]

📌 Commit 9d2a9d9 has been approved by estebank

It is now in the queue for this repository.

[bors]

☔ The latest upstream changes (presumably #98482) made this pull request unmergeable. Please resolve the merge conflicts.

[c410-f3r]

@rustbot label +S-waiting-on-review -S-waiting-on-author

[c410-f3r]

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 r+ again to unblock let_chains.

[tmiasko]

@bors r=estebank

[bors]

📌 Commit 1c3bab2 has been approved by estebank

It is now in the queue for this repository.

[c410-f3r]

Thank you