Fix FP in new_ret_no_self: trigger in trait def instead of impl block

ThibsG · ThibsG · commit 2a3ee5fa854b · 2020-08-27T18:25:38.000+02:00
diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs
@@ -15,6 +15,7 @@ use rustc_ast::ast;
 use rustc_errors::Applicability;
 use rustc_hir as hir;
 use rustc_hir::intravisit::{self, Visitor};
+use rustc_hir::{FnRetTy, FnSig, TraitItem, TraitItemKind};
 use rustc_lint::{LateContext, LateLintPass, Lint, LintContext};
 use rustc_middle::hir::map::Map;
 use rustc_middle::lint::in_external_macro;
@@ -28,11 +29,11 @@ use crate::consts::{constant, Constant};
 use crate::utils::usage::mutated_variables;
 use crate::utils::{
     get_arg_name, get_parent_expr, get_trait_def_id, has_iter_method, higher, implements_trait, in_macro, is_copy,
-    is_ctor_or_promotable_const_function, is_expn_of, is_type_diagnostic_item, iter_input_pats, last_path_segment,
-    match_def_path, match_qpath, match_trait_method, match_type, match_var, method_calls, method_chain_args, paths,
-    remove_blocks, return_ty, single_segment_path, snippet, snippet_with_applicability, snippet_with_macro_callsite,
-    span_lint, span_lint_and_help, span_lint_and_note, span_lint_and_sugg, span_lint_and_then, sugg, walk_ptrs_ty,
-    walk_ptrs_ty_depth, SpanlessEq,
+    is_ctor_or_promotable_const_function, is_expn_of, is_self_ty, is_type_diagnostic_item, iter_input_pats,
+    last_path_segment, match_def_path, match_qpath, match_trait_method, match_type, match_var, method_calls,
+    method_chain_args, paths, remove_blocks, return_ty, single_segment_path, snippet, snippet_with_applicability,
+    snippet_with_macro_callsite, span_lint, span_lint_and_help, span_lint_and_note, span_lint_and_sugg,
+    span_lint_and_then, sugg, walk_ptrs_ty, walk_ptrs_ty_depth, SpanlessEq,
 };
 
 declare_clippy_lint! {
@@ -1631,6 +1632,11 @@ 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);
 
@@ -1670,6 +1676,48 @@ impl<'tcx> LateLintPass<'tcx> for Methods {
             }
         }
     }
+
+    fn check_trait_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx TraitItem<'_>) {
+        if_chain! {
+            if !in_external_macro(cx.tcx.sess, item.span);
+            if !item.span.from_expansion();
+            if item.ident.name == sym!(new);
+            if let TraitItemKind::Fn(FnSig { decl, .. }, _) = &item.kind;
+            if let FnRetTy::Return(ret_ty) = &decl.output;
+
+            then {
+                let mut visitor = HasSelfVisitor { has_self_ty: false };
+                visitor.visit_ty(ret_ty);
+                if !visitor.has_self_ty {
+                    span_lint(
+                        cx,
+                        NEW_RET_NO_SELF,
+                        item.span,
+                        "methods called `new` usually return `Self`",
+                    );
+                }
+            }
+        }
+    }
+}
+
+struct HasSelfVisitor {
+    pub has_self_ty: bool,
+}
+
+impl<'tcx> intravisit::Visitor<'tcx> for HasSelfVisitor {
+    type Map = Map<'tcx>;
+
+    fn visit_ty(&mut self, ty: &'tcx hir::Ty<'_>) {
+        if is_self_ty(ty) {
+            self.has_self_ty = true;
+        } else {
+            intravisit::walk_ty(self, ty);
+        }
+    }
+    fn nested_visit_map(&mut self) -> intravisit::NestedVisitorMap<Self::Map> {
+        intravisit::NestedVisitorMap::None
+    }
 }
 
 /// Checks for the `OR_FUN_CALL` lint.
diff --git a/tests/ui/new_ret_no_self.rs b/tests/ui/new_ret_no_self.rs
@@ -210,3 +210,133 @@ impl<'a> WithLifetime<'a> {
         unimplemented!();
     }
 }
+
+mod issue5435 {
+    struct V;
+
+    pub trait TraitRetSelf {
+        // should not trigger lint
+        fn new() -> Self;
+    }
+
+    pub trait TraitRet {
+        // should trigger lint as we are in trait definition
+        fn new() -> String;
+    }
+    pub struct StructRet;
+    impl TraitRet for StructRet {
+        // should not trigger lint as we are in the impl block
+        fn new() -> String {
+            unimplemented!();
+        }
+    }
+
+    pub trait TraitRet2 {
+        // should trigger lint
+        fn new(_: String) -> String;
+    }
+
+    trait TupleReturnerOk {
+        // should not trigger lint
+        fn new() -> (Self, u32)
+        where
+            Self: Sized,
+        {
+            unimplemented!();
+        }
+    }
+
+    trait TupleReturnerOk2 {
+        // should not trigger lint (it doesn't matter which element in the tuple is Self)
+        fn new() -> (u32, Self)
+        where
+            Self: Sized,
+        {
+            unimplemented!();
+        }
+    }
+
+    trait TupleReturnerOk3 {
+        // should not trigger lint (tuple can contain multiple Self)
+        fn new() -> (Self, Self)
+        where
+            Self: Sized,
+        {
+            unimplemented!();
+        }
+    }
+
+    trait TupleReturnerBad {
+        // should trigger lint
+        fn new() -> (u32, u32) {
+            unimplemented!();
+        }
+    }
+
+    trait MutPointerReturnerOk {
+        // should not trigger lint
+        fn new() -> *mut Self
+        where
+            Self: Sized,
+        {
+            unimplemented!();
+        }
+    }
+
+    trait MutPointerReturnerOk2 {
+        // should not trigger lint
+        fn new() -> *const Self
+        where
+            Self: Sized,
+        {
+            unimplemented!();
+        }
+    }
+
+    trait MutPointerReturnerBad {
+        // should trigger lint
+        fn new() -> *mut V {
+            unimplemented!();
+        }
+    }
+
+    trait GenericReturnerOk {
+        // should not trigger lint
+        fn new() -> Option<Self>
+        where
+            Self: Sized,
+        {
+            unimplemented!();
+        }
+    }
+
+    trait NestedReturnerOk {
+        // should not trigger lint
+        fn new() -> (Option<Self>, u32)
+        where
+            Self: Sized,
+        {
+            unimplemented!();
+        }
+    }
+
+    trait NestedReturnerOk2 {
+        // should not trigger lint
+        fn new() -> ((Self, u32), u32)
+        where
+            Self: Sized,
+        {
+            unimplemented!();
+        }
+    }
+
+    trait NestedReturnerOk3 {
+        // should not trigger lint
+        fn new() -> Option<(Self, u32)>
+        where
+            Self: Sized,
+        {
+            unimplemented!();
+        }
+    }
+}
diff --git a/tests/ui/new_ret_no_self.stderr b/tests/ui/new_ret_no_self.stderr
@@ -48,5 +48,33 @@ LL | |         unimplemented!();
 LL | |     }
    | |_____^
 
-error: aborting due to 6 previous errors
+error: methods called `new` usually return `Self`
+  --> $DIR/new_ret_no_self.rs:224:9
+   |
+LL |         fn new() -> String;
+   |         ^^^^^^^^^^^^^^^^^^^
+
+error: methods called `new` usually return `Self`
+  --> $DIR/new_ret_no_self.rs:236:9
+   |
+LL |         fn new(_: String) -> String;
+   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: methods called `new` usually return `Self`
+  --> $DIR/new_ret_no_self.rs:271:9
+   |
+LL | /         fn new() -> (u32, u32) {
+LL | |             unimplemented!();
+LL | |         }
+   | |_________^
+
+error: methods called `new` usually return `Self`
+  --> $DIR/new_ret_no_self.rs:298:9
+   |
+LL | /         fn new() -> *mut V {
+LL | |             unimplemented!();
+LL | |         }
+   | |_________^
+
+error: aborting due to 10 previous errors
 
