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

Add suggestion to remove if in let..else block #107213

Merged

Conversation

edward-shen
Copy link
Contributor

@edward-shen edward-shen commented Jan 23, 2023

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 #103791.

@rustbot
Copy link
Collaborator

rustbot commented Jan 23, 2023

r? @eholk

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

@rustbot rustbot added A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic 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 Jan 23, 2023
@rustbot
Copy link
Collaborator

rustbot commented Jan 23, 2023

rustc_error_messages was changed

cc @davidtwco, @compiler-errors, @JohnTitor, @estebank, @TaKO8Ki

Comment on lines 40 to 44
help: There may be an extra `if` before the `let...else`
--> $DIR/ensure-that-let-else-does-not-interact-with-let-chains.rs:24:5
|
LL | if let Some(n) = opt else {
| ^^
Copy link
Contributor Author

@edward-shen edward-shen Jan 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Related test source code: rfc-2497-if-let-chains/ensure-that-let-else-does-not-interact-with-let-chains.rs.

This behavior is correct as this hint should only appear when it is ambiguous if the user is try to use an if-let or an let..else.

The other two cases that this test handles are:

    if let Some(n) = opt && n == 1 else {
    //~^ ERROR this `if` expression is missing a block after the condition
        return;
    };
    if let Some(n) = opt && let another = n else {
    //~^ ERROR this `if` expression is missing a block after the condition
        return;
    };

which are less ambiguous.

This also indicates that we don't emit this help on while-let -- this wasn't in scope of the original issue so I took a conservative approach in this PR.

@compiler-errors
Copy link
Member

I can take a look at this tomorrow.

r? @compiler-errors

@rustbot rustbot assigned compiler-errors and unassigned eholk Jan 23, 2023
@edward-shen edward-shen force-pushed the edward-shen/fix-accidental-let-else branch from 0a13106 to e2ea394 Compare January 23, 2023 08:58
@rustbot
Copy link
Collaborator

rustbot commented Jan 23, 2023

rustc_error_messages was changed

cc @davidtwco, @compiler-errors, @JohnTitor, @estebank, @TaKO8Ki

@edward-shen edward-shen force-pushed the edward-shen/fix-accidental-let-else branch 2 times, most recently from 312358b to f76990e Compare January 23, 2023 18:13
Copy link
Member

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks pretty good to me as is. Please address nits (or tell me why I'm wrong 😆) and I think this can get r+'d soon.

compiler/rustc_parse/src/errors.rs Outdated Show resolved Hide resolved
compiler/rustc_error_messages/locales/en-US/parse.ftl Outdated Show resolved Hide resolved
@edward-shen edward-shen force-pushed the edward-shen/fix-accidental-let-else branch from f76990e to e6d7264 Compare January 24, 2023 04:21
@edward-shen edward-shen force-pushed the edward-shen/fix-accidental-let-else branch from e6d7264 to 94a5325 Compare January 24, 2023 04:31
Adds an additional hint to failures where we encounter an else keyword
while we're parsing an if-let block.

This is likely that the user has accidentally mixed if-let and let...else
together.
@edward-shen edward-shen force-pushed the edward-shen/fix-accidental-let-else branch from 94a5325 to a8b77cf Compare January 24, 2023 04:33
@compiler-errors
Copy link
Member

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jan 24, 2023

📌 Commit a8b77cf has been approved by compiler-errors

It is now in the queue for this repository.

@bors 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 Jan 24, 2023
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request 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 added a commit to rust-lang-ci/rust that referenced this pull request Jan 25, 2023
Rollup of 9 pull requests

Successful merges:

 - rust-lang#105552 (Add help message about function pointers)
 - rust-lang#106583 (Suggest coercion of `Result` using `?`)
 - rust-lang#106767 (Allow setting CFG_DISABLE_UNSTABLE_FEATURES to 0)
 - rust-lang#106823 (Allow fmt::Arguments::as_str() to return more Some(_).)
 - rust-lang#107166 (rustc_metadata: Support non-`Option` nullable values in metadata tables)
 - rust-lang#107213 (Add suggestion to remove if in let..else block)
 - rust-lang#107223 (`sub_ptr()` is equivalent to `usize::try_from().unwrap_unchecked()`, not `usize::from().unwrap_unchecked()`)
 - rust-lang#107227 (`new_outside_solver` ->  `evaluate_root_goal`)
 - rust-lang#107232 (rustdoc: simplify settings popover DOM, CSS, JS)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 4e2b5d1 into rust-lang:master Jan 25, 2023
@rustbot rustbot added this to the 1.69.0 milestone Jan 25, 2023
@edward-shen edward-shen deleted the edward-shen/fix-accidental-let-else branch January 25, 2023 21:17
IfExpressionMissingThenBlockSub, InvalidBlockMacroSegment, InvalidComparisonOperator,
InvalidComparisonOperatorSub, InvalidInterpolatedExpression, InvalidLiteralSuffixOnTupleIndex,
InvalidLogicalOperator, InvalidLogicalOperatorSub, LabeledLoopInBreak, LeadingPlusNotSupported,
LeftArrowOperator, LifetimeInBorrowExpression, MacroInvocationWithQualifiedPath,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is horrible noise. Can't one instead use crate::errors::ErrorName instead all over the file? I'm asking this as a general pattern.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively, let's replace this with a glob import

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: this is nothing that this particular PR has caused. The pattern of having a giant use at the top just creates this noise in PRs like this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's discuss it on zulip.

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Jan 31, 2023
…nd-2, r=estebank

Make the "extra if in let...else block" hint a suggestion

Changes the hint to a suggestion, suggested in rust-lang#107213.

r? `@estebank`
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Jan 31, 2023
…nd-2, r=estebank

Make the "extra if in let...else block" hint a suggestion

Changes the hint to a suggestion, suggested in rust-lang#107213.

r? ``@estebank``
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 1, 2023
…nd-2, r=estebank

Make the "extra if in let...else block" hint a suggestion

Changes the hint to a suggestion, suggested in rust-lang#107213.

r? ```@estebank```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Recover from accidental inclusion of if in let else
8 participants