Merge #3338

3338: new_ret_no_self false positives r=flip1995 a=JoshMcguigan

~~WORK IN PROGRESS~~

I plan to fix all of the false positives in #3313 in this PR, but I wanted to open it now to start gathering feedback.

In this first commit, I've updated the lint to allow tuple return types as long as `Self` shows up at least once, in any position of the tuple. I believe this is the broadest possible interpretation of what should be allowed for tuple return types, but I would certainly be okay making the lint more strict. 

fixes #3313 

Co-authored-by: Josh Mcguigan <[email protected]>

clippy_lints/src/methods/mod.rs

@@ -936,10 +936,15 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass {
         if let hir::ImplItemKind::Method(_, _) = implitem.node {
             let ret_ty = return_ty(cx, implitem.id);
 
-            // if return type is impl trait
+            // walk the return type and check for Self (this does not check associated types)
+            for inner_type in ret_ty.walk() {
+                if same_tys(cx, ty, inner_type) { return; }
+            }
+
+            // if return type is impl trait, check the associated types
             if let TyKind::Opaque(def_id, _) = ret_ty.sty {
 
-                // then one of the associated types must be Self
+                // one of the associated types must be Self
                 for predicate in cx.tcx.predicates_of(def_id).predicates.iter() {
                     match predicate {
                         (Predicate::Projection(poly_projection_predicate), _) => {

clippy_lints/src/types.rs

@@ -1920,7 +1920,6 @@ enum ImplicitHasherType<'tcx> {
 
 impl<'tcx> ImplicitHasherType<'tcx> {
     /// Checks that `ty` is a target type without a BuildHasher.
-    #[allow(clippy::new_ret_no_self)]
     fn new<'a>(cx: &LateContext<'a, 'tcx>, hir_ty: &hir::Ty) -> Option<Self> {
         if let TyKind::Path(QPath::Resolved(None, ref path)) = hir_ty.node {
             let params: Vec<_> = path.segments.last().as_ref()?.args.as_ref()?

tests/ui/new_ret_no_self.rs

@@ -91,3 +91,87 @@ impl V {
         unimplemented!();
     }
 }
+
+struct TupleReturnerOk;
+
+impl TupleReturnerOk {
+    // should not trigger lint
+    pub fn new() -> (Self, u32) { unimplemented!(); }
+}
+
+struct TupleReturnerOk2;
+
+impl TupleReturnerOk2 {
+    // should not trigger lint (it doesn't matter which element in the tuple is Self)
+    pub fn new() -> (u32, Self) { unimplemented!(); }
+}
+
+struct TupleReturnerOk3;
+
+impl TupleReturnerOk3 {
+    // should not trigger lint (tuple can contain multiple Self)
+    pub fn new() -> (Self, Self) { unimplemented!(); }
+}
+
+struct TupleReturnerBad;
+
+impl TupleReturnerBad {
+    // should trigger lint
+    pub fn new() -> (u32, u32) { unimplemented!(); }
+}
+
+struct MutPointerReturnerOk;
+
+impl MutPointerReturnerOk {
+    // should not trigger lint
+    pub fn new() -> *mut Self { unimplemented!(); }
+}
+
+struct MutPointerReturnerOk2;
+
+impl MutPointerReturnerOk2 {
+    // should not trigger lint
+    pub fn new() -> *const Self { unimplemented!(); }
+}
+
+struct MutPointerReturnerBad;
+
+impl MutPointerReturnerBad {
+    // should trigger lint
+    pub fn new() -> *mut V { unimplemented!(); }
+}
+
+struct GenericReturnerOk;
+
+impl GenericReturnerOk {
+    // should not trigger lint
+    pub fn new() -> Option<Self> { unimplemented!(); }
+}
+
+struct GenericReturnerBad;
+
+impl GenericReturnerBad {
+    // should trigger lint
+    pub fn new() -> Option<u32> { unimplemented!(); }
+}
+
+struct NestedReturnerOk;
+
+impl NestedReturnerOk {
+    // should not trigger lint
+    pub fn new() -> (Option<Self>, u32) { unimplemented!(); }
+}
+
+struct NestedReturnerOk2;
+
+impl NestedReturnerOk2 {
+    // should not trigger lint
+    pub fn new() -> ((Self, u32), u32) { unimplemented!(); }
+}
+
+struct NestedReturnerOk3;
+
+impl NestedReturnerOk3 {
+    // should not trigger lint
+    pub fn new() -> Option<(Self, u32)> { unimplemented!(); }
+}

tests/ui/new_ret_no_self.stderr

@@ -24,5 +24,23 @@ error: methods called `new` usually return `Self`
 92 | |     }
    | |_____^
 
-error: aborting due to 3 previous errors
+error: methods called `new` usually return `Self`
+   --> $DIR/new_ret_no_self.rs:120:5
+    |
+120 |     pub fn new() -> (u32, u32) { unimplemented!(); }
+    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: methods called `new` usually return `Self`
+   --> $DIR/new_ret_no_self.rs:141:5
+    |
+141 |     pub fn new() -> *mut V { unimplemented!(); }
+    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: methods called `new` usually return `Self`
+   --> $DIR/new_ret_no_self.rs:155:5
+    |
+155 |     pub fn new() -> Option<u32> { unimplemented!(); }
+    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: aborting due to 6 previous errors