From 6966c78be74ec7dea5cbbb18f1ec10771bf4b728 Mon Sep 17 00:00:00 2001
From: Mateusz Gacek <96mateusz.gacek@gmail.com>
Date: Mon, 29 Mar 2021 12:51:23 -0700
Subject: [PATCH] wrong_self_convention: fix FP inside trait impl for `to_*`
 method

When the `to_*` method takes `&self` and it is a trait implementation,
we don't trigger the lint.
---
 clippy_lints/src/methods/mod.rs               | 10 +++++-
 .../src/methods/wrong_self_convention.rs      | 32 +++++++++++++++----
 tests/ui/wrong_self_convention.rs             |  9 ++----
 tests/ui/wrong_self_convention.stderr         |  4 +--
 tests/ui/wrong_self_convention2.rs            | 32 +++++++++++++++++++
 tests/ui/wrong_self_convention2.stderr        | 11 +++++++
 6 files changed, 81 insertions(+), 17 deletions(-)
 create mode 100644 tests/ui/wrong_self_convention2.rs
 create mode 100644 tests/ui/wrong_self_convention2.stderr

diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs
index fccdee07877..95592523928 100644
--- a/clippy_lints/src/methods/mod.rs
+++ b/clippy_lints/src/methods/mod.rs
@@ -205,6 +205,13 @@ declare_clippy_lint! {
     /// |`to_`  | not `_mut` |`self`                 | `Copy`       |
     /// |`to_`  | not `_mut` |`&self`                | not `Copy`   |
     ///
+    /// Note: Clippy doesn't trigger methods with `to_` prefix in:
+    /// - Traits definition.
+    /// Clippy can not tell if a type that implements a trait is `Copy` or not.
+    /// - Traits implementation, when `&self` is taken.
+    /// The method signature is controlled by the trait and often `&self` is required for all types that implement the trait
+    /// (see e.g. the `std::string::ToString` trait).
+    ///
     /// Please find more info here:
     /// https://rust-lang.github.io/api-guidelines/naming.html#ad-hoc-conversions-follow-as_-to_-into_-conventions-c-conv
     ///
@@ -1850,7 +1857,6 @@ impl<'tcx> LateLintPass<'tcx> for Methods {
         let self_ty = cx.tcx.type_of(item.def_id);
 
         let implements_trait = matches!(item.kind, hir::ItemKind::Impl(hir::Impl { of_trait: Some(_), .. }));
-
         if_chain! {
             if let hir::ImplItemKind::Fn(ref sig, id) = impl_item.kind;
             if let Some(first_arg) = iter_input_pats(&sig.decl, cx.tcx.hir().body(id)).next();
@@ -1902,6 +1908,7 @@ impl<'tcx> LateLintPass<'tcx> for Methods {
                     self_ty,
                     first_arg_ty,
                     first_arg.pat.span,
+                    implements_trait,
                     false
                 );
             }
@@ -1972,6 +1979,7 @@ impl<'tcx> LateLintPass<'tcx> for Methods {
                     self_ty,
                     first_arg_ty,
                     first_arg_span,
+                    false,
                     true
                 );
             }
diff --git a/clippy_lints/src/methods/wrong_self_convention.rs b/clippy_lints/src/methods/wrong_self_convention.rs
index 59e683aa9a7..1e0de249a91 100644
--- a/clippy_lints/src/methods/wrong_self_convention.rs
+++ b/clippy_lints/src/methods/wrong_self_convention.rs
@@ -21,8 +21,10 @@ const CONVENTIONS: [(&[Convention], &[SelfKind]); 9] = [
 
     // Conversion using `to_` can use borrowed (non-Copy types) or owned (Copy types).
     // Source: https://rust-lang.github.io/api-guidelines/naming.html#ad-hoc-conversions-follow-as_-to_-into_-conventions-c-conv
-    (&[Convention::StartsWith("to_"), Convention::NotEndsWith("_mut"), Convention::IsSelfTypeCopy(false), Convention::ImplementsTrait(false)], &[SelfKind::Ref]),
-    (&[Convention::StartsWith("to_"), Convention::NotEndsWith("_mut"), Convention::IsSelfTypeCopy(true), Convention::ImplementsTrait(false)], &[SelfKind::Value]),
+    (&[Convention::StartsWith("to_"), Convention::NotEndsWith("_mut"), Convention::IsSelfTypeCopy(false), 
+    Convention::IsTraitItem(false)], &[SelfKind::Ref]),
+    (&[Convention::StartsWith("to_"), Convention::NotEndsWith("_mut"), Convention::IsSelfTypeCopy(true), 
+    Convention::IsTraitItem(false), Convention::ImplementsTrait(false)], &[SelfKind::Value]),
 ];
 
 enum Convention {
@@ -32,18 +34,27 @@ enum Convention {
     NotEndsWith(&'static str),
     IsSelfTypeCopy(bool),
     ImplementsTrait(bool),
+    IsTraitItem(bool),
 }
 
 impl Convention {
     #[must_use]
-    fn check<'tcx>(&self, cx: &LateContext<'tcx>, self_ty: &'tcx TyS<'tcx>, other: &str, is_trait_def: bool) -> bool {
+    fn check<'tcx>(
+        &self,
+        cx: &LateContext<'tcx>,
+        self_ty: &'tcx TyS<'tcx>,
+        other: &str,
+        implements_trait: bool,
+        is_trait_item: bool,
+    ) -> bool {
         match *self {
             Self::Eq(this) => this == other,
             Self::StartsWith(this) => other.starts_with(this) && this != other,
             Self::EndsWith(this) => other.ends_with(this) && this != other,
-            Self::NotEndsWith(this) => !Self::EndsWith(this).check(cx, self_ty, other, is_trait_def),
+            Self::NotEndsWith(this) => !Self::EndsWith(this).check(cx, self_ty, other, implements_trait, is_trait_item),
             Self::IsSelfTypeCopy(is_true) => is_true == is_copy(cx, self_ty),
-            Self::ImplementsTrait(is_true) => is_true == is_trait_def,
+            Self::ImplementsTrait(is_true) => is_true == implements_trait,
+            Self::IsTraitItem(is_true) => is_true == is_trait_item,
         }
     }
 }
@@ -60,12 +71,17 @@ impl fmt::Display for Convention {
             },
             Self::ImplementsTrait(is_true) => {
                 let (negation, s_suffix) = if is_true { ("", "s") } else { (" does not", "") };
-                format!("Method{} implement{} a trait", negation, s_suffix).fmt(f)
+                format!("method{} implement{} a trait", negation, s_suffix).fmt(f)
+            },
+            Self::IsTraitItem(is_true) => {
+                let suffix = if is_true { " is" } else { " is not" };
+                format!("method{} a trait item", suffix).fmt(f)
             },
         }
     }
 }
 
