diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index cb7229ba8a4b..0ae25c3e5f58 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -105,7 +105,7 @@ use bind_instead_of_map::BindInsteadOfMap; use clippy_utils::consts::{constant, Constant}; use clippy_utils::diagnostics::{span_lint, span_lint_and_help}; use clippy_utils::msrvs::{self, Msrv}; -use clippy_utils::ty::{contains_ty_adt_constructor_opaque, implements_trait, is_copy, is_type_diagnostic_item}; +use clippy_utils::ty::{contains_adt_constructor, implements_trait, is_copy, is_type_diagnostic_item}; use clippy_utils::{contains_return, is_bool, is_trait_method, iter_input_pats, return_ty}; use if_chain::if_chain; use rustc_hir as hir; @@ -3414,10 +3414,38 @@ impl<'tcx> LateLintPass<'tcx> for Methods { if let hir::ImplItemKind::Fn(_, _) = impl_item.kind { let ret_ty = return_ty(cx, impl_item.hir_id()); - if contains_ty_adt_constructor_opaque(cx, ret_ty, self_ty) { + // walk the return type and check for Self (this does not check associated types) + if let Some(self_adt) = self_ty.ty_adt_def() { + if contains_adt_constructor(ret_ty, self_adt) { + return; + } + } else if ret_ty.contains(self_ty) { return; } + // if return type is impl trait, check the associated types + if let ty::Opaque(def_id, _) = *ret_ty.kind() { + // one of the associated types must be Self + for &(predicate, _span) in cx.tcx.explicit_item_bounds(def_id) { + if let ty::PredicateKind::Clause(ty::Clause::Projection(projection_predicate)) = + predicate.kind().skip_binder() + { + let assoc_ty = match projection_predicate.term.unpack() { + ty::TermKind::Ty(ty) => ty, + ty::TermKind::Const(_c) => continue, + }; + // walk the associated type and check for Self + if let Some(self_adt) = self_ty.ty_adt_def() { + if contains_adt_constructor(assoc_ty, self_adt) { + return; + } + } else if assoc_ty.contains(self_ty) { + return; + } + } + } + } + if name == "new" && ret_ty != self_ty { span_lint( cx, diff --git a/clippy_utils/src/ty.rs b/clippy_utils/src/ty.rs index 81b08ae5600d..8dec2bebfbf3 100644 --- a/clippy_utils/src/ty.rs +++ b/clippy_utils/src/ty.rs @@ -63,58 +63,6 @@ pub fn contains_adt_constructor<'tcx>(ty: Ty<'tcx>, adt: AdtDef<'tcx>) -> bool { }) } -/// Walks into `ty` and returns `true` if any inner type is an instance of the given type, or adt -/// constructor of the same type. -/// -/// This method also recurses into opaque type predicates, so call it with `impl Trait` and `U` -/// will also return `true`. -pub fn contains_ty_adt_constructor_opaque<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>, needle: Ty<'tcx>) -> bool { - ty.walk().any(|inner| match inner.unpack() { - GenericArgKind::Type(inner_ty) => { - if inner_ty == needle { - return true; - } - - if inner_ty.ty_adt_def() == needle.ty_adt_def() { - return true; - } - - if let ty::Opaque(def_id, _) = *inner_ty.kind() { - for &(predicate, _span) in cx.tcx.explicit_item_bounds(def_id) { - match predicate.kind().skip_binder() { - // For `impl Trait`, it will register a predicate of `T: Trait`, so we go through - // and check substituions to find `U`. - ty::PredicateKind::Clause(ty::Clause::Trait(trait_predicate)) => { - if trait_predicate - .trait_ref - .substs - .types() - .skip(1) // Skip the implicit `Self` generic parameter - .any(|ty| contains_ty_adt_constructor_opaque(cx, ty, needle)) - { - return true; - } - }, - // For `impl Trait`, it will register a predicate of `::Assoc = U`, - // so we check the term for `U`. - ty::PredicateKind::Clause(ty::Clause::Projection(projection_predicate)) => { - if let ty::TermKind::Ty(ty) = projection_predicate.term.unpack() { - if contains_ty_adt_constructor_opaque(cx, ty, needle) { - return true; - } - }; - }, - _ => (), - } - } - } - - false - }, - GenericArgKind::Lifetime(_) | GenericArgKind::Const(_) => false, - }) -} - /// Resolves `::Item` for `T` /// Do not invoke without first verifying that the type implements `Iterator` pub fn get_iterator_item_ty<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> Option> { diff --git a/tests/ui/new_ret_no_self.rs b/tests/ui/new_ret_no_self.rs index f69982d63a89..247e7554719a 100644 --- a/tests/ui/new_ret_no_self.rs +++ b/tests/ui/new_ret_no_self.rs @@ -351,52 +351,12 @@ impl RetOtherSelf { } } -mod issue7344 { - struct RetImplTraitSelf(T); +mod issue10041 { + struct Bomb; - impl RetImplTraitSelf { - // should not trigger lint - fn new(t: T) -> impl Into { - Self(t) - } - } - - struct RetImplTraitNoSelf(T); - - impl RetImplTraitNoSelf { - // should trigger lint - fn new(t: T) -> impl Into { - 1 - } - } - - trait Trait2 {} - impl Trait2 for () {} - - struct RetImplTraitSelf2(T); - - impl RetImplTraitSelf2 { - // should not trigger lint - fn new(t: T) -> impl Trait2<(), Self> { - unimplemented!() - } - } - - struct RetImplTraitNoSelf2(T); - - impl RetImplTraitNoSelf2 { - // should trigger lint - fn new(t: T) -> impl Trait2<(), i32> { - unimplemented!() - } - } - - struct RetImplTraitSelfAdt<'a>(&'a str); - - impl<'a> RetImplTraitSelfAdt<'a> { - // should not trigger lint - fn new<'b: 'a>(s: &'b str) -> impl Into> { - RetImplTraitSelfAdt(s) + impl Bomb { + pub fn explode(&self) -> impl PartialOrd { + 0i32 } } } diff --git a/tests/ui/new_ret_no_self.stderr b/tests/ui/new_ret_no_self.stderr index bc13be47927b..8217bc6187f9 100644 --- a/tests/ui/new_ret_no_self.stderr +++ b/tests/ui/new_ret_no_self.stderr @@ -76,21 +76,5 @@ LL | | unimplemented!(); LL | | } | |_________^ -error: methods called `new` usually return `Self` - --> $DIR/new_ret_no_self.rs:368:9 - | -LL | / fn new(t: T) -> impl Into { -LL | | 1 -LL | | } - | |_________^ - -error: methods called `new` usually return `Self` - --> $DIR/new_ret_no_self.rs:389:9 - | -LL | / fn new(t: T) -> impl Trait2<(), i32> { -LL | | unimplemented!() -LL | | } - | |_________^ - -error: aborting due to 12 previous errors +error: aborting due to 10 previous errors