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

False positive wrong_self_convention inside trait impls #6983

Closed
dtolnay opened this issue Mar 27, 2021 · 3 comments · Fixed by #7002
Closed

False positive wrong_self_convention inside trait impls #6983

dtolnay opened this issue Mar 27, 2021 · 3 comments · Fixed by #7002
Labels
C-bug Category: Clippy is not doing the correct thing I-false-positive Issue: The lint was triggered on code it shouldn't have

Comments

@dtolnay
Copy link
Member

dtolnay commented Mar 27, 2021

Beginning with the most recent nightly (clippy 0.1.53 5e65467 2021-03-26, rustc 1.53.0-nightly 5e65467 2021-03-26) the following code incorrectly triggers wrong_self_convention. The trait method signature is controlled by the trait; it doesn't matter that the impl is for a type implementing Copy, that doesn't mean that the trait can or should take self by value. In particular there are likely other impls which are not Copy.

pub struct Thing;

pub trait Trait {
    fn to_thing(&self) -> Thing;
}

impl Trait for u8 {
    fn to_thing(&self) -> Thing {
        Thing
    }
}
warning: methods with the following characteristics: (`to_*` and `self` type is `Copy`) usually take `self` by value
 --> src/main.rs:8:17
  |
8 |     fn to_thing(&self) -> Thing {
  |                 ^^^^^
  |
  = note: `#[warn(clippy::wrong_self_convention)]` on by default
  = help: consider choosing a less ambiguous name
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#wrong_self_convention

Mentioning @mgacek8 @llogiq who touched this lint recently in #6924.

@dtolnay dtolnay 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 Mar 27, 2021
dtolnay added a commit to dtolnay/quote that referenced this issue Mar 27, 2021
rust-lang/rust-clippy#6983

    warning: methods with the following characteristics: (`to_*` and `self` type is `Copy`) usually take `self` by value
      --> src/to_tokens.rs:80:18
       |
    80 |     fn to_tokens(&self, tokens: &mut TokenStream) {
       |                  ^^^^^
       |
       = note: `#[warn(clippy::wrong_self_convention)]` on by default
       = help: consider choosing a less ambiguous name
       = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#wrong_self_convention
dtolnay added a commit to dtolnay/syn that referenced this issue Mar 27, 2021
rust-lang/rust-clippy#6983

    error: methods with the following characteristics: (`to_*` and `self` type is `Copy`) usually take `self` by value
       --> src/token.rs:563:18
        |
    563 |     fn to_tokens(&self, tokens: &mut TokenStream) {
        |                  ^^^^^
        |
        = note: `-D clippy::wrong-self-convention` implied by `-D clippy::all`
        = help: consider choosing a less ambiguous name
        = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#wrong_self_convention
dtolnay added a commit to dtolnay/cxx that referenced this issue Mar 27, 2021
rust-lang/rust-clippy#6983

    warning: methods with the following characteristics: (`to_*` and `self` type is `Copy`) usually take `self` by value
       --> macro/src/syntax/discriminant.rs:194:18
        |
    194 |     fn to_tokens(&self, tokens: &mut TokenStream) {
        |                  ^^^^^
        |
        = note: `#[warn(clippy::wrong_self_convention)]` on by default
        = help: consider choosing a less ambiguous name
        = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#wrong_self_convention
@dtolnay
Copy link
Member Author

dtolnay commented Mar 27, 2021

Mentioning @matthiaskrgr too since I am surprised this was not caught by b9a7a2a -- all three of the above affected crates (quote, syn, cxx) are in lintcheck_crates.

quote = {name = "quote", versions = ['1.0.7']}

syn = {name = "syn", versions = ['1.0.54']}

cxx = {name = "cxx", versions = ['1.0.32']}

@llogiq
Copy link
Contributor

llogiq commented Mar 28, 2021

Oops. I was under the impression that methods taking &self should be named as_* instead of to_.

@extrawurst
Copy link

this is even more problematic because the trait ToString in std expects me to pass self as &self in to_string and now clippy starts complaining about that :)

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-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