Auto merge of rust-lang#5774 - ThibsG:FixNewRetNoSelf, r=ebroto

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

Lint in trait def instead of impl block.

Fixes: rust-lang#5435

changelog: none

Author: bors

clippy_lints/src/methods/mod.rs

@@ -15,24 +15,24 @@ use rustc_ast::ast;
 use rustc_errors::Applicability;
 use rustc_hir as hir;
 use rustc_hir::intravisit::{self, Visitor};
+use rustc_hir::{TraitItem, TraitItemKind};
 use rustc_lint::{LateContext, LateLintPass, Lint, LintContext};
 use rustc_middle::hir::map::Map;
 use rustc_middle::lint::in_external_macro;
-use rustc_middle::ty::subst::GenericArgKind;
-use rustc_middle::ty::{self, Ty, TyS};
+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 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,
+    contains_ty, 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,
 };
 
 declare_clippy_lint! {
@@ -724,6 +724,7 @@ declare_clippy_lint! {
     /// **Known problems:** None.
     ///
     /// **Example:**
+    /// In an impl block:
     /// ```rust
     /// # struct Foo;
     /// # struct NotAFoo;
@@ -736,25 +737,40 @@ declare_clippy_lint! {
     ///
     /// ```rust
     /// # struct Foo;
-    /// # struct FooError;
+    /// struct Bar(Foo);
     /// impl Foo {
-    ///     // Good. Return type contains `Self`
-    ///     fn new() -> Result<Foo, FooError> {
-    ///         # Ok(Foo)
+    ///     // Bad. The type name must contain `Self`
+    ///     fn new() -> Bar {
+    /// # Bar(Foo)
     ///     }
     /// }
     /// ```
     ///
     /// ```rust
     /// # struct Foo;
-    /// struct Bar(Foo);
+    /// # struct FooError;
     /// impl Foo {
-    ///     // Bad. The type name must contain `Self`.
-    ///     fn new() -> Bar {
-    ///         # Bar(Foo)
+    ///     // Good. Return type contains `Self`
+    ///     fn new() -> Result<Foo, FooError> {
+    /// # Ok(Foo)
     ///     }
     /// }
     /// ```
+    ///
+    /// Or in a trait definition:
+    /// ```rust
+    /// pub trait Trait {
+    ///     // Bad. The type name must contain `Self`
+    ///     fn new();
+    /// }
+    /// ```
+    ///
+    /// ```rust
+    /// pub trait Trait {
+    ///     // Good. Return type contains `Self`
+    ///     fn new() -> Self;
+    /// }
+    /// ```
     pub NEW_RET_NO_SELF,
     style,
     "not returning type containing `Self` in a `new` method"
@@ -1631,19 +1647,16 @@ 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);
 
-            let contains_self_ty = |ty: Ty<'tcx>| {
-                ty.walk().any(|inner| match inner.unpack() {
-                    GenericArgKind::Type(inner_ty) => TyS::same_type(self_ty, inner_ty),
-
-                    GenericArgKind::Lifetime(_) | GenericArgKind::Const(_) => false,
-                })
-            };
-
             // walk the return type and check for Self (this does not check associated types)
-            if contains_self_ty(ret_ty) {
+            if contains_ty(ret_ty, self_ty) {
                 return;
             }
 
@@ -1653,7 +1666,7 @@ impl<'tcx> LateLintPass<'tcx> for Methods {
                 for &(predicate, _span) in cx.tcx.predicates_of(def_id).predicates {
                     if let ty::PredicateAtom::Projection(projection_predicate) = predicate.skip_binders() {
                         // walk the associated type and check for Self
-                        if contains_self_ty(projection_predicate.ty) {
+                        if contains_ty(projection_predicate.ty, self_ty) {
                             return;
                         }
                     }
@@ -1670,6 +1683,26 @@ 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.ident.name == sym!(new);
+            if let TraitItemKind::Fn(_, _) = item.kind;
+            let ret_ty = return_ty(cx, item.hir_id);
+            let self_ty = TraitRef::identity(cx.tcx, item.hir_id.owner.to_def_id()).self_ty();
+            if !contains_ty(ret_ty, self_ty);
+
+            then {
+                span_lint(
+                    cx,
+                    NEW_RET_NO_SELF,
+                    item.span,
+                    "methods called `new` usually return `Self`",
+                );
+            }
+        }
+    }
 }
 
 /// Checks for the `OR_FUN_CALL` lint.

clippy_lints/src/utils/mod.rs

@@ -42,7 +42,8 @@ use rustc_hir::{
 use rustc_infer::infer::TyCtxtInferExt;
 use rustc_lint::{LateContext, Level, Lint, LintContext};
 use rustc_middle::hir::map::Map;
-use rustc_middle::ty::{self, layout::IntegerExt, subst::GenericArg, Ty, TyCtxt, TypeFoldable};
+use rustc_middle::ty::subst::{GenericArg, GenericArgKind};
+use rustc_middle::ty::{self, layout::IntegerExt, Ty, TyCtxt, TypeFoldable};
 use rustc_mir::const_eval;
 use rustc_span::hygiene::{ExpnKind, MacroKind};
 use rustc_span::source_map::original_sp;
@@ -866,6 +867,14 @@ pub fn return_ty<'tcx>(cx: &LateContext<'tcx>, fn_item: hir::HirId) -> Ty<'tcx>
     cx.tcx.erase_late_bound_regions(&ret_ty)
 }
 
+/// Walks into `ty` and returns `true` if any inner type is the same as `other_ty`
+pub fn contains_ty(ty: Ty<'_>, other_ty: Ty<'_>) -> bool {
+    ty.walk().any(|inner| match inner.unpack() {
+        GenericArgKind::Type(inner_ty) => ty::TyS::same_type(other_ty, inner_ty),
+        GenericArgKind::Lifetime(_) | GenericArgKind::Const(_) => false,
+    })
+}
+
 /// Returns `true` if the given type is an `unsafe` function.
 pub fn type_is_unsafe_function<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> bool {
     match ty.kind {

tests/ui/new_ret_no_self.rs

@@ -137,9 +137,9 @@ impl MutPointerReturnerOk {
     }
 }
 
-struct MutPointerReturnerOk2;
+struct ConstPointerReturnerOk2;
 
-impl MutPointerReturnerOk2 {
+impl ConstPointerReturnerOk2 {
     // should not trigger lint
     pub fn new() -> *const Self {
         unimplemented!();
@@ -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 ConstPointerReturnerOk2 {
+        // 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!();
+        }
+    }
+}

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