Skip to content

Commit

Permalink
Auto merge of #11881 - y21:issue11880, r=Alexendoo
Browse files Browse the repository at this point in the history
[`implied_bounds_in_impls`]: avoid linting on overlapping associated tys

Fixes #11880

Before this change, we were simply ignoring associated types (except for suggestion purposes), because of an incorrect assumption (see the comment that I also removed).

For something like
```rs
trait X { type T; }
trait Y: X { type T; }

// Can't constrain `X::T` through `Y`
fn f() -> impl X<T = i32> + Y<T = u32> { ... }
```
We now avoid linting if the implied bound (`X<T = i32>`) "names" associated types that also exists in the implying trait (`trait Y`). Here that would be the case.
But if we only wrote `impl X + Y<T = u32>` then that's ok because `X::T` was never constrained in the first place.

I haven't really thought about how this interacts with GATs, but I think it's fine. Fine as in, it might create false negatives, but hopefully no false positives.

(The diff is slightly annoying because of formatting things. Really the only thing that changed in the if chain is extracting the `implied_by_def_id` which is needed for getting associated types from the trait, and of course actually checking for overlap)

cc `@Jarcho` ? idk if you want to review this or not. I assume you looked into this code a bit to find this bug.

changelog: [`implied_bounds_in_impls`]: avoid linting when associated type from supertrait can't be constrained through the implying trait bound
  • Loading branch information
bors committed Feb 13, 2024
2 parents 4350678 + 92616c0 commit b78c157
Show file tree
Hide file tree
Showing 4 changed files with 104 additions and 1 deletion.
9 changes: 9 additions & 0 deletions clippy_lints/src/implied_bounds_in_impls.rs
Original file line number Diff line number Diff line change
Expand Up @@ -317,6 +317,15 @@ fn check(cx: &LateContext<'_>, decl: &FnDecl<'_>) {
&& let implied_bindings = path.args.map_or([].as_slice(), |a| a.bindings)
&& let Some(def_id) = poly_trait.trait_ref.path.res.opt_def_id()
&& let Some(bound) = find_bound_in_supertraits(cx, def_id, implied_args, &supertraits)
// If the implied bound has a type binding that also exists in the implied-by trait,
// then we shouldn't lint. See #11880 for an example.
&& let assocs = cx.tcx.associated_items(bound.trait_def_id)
&& !implied_bindings.iter().any(|binding| {
assocs
.filter_by_name_unhygienic(binding.ident.name)
.next()
.is_some_and(|assoc| assoc.kind == ty::AssocKind::Type)
})
{
emit_lint(cx, poly_trait, opaque_ty, index, implied_bindings, bound);
}
Expand Down
29 changes: 29 additions & 0 deletions tests/ui/implied_bounds_in_impls.fixed
Original file line number Diff line number Diff line change
Expand Up @@ -121,4 +121,33 @@ mod issue11435 {
fn f3() -> impl Trait4<i8, X = i32, Y = i128> {}
}

fn issue11880() {
trait X {
type T;
type U;
}
trait Y: X {
type T;
type V;
}
impl X for () {
type T = i32;
type U = String;
}
impl Y for () {
type T = u32;
type V = Vec<u8>;
}

// Can't constrain `X::T` through `Y`
fn f() -> impl X<T = i32> + Y {}
fn f2() -> impl X<T = i32> + Y<T = u32> {}

// X::T is never constrained in the first place, so it can be omitted
// and left unconstrained
fn f3() -> impl Y {}
fn f4() -> impl Y<T = u32> {}
fn f5() -> impl Y<T = u32, U = String> {}
}

fn main() {}
29 changes: 29 additions & 0 deletions tests/ui/implied_bounds_in_impls.rs
Original file line number Diff line number Diff line change
Expand Up @@ -121,4 +121,33 @@ mod issue11435 {
fn f3() -> impl Trait3<i8, i16, i64, X = i32, Y = i128> + Trait4<i8, X = i32> {}
}

fn issue11880() {
trait X {
type T;
type U;
}
trait Y: X {
type T;
type V;
}
impl X for () {
type T = i32;
type U = String;
}
impl Y for () {
type T = u32;
type V = Vec<u8>;
}

// Can't constrain `X::T` through `Y`
fn f() -> impl X<T = i32> + Y {}
fn f2() -> impl X<T = i32> + Y<T = u32> {}

// X::T is never constrained in the first place, so it can be omitted
// and left unconstrained
fn f3() -> impl X + Y {}
fn f4() -> impl X + Y<T = u32> {}
fn f5() -> impl X<U = String> + Y<T = u32> {}
}

fn main() {}
38 changes: 37 additions & 1 deletion tests/ui/implied_bounds_in_impls.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -192,5 +192,41 @@ LL - fn f3() -> impl Trait3<i8, i16, i64, X = i32, Y = i128> + Trait4<i8, X
LL + fn f3() -> impl Trait4<i8, X = i32, Y = i128> {}
|

error: aborting due to 16 previous errors
error: this bound is already specified as the supertrait of `Y`
--> $DIR/implied_bounds_in_impls.rs:148:21
|
LL | fn f3() -> impl X + Y {}
| ^
|
help: try removing this bound
|
LL - fn f3() -> impl X + Y {}
LL + fn f3() -> impl Y {}
|

error: this bound is already specified as the supertrait of `Y<T = u32>`
--> $DIR/implied_bounds_in_impls.rs:149:21
|
LL | fn f4() -> impl X + Y<T = u32> {}
| ^
|
help: try removing this bound
|
LL - fn f4() -> impl X + Y<T = u32> {}
LL + fn f4() -> impl Y<T = u32> {}
|

error: this bound is already specified as the supertrait of `Y<T = u32>`
--> $DIR/implied_bounds_in_impls.rs:150:21
|
LL | fn f5() -> impl X<U = String> + Y<T = u32> {}
| ^^^^^^^^^^^^^
|
help: try removing this bound
|
LL - fn f5() -> impl X<U = String> + Y<T = u32> {}
LL + fn f5() -> impl Y<T = u32, U = String> {}
|

error: aborting due to 19 previous errors

0 comments on commit b78c157

Please sign in to comment.