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

wrong_self_convention: FN with is_* taking &mut self #8142

Closed
jendrikw opened this issue Dec 18, 2021 · 3 comments · Fixed by #8208
Closed

wrong_self_convention: FN with is_* taking &mut self #8142

jendrikw opened this issue Dec 18, 2021 · 3 comments · Fixed by #8208
Labels
C-bug Category: Clippy is not doing the correct thing I-false-negative Issue: The lint should have been triggered on code, but wasn't

Comments

@jendrikw
Copy link
Contributor

Summary

My interpretation of the table at https://rust-lang.github.io/rust-clippy/master/index.html#wrong_self_convention is that a method called is_* is allowed to take no self, or &self, but not self or &mut self. In reality, only self produces a warning. Is this intentional?

Lint Name

wrong_self_convention

Reproducer

I tried this code:

#![allow(unused)]
struct S { field: String }

impl S {
    fn is_foo(&mut self) -> bool { todo!() }
}

fn main() {}

I expected to see this happen:

warning: methods called `is_*` usually take `self` by reference or no `self`
  --> src/main.rs:16:15
   |
16 |     fn is_foo(&mut self) -> bool {
   |               ^^^^

Instead, this happened:

No warning

Version

rustc 1.57.0 (f1edd0429 2021-11-29)
binary: rustc
commit-hash: f1edd0429582dd29cccacaf50fd134b05593bd9c
commit-date: 2021-11-29
host: x86_64-unknown-linux-gnu
release: 1.57.0
LLVM version: 13.0.0
@jendrikw jendrikw added C-bug Category: Clippy is not doing the correct thing I-false-negative Issue: The lint should have been triggered on code, but wasn't labels Dec 18, 2021
@jendrikw
Copy link
Contributor Author

jendrikw commented Dec 18, 2021

This issue was filed because intellij-rust warns for both &mut self and self and it wants to be consistent with clippy. See intellij-rust/intellij-rust#7557.

image
(The yellow background is a warning from intellij-rust, the yellow underline is from clippy.)

@ghost
Copy link

ghost commented Dec 19, 2021

I agree. It should warn on &mut self.

@nmathewson
Copy link
Contributor

This one looks neat, and I think I can see what the trouble is; I'll try writing a patch for this in the next day or two. (If I haven't written a PR by 5 January, I have probably given up or forgotten this.)

bors added a commit that referenced this issue Jan 2, 2022
wrong_self_convention: Match `SelfKind::No` more restrictively

The `wrong_self_convention` lint uses a `SelfKind` type to decide
whether a method has the right kind of "self" for its name, or whether
the kind of "self" it has makes its name confusable for a method in
a common trait.  One possibility is `SelfKind::No`, which is supposed
to mean "No `self`".

Previously, SelfKind::No matched everything _except_ Self, including
references to Self.  This patch changes it to match Self, &Self, &mut
Self, Box<Self>, and so on.

For example, this kind of method was allowed before:

```
impl S {
    // Should trigger the lint, because
    // "methods called `is_*` usually take `self` by reference or no `self`"
    fn is_foo(&mut self) -> bool { todo!() }
}
```

But since SelfKind::No matched "&mut self", no lint was triggered
(see #8142).

With this patch, the code above now gives a lint as expected.

fixes #8142

changelog: [`wrong_self_convention`] rejects `self` references in more cases
@bors bors closed this as completed in 3d41358 Jan 2, 2022
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 I-false-negative Issue: The lint should have been triggered on code, but wasn't
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants