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 4e33b2ff14cde..8a2a468c85233 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 1cf349f8aa7c7..1991a01fb60be 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 20419457b47f0..e5042aaa776b4 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 1b9da8a55e53f..f8fee4b3ab2d8 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 590ee6d9c529d..5493a99572e06 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