From 3d41358a554cb70e23e001f6ac92cf79d805b671 Mon Sep 17 00:00:00 2001
From: Nick Mathewson <nickm@torproject.org>
Date: Fri, 31 Dec 2021 23:39:40 -0500
Subject: [PATCH] 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
---
clippy_lints/src/methods/mod.rs | 8 +++++++-
clippy_lints/src/redundant_clone.rs | 4 ++--
tests/ui/issue_4266.stderr | 11 ++++++++++-
tests/ui/wrong_self_convention.rs | 21 +++++++++++++++++++++
tests/ui/wrong_self_convention.stderr | 10 +++++++++-
5 files changed, 49 insertions(+), 5 deletions(-)
diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs
index 4e33b2ff14c..8a2a468c852 100644
--- a/clippy_lints/src/methods/mod.rs
+++ b/clippy_lints/src/methods/mod.rs
@@ -2535,11 +2535,17 @@ impl SelfKind {
implements_trait(cx, ty, trait_def_id, &[parent_ty.into()])
}
+ fn matches_none<'a>(cx: &LateContext<'a>, parent_ty: Ty<'a>, ty: Ty<'a>) -> bool {
+ !matches_value(cx, parent_ty, ty)
+ && !matches_ref(cx, hir::Mutability::Not, parent_ty, ty)
+ && !matches_ref(cx, hir::Mutability::Mut, parent_ty, ty)
+ }
+
match self {
Self::Value => matches_value(cx, parent_ty, ty),
Self::Ref => matches_ref(cx, hir::Mutability::Not, parent_ty, ty) || ty == parent_ty && is_copy(cx, ty),
Self::RefMut => matches_ref(cx, hir::Mutability::Mut, parent_ty, ty),
- Self::No => ty != parent_ty,
+ Self::No => matches_none(cx, parent_ty, ty),
}
}
diff --git a/clippy_lints/src/redundant_clone.rs b/clippy_lints/src/redundant_clone.rs
index 1cf349f8aa7..1991a01fb60 100644
--- a/clippy_lints/src/redundant_clone.rs
+++ b/clippy_lints/src/redundant_clone.rs
@@ -220,7 +220,7 @@ impl<'tcx> LateLintPass<'tcx> for RedundantClone {
continue;
} else if let Some(loc) = clone_usage.cloned_consume_or_mutate_loc {
// cloned value is mutated, and the clone is alive.
- if possible_borrower.is_alive_at(ret_local, loc) {
+ if possible_borrower.local_is_alive_at(ret_local, loc) {
continue;
}
}
@@ -767,7 +767,7 @@ impl PossibleBorrowerMap<'_, '_> {
self.bitset.0 == self.bitset.1
}
- fn is_alive_at(&mut self, local: mir::Local, at: mir::Location) -> bool {
+ fn local_is_alive_at(&mut self, local: mir::Local, at: mir::Location) -> bool {
self.maybe_live.seek_after_primary_effect(at);
self.maybe_live.contains(local)
}
diff --git a/tests/ui/issue_4266.stderr b/tests/ui/issue_4266.stderr
index 20419457b47..e5042aaa776 100644
--- a/tests/ui/issue_4266.stderr
+++ b/tests/ui/issue_4266.stderr
@@ -12,5 +12,14 @@ error: explicit lifetimes given in parameter types where they could be elided (o
LL | async fn one_to_one<'a>(s: &'a str) -> &'a str {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
-error: aborting due to 2 previous errors
+error: methods called `new` usually take no `self`
+ --> $DIR/issue_4266.rs:27:22
+ |
+LL | pub async fn new(&mut self) -> Self {
+ | ^^^^^^^^^
+ |
+ = note: `-D clippy::wrong-self-convention` implied by `-D warnings`
+ = help: consider choosing a less ambiguous name
+
+error: aborting due to 3 previous errors
diff --git a/tests/ui/wrong_self_convention.rs b/tests/ui/wrong_self_convention.rs
index 1b9da8a55e5..f8fee4b3ab2 100644
--- a/tests/ui/wrong_self_convention.rs
+++ b/tests/ui/wrong_self_convention.rs
@@ -188,3 +188,24 @@ mod issue6727 {
}
}
}
+
+pub mod issue8142 {
+ struct S;
+
+ impl S {
+ // Should lint: is_ methods should only take &self, or no self at all.
+ fn is_still_buggy(&mut self) -> bool {
+ false
+ }
+
+ // Should not lint: "no self at all" is allowed.
+ fn is_forty_two(x: u32) -> bool {
+ x == 42
+ }
+
+ // Should not lint: &self is allowed.
+ fn is_test_code(&self) -> bool {
+ true
+ }
+ }
+}
diff --git a/tests/ui/wrong_self_convention.stderr b/tests/ui/wrong_self_convention.stderr
index 590ee6d9c52..5493a99572e 100644
--- a/tests/ui/wrong_self_convention.stderr
+++ b/tests/ui/wrong_self_convention.stderr
@@ -191,5 +191,13 @@ LL | fn to_u64(self) -> u64 {
|
= help: consider choosing a less ambiguous name
-error: aborting due to 24 previous errors
+error: methods called `is_*` usually take `self` by reference or no `self`
+ --> $DIR/wrong_self_convention.rs:197:27
+ |
+LL | fn is_still_buggy(&mut self) -> bool {
+ | ^^^^^^^^^
+ |
+ = help: consider choosing a less ambiguous name
+
+error: aborting due to 25 previous errors