+#[allow(clippy::too_many_arguments)]
 pub(super) fn check<'tcx>(
     cx: &LateContext<'tcx>,
     item_name: &str,
@@ -73,6 +89,7 @@ pub(super) fn check<'tcx>(
     self_ty: &'tcx TyS<'tcx>,
     first_arg_ty: &'tcx TyS<'tcx>,
     first_arg_span: Span,
+    implements_trait: bool,
     is_trait_item: bool,
 ) {
     let lint = if is_pub {
@@ -83,7 +100,7 @@ pub(super) fn check<'tcx>(
     if let Some((conventions, self_kinds)) = &CONVENTIONS.iter().find(|(convs, _)| {
         convs
             .iter()
-            .all(|conv| conv.check(cx, self_ty, item_name, is_trait_item))
+            .all(|conv| conv.check(cx, self_ty, item_name, implements_trait, is_trait_item))
     }) {
         if !self_kinds.iter().any(|k| k.matches(cx, self_ty, first_arg_ty)) {
             let suggestion = {
@@ -99,6 +116,7 @@ pub(super) fn check<'tcx>(
                         .filter_map(|conv| {
                             if (cut_ends_with_conv && matches!(conv, Convention::NotEndsWith(_)))
                                 || matches!(conv, Convention::ImplementsTrait(_))
+                                || matches!(conv, Convention::IsTraitItem(_))
                             {
                                 None
                             } else {
diff --git a/tests/ui/wrong_self_convention.rs b/tests/ui/wrong_self_convention.rs
index ba9e19a1722..cdfbdb8b0db 100644
--- a/tests/ui/wrong_self_convention.rs
+++ b/tests/ui/wrong_self_convention.rs
@@ -165,15 +165,10 @@ mod issue6307 {
 }
 
 mod issue6727 {
-    trait ToU64 {
-        fn to_u64(self) -> u64;
-        fn to_u64_v2(&self) -> u64;
-    }
-
     #[derive(Clone, Copy)]
     struct FooCopy;
 
-    impl ToU64 for FooCopy {
+    impl FooCopy {
         fn to_u64(self) -> u64 {
             1
         }
@@ -185,7 +180,7 @@ mod issue6727 {
 
     struct FooNoCopy;
 
-    impl ToU64 for FooNoCopy {
+    impl FooNoCopy {
         // trigger lint
         fn to_u64(self) -> u64 {
             2
diff --git a/tests/ui/wrong_self_convention.stderr b/tests/ui/wrong_self_convention.stderr
index 1d58a12ac79..29f5ba82695 100644
--- a/tests/ui/wrong_self_convention.stderr
+++ b/tests/ui/wrong_self_convention.stderr
@@ -176,7 +176,7 @@ LL |         fn from_i32(self);
    = help: consider choosing a less ambiguous name
 
 error: methods with the following characteristics: (`to_*` and `self` type is `Copy`) usually take `self` by value
-  --> $DIR/wrong_self_convention.rs:181:22
+  --> $DIR/wrong_self_convention.rs:176:22
    |
 LL |         fn to_u64_v2(&self) -> u64 {
    |                      ^^^^^
@@ -184,7 +184,7 @@ LL |         fn to_u64_v2(&self) -> u64 {
    = help: consider choosing a less ambiguous name
 
 error: methods with the following characteristics: (`to_*` and `self` type is not `Copy`) usually take `self` by reference
-  --> $DIR/wrong_self_convention.rs:190:19
+  --> $DIR/wrong_self_convention.rs:185:19
    |
 LL |         fn to_u64(self) -> u64 {
    |                   ^^^^
diff --git a/tests/ui/wrong_self_convention2.rs b/tests/ui/wrong_self_convention2.rs
new file mode 100644
index 00000000000..8b42aa59e13
--- /dev/null
+++ b/tests/ui/wrong_self_convention2.rs
@@ -0,0 +1,32 @@
+// edition:2018
+#![warn(clippy::wrong_self_convention)]
+#![warn(clippy::wrong_pub_self_convention)]
+#![allow(dead_code)]
+
+fn main() {}
+
+mod issue6983 {
+    pub struct Thing;
+    pub trait Trait {
+        fn to_thing(&self) -> Thing;
+    }
+
+    impl Trait for u8 {
+        // don't trigger, e.g. `ToString` from `std` requires `&self`
+        fn to_thing(&self) -> Thing {
+            Thing
+        }
+    }
+
+    trait ToU64 {
+        fn to_u64(self) -> u64;
+    }
+
+    struct FooNoCopy;
+    // trigger lint
+    impl ToU64 for FooNoCopy {
+        fn to_u64(self) -> u64 {
+            2
+        }
+    }
+}
diff --git a/tests/ui/wrong_self_convention2.stderr b/tests/ui/wrong_self_convention2.stderr
new file mode 100644
index 00000000000..0ca1a390974
--- /dev/null
+++ b/tests/ui/wrong_self_convention2.stderr
@@ -0,0 +1,11 @@
+error: methods with the following characteristics: (`to_*` and `self` type is not `Copy`) usually take `self` by reference
+  --> $DIR/wrong_self_convention2.rs:28:19
+   |
+LL |         fn to_u64(self) -> u64 {
+   |                   ^^^^
+   |
+   = note: `-D clippy::wrong-self-convention` implied by `-D warnings`
+   = help: consider choosing a less ambiguous name
+
+error: aborting due to previous error
+