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

manual_let_else: do not suggest semantically different replacements #10336

Merged
merged 1 commit into from
Feb 16, 2023

Conversation

samueltardieu
Copy link
Contributor

The problem is that this lint does not consider the possibility that the divergent branch can come first and that the patterns may overlap. This led to incorrect suggestions, previously registered as correct in the tests themselves:

let v = match build_enum() {
    _ => continue,
    Variant::Bar(v) | Variant::Baz(v) => v,
};

had a let Variant::Bar(v) | Variant::Baz(v) = v else { continue; } suggestion, which is obviously wrong as the original code continues in any case. Issue #10241 gives another example.

The code now checks that the divergent branch comes second. It could be extended later (I've added a TODO) to check for non-overlapping patterns.

Fixes #10241.

changelog: [manual_let_else] do not suggest non equivalent replacements in match

@rustbot
Copy link
Collaborator

rustbot commented Feb 12, 2023

r? @dswij

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

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Feb 12, 2023
@samueltardieu
Copy link
Contributor Author

r? @llogiq

@rustbot rustbot assigned llogiq and unassigned dswij Feb 16, 2023
@llogiq
Copy link
Contributor

llogiq commented Feb 16, 2023

Thanks, that makes sense.

@bors r+

@bors
Copy link
Collaborator

bors commented Feb 16, 2023

📌 Commit 09d3097 has been approved by llogiq

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Feb 16, 2023

⌛ Testing commit 09d3097 with merge 6444621...

@bors
Copy link
Collaborator

bors commented Feb 16, 2023

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: llogiq
Pushing 6444621 to master...

@bors bors merged commit 6444621 into rust-lang:master Feb 16, 2023
@samueltardieu samueltardieu deleted the issue-10241 branch February 19, 2023 18:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

manual_let_else suggests invalid replacement
5 participants