Auto merge of #6316 - ThibsG:WrongSelfConventionTraitDef, r=ebroto

Lint also in trait def for `wrong_self_convention` Extends `wrong_self_convention` to lint also in trait definition. By the way, I think the `wrong_pub_self_convention` [example](https://github.com/rust-lang/rust-clippy/blob/dd826b4626c00da53f76f00f02f03556803e9cdb/clippy_lints/src/methods/mod.rs#L197) is misleading. On [playground](https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=32615ab3f6009e7e42cc3754be0ca17f), it fires `wrong_self_convention`, so the example (or the lint maybe?) needs to be reworked. The difference with `wrong_self_convention` [example](https://github.com/rust-lang/rust-clippy/blob/dd826b4626c00da53f76f00f02f03556803e9cdb/clippy_lints/src/methods/mod.rs#L172) is mainly the `pub` keyword on the method `as_str`, but the lint doesn't use the function visibility as condition to choose which lint to fire (in fact it uses the visibility of the impl item). fixes: #6307 changelog: Lint `wrong_self_convention` lint in trait def also
rust-lang · Dec 19, 2020 · 9f9e9f7 · 9f9e9f7
2 parents 0904f54 + 90a16e4
commit 9f9e9f7
Show file tree

Hide file tree

Showing 6 changed files with 195 additions and 53 deletions.
diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs
@@ -22,6 +22,7 @@ use rustc_semver::RustcVersion;
 use rustc_session::{declare_tool_lint, impl_lint_pass};
 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;
@@ -1623,10 +1624,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);
@@ -1668,40 +1674,17 @@ impl<'tcx> LateLintPass<'tcx> for Methods {
                     }
                 }
 
-                if let Some((ref conv, self_kinds)) = &CONVENTIONS
-                    .iter()
-                    .find(|(ref conv, _)| conv.check(&name))
-                {
-                    if !self_kinds.iter().any(|k| k.matches(cx, self_ty, first_arg_ty)) {
-                        let lint = if item.vis.node.is_pub() {
-                            WRONG_PUB_SELF_CONVENTION
-                        } else {
-                            WRONG_SELF_CONVENTION
-                        };
-
-                        span_lint(
-                            cx,
-                            lint,
-                            first_arg.pat.span,
-                            &format!("methods called `{}` usually take {}; consider choosing a less ambiguous name",
-                                conv,
-                                &self_kinds
-                                    .iter()
-                                    .map(|k| k.description())
-                                    .collect::<Vec<_>>()
-                                    .join(" or ")
-                            ),
-                        );
-                    }
-                }
+                lint_wrong_self_convention(
+                    cx,
+                    &name,
+                    item.vis.node.is_pub(),
+                    self_ty,
+                    first_arg_ty,
+                    first_arg.pat.span
+                );
             }
         }
 
-        // 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);
 
@@ -1735,8 +1718,23 @@ 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 {
+                lint_wrong_self_convention(cx, &item.ident.name.as_str(), false, self_ty, first_arg_ty, first_arg_span);
+            }
+        }
+
         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);
@@ -1757,6 +1755,39 @@ impl<'tcx> LateLintPass<'tcx> for Methods {
     extract_msrv_attr!(LateContext);
 }
 
+fn lint_wrong_self_convention<'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,
+) {
+    let lint = if is_pub {
+        WRONG_PUB_SELF_CONVENTION
+    } else {
+        WRONG_SELF_CONVENTION
+    };
+    if let Some((ref conv, self_kinds)) = &CONVENTIONS.iter().find(|(ref conv, _)| conv.check(item_name)) {
+        if !self_kinds.iter().any(|k| k.matches(cx, self_ty, first_arg_ty)) {
+            span_lint(
+                cx,
+                lint,
+                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 ")
+                ),
+            );
+        }
+    }
+}
+
 /// Checks for the `OR_FUN_CALL` lint.
 #[allow(clippy::too_many_lines)]
 fn lint_or_fun_call<'tcx>(

diff --git a/tests/ui/use_self.fixed b/tests/ui/use_self.fixed
@@ -71,6 +71,7 @@ mod lifetimes {
 
 mod issue2894 {
     trait IntoBytes {
+        #[allow(clippy::wrong_self_convention)]
         fn into_bytes(&self) -> Vec<u8>;
     }
 

diff --git a/tests/ui/use_self.rs b/tests/ui/use_self.rs
@@ -71,6 +71,7 @@ mod lifetimes {
 
 mod issue2894 {
     trait IntoBytes {
+        #[allow(clippy::wrong_self_convention)]
         fn into_bytes(&self) -> Vec<u8>;
     }
 

diff --git a/tests/ui/use_self.stderr b/tests/ui/use_self.stderr
@@ -37,19 +37,19 @@ LL |             Foo::new()
    |             ^^^ help: use the applicable keyword: `Self`
 
 error: unnecessary structure name repetition
-  --> $DIR/use_self.rs:89:56
+  --> $DIR/use_self.rs:90:56
    |
 LL |         fn bad(foos: &[Self]) -> impl Iterator<Item = &Foo> {
    |                                                        ^^^ help: use the applicable keyword: `Self`
 
 error: unnecessary structure name repetition
-  --> $DIR/use_self.rs:104:13
+  --> $DIR/use_self.rs:105:13
    |
 LL |             TS(0)
    |             ^^ help: use the applicable keyword: `Self`
 
 error: unnecessary structure name repetition
-  --> $DIR/use_self.rs:112:25
+  --> $DIR/use_self.rs:113:25
    |
 LL |             fn new() -> Foo {
    |                         ^^^ help: use the applicable keyword: `Self`
@@ -60,7 +60,7 @@ LL |         use_self_expand!(); // Should lint in local macros
    = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)
 
 error: unnecessary structure name repetition
-  --> $DIR/use_self.rs:113:17
+  --> $DIR/use_self.rs:114:17
    |
 LL |                 Foo {}
    |                 ^^^ help: use the applicable keyword: `Self`
@@ -71,91 +71,91 @@ LL |         use_self_expand!(); // Should lint in local macros
    = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)
 
 error: unnecessary structure name repetition
-  --> $DIR/use_self.rs:148:21
+  --> $DIR/use_self.rs:149:21
    |
 LL |         fn baz() -> Foo {
    |                     ^^^ help: use the applicable keyword: `Self`
 
 error: unnecessary structure name repetition
-  --> $DIR/use_self.rs:149:13
+  --> $DIR/use_self.rs:150:13
    |
 LL |             Foo {}
    |             ^^^ help: use the applicable keyword: `Self`
 
 error: unnecessary structure name repetition
-  --> $DIR/use_self.rs:136:29
+  --> $DIR/use_self.rs:137:29
    |
 LL |                 fn bar() -> Bar {
    |                             ^^^ help: use the applicable keyword: `Self`
 
 error: unnecessary structure name repetition
-  --> $DIR/use_self.rs:137:21
+  --> $DIR/use_self.rs:138:21
    |
 LL |                     Bar { foo: Foo {} }
    |                     ^^^ help: use the applicable keyword: `Self`
 
 error: unnecessary structure name repetition
-  --> $DIR/use_self.rs:166:21
+  --> $DIR/use_self.rs:167:21
    |
 LL |             let _ = Enum::B(42);
    |                     ^^^^ help: use the applicable keyword: `Self`
 
 error: unnecessary structure name repetition
-  --> $DIR/use_self.rs:167:21
+  --> $DIR/use_self.rs:168:21
    |
 LL |             let _ = Enum::C { field: true };
    |                     ^^^^ help: use the applicable keyword: `Self`
 
 error: unnecessary structure name repetition
-  --> $DIR/use_self.rs:168:21
+  --> $DIR/use_self.rs:169:21
    |
 LL |             let _ = Enum::A;
    |                     ^^^^ help: use the applicable keyword: `Self`
 
 error: unnecessary structure name repetition
-  --> $DIR/use_self.rs:199:13
+  --> $DIR/use_self.rs:200:13
    |
 LL |             nested::A::fun_1();
    |             ^^^^^^^^^ help: use the applicable keyword: `Self`
 
 error: unnecessary structure name repetition
-  --> $DIR/use_self.rs:200:13
+  --> $DIR/use_self.rs:201:13
    |
 LL |             nested::A::A;
    |             ^^^^^^^^^ help: use the applicable keyword: `Self`
 
 error: unnecessary structure name repetition
-  --> $DIR/use_self.rs:202:13
+  --> $DIR/use_self.rs:203:13
    |
 LL |             nested::A {};
    |             ^^^^^^^^^ help: use the applicable keyword: `Self`
 
 error: unnecessary structure name repetition
-  --> $DIR/use_self.rs:221:13
+  --> $DIR/use_self.rs:222:13
    |
 LL |             TestStruct::from_something()
    |             ^^^^^^^^^^ help: use the applicable keyword: `Self`
 
 error: unnecessary structure name repetition
-  --> $DIR/use_self.rs:235:25
+  --> $DIR/use_self.rs:236:25
    |
 LL |         async fn g() -> S {
    |                         ^ help: use the applicable keyword: `Self`
 
 error: unnecessary structure name repetition
-  --> $DIR/use_self.rs:236:13
+  --> $DIR/use_self.rs:237:13
    |
 LL |             S {}
    |             ^ help: use the applicable keyword: `Self`
 
 error: unnecessary structure name repetition
-  --> $DIR/use_self.rs:240:16
+  --> $DIR/use_self.rs:241:16
    |
 LL |             &p[S::A..S::B]
    |                ^ help: use the applicable keyword: `Self`
 
 error: unnecessary structure name repetition
-  --> $DIR/use_self.rs:240:22
+  --> $DIR/use_self.rs:241:22
    |
 LL |             &p[S::A..S::B]
    |                      ^ help: use the applicable keyword: `Self`

diff --git a/tests/ui/wrong_self_convention.rs b/tests/ui/wrong_self_convention.rs
@@ -88,3 +88,52 @@ 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_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) {}
+    }
+
+    trait U {
+        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_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);
+    }
+}