Auto merge of rust-lang#7002 - mgacek8:issue6983_wrong_self_convention_inside_trait_impls, r=phansch

wrong_self_convention: fix FP inside trait impl for `to_*` method taking `&self`

fixes rust-lang#6983
changelog: `wrong_self_convention`: fix FP inside trait impl for `to_*` method taking `&self`

Author: bors

clippy_lints/src/methods/mod.rs

@@ -206,6 +206,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
     ///
@@ -1772,7 +1779,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();
@@ -1824,6 +1830,7 @@ impl<'tcx> LateLintPass<'tcx> for Methods {
                     self_ty,
                     first_arg_ty,
                     first_arg.pat.span,
+                    implements_trait,
                     false
                 );
             }
@@ -1893,6 +1900,7 @@ impl<'tcx> LateLintPass<'tcx> for Methods {
                     self_ty,
                     first_arg_ty,
                     first_arg_span,
+                    false,
                     true
                 );
             }

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,19 +71,25 @@ 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,
     is_pub: bool,
     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 {

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

tests/ui/wrong_self_convention.stderr

@@ -176,15 +176,15 @@ 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 {
    |                      ^^^^^
    |
    = 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 {
    |                   ^^^^

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

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
+