Improve invalid let expression handling #115677

matthewjasper · 2023-09-08T15:23:32Z

Move all of the checks for valid let expression positions to parsing.
Add a field to ExprKind::Let in AST/HIR to mark whether it's in a valid location.
Suppress some later errors and MIR construction for invalid let expressions.
Fix a (drop) scope issue that was also responsible for ICE "region scope ... not in scope to drop" #104172.

Fixes #104172
Fixes #104868

rustbot · 2023-09-08T15:23:40Z

r? @b-naber

(rustbot has picked a reviewer for you, use r? to override)

rustbot · 2023-09-08T16:14:57Z

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

Some changes occurred in src/tools/rustfmt

cc @rust-lang/rustfmt

These scopes would not exist in MIR and can cause ICEs with invalid uses of let expressions.

There was an incomplete version of the check in parsing and a second version in AST validation. This meant that some, but not all, invalid uses were allowed inside macros/disabled cfgs. It also means that later passes have a hard time knowing when the let expression is in a valid location, sometimes causing ICEs. - Add a field to ExprKind::Let in AST/HIR to mark whether it's in a valid location. - Suppress later errors and MIR construction for invalid let expressions.

Previously some invalid let expressions would result in both a feature error and a parsing error. Avoid this and ensure that we only emit the parsing error when this happens.

b-naber · 2023-09-12T19:15:22Z

