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

Refactor clippy::match_ref_pats to check for multiple reference patterns #7800

Merged
merged 2 commits into from
Oct 10, 2021

Conversation

1nF0rmed
Copy link
Contributor

@1nF0rmed 1nF0rmed commented Oct 10, 2021

fixes #7740

When there is only one pattern, to begin with, i.e. a single deref(&), then in such cases we suppress clippy::match_ref_pats.
This is done by checking the count of the reference pattern and emitting clippy::match_ref_pats when more than one pattern is present.

Removed certain errors in the stderr tests as they would not be triggered.

changelog: Refactor clippy::match_ref_pats to check for multiple reference patterns

@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @llogiq (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Oct 10, 2021
@1nF0rmed 1nF0rmed marked this pull request as ready for review October 10, 2021 02:21
@llogiq
Copy link
Contributor

llogiq commented Oct 10, 2021

Cool stuff! The only downside I see is that some test cases are now effectively disabled. Would you be OK with changing them so they apply again and adding one or two new tests that show single ref patterns are not linted?

@1nF0rmed
Copy link
Contributor Author

1nF0rmed commented Oct 10, 2021

@llogiq Thank you! I can update the test cases but if I'm not wrong,
given the change, errors in tests/ui/match_expr_like_matches_macro.stderr will not occur

Let me know if I can approach this differently

@llogiq
Copy link
Contributor

llogiq commented Oct 10, 2021

I'm only talking about the lint's own tests. To make the error re-occur, you may need an enum with more than two cases.

@1nF0rmed
Copy link
Contributor Author

1nF0rmed commented Oct 10, 2021

Given that we want to use an enum with more than two cases, would something like this work?

    fn issue_7740() {
        // Should trigger lint
        match foobar_variant!(0) {
            &FooBar::Foo => println!("Foo"),
            &FooBar::Bar => println!("Bar"),
            &FooBar::FooBar => println!("FooBar"),
            _ => println!("Wild"),
        }
        
        // This shouldn't trigger
        if let &FooBar::BarFoo = foobar_variant!(3) {
            println!("BarFoo");
        } else {
            println!("Wild");
        }
    }

@llogiq
Copy link
Contributor

llogiq commented Oct 10, 2021

Thank you! @bors r+

@bors
Copy link
Collaborator

bors commented Oct 10, 2021

📌 Commit 1080f6c has been approved by llogiq

@bors
Copy link
Collaborator

bors commented Oct 10, 2021

⌛ Testing commit 1080f6c with merge 9205e3d...

@bors
Copy link
Collaborator

bors commented Oct 10, 2021

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

@bors bors merged commit 9205e3d into rust-lang:master Oct 10, 2021
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.

clippy::match_ref_pats probably shouldn't fire for if let
4 participants