Skip to content

Commit 8c48b93

Browse files
committed
Auto merge of #11459 - y21:issue11435, r=blyxyas
[`implied_bounds_in_impls`]: include (previously omitted) associated types in suggestion Fixes #11435 It now includes associated types from the implied bound that were omitted in the second bound. Example: ```rs fn f() -> impl Iterator<Item = u8> + ExactSizeIterator> {..} ``` Suggestion before this change: ```diff - pub fn my_iter() -> impl Iterator<Item = u32> + ExactSizeIterator { + pub fn my_iter() -> impl ExactSizeIterator { ``` It didn't include `<Item = u32>` on `ExactSizeIterator`. Now, with this change, it does. ```diff - pub fn my_iter() -> impl Iterator<Item = u32> + ExactSizeIterator { + pub fn my_iter() -> impl ExactSizeIterator<Item = u32> { ``` We also now extend the span to include not just possible `+` ahead of it, but also behind it (an example for this is in the linked issue as well). **Note:** The overall diff is a bit noisy, because building up the suggestion involves quite a bit more logic now and I decided to extract that into its own function. For that reason, I split this PR up into two commits. The first commit contains the actual "logic" changes. Second commit just moves code around. changelog: [`implied_bounds_in_impls`]: include (previously omitted) associated types in suggestion changelog: [`implied_bounds_in_impls`]: include the `+` behind bound if it's the last bound
2 parents ec6f1bd + 30846b1 commit 8c48b93

File tree

4 files changed

+233
-28
lines changed

4 files changed

+233
-28
lines changed

clippy_lints/src/implied_bounds_in_impls.rs

+106-27
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,95 @@ declare_clippy_lint! {
5050
}
5151
declare_lint_pass!(ImpliedBoundsInImpls => [IMPLIED_BOUNDS_IN_IMPLS]);
5252

