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

match_same_arms fix #8232

Merged
merged 5 commits into from
Mar 21, 2022
Merged

match_same_arms fix #8232

merged 5 commits into from
Mar 21, 2022

Conversation

Jarcho
Copy link
Contributor

@Jarcho Jarcho commented Jan 6, 2022

fixes #860
fixes #1140

changelog: Don't lint match_same_arms when an interposing arm's pattern would overlap

@rust-highfive
Copy link

r? @xFrednet

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jan 6, 2022
@camsteffen
Copy link
Contributor

Could we use rust-lang/rust#89570 when that is ready?

@Jarcho
Copy link
Contributor Author

Jarcho commented Jan 6, 2022

A quick look says no. Two things that are different

  1. Exhaustiveness checking is comparing patterns checking if there is anything which doesn't overlap. This is checking if there is anything which does overlap.
  2. Exhaustiveness checking does a rolling comparison (p1 with p2, p1+p2 with p3...). This needs a full pairwise comparison to work.

@Jarcho
Copy link
Contributor Author

Jarcho commented Jan 6, 2022

Basically done now. Just need to clarify the message for which direction the patterns need to be merged. In some cases it can only be merged in one direction. e.g.

match foo {
    Foo::A(0) => x, // can be Foo::A(0) | Foo::B(0)
    Foo::A(_) => y,
    Foo::B(0) => x, // can't merge Foo::A(0) here
}

If Foo::A(_) were changed to Foo::B(_) the merge would have to go in the opposite direction. If it were instead Foo::C then either change would work. I'm not really sure the best way to display that in the message.

@bors
Copy link
Collaborator

bors commented Jan 19, 2022

☔ The latest upstream changes (presumably #8310) made this pull request unmergeable. Please resolve the merge conflicts.

@Jarcho Jarcho force-pushed the match_same_arm_860 branch 3 times, most recently from a9a086a to 13fd7ac Compare January 22, 2022 03:07
@xFrednet
Copy link
Member

Hey @Jarcho, this PR is on my todo list but sadly a bit too big to review on the side (That's why I haven't left a review yet). I'll be free in about two weeks, would it be alright if I keep this in my backlog until then? Otherwise, I can also see if someone else wants to review this 🙃

@Jarcho
Copy link
Contributor Author

Jarcho commented Jan 25, 2022

The issue's been open for more than five years. I think it can wait a couple weeks. Now get back to your thesis.

@xFrednet
Copy link
Member

😂 Will do, thanks! 🙃

@bors
Copy link
Collaborator

bors commented Jan 30, 2022

☔ The latest upstream changes (presumably #8322) made this pull request unmergeable. Please resolve the merge conflicts.

@Jarcho Jarcho changed the title WIP match_same_arms fix match_same_arms fix Feb 18, 2022
Copy link
Member

@xFrednet xFrednet left a comment

Choose a reason for hiding this comment

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

Alright, I've looked through the implementation and everything looks good to me. I've marked some smaller nits but nothing major. I still have to go through the tests and test output. But you can rebase if you want.

Thank you very much for giving me the time to write on my thesis. It took quite some work, But I'm happy with the result.

clippy_lints/src/matches.rs Outdated Show resolved Hide resolved
clippy_lints/src/matches.rs Outdated Show resolved Hide resolved
tests/ui/match_same_arms2.rs Outdated Show resolved Hide resolved
clippy_lints/src/matches.rs Outdated Show resolved Hide resolved
clippy_lints/src/matches.rs Outdated Show resolved Hide resolved
Copy link
Member

@xFrednet xFrednet left a comment

Choose a reason for hiding this comment

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

Okay, I've also looked through the test and the output looks really clean 👍. This should be ready for merging after a rebase and after the comments have been addressed 🙃

@xFrednet
Copy link
Member

Hey @Jarcho, this is a ping from triage. Do you plan to continue working on this PR? The direction and changes still look good to me, besides the NITs 🙃

@Jarcho
Copy link
Contributor Author

Jarcho commented Mar 16, 2022

Still need to look into your last point. It's on the list of things to do.

@Jarcho Jarcho force-pushed the match_same_arm_860 branch 5 times, most recently from 916365c to b51f3c1 Compare March 17, 2022 05:07
@xFrednet
Copy link
Member

Looks good to me! Thank you for the refactoring. I've also tested this implementation with lintcheck and everything looks good. 👍 💪

@bors r+

@bors
Copy link
Collaborator

bors commented Mar 21, 2022

📌 Commit 773d203 has been approved by xFrednet

@bors
Copy link
Collaborator

bors commented Mar 21, 2022

⌛ Testing commit 773d203 with merge f07ee8a...

@bors
Copy link
Collaborator

bors commented Mar 21, 2022

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

@bors bors merged commit f07ee8a into rust-lang:master Mar 21, 2022
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.

match_same_arms: suppress when reordering arms? False positive on match_same_arms
5 participants