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

clippy::match_ref_pats probably shouldn't fire for if let #7740

Closed
CAD97 opened this issue Sep 30, 2021 · 10 comments · Fixed by #7800
Closed

clippy::match_ref_pats probably shouldn't fire for if let #7740

CAD97 opened this issue Sep 30, 2021 · 10 comments · Fixed by #7800
Assignees
Labels
C-bug Category: Clippy is not doing the correct thing good-first-issue These issues are a good way to get started with Clippy I-false-positive Issue: The lint was triggered on code it shouldn't have

Comments

@CAD97
Copy link
Contributor

CAD97 commented Sep 30, 2021

Lint name:

I tried this code:

if let &KeyboardInput {
    keycode: Some(keycode),
    state,
    ..
} = event
{
    // ...
}

I expected to see this happen: warning free

Instead, this happened: clippy::match_ref_pats matched

The point of clippy::match_ref_pats is that it's less notation to put a single deref on the matched value than a ref on every pattern, but with if let this doesn't apply, as there's only one pattern to begin with.

Either the description/help message should change when only one pattern is present, or clippy::match_ref_pats should only be emitted when more than one pattern are present.

Meta

Rust version (rustc -Vv):

rustc 1.56.0-nightly (ad02dc46b 2021-08-26)
binary: rustc
commit-hash: ad02dc46badee510bd3a2c093edf80fcaade91b1
commit-date: 2021-08-26
host: x86_64-pc-windows-msvc
release: 1.56.0-nightly
LLVM version: 13.0.0
@CAD97 CAD97 added C-bug Category: Clippy is not doing the correct thing I-false-positive Issue: The lint was triggered on code it shouldn't have labels Sep 30, 2021
@camsteffen
Copy link
Contributor

For that matter, we should probably not lint match if there is just one &.

@camsteffen camsteffen added the good-first-issue These issues are a good way to get started with Clippy label Oct 4, 2021
@1nF0rmed
Copy link
Contributor

1nF0rmed commented Oct 8, 2021

So, would we be suggesting that for a single &, the clippy::match_ref_pats should be suppressed?

If so, are there any tests around this and what's the part of the code that I should be looking at for this?

@camsteffen
Copy link
Contributor

Hi @1nF0rmed. Yes that's correct. You can find the implementation here and tests here. You can comment with @rustbot claim to self-assign this issue.

@1nF0rmed
Copy link
Contributor

1nF0rmed commented Oct 8, 2021

Thank you for the pointers!
Just wanted to confirm my approach. Given we are passing the patterns here, should we add a check in the implementation

fn check_match_ref_pats<'a, 'b, I>(cx: &LateContext<'_>, ex: &Expr<'_>, pats: I, expr: &Expr<'_>)

where we iterate through the patterns and count the number of occurrences of PatKind::Ref(..).
If the count, in that case, is 1, then we just return

@camsteffen
Copy link
Contributor

It looks like you could just modify has_only_ref_pats (and rename to has_multiple_ref_pats).

@1nF0rmed
Copy link
Contributor

1nF0rmed commented Oct 9, 2021

Hey, thank you so much for the pointers. are we able to add this to hacktoberfest?

@camsteffen
Copy link
Contributor

I think any PR will count?

@1nF0rmed
Copy link
Contributor

1nF0rmed commented Oct 9, 2021

Can I pick up this issue? @camsteffen

@camsteffen
Copy link
Contributor

Yes just comment with @rustbot claim

@1nF0rmed
Copy link
Contributor

@rustbot claim

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: Clippy is not doing the correct thing good-first-issue These issues are a good way to get started with Clippy I-false-positive Issue: The lint was triggered on code it shouldn't have
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants