use_self - lint at impl item level rust-lang#5078

tnielens · Apr 28, 2020 · b1f4643 · b1f4643
1 parent 5d91ead
commit b1f4643
Show file tree

Hide file tree

Showing 4 changed files with 131 additions and 49 deletions.
diff --git a/clippy_lints/src/use_self.rs b/clippy_lints/src/use_self.rs
@@ -4,7 +4,7 @@ use rustc_hir as hir;
 use rustc_hir::def::DefKind;
 use rustc_hir::intravisit::{walk_expr, walk_impl_item, walk_ty, NestedVisitorMap, Visitor};
 use rustc_hir::{
-    def, Expr, ExprKind, FnDecl, FnRetTy, FnSig, GenericArg, ImplItem, ImplItemKind, Item, ItemKind, Node, Path, QPath,
+    def, Expr, ExprKind, FnDecl, FnRetTy, FnSig, GenericArg, ImplItem, ImplItemKind, ItemKind, Node, Path, QPath,
     TyKind,
 };
 use rustc_lint::{LateContext, LateLintPass, LintContext};
@@ -101,7 +101,6 @@ fn truncate_last_segment<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, sp: Span) -> Span
 
 struct ImplVisitor<'a, 'tcx> {
     cx: &'a LateContext<'a, 'tcx>,
-    item: &'tcx Item<'tcx>,
     self_ty: Ty<'tcx>,
 }
 
@@ -238,58 +237,52 @@ impl<'a, 'tcx> Visitor<'tcx> for ImplVisitor<'a, 'tcx> {
         }
         walk_expr(self, expr);
     }
-
-    fn visit_impl_item(&mut self, ii: &'tcx ImplItem<'tcx>) {
-        let tcx = self.cx.tcx;
-        let impl_def_id = tcx.hir().local_def_id(self.item.hir_id);
-        let impl_trait_ref = tcx.impl_trait_ref(impl_def_id);
-        if_chain! {
-            if let Some(impl_trait_ref) = impl_trait_ref;
-            if let ImplItemKind::Fn(FnSig { decl: impl_decl, .. }, impl_body_id) = &ii.kind;
-            then {
-                self.check_trait_method_impl_decl(ii, impl_decl, impl_trait_ref);
-                let body = tcx.hir().body(*impl_body_id);
-                self.visit_body(body);
-            } else {
-                walk_impl_item(self, ii)
-            }
-        }
-    }
 }
 
 impl<'a, 'tcx> LateLintPass<'a, 'tcx> for UseSelf {
-    fn check_item(&mut self, cx: &LateContext<'a, 'tcx>, item: &'tcx Item<'_>) {
-        if in_external_macro(cx.sess(), item.span) {
+    fn check_impl_item(&mut self, cx: &LateContext<'a, 'tcx>, impl_item: &'tcx ImplItem<'_>) {
+        if in_external_macro(cx.sess(), impl_item.span) {
             return;
         }
+
+        let parent_id = cx.tcx.hir().get_parent_item(impl_item.hir_id);
+        let imp = cx.tcx.hir().expect_item(parent_id);
+
         if_chain! {
-            if let ItemKind::Impl{ self_ty: ref item_type, items: refs, .. } = item.kind;
-            if let TyKind::Path(QPath::Resolved(_, ref item_path)) = item_type.kind;
+            if let ItemKind::Impl { self_ty: hir_self_ty, .. } = imp.kind;
+            if let TyKind::Path(QPath::Resolved(_, ref item_path)) = hir_self_ty.kind;
             then {
                 let parameters = &item_path.segments.last().expect(SEGMENTS_MSG).args;
                 let should_check = if let Some(ref params) = *parameters {
-                    !params.parenthesized && !params.args.iter().any(|arg| match arg {
-                        GenericArg::Lifetime(_) => true,
-                        _ => false,
-                    })
+                    !params.parenthesized
+                        && !params.args.iter().any(|arg| match arg {
+                            GenericArg::Lifetime(_) => true,
+                            _ => false,
+                        })
                 } else {
                     true
                 };
 
                 // TODO: don't short-circuit upon lifetime parameters
                 if should_check {
-                    let self_ty= hir_ty_to_ty(cx.tcx, item_type);
-                    let visitor = &mut ImplVisitor {
-                        cx,
-                        item,
-                        self_ty,
-                    };
+                    let self_ty = hir_ty_to_ty(cx.tcx, hir_self_ty);
+                    let visitor = &mut ImplVisitor { cx, self_ty };
 
-                    for impl_item_ref in refs {
-                        let impl_item = cx.tcx.hir().impl_item(impl_item_ref.id);
-                        visitor.visit_impl_item(impl_item);
+                    let tcx = cx.tcx;
+                    let impl_def_id = tcx.hir().local_def_id(imp.hir_id);
+                    let impl_trait_ref = tcx.impl_trait_ref(impl_def_id);
+                    if_chain! {
+                        if let Some(impl_trait_ref) = impl_trait_ref;
+                        if let ImplItemKind::Fn(FnSig { decl: impl_decl, .. }, impl_body_id) = &impl_item.kind;
+                        then {
+                            visitor.check_trait_method_impl_decl(impl_item, impl_decl, impl_trait_ref);
+                            let body = tcx.hir().body(*impl_body_id);
+                            visitor.visit_body(body);
+                        } else {
+                            walk_impl_item(visitor, impl_item)
+                        }
                     }
-                }
+               }
             }
         }
     }

diff --git a/tests/ui/use_self.fixed b/tests/ui/use_self.fixed
@@ -378,3 +378,48 @@ mod issue3859 {
         }
     }
 } */
+
+mod lint_at_item_level {
+    struct Foo {}
+
+    #[allow(clippy::use_self)]
+    impl Foo {
+        fn new() -> Foo {
+            Foo {}
+        }
+        fn test() -> Foo {
+            Foo::new()
+        }
+    }
+
+    #[allow(clippy::use_self)]
+    impl Default for Foo {
+        fn default() -> Foo {
+            Foo::new()
+        }
+    }
+}
+
+ mod lint_at_impl_item_level {
+    struct Foo {}
+
+    impl Foo {
+        #[allow(clippy::use_self)]
+        fn new() -> Foo {
+            Foo {}
+        }
+
+        #[allow(clippy::use_self)]
+        fn test() -> Foo {
+            Foo::new()
+        }
+    }
+
+    impl Default for Foo {
+
+        #[allow(clippy::use_self)]
+        fn default() -> Foo {
+            Foo::new()
+        }
+    }
+ }
diff --git a/tests/ui/use_self.rs b/tests/ui/use_self.rs
@@ -378,3 +378,47 @@ mod issue3859 {
         }
     }
 } */
+
+mod lint_at_item_level {
+    struct Foo {}
+
+    #[allow(clippy::use_self)]
+    impl Foo {
+        fn new() -> Foo {
+            Foo {}
+        }
+        fn test() -> Foo {
+            Foo::new()
+        }
+    }
+
+    #[allow(clippy::use_self)]
+    impl Default for Foo {
+        fn default() -> Foo {
+            Foo::new()
+        }
+    }
+}
+
+mod lint_at_impl_item_level {
+    struct Foo {}
+
+    impl Foo {
+        #[allow(clippy::use_self)]
+        fn new() -> Foo {
+            Foo {}
+        }
+
+        #[allow(clippy::use_self)]
+        fn test() -> Foo {
+            Foo::new()
+        }
+    }
+
+    impl Default for Foo {
+        #[allow(clippy::use_self)]
+        fn default() -> Foo {
+            Foo::new()
+        }
+    }
+}
diff --git a/tests/ui/use_self.stderr b/tests/ui/use_self.stderr
@@ -70,18 +70,6 @@ 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:151:21
-   |
-LL |         fn baz() -> Foo {
-   |                     ^^^ help: use the applicable keyword: `Self`
-
-error: unnecessary structure name repetition
-  --> $DIR/use_self.rs:152:13
-   |
-LL |             Foo {}
-   |             ^^^ help: use the applicable keyword: `Self`
-
 error: unnecessary structure name repetition
   --> $DIR/use_self.rs:139:29
    |
@@ -94,6 +82,18 @@ error: unnecessary structure name repetition
 LL |                     Bar { foo: Foo {} }
    |                     ^^^ help: use the applicable keyword: `Self`
 
+error: unnecessary structure name repetition
+  --> $DIR/use_self.rs:151:21
+   |
+LL |         fn baz() -> Foo {
+   |                     ^^^ help: use the applicable keyword: `Self`
+
+error: unnecessary structure name repetition
+  --> $DIR/use_self.rs:152:13
+   |
+LL |             Foo {}
+   |             ^^^ help: use the applicable keyword: `Self`
+
 error: unnecessary structure name repetition
   --> $DIR/use_self.rs:169:21
    |