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

chore: use multipart_suggestions for match_same_arms #13803

Merged
merged 1 commit into from
Dec 15, 2024

Conversation

scottgerring
Copy link
Contributor

This addresses #13099 for the match_same_arms lint.

changelog: [match_same_arms]: Updated match_same_arms to use multipart_suggestions where appropriate

@rustbot
Copy link
Collaborator

rustbot commented Dec 9, 2024

r? @blyxyas

rustbot has assigned @blyxyas.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Dec 9, 2024
@scottgerring scottgerring force-pushed the chore/fix-match-same-arms branch from 9ac0f8c to 7d1e0da Compare December 9, 2024 16:46
@scottgerring scottgerring marked this pull request as ready for review December 9, 2024 16:58
Copy link
Member

@blyxyas blyxyas left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! ❤️

I'm wondering if we could also remove the @no-rustfix from match_same_arms.rs, now that rust-lang/cargo#14747 is merged. Could you test that? (Different issue, but I figure that it's related enough.)

@scottgerring
Copy link
Contributor Author

Hey @blyxyas thanks for taking a look!
Naively removing @rust-nofix from match_same_arms.rs causes an issue with this part of the test:

let _ = match 42 {
1 => 2,
2 => 2, //~ ERROR: this match arm has an identical body to another arm
//~^ ERROR: this match arm has an identical body to another arm
3 => 2, //~ ERROR: this match arm has an identical body to another arm
4 => 3,
_ => 0,
};

We generate two suggestions - one to combine 1 and 2, and one to combine 2 and 3. These overlap and the second application fails. It looks like the issue is that we're pairwise matching arms up, rather than collecting all arms that have the same body and suggesting an aggregate fix:

for (&(i, arm1), &(j, arm2)) in search_same(&indexed_arms, hash, eq) {
if matches!(arm2.pat.kind, PatKind::Wild) {

I'm happy to fix this but feel like it's probably "removed enough" from the multipart_suggestion to throw another issue in and fix it in a separate PR. What do you think?

@blyxyas
Copy link
Member

blyxyas commented Dec 15, 2024

I agree with you, fixing that would count as another issue, and opening another PR instead of making this one bigger would help with atomic PRs.

Copy link
Member

@blyxyas blyxyas left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! ❤️

@blyxyas blyxyas added this pull request to the merge queue Dec 15, 2024
Merged via the queue into rust-lang:master with commit b8e569e Dec 15, 2024
9 checks passed
@scottgerring scottgerring deleted the chore/fix-match-same-arms branch December 16, 2024 08:33
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.

3 participants