diff options
| -rw-r--r-- | clippy_lints/src/methods/mod.rs | 41 | ||||
| -rw-r--r-- | clippy_lints/src/types.rs | 1 | ||||
| -rw-r--r-- | tests/ui/methods.rs | 4 | ||||
| -rw-r--r-- | tests/ui/methods.stderr | 12 | ||||
| -rw-r--r-- | tests/ui/new_ret_no_self.rs | 93 | ||||
| -rw-r--r-- | tests/ui/new_ret_no_self.stderr | 28 |
6 files changed, 157 insertions, 22 deletions
diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 7c15eb677cc..f9c010beea7 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/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`"); + } } } } diff --git a/clippy_lints/src/types.rs b/clippy_lints/src/types.rs index 035ca2b0496..59c55168232 100644 --- a/clippy_lints/src/types.rs +++ b/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()? diff --git a/tests/ui/methods.rs b/tests/ui/methods.rs index 883dbf589d7..ae1b1642be7 100644 --- a/tests/ui/methods.rs +++ b/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> { diff --git a/tests/ui/methods.stderr b/tests/ui/methods.stderr index 124edee6a52..307814824ea 100644 --- a/tests/ui/methods.stderr +++ b/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 diff --git a/tests/ui/new_ret_no_self.rs b/tests/ui/new_ret_no_self.rs new file mode 100644 index 00000000000..1a4b91cc9da --- /dev/null +++ b/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!(); + } +} diff --git a/tests/ui/new_ret_no_self.stderr b/tests/ui/new_ret_no_self.stderr new file mode 100644 index 00000000000..ad26438d4ef --- /dev/null +++ b/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 + |