Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Recover from accidental inclusion of if in let else #103791

Closed
Rageking8 opened this issue Oct 31, 2022 · 10 comments · Fixed by #107213
Closed

Recover from accidental inclusion of if in let else #103791

Rageking8 opened this issue Oct 31, 2022 · 10 comments · Fixed by #107213
Assignees
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-parser Area: The parsing of Rust source code to an AST D-confusing Diagnostics: Confusing error or lint that should be reworked. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. F-let_else Issues related to let-else statements (RFC 3137) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@Rageking8
Copy link
Contributor

Rageking8 commented Oct 31, 2022

Given the following code: link

fn main() {
    let x = Some(123);
    if let Some(y) = x else {
        return;
    };
}

The current output is:

   Compiling playground v0.0.1 (/playground)
error: this `if` expression is missing a block after the condition
 --> src/main.rs:3:5
  |
3 |     if let Some(y) = x else {
  |     ^^
  |
help: add a block here
 --> src/main.rs:3:23
  |
3 |     if let Some(y) = x else {
  |                       ^

error: could not compile `playground` due to previous error

Ideally the output should suggest removing the additional if keyword in front of the let else. This mistake can occur due to muscle memory as if, if let, let else and in the future let chains are somewhat similar syntactically. Thanks.

@rustbot label +D-confusing +F-let-else

@Rageking8 Rageking8 added A-diagnostics Area: Messages for errors, warnings, and lints T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 31, 2022
@rustbot rustbot added D-confusing Diagnostics: Confusing error or lint that should be reworked. F-let_else Issues related to let-else statements (RFC 3137) labels Oct 31, 2022
@faptc
Copy link

faptc commented Oct 31, 2022

I think the current suggestion is valid.
Because one writes let y = Some(0); if let Some(x) = y else {} instead of
writing if let x = y else {}.

So the suggestion if changed maybe fall-negative.

@estebank
Copy link
Contributor

We should likely add an alternative suggestion to remove the if, but keep both. I think that the current suggestion is likely going to be the more common situation, so it should still come first.

@estebank estebank added E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. A-parser Area: The parsing of Rust source code to an AST labels Oct 31, 2022
@ffmancera
Copy link
Contributor

@rustbot claim

@hgrahamcs
Copy link

What would be the best way to go about adding an alternate suggestion? I'd like to try my hand at this.

What I think needs to be done is adding help text to compiler/rustc_error_messages/locales/en-US/parser.ftl,
add a variant to compiler/rustc_parse/src/errors.rs IfExpressionMissingThenBlockSub,
and then add something to compiler/rustc_parse/src/parser/expr.rs in recover_block_from_condition to add the alternate after the primary suggestion.

Is this a good starting point?

@estebank
Copy link
Contributor

@hgrahamcs it sounds like you are in the right path.

@HintringerFabian
Copy link
Contributor

HintringerFabian commented Nov 20, 2022

I looked a bit into this issue and have already found where to add text to the parser.ftl and the errors.rs as @hgrahamcs mentioned, but i am a bit overwhelmed how and where to start in the recover_block_from_condition in the expr.rs.

Does anyone have a hint for me?

@HintringerFabian
Copy link
Contributor

@rustbot claim

@rustbot rustbot assigned HintringerFabian and unassigned ffmancera Dec 1, 2022
@HintringerFabian HintringerFabian removed their assignment Dec 7, 2022
@sladyn98
Copy link
Contributor

@rustbot claim

@estebank
Copy link
Contributor

but i am a bit overwhelmed how and where to start in the recover_block_from_condition in the expr.rs.

You can add info!(?binding) instructions in the parser to track your progress.

I'm not sure about the specifics out of the top of my head here of what type each step in the parse is, but you will have successfully parsed the if, the condition and then you'll encounter an else token when a body/block was expected. That's where the new recovery should kick in, pointing at the if span saying that you might want to remove it.

@edward-shen
Copy link
Contributor

@rustbot claim

@rustbot rustbot assigned edward-shen and unassigned sladyn98 Jan 23, 2023
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Jan 24, 2023
…ntal-let-else, r=compiler-errors

Add suggestion to remove if in let..else block

Adds an additional hint to failures where we encounter an else keyword while we're parsing an if-let expression.

This is likely that the user has accidentally mixed if-let and let..else together.

Fixes rust-lang#103791.
@bors bors closed this as completed in 4e2b5d1 Jan 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-parser Area: The parsing of Rust source code to an AST D-confusing Diagnostics: Confusing error or lint that should be reworked. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. F-let_else Issues related to let-else statements (RFC 3137) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants