Catch stray `{` in let-chains #117770

sjwang05 · 2023-11-10T02:50:24Z

rustbot · 2023-11-10T02:50:33Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @TaKO8Ki (or someone else) soon.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

@rustbot author: the review is finished, PR author should check the comments and take action accordingly
@rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

estebank · 2023-11-10T21:59:29Z

compiler/rustc_parse/src/lexer/tokentrees.rs

+                } else if parser.is_keyword_ahead(0, &[kw::If, kw::While]) {
+                    in_cond = true;
+                } else if matches!(
+                    parser.token.kind,
+                    token::CloseDelim(Delimiter::Brace) | token::FatArrow
+                ) {
+                    // end of the `if`/`while` body, or the end of a `match` guard
+                    in_cond = false;
+                } else if in_cond && parser.token == token::OpenDelim(Delimiter::Brace) {
+                    // Store the `&&` and `let` to use their spans later when creating the diagnostic
+                    let maybe_andand = parser.look_ahead(1, |t| t.clone());
+                    let maybe_let = parser.look_ahead(2, |t| t.clone());
+                    if maybe_andand == token::OpenDelim(Delimiter::Brace) {
+                        // This might be the beginning of the `if`/`while` body (i.e., the end of the condition)
+                        in_cond = false;
+                    } else if maybe_andand == token::AndAnd && maybe_let.is_keyword(kw::Let) {
+                        let mut err = parser.struct_span_err(
+                            parser.token.span,
+                            "found a `{` in the middle of a let-chain",
+                        );
+                        err.span_suggestion(
+                            parser.token.span,
+                            "consider removing this brace to parse the `let` as part of the same chain",
+                            "", Applicability::MachineApplicable
+                        );
+                        err.span_note(
+                            maybe_andand.span.to(maybe_let.span),
+                            "you might have meant to continue the let-chain here",
+                        );
+                        errs.push(err);
+                    }


Let's move this logic to a separate method to make it less difficult to read the happy path. Either the new logic or the whole parser loop.

estebank · 2023-11-10T21:59:56Z

compiler/rustc_parse/src/lexer/tokentrees.rs

+                            "consider removing this brace to parse the `let` as part of the same chain",
+                            "", Applicability::MachineApplicable
+                        );
+                        err.span_note(


Suggested change

err.span_note(

err.span_label(

estebank

The approach is quite smart! Thank you for taking this on.

estebank · 2023-11-11T17:16:55Z

tests/ui/parser/brace-in-let-chain.stderr

+error: this file contains an unclosed delimiter
+  --> $DIR/brace-in-let-chain.rs:58:54
+   |
+LL | fn main() {
+   |           - unclosed delimiter
+...
+LL | fn quux() {
+   |           - unclosed delimiter
+...
+LL | fn foobar() {
+   |             - unclosed delimiter
+...
+LL | fn fubar() {
+   |            - unclosed delimiter
+...
+LL | fn qux() {
+   |          - unclosed delimiter
+...
+LL | fn foo() {
+   |          - unclosed delimiter
+...
+LL | fn bar() {
+   |          - unclosed delimiter
+...
+LL | fn baz() {
+   |          - unclosed delimiter
+LL |     if false {
+LL |         {
+   |         - this delimiter might not be properly closed...
+LL |             && let () = ()
+LL |     }
+   |     - ...as it matches this but it has different indentation
+LL | }
+   |                                                      ^


I'm trying to decide if it's better in general to silence this error (like we do for diff markers) or not is best for these cases... I would leave it as is for now, because I don't know if I can come up with a case where doing so would be detrimental, but suspect we can silence it.

sjwang05 · 2023-11-11T22:32:59Z

Thanks for the feedback! I've moved all error handling logic into its separate method and replaced the note with a label as suggested.

I'm trying to decide if it's better in general to silence this error (like we do for diff markers) or not is best for these cases

I just tried doing that, and strangely enough, all "found a { in the middle of a let-chain" errors except the first get silenced as well:

error: found a `{` in the middle of a let-chain
  --> $DIR/brace-in-let-chain.rs:6:24
   |
LL |         && let () = () {
   |                        ^
LL |         && let () = ()
   |         ------ you might have meant to continue the let-chain here
   |
help: consider removing this brace to parse the `let` as part of the same chain
   |
LL -         && let () = () {
LL +         && let () = ()
   |

error: aborting due to previous error

We should be getting 2 errors here, but we're only getting one 😕.

FWIW, here's what the silenced stderr would probably look like if I manage to get that problem sorted out:

error: found a `{` in the middle of a let-chain
  --> $DIR/brace-in-let-chain.rs:14:24
   |
LL |         && let () = () {
   |                        ^
LL |         && let () = ()
   |         ------ you might have meant to continue the let-chain here
   |
help: consider removing this brace to parse the `let` as part of the same chain
   |
LL -         && let () = () {
LL +         && let () = ()
   |

error: found a `{` in the middle of a let-chain
  --> $DIR/brace-in-let-chain.rs:6:24
   |
LL |         && let () = () {
   |                        ^
LL |         && let () = ()
   |         ------ you might have meant to continue the let-chain here
   |
help: consider removing this brace to parse the `let` as part of the same chain
   |
LL -         && let () = () {
LL +         && let () = ()
   |

error: aborting due to 2 previous errors

Though I feel like it's definitely easier to read, the unclosed delimiter errors aren't necessarily wrong like they could be with diff markers, so silencing them might not be as helpful. The ideal solution is probably something like the stderr you suggested in the original issue, where the label is printed inline with the unclosed delim error, though I have no clue how I'd go about doing that--the only way I can think of is modifying the errors in report_suspicious_mismatch_block, which would require somehow pairing up each erroneous let-chain with the correct {.

TaKO8Ki

Thank you for the great work! I left one suggestion.

compiler/rustc_parse/src/parser/mod.rs

Co-authored-by: Takayuki Maeda <takoyaki0316@gmail.com>

estebank · 2023-11-13T01:56:50Z

@bors r=estebank,TaKO8Ki

bors · 2023-11-13T01:56:53Z

📌 Commit 274824b has been approved by estebank,TaKO8Ki

It is now in the queue for this repository.

bors · 2023-11-13T01:58:02Z

⌛ Testing commit 274824b with merge ea1e5cc...

bors · 2023-11-13T04:06:52Z

☀️ Test successful - checks-actions
Approved by: estebank,TaKO8Ki
Pushing ea1e5cc to master...

rust-timer · 2023-11-13T07:53:59Z

Finished benchmarking commit (ea1e5cc): comparison URL.

Overall result: ✅ improvements - 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
Improvements ✅ (primary)	-0.6%	[-0.8%, -0.3%]	2
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.6%	[-0.8%, -0.3%]	2

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)	3.8%	[3.8%, 3.8%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.4%	[-0.4%, -0.4%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.7%	[-0.4%, 3.8%]	2

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.8%	[0.6%, 0.9%]	2
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.7%	[-0.7%, -0.7%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.3%	[-0.7%, 0.9%]	3

Binary size

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

Bootstrap: 673.749s -> 674.078s (0.05%)
Artifact size: 311.08 MiB -> 311.07 MiB (-0.00%)

Catch stray { in let-chains

0094238

rustbot assigned TaKO8Ki Nov 10, 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 Nov 10, 2023

sjwang05 added 2 commits November 9, 2023 20:07

Catch an edge case

9455259

Correctly handle while-let-chains

a49368f

estebank reviewed Nov 10, 2023

View reviewed changes

estebank approved these changes Nov 10, 2023

View reviewed changes

estebank reviewed Nov 11, 2023

View reviewed changes

Move unclosed delim errors to separate function

f88cf02

TaKO8Ki reviewed Nov 12, 2023

View reviewed changes

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

Fix is_keyword_ahead visibility

274824b

Co-authored-by: Takayuki Maeda <takoyaki0316@gmail.com>

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

bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 13, 2023

bors merged commit ea1e5cc into rust-lang:master Nov 13, 2023
12 checks passed

rustbot added this to the 1.76.0 milestone Nov 13, 2023

sjwang05 deleted the issue-117766 branch January 16, 2024 03:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Catch stray `{` in let-chains #117770

Catch stray `{` in let-chains #117770

sjwang05 commented Nov 10, 2023

rustbot commented Nov 10, 2023

estebank Nov 10, 2023

estebank Nov 10, 2023

estebank left a comment

estebank Nov 11, 2023

sjwang05 commented Nov 11, 2023 •

edited

TaKO8Ki left a comment

estebank commented Nov 13, 2023

bors commented Nov 13, 2023

bors commented Nov 13, 2023

bors commented Nov 13, 2023

rust-timer commented Nov 13, 2023

Catch stray { in let-chains #117770

Catch stray { in let-chains #117770

Conversation

sjwang05 commented Nov 10, 2023

rustbot commented Nov 10, 2023

estebank Nov 10, 2023

Choose a reason for hiding this comment

estebank Nov 10, 2023

Choose a reason for hiding this comment

estebank left a comment

Choose a reason for hiding this comment

estebank Nov 11, 2023

Choose a reason for hiding this comment

sjwang05 commented Nov 11, 2023 • edited

TaKO8Ki left a comment

Choose a reason for hiding this comment

estebank commented Nov 13, 2023

bors commented Nov 13, 2023

bors commented Nov 13, 2023

bors commented Nov 13, 2023

rust-timer commented Nov 13, 2023

Overall result: ✅ improvements - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size

Catch stray `{` in let-chains #117770

Catch stray `{` in let-chains #117770

sjwang05 commented Nov 11, 2023 •

edited