Lint wrong self convention in trait also

Author: ThibsG

clippy_lints/src/methods/mod.rs

@@ -23,6 +23,7 @@ use rustc_middle::ty::{self, TraitRef, Ty, TyS};
 use rustc_session::{declare_lint_pass, declare_tool_lint};
 use rustc_span::source_map::Span;
 use rustc_span::symbol::{sym, SymbolStr};
+use rustc_typeck::hir_ty_to_ty;
 
 use crate::consts::{constant, Constant};
 use crate::utils::eager_or_lazy::is_lazyness_candidate;
@@ -1608,10 +1609,15 @@ impl<'tcx> LateLintPass<'tcx> for Methods {
         let item = cx.tcx.hir().expect_item(parent);
         let def_id = cx.tcx.hir().local_def_id(item.hir_id);
         let self_ty = cx.tcx.type_of(def_id);
+
+        // if this impl block implements a trait, lint in trait definition instead
+        if let hir::ItemKind::Impl { of_trait: Some(_), .. } = item.kind {
+            return;
+        }
+
         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();
-            if let hir::ItemKind::Impl{ of_trait: None, .. } = item.kind;
 
             let method_def_id = cx.tcx.hir().local_def_id(impl_item.hir_id);
             let method_sig = cx.tcx.fn_sig(method_def_id);
@@ -1682,11 +1688,6 @@ impl<'tcx> LateLintPass<'tcx> for Methods {
             }
         }
 
-        // if this impl block implements a trait, lint in trait definition instead
-        if let hir::ItemKind::Impl { of_trait: Some(_), .. } = item.kind {
-            return;
-        }
-
         if let hir::ImplItemKind::Fn(_, _) = impl_item.kind {
             let ret_ty = return_ty(cx, impl_item.hir_id);
 
@@ -1720,8 +1721,42 @@ impl<'tcx> LateLintPass<'tcx> for Methods {
     }
 
     fn check_trait_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx TraitItem<'_>) {
+        if in_external_macro(cx.tcx.sess, item.span) {
+            return;
+        }
+
+        if_chain! {
+            if let TraitItemKind::Fn(ref sig, _) = item.kind;
+            if let Some(first_arg_ty) = sig.decl.inputs.iter().next();
+            let first_arg_span = first_arg_ty.span;
+            let first_arg_ty = hir_ty_to_ty(cx.tcx, first_arg_ty);
+            let self_ty = TraitRef::identity(cx.tcx, item.hir_id.owner.to_def_id()).self_ty();
+
+            then {
+                if let Some((ref conv, self_kinds)) = &CONVENTIONS
+                    .iter()
+                    .find(|(ref conv, _)| conv.check(&item.ident.name.as_str()))
+                {
+                    if !self_kinds.iter().any(|k| k.matches(cx, self_ty, first_arg_ty)) {
+                        span_lint(
+                            cx,
+                            WRONG_PUB_SELF_CONVENTION,
+                            first_arg_span,
+                            &format!("methods called `{}` usually take {}; consider choosing a less ambiguous name",
+                                conv,
+                                &self_kinds
+                                    .iter()
+                                    .map(|k| k.description())
+                                    .collect::<Vec<_>>()
+                                    .join(" or ")
+                            ),
+                        );
+                    }
+                }
+            }
+        }
+
         if_chain! {
-            if !in_external_macro(cx.tcx.sess, item.span);
             if item.ident.name == sym::new;
             if let TraitItemKind::Fn(_, _) = item.kind;
             let ret_ty = return_ty(cx, item.hir_id);

tests/ui/wrong_self_convention.rs

@@ -88,3 +88,29 @@ mod issue4037 {
         }
     }
 }
+
+// Lint also in trait definition (see #6307)
+mod issue6307{
+    trait T: Sized {
+        fn as_i32(self) {}
+        fn as_u32(&self) {}
+        fn into_i32(&self) {}
+        fn into_u32(self) {}
+        fn is_i32(self) {}
+        fn is_u32(&self) {}
+        fn to_i32(self) {}
+        fn to_u32(&self) {}
+        fn from_i32(self) {}
+        // check whether the lint can be allowed at the function level
+        #[allow(clippy::wrong_pub_self_convention)]
+        fn from_cake(self) {}
+
+        // test for false positives
+        fn as_(self) {}
+        fn into_(&self) {}
+        fn is_(self) {}
+        fn to_(self) {}
+        fn from_(self) {}
+        fn to_mut(&mut self) {}
+    }
+}

tests/ui/wrong_self_convention.stderr

@@ -72,5 +72,37 @@ error: methods called `from_*` usually take no self; consider choosing a less am
 LL |     pub fn from_i64(self) {}
    |                     ^^^^
 
-error: aborting due to 12 previous errors
+error: methods called `as_*` usually take self by reference or self by mutable reference; consider choosing a less ambiguous name
+  --> $DIR/wrong_self_convention.rs:95:19
+   |
+LL |         fn as_i32(self) {}
+   |                   ^^^^
+   |
+   = note: `-D clippy::wrong-pub-self-convention` implied by `-D warnings`
+
+error: methods called `into_*` usually take self by value; consider choosing a less ambiguous name
+  --> $DIR/wrong_self_convention.rs:97:21
+   |
+LL |         fn into_i32(&self) {}
+   |                     ^^^^^
+
+error: methods called `is_*` usually take self by reference or no self; consider choosing a less ambiguous name
+  --> $DIR/wrong_self_convention.rs:99:19
+   |
+LL |         fn is_i32(self) {}
+   |                   ^^^^
+
+error: methods called `to_*` usually take self by reference; consider choosing a less ambiguous name
+  --> $DIR/wrong_self_convention.rs:101:19
+   |
+LL |         fn to_i32(self) {}
+   |                   ^^^^
+
+error: methods called `from_*` usually take no self; consider choosing a less ambiguous name
+  --> $DIR/wrong_self_convention.rs:103:21
+   |
+LL |         fn from_i32(self) {}
+   |                     ^^^^
+
+error: aborting due to 17 previous errors