Skip to content

Commit 2ebff58

Browse files
y21y21
committed
make generics work
fix compile error in doc example
1 parent 42bd6d7 commit 2ebff58

File tree

3 files changed

+146
-49
lines changed

3 files changed

+146
-49
lines changed

‎clippy_lints/src/implied_bounds_in_impl.rs

Lines changed: 82 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,10 @@
11
use clippy_utils::diagnostics::span_lint;
22
use rustc_hir::def_id::LocalDefId;
33
use rustc_hir::intravisit::FnKind;
4-
use rustc_hir::{Body, FnDecl, FnRetTy, GenericArgs, GenericBound, ItemKind, TraitBoundModifier, TyKind};
4+
use rustc_hir::{Body, FnDecl, FnRetTy, GenericArg, GenericBound, ItemKind, TraitBoundModifier, TyKind};
5+
use rustc_hir_analysis::hir_ty_to_ty;
56
use rustc_lint::{LateContext, LateLintPass};
6-
use rustc_middle::ty::ClauseKind;
7+
use rustc_middle::ty::{self, ClauseKind, TyCtxt};
78
use rustc_session::{declare_lint_pass, declare_tool_lint};
89
use rustc_span::Span;
910

@@ -17,17 +18,19 @@ declare_clippy_lint! {
1718
/// Unnecessary complexity.
1819
///
1920
/// ### Known problems
20-
/// This lint currently does not work with generic traits (i.e. will not lint even if redundant).
21+
/// This lint does not transitively look for implied bounds past the first supertrait.
2122
///
2223
/// ### Example
2324
/// ```rust
25+
/// # use std::ops::{Deref,DerefMut};
2426
/// fn f() -> impl Deref<Target = i32> + DerefMut<Target = i32> {
2527
/// // ^^^^^^^^^^^^^^^^^^^ unnecessary bound, already implied by the `DerefMut` trait bound
2628
/// Box::new(123)
2729
/// }
2830
/// ```
2931
/// Use instead:
3032
/// ```rust
33+
/// # use std::ops::{Deref,DerefMut};
3134
/// fn f() -> impl DerefMut<Target = i32> {
3235
/// Box::new(123)
3336
/// }
@@ -39,6 +42,53 @@ declare_clippy_lint! {
3942
}
4043
declare_lint_pass!(ImpliedBoundsInImpl => [IMPLIED_BOUNDS_IN_IMPL]);
4144

45+
/// This function tries to, for all type parameters in a supertype predicate `GenericTrait<U>`,
46+
/// check if the substituted type in the implied-by bound matches with what's subtituted in the
47+
/// implied type.
48+
///
49+
/// Consider this example function.
50+
/// ```rust,ignore
51+
/// trait GenericTrait<T> {}
52+
/// trait GenericSubTrait<T, U, V>: GenericTrait<U> {}
53+
/// ^ trait_predicate_args: [Self#0, U#2]
54+
/// impl GenericTrait<i32> for () {}
55+
/// impl GenericSubTrait<(), i32, ()> for () {}
56+
/// impl GenericSubTrait<(), [u8; 8], ()> for () {}
57+
///
58+
/// fn f() -> impl GenericTrait<i32> + GenericSubTrait<(), [u8; 8], ()> {
59+
/// ^^^ implied_args ^^^^^^^^^^^^^^^ implied_by_args
60+
/// (we are interested in `[u8; 8]` specifically, as that
61+
/// is what `U` in `GenericTrait<U>` is substituted with)
62+
/// ()
63+
/// }
64+
/// ```
65+
/// Here i32 != [u8; 8], so this will return false.
66+
fn is_same_generics(
67+
tcx: TyCtxt<'_>,
68+
trait_predicate_args: &[ty::GenericArg<'_>],
69+
implied_by_args: &[GenericArg<'_>],
70+
implied_args: &[GenericArg<'_>],
71+
) -> bool {
72+
trait_predicate_args
73+
.iter()
74+
.enumerate()
75+
.skip(1) // skip `Self` implicit arg
76+
.all(|(arg_index, arg)| {
77+
if let Some(ty) = arg.as_type()
78+
&& let &ty::Param(ty::ParamTy{ index, .. }) = ty.kind()
79+
// Since `trait_predicate_args` and type params in traits start with `Self=0`
80+
// and generic argument lists `GenericTrait<i32>` don't have `Self`,
81+
// we need to subtract 1 from the index.
82+
&& let GenericArg::Type(ty_a) = implied_by_args[index as usize - 1]
83+
&& let GenericArg::Type(ty_b) = implied_args[arg_index - 1]
84+
{
85+
hir_ty_to_ty(tcx, ty_a) == hir_ty_to_ty(tcx, ty_b)
86+
} else {
87+
false
88+
}
89+
})
90+
}
91+
4292
impl LateLintPass<'_> for ImpliedBoundsInImpl {
4393
fn check_fn(
4494
&mut self,
@@ -54,37 +104,52 @@ impl LateLintPass<'_> for ImpliedBoundsInImpl {
54104
&& let item = cx.tcx.hir().item(item_id)
55105
&& let ItemKind::OpaqueTy(opaque_ty) = item.kind
56106
{
57-
// Get all `DefId`s of (implied) trait predicates in all the bounds.
107+
// Very often there is only a single bound, e.g. `impl Deref<..>`, in which case
108+
// we can avoid doing a bunch of stuff unnecessarily.
109+
if opaque_ty.bounds.is_empty() {
110+
return;
111+
}
112+
113+
// Get all the (implied) trait predicates in the bounds.
58114
// For `impl Deref + DerefMut` this will contain [`Deref`].
59115
// The implied `Deref` comes from `DerefMut` because `trait DerefMut: Deref {}`.
60116

61-
// N.B. Generic args on trait bounds are currently ignored and (G)ATs are fine to disregard,
62-
// because they must be the same for all of its supertraits. Example:
117+
// N.B. (G)ATs are fine to disregard, because they must be the same for all of its supertraits.
118+
// Example:
63119
// `impl Deref<Target = i32> + DerefMut<Target = u32>` is not allowed.
64120
// `DerefMut::Target` needs to match `Deref::Target`
65-
let implied_bounds = opaque_ty.bounds.iter().flat_map(|bound| {
121+
let implied_bounds: Vec<_> = opaque_ty.bounds.iter().filter_map(|bound| {
66122
if let GenericBound::Trait(poly_trait, TraitBoundModifier::None) = bound
67123
&& let [.., path] = poly_trait.trait_ref.path.segments
68124
&& poly_trait.bound_generic_params.is_empty()
69-
&& path.args.map_or(true, GenericArgs::is_empty)
70125
&& let Some(trait_def_id) = path.res.opt_def_id()
126+
&& let predicates = cx.tcx.super_predicates_of(trait_def_id).predicates
127+
&& !predicates.is_empty() // If the trait has no supertrait, there is nothing to add.
71128
{
72-
cx.tcx.implied_predicates_of(trait_def_id).predicates
129+
Some((path.args.map_or([].as_slice(), |a| a.args), predicates))
73130
} else {
74-
&[]
131+
None
75132
}
76-
}).collect::<Vec<_>>();
133+
}).collect();
77134

78135
// Lint all bounds in the `impl Trait` type that are also in the `implied_bounds` vec.
136+
// This involves some extra logic when generic arguments are present, since
137+
// simply comparing trait `DefId`s won't be enough. We also need to compare the generics.
79138
for bound in opaque_ty.bounds {
80139
if let GenericBound::Trait(poly_trait, TraitBoundModifier::None) = bound
140+
&& let [.., path] = poly_trait.trait_ref.path.segments
141+
&& let implied_args = path.args.map_or([].as_slice(), |a| a.args)
81142
&& let Some(def_id) = poly_trait.trait_ref.path.res.opt_def_id()
82-
&& implied_bounds.iter().any(|(clause, _)| {
83-
if let ClauseKind::Trait(tr) = clause.kind().skip_binder() {
84-
tr.def_id() == def_id
85-
} else {
86-
false
87-
}
143+
&& implied_bounds.iter().any(|(implied_by_args, preds)| {
144+
preds.iter().any(|(clause, _)| {
145+
if let ClauseKind::Trait(tr) = clause.kind().skip_binder()
146+
&& tr.def_id() == def_id
147+
{
148+
is_same_generics(cx.tcx, tr.trait_ref.args, implied_by_args, implied_args)
149+
} else {
150+
false
151+
}
152+
})
88153
})
89154
{
90155
span_lint(cx, IMPLIED_BOUNDS_IN_IMPL, poly_trait.span, "this bound is implied by another bound and can be removed");

‎tests/ui/implied_bounds_in_impl.rs

Lines changed: 29 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -3,43 +3,45 @@
33

44
use std::ops::{Deref, DerefMut};
55

6-
trait Trait1<T> {}
7-
// T is intentionally at a different position in Trait2 than in Trait1,
8-
// since that also needs to be taken into account when making this lint work with generics
9-
trait Trait2<U, T>: Trait1<T> {}
10-
impl Trait1<i32> for () {}
11-
impl Trait1<String> for () {}
12-
impl Trait2<u32, i32> for () {}
13-
impl Trait2<u32, String> for () {}
6+
// Only one bound, nothing to lint.
7+
fn normal_deref<T>(x: T) -> impl Deref<Target = T> {
8+
Box::new(x)
9+
}
1410

1511
// Deref implied by DerefMut
1612
fn deref_derefmut<T>(x: T) -> impl Deref<Target = T> + DerefMut<Target = T> {
1713
Box::new(x)
1814
}
1915

20-
// Note: no test for different associated types needed since that isn't allowed in the first place.
21-
// E.g. `-> impl Deref<Target = T> + DerefMut<Target = U>` is a compile error.
22-
23-
// DefIds of the traits match, but the generics do not, so it's *not* redundant.
24-
// `Trait2: Trait` holds, but not `Trait2<_, String>: Trait1<i32>`.
25-
// (Generic traits are currently not linted anyway but once/if ever implemented this should not
26-
// warn.)
27-
fn different_generics() -> impl Trait1<i32> + Trait2<u32, String> {
28-
/* () */
16+
trait GenericTrait<T> {}
17+
trait GenericTrait2<V> {}
18+
// U is intentionally at a different "index" in GenericSubtrait than `T` is in GenericTrait
19+
trait GenericSubtrait<T, U, V>: GenericTrait<U> + GenericTrait2<V> {}
20+
21+
impl GenericTrait<i32> for () {}
22+
impl GenericTrait<i64> for () {}
23+
impl<V> GenericTrait2<V> for () {}
24+
impl<V> GenericSubtrait<(), i32, V> for () {}
25+
impl<V> GenericSubtrait<(), i64, V> for () {}
26+
27+
fn generics_implied<T>() -> impl GenericTrait<T> + GenericSubtrait<(), T, ()>
28+
where
29+
(): GenericSubtrait<(), T, ()>,
30+
{
2931
}
3032

31-
trait NonGenericTrait1 {}
32-
trait NonGenericTrait2: NonGenericTrait1 {}
33-
impl NonGenericTrait1 for i32 {}
34-
impl NonGenericTrait2 for i32 {}
33+
fn generics_implied_multi<V>() -> impl GenericTrait<i32> + GenericTrait2<V> + GenericSubtrait<(), i32, V> {}
3534

36-
// Only one bound. Nothing to lint.
37-
fn normal1() -> impl NonGenericTrait1 {
38-
1
35+
fn generics_implied_multi2<T, V>() -> impl GenericTrait<T> + GenericTrait2<V> + GenericSubtrait<(), T, V>
36+
where
37+
(): GenericSubtrait<(), T, V> + GenericTrait<T>,
38+
{
3939
}
4040

41-
fn normal2() -> impl NonGenericTrait1 + NonGenericTrait2 {
42-
1
43-
}
41+
// i32 != i64, GenericSubtrait<_, i64, _> does not imply GenericTrait<i32>, don't lint
42+
fn generics_different() -> impl GenericTrait<i32> + GenericSubtrait<(), i64, ()> {}
43+
44+
// i32 == i32, GenericSubtrait<_, i32, _> does imply GenericTrait<i32>, lint
45+
fn generics_same() -> impl GenericTrait<i32> + GenericSubtrait<(), i32, ()> {}
4446

4547
fn main() {}

‎tests/ui/implied_bounds_in_impl.stderr

Lines changed: 35 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,16 +1,46 @@
11
error: this bound is implied by another bound and can be removed
2-
--> $DIR/implied_bounds_in_impl.rs:16:36
2+
--> $DIR/implied_bounds_in_impl.rs:12:36
33
|
44
LL | fn deref_derefmut<T>(x: T) -> impl Deref<Target = T> + DerefMut<Target = T> {
55
| ^^^^^^^^^^^^^^^^^
66
|
77
= note: `-D clippy::implied-bounds-in-impl` implied by `-D warnings`
88

99
error: this bound is implied by another bound and can be removed
10-
--> $DIR/implied_bounds_in_impl.rs:41:22
10+
--> $DIR/implied_bounds_in_impl.rs:27:34
1111
|
12-
LL | fn normal2() -> impl NonGenericTrait1 + NonGenericTrait2 {
13-
| ^^^^^^^^^^^^^^^^
12+
LL | fn generics_implied<T>() -> impl GenericTrait<T> + GenericSubtrait<(), T, ()>
13+
| ^^^^^^^^^^^^^^^
1414

15-
error: aborting due to 2 previous errors
15+
error: this bound is implied by another bound and can be removed
16+
--> $DIR/implied_bounds_in_impl.rs:33:40
17+
|
18+
LL | fn generics_implied_multi<V>() -> impl GenericTrait<i32> + GenericTrait2<V> + GenericSubtrait<(), i32, V> {}
19+
| ^^^^^^^^^^^^^^^^^
20+
21+
error: this bound is implied by another bound and can be removed
22+
--> $DIR/implied_bounds_in_impl.rs:33:60
23+
|
24+
LL | fn generics_implied_multi<V>() -> impl GenericTrait<i32> + GenericTrait2<V> + GenericSubtrait<(), i32, V> {}
25+
| ^^^^^^^^^^^^^^^^
26+
27+
error: this bound is implied by another bound and can be removed
28+
--> $DIR/implied_bounds_in_impl.rs:35:44
29+
|
30+
LL | fn generics_implied_multi2<T, V>() -> impl GenericTrait<T> + GenericTrait2<V> + GenericSubtrait<(), T, V>
31+
| ^^^^^^^^^^^^^^^
32+
33+
error: this bound is implied by another bound and can be removed
34+
--> $DIR/implied_bounds_in_impl.rs:35:62
35+
|
36+
LL | fn generics_implied_multi2<T, V>() -> impl GenericTrait<T> + GenericTrait2<V> + GenericSubtrait<(), T, V>
37+
| ^^^^^^^^^^^^^^^^
38+
39+
error: this bound is implied by another bound and can be removed
40+
--> $DIR/implied_bounds_in_impl.rs:45:28
41+
|
42+
LL | fn generics_same() -> impl GenericTrait<i32> + GenericSubtrait<(), i32, ()> {}
43+
| ^^^^^^^^^^^^^^^^^
44+
45+
error: aborting due to 7 previous errors
1646

0 commit comments

Comments
 (0)