Merge pull request #3253 from JoshMcguigan/new_ret_no_self-3220

new_ret_no_self

Author: oli-obk

clippy_lints/src/methods/mod.rs

@@ -11,7 +11,7 @@
 use crate::rustc::hir;
 use crate::rustc::hir::def::Def;
 use crate::rustc::lint::{in_external_macro, LateContext, LateLintPass, Lint, LintArray, LintContext, LintPass};
-use crate::rustc::ty::{self, Ty};
+use crate::rustc::ty::{self, Ty, TyKind, Predicate};
 use crate::rustc::{declare_tool_lint, lint_array};
 use crate::rustc_errors::Applicability;
 use crate::syntax::ast;
@@ -878,6 +878,8 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass {
         let name = implitem.ident.name;
         let parent = cx.tcx.hir.get_parent(implitem.id);
         let item = cx.tcx.hir.expect_item(parent);
+        let def_id = cx.tcx.hir.local_def_id(item.id);
+        let ty = cx.tcx.type_of(def_id);
         if_chain! {
             if let hir::ImplItemKind::Method(ref sig, id) = implitem.node;
             if let Some(first_arg_ty) = sig.decl.inputs.get(0);
@@ -899,8 +901,6 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass {
                 }
 
                 // check conventions w.r.t. conversion method names and predicates
-                let def_id = cx.tcx.hir.local_def_id(item.id);
-                let ty = cx.tcx.type_of(def_id);
                 let is_copy = is_copy(cx, ty);
                 for &(ref conv, self_kinds) in &CONVENTIONS {
                     if conv.check(&name.as_str()) {
@@ -928,16 +928,37 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass {
                         break;
                     }
                 }
+            }
+        }
+
+        if let hir::ImplItemKind::Method(_, _) = implitem.node {
+            let ret_ty = return_ty(cx, implitem.id);
+
+            // if return type is impl trait
+            if let TyKind::Opaque(def_id, _) = ret_ty.sty {
+
+                // then 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), _) => {
+                            let binder = poly_projection_predicate.ty();
+                            let associated_type = binder.skip_binder();
+                            let associated_type_is_self_type = same_tys(cx, ty, associated_type);
 
-                let ret_ty = return_ty(cx, implitem.id);
-                if name == "new" &&
-                   !ret_ty.walk().any(|t| same_tys(cx, t, ty)) {
-                    span_lint(cx,
-                              NEW_RET_NO_SELF,
-                              implitem.span,
-                              "methods called `new` usually return `Self`");
+                            // if the associated type is self, early return and do not trigger lint
+                            if associated_type_is_self_type { return; }
+                        },
+                        (_, _) => {},
+                    }
                 }
             }
+
+            if name == "new" && !same_tys(cx, ret_ty, ty) {
+                span_lint(cx,
+                          NEW_RET_NO_SELF,
+                          implitem.span,
+                          "methods called `new` usually return `Self`");
+            }
         }
     }
 }

clippy_lints/src/types.rs

@@ -1920,6 +1920,7 @@ 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/methods.rs

@@ -14,7 +14,7 @@
 #![warn(clippy::all, clippy::pedantic, clippy::option_unwrap_used)]
 #![allow(clippy::blacklisted_name, unused, clippy::print_stdout, clippy::non_ascii_literal, clippy::new_without_default,
     clippy::new_without_default_derive, clippy::missing_docs_in_private_items, clippy::needless_pass_by_value,
-    clippy::default_trait_access, clippy::use_self, clippy::useless_format)]
+    clippy::default_trait_access, clippy::use_self, clippy::new_ret_no_self, clippy::useless_format)]
 
 use std::collections::BTreeMap;
 use std::collections::HashMap;
@@ -43,7 +43,7 @@ impl T {
 
     fn to_something(self) -> u32 { 0 }
 
-    fn new(self) {}
+    fn new(self) -> Self { unimplemented!(); }
 }
 
 struct Lt<'a> {

tests/ui/methods.stderr

@@ -23,17 +23,9 @@ error: methods called `to_*` usually take self by reference; consider choosing a
 error: methods called `new` usually take no self; consider choosing a less ambiguous name
   --> $DIR/methods.rs:46:12
    |
-46 |     fn new(self) {}
+46 |     fn new(self) -> Self { unimplemented!(); }
    |            ^^^^
 
-error: methods called `new` usually return `Self`
-  --> $DIR/methods.rs:46:5
-   |
-46 |     fn new(self) {}
-   |     ^^^^^^^^^^^^^^^
-   |
-   = note: `-D clippy::new-ret-no-self` implied by `-D warnings`
-
 error: called `map(f).unwrap_or(a)` on an Option value. This can be done more directly by calling `map_or(a, f)` instead
    --> $DIR/methods.rs:114:13
     |
@@ -465,5 +457,5 @@ error: used unwrap() on an Option value. If you don't want to handle the None ca
     |
     = note: `-D clippy::option-unwrap-used` implied by `-D warnings`
 
-error: aborting due to 58 previous errors
+error: aborting due to 57 previous errors
 

tests/ui/new_ret_no_self.rs

@@ -0,0 +1,93 @@
+#![warn(clippy::new_ret_no_self)]
+#![allow(dead_code, clippy::trivially_copy_pass_by_ref)]
+
+fn main(){}
+
+trait R {
+    type Item;
+}
+
+trait Q {
+    type Item;
+    type Item2;
+}
+
+struct S;
+
+impl R for S {
+    type Item = Self;
+}
+
+impl S {
+    // should not trigger the lint
+    pub fn new() -> impl R<Item = Self> {
+        S
+    }
+}
+
+struct S2;
+
+impl R for S2 {
+    type Item = Self;
+}
+
+impl S2 {
+    // should not trigger the lint
+    pub fn new(_: String) -> impl R<Item = Self> {
+        S2
+    }
+}
+
+struct S3;
+
+impl R for S3 {
+    type Item = u32;
+}
+
+impl S3 {
+    // should trigger the lint
+    pub fn new(_: String) -> impl R<Item = u32> {
+        S3
+    }
+}
+
+struct S4;
+
+impl Q for S4 {
+    type Item = u32;
+    type Item2 = Self;
+}
+
+impl S4 {
+    // should not trigger the lint
+    pub fn new(_: String) -> impl Q<Item = u32, Item2 = Self> {
+        S4
+    }
+}
+
+struct T;
+
+impl T {
+    // should not trigger lint
+    pub fn new() -> Self {
+        unimplemented!();
+    }
+}
+
+struct U;
+
+impl U {
+    // should trigger lint
+    pub fn new() -> u32 {
+        unimplemented!();
+    }
+}
+
+struct V;
+
+impl V {
+    // should trigger lint
+    pub fn new(_: String) -> u32 {
+        unimplemented!();
+    }
+}

tests/ui/new_ret_no_self.stderr

@@ -0,0 +1,28 @@
+error: methods called `new` usually return `Self`
+  --> $DIR/new_ret_no_self.rs:49:5
+   |
+49 | /     pub fn new(_: String) -> impl R<Item = u32> {
+50 | |         S3
+51 | |     }
+   | |_____^
+   |
+   = note: `-D clippy::new-ret-no-self` implied by `-D warnings`
+
+error: methods called `new` usually return `Self`
+  --> $DIR/new_ret_no_self.rs:81:5
+   |
+81 | /     pub fn new() -> u32 {
+82 | |         unimplemented!();
+83 | |     }
+   | |_____^
+
+error: methods called `new` usually return `Self`
+  --> $DIR/new_ret_no_self.rs:90:5
+   |
+90 | /     pub fn new(_: String) -> u32 {
+91 | |         unimplemented!();
+92 | |     }
+   | |_____^
+
+error: aborting due to 3 previous errors
+