Detect missing else in let statement

[bb1yd]

View all comments

Fixes #135857

I add the help message in rustc_resolve. It now will display something like this:

error: expected identifier, found keyword `return`
 --> ./test.rs:8:24
  |
8 |      let Some(a) = bar{return;};
  |                    --- ^^^^^^ expected identifier, found keyword
  |                    |
  |                    while parsing this struct
  |
help: escape `return` to use it as an identifier
  |
8 |      let Some(a) = bar{r#return;};
  |                        ++

error[E0574]: expected struct, variant or union type, found local variable `bar`
 --> ./test.rs:8:20
  |
8 |      let Some(a) = bar{return;};
  |                    ^^^ not a struct, variant or union type
  |
help: try adding `else` here:
  |
8 |      let Some(a) = bar else {return;};
  |                        ++++

error: aborting due to 2 previous errors

For more information about this error, try `rustc --explain E0574`.

r? estebank

[bb1yd]

@bors squash msg="fix issue-156949"

[rust-bors]

🔨 4 commits were squashed into 6bbafec.

[estebank]

Thank you for taking this on! I have some nitpicks for things that we should touch-up, but I think you're in the right direction.

View changes since this review

[bb1yd]

@estebank Fixed

[bb1yd]

@rustbot author

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[rustbot]

The parser was modified, potentially altering the grammar of (stable) Rust
which would be a breaking change.

cc @fmease

[bb1yd]

I change the code to rustc_parse and it now only emit one error:

error: expected identifier, found keyword `return`
  --> $DIR/detect-missing-else-in-let-1.rs:4:23
   |
LL |     let Some(a) = foo{return;};
   |                   --- ^^^^^^ expected identifier, found keyword
   |                   |
   |                   while parsing this struct
   |
help: escape `return` to use it as an identifier
   |
LL |     let Some(a) = foo{r#return;};
   |                       ++
help: you might have meant to write a diverging block on a refutable `let` statement by using `let-else`
      for more information, visit <https://doc.rust-lang.org/beta/rust-by-example/flow_control/let_else.html>
   |
LL |     let Some(a) = foo else {return;};
   |                       ++++

error: aborting due to 1 previous error

@rustbot ready

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[estebank]

This doesn't feel quite right. You're recovering the current statement, so if we had foo { return 42; }, it would recover, but I feel like (given that we have a snapshot), we should instead look how we parse the block in let pat = expr else { block };, and call whatever method we call for it.

I'm not entirely sure what we've advanced in each step above. I would write this like this:

// Given `foo { return 42; };`
let mut snapshot = self.create_snapshot_for_diagnostic(); // We've consumed `foo {` already
let might_be_stmt = snapshot.token.is_keyword(kw::Return); // `return`
snapshot.recover_stmt(); // consume `return 42;` 
snapshot.bump(); // `}`
if might_be_stmt && snapshot.eat(exp!(Semi)) {

Maybe instead of snapshot.recover_stmt() we should do consume_block(), which will eat everything within the foo { .. }, regardless of what it is.

View changes since the review

[bb1yd]

recover_stmt actually behaves the same as consume_block which consumes everything until reaching } (except we can let consume_block to consume the closing delim), but yes, I think consume_block is better since it makes the intent clearer.

[bb1yd]

I actually tried to use parse_block in my first attempt to detect whether the struct field is actually a block, so the compiler will emit the diagnostic for any valid block (currently we only accept single return). If parse_block fails we cancel the error produced by it and skip this diagnostics, but parse_block will try to internally recover instead of returning an error for us to cancel even when I set Recovery to No .

That's why I choose to use is_keyword.🙁

Is this considered a bug that (maybe) a lot of places does not respect Recovery field, or it's just not how Recovery field is used for?

[bb1yd]

@estebank Fixed