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: Match SelfKind::No more restrictively #8208

Merged
merged 1 commit into from
Jan 2, 2022

Conversation

nmathewson
Copy link
Contributor

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

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 rust-lang#8142).

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

Fixes rust-lang#8142

changelog: [`wrong_self_convention`] rejects `self` references in more cases
@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 1, 2022
@nmathewson
Copy link
Contributor Author

(FWIW, lintcheck reports that there are are a couple of new positives here; I haven't evaluated whether they are desirable or not.)

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.

Looks good to me, thank you very much! I've also looked at the output of lintcheck, we have five new reports which all seem to be correct. This is a very nice fix 👍

I usually use cargo lintcheck --markdown --filter wrong_self_convention to create a markdown file which has the file locations as lints. The usage depends a bit on the setup, just a side note, in case you're interested. These are new correct reports, which all seem correct:

file lint message
cargo-0.49.0/src/cargo/core/source/mod.rs:103:18 clippy::wrong_self_convention "methods called is_* usually take self by reference or no self"
cargo-0.49.0/src/cargo/ops/tree/graph.rs:166:27 clippy::wrong_self_convention "methods called from_* usually take no self"
cargo-0.49.0/src/cargo/sources/registry/index.rs:441:22 clippy::wrong_self_convention "methods called is_* usually take self by reference or no self"
xsv-0.13.0/src/config.rs:282:33 clippy::wrong_self_convention "methods called from_* usually take no self"
xsv-0.13.0/src/config.rs:300:38 clippy::wrong_self_convention "methods called from_* usually take no self"

@xFrednet
Copy link
Member

xFrednet commented Jan 2, 2022

btw a second side note, you can assign yourself to an issue including @rustbot claim in your comment. That indicates to others that you want to work on that issue 🙃

Thank you very much for this PR 👍

@bors r+

@bors
Copy link
Contributor

bors commented Jan 2, 2022

📌 Commit 3d41358 has been approved by xFrednet

@bors
Copy link
Contributor

bors commented Jan 2, 2022

⌛ Testing commit 3d41358 with merge b25dbc6...

@bors
Copy link
Contributor

bors commented Jan 2, 2022

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

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.

wrong_self_convention: FN with is_* taking &mut self
4 participants