Skip to content

Commit

Permalink
Auto merge of #10086 - nbdd0121:master, r=matthiaskrgr
Browse files Browse the repository at this point in the history 
Fix new_return_no_self with recursive bounds

Fix #10041

This uses a hash set, as described in #10068 (comment)

changelog: [`new_return_no_self`]: fix stack overflow when the return type is `impl Trait` and contains recursive bounds
  • Loading branch information
@bors
bors committed Dec 16, 2022
2 parents 3905f51 + 7574c98 commit 02f3959
Show file tree
Hide file tree
Showing 3 changed files with 105 additions and 50 deletions.
90 changes: 53 additions & 37 deletions clippy_utils/src/ty.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -69,50 +69,66 @@ pub fn contains_adt_constructor<'tcx>(ty: Ty<'tcx>, adt: AdtDef<'tcx>) -> bool {
/// This method also recurses into opaque type predicates, so call it with `impl Trait<U>` 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;
}
fn contains_ty_adt_constructor_opaque_inner<'tcx>(
cx: &LateContext<'tcx>,
ty: Ty<'tcx>,
needle: Ty<'tcx>,
seen: &mut FxHashSet<DefId>,
) -> 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 inner_ty.ty_adt_def() == needle.ty_adt_def() {
return true;
}

if let ty::Opaque(def_id, _) = *inner_ty.kind() {
if !seen.insert(def_id) {
return false;
}

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<U>`, it will register a predicate of `T: Trait<U>`, 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<Assoc=U>`, it will register a predicate of `<T as Trait>::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) {
for &(predicate, _span) in cx.tcx.explicit_item_bounds(def_id) {
match predicate.kind().skip_binder() {
// For `impl Trait<U>`, it will register a predicate of `T: Trait<U>`, 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_inner(cx, ty, needle, seen))
{
return true;
}
};
},
_ => (),
},
// For `impl Trait<Assoc=U>`, it will register a predicate of `<T as Trait>::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_inner(cx, ty, needle, seen) {
return true;
}
};
},
_ => (),
}
}
}
}

false
},
GenericArgKind::Lifetime(_) | GenericArgKind::Const(_) => false,
})
false
},
GenericArgKind::Lifetime(_) | GenericArgKind::Const(_) => false,
})
}

// A hash set to ensure that the same opaque type (`impl Trait` in RPIT or TAIT) is not
// visited twice.
let mut seen = FxHashSet::default();
contains_ty_adt_constructor_opaque_inner(cx, ty, needle, &mut seen)
}

/// Resolves `<T as Iterator>::Item` for `T`
Expand Down
23 changes: 23 additions & 0 deletions tests/ui/new_ret_no_self.rs
View file
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
#![feature(type_alias_impl_trait)]
#![warn(clippy::new_ret_no_self)]
#![allow(dead_code)]

Expand Down Expand Up @@ -400,3 +401,25 @@ mod issue7344 {
}
}
}

mod issue10041 {
struct Bomb;

impl Bomb {
// Hidden <Rhs = Self> default generic paramter.
pub fn new() -> impl PartialOrd {
0i32
}
}

// TAIT with self-referencing bounds
type X = impl std::ops::Add<Output = X>;

struct Bomb2;

impl Bomb2 {
pub fn new() -> X {
0i32
}
}
}
42 changes: 29 additions & 13 deletions tests/ui/new_ret_no_self.stderr
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
error: methods called `new` usually return `Self`
--> $DIR/new_ret_no_self.rs:49:5
--> $DIR/new_ret_no_self.rs:50:5
|
LL | / pub fn new(_: String) -> impl R<Item = u32> {
LL | | S3
Expand All @@ -9,88 +9,104 @@ LL | | }
= 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
--> $DIR/new_ret_no_self.rs:82:5
|
LL | / pub fn new() -> u32 {
LL | | unimplemented!();
LL | | }
| |_____^

error: methods called `new` usually return `Self`
--> $DIR/new_ret_no_self.rs:90:5
--> $DIR/new_ret_no_self.rs:91:5
|
LL | / pub fn new(_: String) -> u32 {
LL | | unimplemented!();
LL | | }
| |_____^

error: methods called `new` usually return `Self`
--> $DIR/new_ret_no_self.rs:126:5
--> $DIR/new_ret_no_self.rs:127:5
|
LL | / pub fn new() -> (u32, u32) {
LL | | unimplemented!();
LL | | }
| |_____^

error: methods called `new` usually return `Self`
--> $DIR/new_ret_no_self.rs:153:5
--> $DIR/new_ret_no_self.rs:154:5
|
LL | / pub fn new() -> *mut V {
LL | | unimplemented!();
LL | | }
| |_____^

error: methods called `new` usually return `Self`
--> $DIR/new_ret_no_self.rs:171:5
--> $DIR/new_ret_no_self.rs:172:5
|
LL | / pub fn new() -> Option<u32> {
LL | | unimplemented!();
LL | | }
| |_____^

error: methods called `new` usually return `Self`
--> $DIR/new_ret_no_self.rs:224:9
--> $DIR/new_ret_no_self.rs:225:9
|
LL | fn new() -> String;
| ^^^^^^^^^^^^^^^^^^^

error: methods called `new` usually return `Self`
--> $DIR/new_ret_no_self.rs:236:9
--> $DIR/new_ret_no_self.rs:237:9
|
LL | fn new(_: String) -> String;
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: methods called `new` usually return `Self`
--> $DIR/new_ret_no_self.rs:271:9
--> $DIR/new_ret_no_self.rs:272:9
|
LL | / fn new() -> (u32, u32) {
LL | | unimplemented!();
LL | | }
| |_________^

error: methods called `new` usually return `Self`
--> $DIR/new_ret_no_self.rs:298:9
--> $DIR/new_ret_no_self.rs:299:9
|
LL | / fn new() -> *mut V {
LL | | unimplemented!();
LL | | }
| |_________^

error: methods called `new` usually return `Self`
--> $DIR/new_ret_no_self.rs:368:9
--> $DIR/new_ret_no_self.rs:369:9
|
LL | / fn new(t: T) -> impl Into<i32> {
LL | | 1
LL | | }
| |_________^

error: methods called `new` usually return `Self`
--> $DIR/new_ret_no_self.rs:389:9
--> $DIR/new_ret_no_self.rs:390:9
|
LL | / fn new(t: T) -> impl Trait2<(), i32> {
LL | | unimplemented!()
LL | | }
| |_________^

error: aborting due to 12 previous errors
error: methods called `new` usually return `Self`
--> $DIR/new_ret_no_self.rs:410:9
|
LL | / pub fn new() -> impl PartialOrd {
LL | | 0i32
LL | | }
| |_________^

error: methods called `new` usually return `Self`
--> $DIR/new_ret_no_self.rs:421:9
|
LL | / pub fn new() -> X {
LL | | 0i32
LL | | }
| |_________^

error: aborting due to 14 previous errors

0 comments on commit 02f3959

Please sign in to comment.