compiler/rustc_parse/src/parser/expr.rs

                } else {
-                    self.parse_expr_closure()
+                    this.parse_expr_lit()


Why is this change necessary?

The diff isn't showing the changes here very well (maybe due to the extra indent). There isn't actually a change in the methods being called here.

b-naber · 2023-09-12T19:28:55Z

compiler/rustc_parse/src/parser/expr.rs

+#[derive(Clone, Copy, Subdiagnostic)]
+pub(crate) enum ForbiddenLetReason {
+    /// `let` is not valid and the source environment is not important
+    GenericForbidden,


The generic in here is slightly confusing, can we maybe use another name here?

b-naber · 2023-09-12T19:30:49Z

compiler/rustc_parse/src/parser/expr.rs

+    NotSupportedParentheses(#[primary_span] Span),
+}
+
+struct CondChecker<'a> {


A comment for this struct would be helpful, I think.

b-naber · 2023-09-12T19:42:16Z

tests/ui/expr/if/bad-if-let-suggestion.stderr

  --> $DIR/bad-if-let-suggestion.rs:5:8
   |
 LL |     if let x = 1 && i = 2 {}
   |        ^^^^^^^^^
-   |
-   = note: only supported directly in conditions of `if` and `while` expressions


Not having this note is a slight regression imo.

- Add doc comment to new type - Restore "only supported directly in conditions of `if` and `while` expressions" note - Rename variant with clearer name

b-naber · 2023-09-13T19:49:26Z

@bors r+

bors · 2023-09-13T19:49:28Z

📌 Commit e324a59 has been approved by b-naber

It is now in the queue for this repository.

bors · 2023-09-14T02:52:47Z

⌛ Testing commit e324a59 with merge 9d21092...

…-naber Improve invalid let expression handling - Move all of the checks for valid let expression positions to parsing. - Add a field to ExprKind::Let in AST/HIR to mark whether it's in a valid location. - Suppress some later errors and MIR construction for invalid let expressions. - Fix a (drop) scope issue that was also responsible for rust-lang#104172. Fixes rust-lang#104172 Fixes rust-lang#104868

bors · 2023-09-14T04:41:01Z

☀️ Test successful - checks-actions
Approved by: b-naber
Pushing 9d21092 to master...

bors · 2023-09-14T04:42:01Z

👀 Test was successful, but fast-forwarding failed: 422 Update is not a fast forward

rust-timer · 2023-09-14T07:20:22Z

Finished benchmarking commit (9d21092): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.3%	[0.3%, 0.3%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.6%	[0.6%, 0.6%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.6%	[0.6%, 0.6%]	1

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 631.556s -> 632.444s (0.14%)
Artifact size: 317.95 MiB -> 317.98 MiB (0.01%)

b-naber · 2023-09-14T19:56:49Z

@bors retry

bors · 2023-09-14T19:56:59Z

⌛ Testing commit e324a59 with merge dac91a8...

bors · 2023-09-14T21:47:41Z

☀️ Test successful - checks-actions
Approved by: b-naber
Pushing dac91a8 to master...

rust-timer · 2023-09-15T03:03:16Z

Finished benchmarking commit (dac91a8): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.3%	[0.2%, 0.3%]	4
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	4.2%	[4.2%, 4.2%]	1
Regressions ❌ (secondary)	1.0%	[0.7%, 1.3%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	4.2%	[4.2%, 4.2%]	1

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 631.307s -> 631.195s (-0.02%)
Artifact size: 317.98 MiB -> 318.01 MiB (0.01%)

…-naber Improve invalid let expression handling - Move all of the checks for valid let expression positions to parsing. - Add a field to ExprKind::Let in AST/HIR to mark whether it's in a valid location. - Suppress some later errors and MIR construction for invalid let expressions. - Fix a (drop) scope issue that was also responsible for rust-lang#104172. Fixes rust-lang#104172 Fixes rust-lang#104868

…compiler-errors Additional tests to ensure let is rejected during parsing In the original stabilization PR, @ `compiler-errors` has [pointed out](rust-lang#94927 (comment)) that rust-lang#97295 wasn't enough to address the concerns about having `let` in expressions being rejected at parsing time, instead of later. Thankfully, since then the situation has been greatly improved by rust-lang#115677. This PR adds some additional tests to `disallowed-positions.rs`, and adds two additional revisions to the "normal" case which is now given the `feature` name: * `no_feature`: Added to incorporate `disallowed-positions-without-feature-gate.rs` into the file, reducing duplication. * `nothing`: like feature, but all functions are cfg'd out. Ensures that the errors are really emitted during parsing. cc tracking issue rust-lang#53667

Rollup merge of rust-lang#132828 - est31:let_chains_parsing_tests, r=compiler-errors Additional tests to ensure let is rejected during parsing In the original stabilization PR, @ `compiler-errors` has [pointed out](rust-lang#94927 (comment)) that rust-lang#97295 wasn't enough to address the concerns about having `let` in expressions being rejected at parsing time, instead of later. Thankfully, since then the situation has been greatly improved by rust-lang#115677. This PR adds some additional tests to `disallowed-positions.rs`, and adds two additional revisions to the "normal" case which is now given the `feature` name: * `no_feature`: Added to incorporate `disallowed-positions-without-feature-gate.rs` into the file, reducing duplication. * `nothing`: like feature, but all functions are cfg'd out. Ensures that the errors are really emitted during parsing. cc tracking issue rust-lang#53667

rustbot assigned b-naber Sep 8, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Sep 8, 2023

This comment has been minimized.

Sign in to view

matthewjasper force-pushed the let-expr-recovery branch from 7964b11 to c96c81f Compare September 11, 2023 08:27

matthewjasper added 4 commits September 11, 2023 15:51

Don't create drop scopes after item statements

7b61f7f

These scopes would not exist in MIR and can cause ICEs with invalid uses of let expressions.

Update tools and fulldeps tests

2d7a5f5

Reduce double errors for invalid let expressions

b011a0a

Previously some invalid let expressions would result in both a feature error and a parsing error. Avoid this and ensure that we only emit the parsing error when this happens.

matthewjasper force-pushed the let-expr-recovery branch from c96c81f to b011a0a Compare September 11, 2023 16:22

b-naber reviewed Sep 12, 2023

View reviewed changes

Address review comments

e324a59

- Add doc comment to new type - Restore "only supported directly in conditions of `if` and `while` expressions" note - Rename variant with clearer name

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 Sep 13, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 14, 2023

bors merged commit dac91a8 into rust-lang:master Sep 14, 2023
11 checks passed

rustbot added this to the 1.74.0 milestone Sep 14, 2023

matthewjasper deleted the let-expr-recovery branch September 18, 2023 08:30

Zalathar mentioned this pull request Jul 1, 2024

Replace some magic booleans in match-lowering with enums #126981

Merged

matthewjasper added F-if_let_guard `#![feature(if_let_guard)]` F-let_chains `#![feature(let_chains)]` labels Nov 7, 2024

est31 mentioned this pull request Nov 9, 2024

Additional tests to ensure let is rejected during parsing #132828

Merged

est31 mentioned this pull request Nov 10, 2024

Stabilize let chains in the 2024 edition #132833

Open

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve invalid let expression handling #115677

Improve invalid let expression handling #115677

matthewjasper commented Sep 8, 2023 •

edited

Loading

rustbot commented Sep 8, 2023

This comment has been minimized.

rustbot commented Sep 8, 2023

This comment has been minimized.

b-naber Sep 12, 2023

matthewjasper Sep 13, 2023

b-naber Sep 12, 2023

b-naber Sep 12, 2023

b-naber Sep 12, 2023

b-naber commented Sep 13, 2023

bors commented Sep 13, 2023

bors commented Sep 14, 2023

bors commented Sep 14, 2023

bors commented Sep 14, 2023

rust-timer commented Sep 14, 2023

b-naber commented Sep 14, 2023

bors commented Sep 14, 2023

bors commented Sep 14, 2023

rust-timer commented Sep 15, 2023

Improve invalid let expression handling #115677

Improve invalid let expression handling #115677

Conversation

matthewjasper commented Sep 8, 2023 • edited Loading

rustbot commented Sep 8, 2023

This comment has been minimized.

rustbot commented Sep 8, 2023

This comment has been minimized.

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

b-naber commented Sep 13, 2023

bors commented Sep 13, 2023

bors commented Sep 14, 2023

bors commented Sep 14, 2023

bors commented Sep 14, 2023

rust-timer commented Sep 14, 2023

Overall result: ❌ regressions - no action needed

b-naber commented Sep 14, 2023

bors commented Sep 14, 2023

bors commented Sep 14, 2023

rust-timer commented Sep 15, 2023

Overall result: ❌ regressions - no action needed

matthewjasper commented Sep 8, 2023 •

edited

Loading