53+
#[allow(clippy::too_many_arguments)]
54+
fn emit_lint(
55+
cx: &LateContext<'_>,
56+
poly_trait: &rustc_hir::PolyTraitRef<'_>,
57+
opaque_ty: &rustc_hir::OpaqueTy<'_>,
58+
index: usize,
59+
// The bindings that were implied
60+
implied_bindings: &[rustc_hir::TypeBinding<'_>],
61+
// The original bindings that `implied_bindings` are implied from
62+
implied_by_bindings: &[rustc_hir::TypeBinding<'_>],
63+
implied_by_args: &[GenericArg<'_>],
64+
implied_by_span: Span,
65+
) {
66+
let implied_by = snippet(cx, implied_by_span, "..");
67+
68+
span_lint_and_then(
69+
cx,
70+
IMPLIED_BOUNDS_IN_IMPLS,
71+
poly_trait.span,
72+
&format!("this bound is already specified as the supertrait of `{implied_by}`"),
73+
|diag| {
74+
// If we suggest removing a bound, we may also need to extend the span
75+
// to include the `+` token that is ahead or behind,
76+
// so we don't end up with something like `impl + B` or `impl A + `
77+
78+
let implied_span_extended = if let Some(next_bound) = opaque_ty.bounds.get(index + 1) {
79+
poly_trait.span.to(next_bound.span().shrink_to_lo())
80+
} else if index > 0
81+
&& let Some(prev_bound) = opaque_ty.bounds.get(index - 1)
82+
{
83+
prev_bound.span().shrink_to_hi().to(poly_trait.span.shrink_to_hi())
84+
} else {
85+
poly_trait.span
86+
};
87+
88+
let mut sugg = vec![(implied_span_extended, String::new())];
89+
90+
// We also might need to include associated type binding that were specified in the implied bound,
91+
// but omitted in the implied-by bound:
92+
// `fn f() -> impl Deref<Target = u8> + DerefMut`
93+
// If we're going to suggest removing `Deref<..>`, we'll need to put `<Target = u8>` on `DerefMut`
94+
let omitted_assoc_tys: Vec<_> = implied_bindings
95+
.iter()
96+
.filter(|binding| !implied_by_bindings.iter().any(|b| b.ident == binding.ident))
97+
.collect();
98+
99+
if !omitted_assoc_tys.is_empty() {
100+
// `<>` needs to be added if there aren't yet any generic arguments or bindings
101+
let needs_angle_brackets = implied_by_args.is_empty() && implied_by_bindings.is_empty();
102+
let insert_span = match (implied_by_args, implied_by_bindings) {
103+
([.., arg], [.., binding]) => arg.span().max(binding.span).shrink_to_hi(),
104+
([.., arg], []) => arg.span().shrink_to_hi(),
105+
([], [.., binding]) => binding.span.shrink_to_hi(),
106+
([], []) => implied_by_span.shrink_to_hi(),
107+
};
108+
109+
let mut associated_tys_sugg = if needs_angle_brackets {
110+
"<".to_owned()
111+
} else {
112+
// If angle brackets aren't needed (i.e., there are already generic arguments or bindings),
113+
// we need to add a comma:
114+
// `impl A<B, C >`
115+
// ^ if we insert `Assoc=i32` without a comma here, that'd be invalid syntax:
116+
// `impl A<B, C Assoc=i32>`
117+
", ".to_owned()
118+
};
119+
120+
for (index, binding) in omitted_assoc_tys.into_iter().enumerate() {
121+
if index > 0 {
122+
associated_tys_sugg += ", ";
123+
}
124+
associated_tys_sugg += &snippet(cx, binding.span, "..");
125+
}
126+
if needs_angle_brackets {
127+
associated_tys_sugg += ">";
128+
}
129+
sugg.push((insert_span, associated_tys_sugg));
130+
}
131+
132+
diag.multipart_suggestion_with_style(
133+
"try removing this bound",
134+
sugg,
135+
Applicability::MachineApplicable,
136+
SuggestionStyle::ShowAlways,
137+
);
138+
},
139+
);
140+
}
141+
53142
/// Tries to "resolve" a type.
54143
/// The index passed to this function must start with `Self=0`, i.e. it must be a valid
55144
/// type parameter index.
@@ -149,7 +238,7 @@ fn check(cx: &LateContext<'_>, decl: &FnDecl<'_>) {
149238
&& let predicates = cx.tcx.super_predicates_of(trait_def_id).predicates
150239
&& !predicates.is_empty() // If the trait has no supertrait, there is nothing to add.
151240
{
152-
Some((bound.span(), path.args.map_or([].as_slice(), |a| a.args), predicates, trait_def_id))
241+
Some((bound.span(), path, predicates, trait_def_id))
153242
} else {
154243
None
155244
}
@@ -162,10 +251,14 @@ fn check(cx: &LateContext<'_>, decl: &FnDecl<'_>) {
162251
if let GenericBound::Trait(poly_trait, TraitBoundModifier::None) = bound
163252
&& let [.., path] = poly_trait.trait_ref.path.segments
164253
&& let implied_args = path.args.map_or([].as_slice(), |a| a.args)
254+
&& let implied_bindings = path.args.map_or([].as_slice(), |a| a.bindings)
165255
&& let Some(def_id) = poly_trait.trait_ref.path.res.opt_def_id()
166-
&& let Some(implied_by_span) = implied_bounds
256+
&& let Some((implied_by_span, implied_by_args, implied_by_bindings)) = implied_bounds
167257
.iter()
168-
.find_map(|&(span, implied_by_args, preds, implied_by_def_id)| {
258+
.find_map(|&(span, implied_by_path, preds, implied_by_def_id)| {
259+
let implied_by_args = implied_by_path.args.map_or([].as_slice(), |a| a.args);
260+
let implied_by_bindings = implied_by_path.args.map_or([].as_slice(), |a| a.bindings);
261+
169262
preds.iter().find_map(|(clause, _)| {
170263
if let ClauseKind::Trait(tr) = clause.kind().skip_binder()
171264
&& tr.def_id() == def_id
@@ -178,36 +271,22 @@ fn check(cx: &LateContext<'_>, decl: &FnDecl<'_>) {
178271
def_id,
179272
)
180273
{
181-
Some(span)
274+
Some((span, implied_by_args, implied_by_bindings))
182275
} else {
183276
None
184277
}
185278
})
186279
})
187280
{
188-
let implied_by = snippet(cx, implied_by_span, "..");
189-
span_lint_and_then(
190-
cx, IMPLIED_BOUNDS_IN_IMPLS,
191-
poly_trait.span,
192-
&format!("this bound is already specified as the supertrait of `{implied_by}`"),
193-
|diag| {
194-
// If we suggest removing a bound, we may also need extend the span
195-
// to include the `+` token, so we don't end up with something like `impl + B`
196-
197-
let implied_span_extended = if let Some(next_bound) = opaque_ty.bounds.get(index + 1) {
198-
poly_trait.span.to(next_bound.span().shrink_to_lo())
199-
} else {
200-
poly_trait.span
201-
};
202-
203-
diag.span_suggestion_with_style(
204-
implied_span_extended,
205-
"try removing this bound",
206-
"",
207-
Applicability::MachineApplicable,
208-
SuggestionStyle::ShowAlways
209-
);
210-
}
281+
emit_lint(
282+
cx,
283+
poly_trait,
284+
opaque_ty,
285+
index,
286+
implied_bindings,
287+
implied_by_bindings,
288+
implied_by_args,
289+
implied_by_span
211290
);
212291
}
213292
}

tests/ui/implied_bounds_in_impls.fixed

+39
Original file line numberDiff line numberDiff line change
@@ -83,4 +83,43 @@ mod issue11422 {
8383
fn f() -> impl Trait1<()> + Trait2 {}
8484
}
8585

86+
mod issue11435 {
87+
// Associated type needs to be included on DoubleEndedIterator in the suggestion
88+
fn my_iter() -> impl DoubleEndedIterator<Item = u32> {
89+
0..5
90+
}
91+
92+
// Removing the `Clone` bound should include the `+` behind it in its remove suggestion
93+
fn f() -> impl Copy {
94+
1
95+
}
96+
97+
trait Trait1<T> {
98+
type U;
99+
}
100+
impl Trait1<i32> for () {
101+
type U = i64;
102+
}
103+
trait Trait2<T>: Trait1<T> {}
104+
impl Trait2<i32> for () {}
105+
106+
// When the other trait has generics, it shouldn't add another pair of `<>`
107+
fn f2() -> impl Trait2<i32, U = i64> {}
108+
109+
trait Trait3<T, U, V> {
110+
type X;
111+
type Y;
112+
}
113+
trait Trait4<T>: Trait3<T, i16, i64> {}
114+
impl Trait3<i8, i16, i64> for () {
115+
type X = i32;
116+
type Y = i128;
117+
}
118+
impl Trait4<i8> for () {}
119+
120+
// Associated type `X` is specified, but `Y` is not, so only that associated type should be moved
121+
// over
122+
fn f3() -> impl Trait4<i8, X = i32, Y = i128> {}
123+
}
124+
86125
fn main() {}

tests/ui/implied_bounds_in_impls.rs

+39
Original file line numberDiff line numberDiff line change
@@ -83,4 +83,43 @@ mod issue11422 {
8383
fn f() -> impl Trait1<()> + Trait2 {}
8484
}
8585

86+
mod issue11435 {
87+
// Associated type needs to be included on DoubleEndedIterator in the suggestion
88+
fn my_iter() -> impl Iterator<Item = u32> + DoubleEndedIterator {
89+
0..5
90+
}
91+
92+
// Removing the `Clone` bound should include the `+` behind it in its remove suggestion
93+
fn f() -> impl Copy + Clone {
94+
1
95+
}
96+
97+
trait Trait1<T> {
98+
type U;
99+
}
100+
impl Trait1<i32> for () {
101+
type U = i64;
102+
}
103+
trait Trait2<T>: Trait1<T> {}
104+
impl Trait2<i32> for () {}
105+
106+
// When the other trait has generics, it shouldn't add another pair of `<>`
107+
fn f2() -> impl Trait1<i32, U = i64> + Trait2<i32> {}
108+
109+
trait Trait3<T, U, V> {
110+
type X;
111+
type Y;
112+
}
113+
trait Trait4<T>: Trait3<T, i16, i64> {}
114+
impl Trait3<i8, i16, i64> for () {
115+
type X = i32;
116+
type Y = i128;
117+
}
118+
impl Trait4<i8> for () {}
119+
120+
// Associated type `X` is specified, but `Y` is not, so only that associated type should be moved
121+
// over
122+
fn f3() -> impl Trait3<i8, i16, i64, X = i32, Y = i128> + Trait4<i8, X = i32> {}
123+
}
124+
86125
fn main() {}

tests/ui/implied_bounds_in_impls.stderr

+49-1
Original file line numberDiff line numberDiff line change
@@ -144,5 +144,53 @@ LL - fn default_generic_param2() -> impl PartialOrd + PartialEq + Debug {}
144144
LL + fn default_generic_param2() -> impl PartialOrd + Debug {}
145145
|
146146

147-
error: aborting due to 12 previous errors
147+
error: this bound is already specified as the supertrait of `DoubleEndedIterator`
148+
--> $DIR/implied_bounds_in_impls.rs:88:26
149+
|
150+
LL | fn my_iter() -> impl Iterator<Item = u32> + DoubleEndedIterator {
151+
| ^^^^^^^^^^^^^^^^^^^^
152+
|
153+
help: try removing this bound
154+
|
155+
LL - fn my_iter() -> impl Iterator<Item = u32> + DoubleEndedIterator {
156+
LL + fn my_iter() -> impl DoubleEndedIterator<Item = u32> {
157+
|
158+
159+
error: this bound is already specified as the supertrait of `Copy`
160+
--> $DIR/implied_bounds_in_impls.rs:93:27
161+
|
162+
LL | fn f() -> impl Copy + Clone {
163+
| ^^^^^
164+
|
165+
help: try removing this bound
166+
|
167+
LL - fn f() -> impl Copy + Clone {
168+
LL + fn f() -> impl Copy {
169+
|
170+
171+
error: this bound is already specified as the supertrait of `Trait2<i32>`
172+
--> $DIR/implied_bounds_in_impls.rs:107:21
173+
|
174+
LL | fn f2() -> impl Trait1<i32, U = i64> + Trait2<i32> {}
175+
| ^^^^^^^^^^^^^^^^^^^^
176+
|
177+
help: try removing this bound
178+
|
179+
LL - fn f2() -> impl Trait1<i32, U = i64> + Trait2<i32> {}
180+
LL + fn f2() -> impl Trait2<i32, U = i64> {}
181+
|
182+
183+
error: this bound is already specified as the supertrait of `Trait4<i8, X = i32>`
184+
--> $DIR/implied_bounds_in_impls.rs:122:21
185+
|
186+
LL | fn f3() -> impl Trait3<i8, i16, i64, X = i32, Y = i128> + Trait4<i8, X = i32> {}
187+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
188+
|
189+
help: try removing this bound
190+
|
191+
LL - fn f3() -> impl Trait3<i8, i16, i64, X = i32, Y = i128> + Trait4<i8, X = i32> {}
192+
LL + fn f3() -> impl Trait4<i8, X = i32, Y = i128> {}
193+
|
194+
195+
error: aborting due to 16 previous errors
148196

0 commit comments

Comments
 (0)