Skip to content

Commit 7262145

Browse files
y21y21
committed
[implied_bounds_in_impl]: fix suggestion for assoc types
1 parent 3de0f19 commit 7262145

File tree

4 files changed

+194
-13
lines changed

4 files changed

+194
-13
lines changed

‎clippy_lints/src/implied_bounds_in_impls.rs

Lines changed: 67 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -149,7 +149,7 @@ fn check(cx: &LateContext<'_>, decl: &FnDecl<'_>) {
149149
&& let predicates = cx.tcx.super_predicates_of(trait_def_id).predicates
150150
&& !predicates.is_empty() // If the trait has no supertrait, there is nothing to add.
151151
{
152-
Some((bound.span(), path.args.map_or([].as_slice(), |a| a.args), predicates, trait_def_id))
152+
Some((bound.span(), path, predicates, trait_def_id))
153153
} else {
154154
None
155155
}
@@ -162,10 +162,14 @@ fn check(cx: &LateContext<'_>, decl: &FnDecl<'_>) {
162162
if let GenericBound::Trait(poly_trait, TraitBoundModifier::None) = bound
163163
&& let [.., path] = poly_trait.trait_ref.path.segments
164164
&& let implied_args = path.args.map_or([].as_slice(), |a| a.args)
165+
&& let implied_bindings = path.args.map_or([].as_slice(), |a| a.bindings)
165166
&& let Some(def_id) = poly_trait.trait_ref.path.res.opt_def_id()
166-
&& let Some(implied_by_span) = implied_bounds
167+
&& let Some((implied_by_span, implied_by_args, implied_by_bindings)) = implied_bounds
167168
.iter()
168-
.find_map(|&(span, implied_by_args, preds, implied_by_def_id)| {
169+
.find_map(|&(span, implied_by_path, preds, implied_by_def_id)| {
170+
let implied_by_args = implied_by_path.args.map_or([].as_slice(), |a| a.args);
171+
let implied_by_bindings = implied_by_path.args.map_or([].as_slice(), |a| a.bindings);
172+
169173
preds.iter().find_map(|(clause, _)| {
170174
if let ClauseKind::Trait(tr) = clause.kind().skip_binder()
171175
&& tr.def_id() == def_id
@@ -178,7 +182,7 @@ fn check(cx: &LateContext<'_>, decl: &FnDecl<'_>) {
178182
def_id,
179183
)
180184
{
181-
Some(span)
185+
Some((span, implied_by_args, implied_by_bindings))
182186
} else {
183187
None
184188
}
@@ -192,21 +196,72 @@ fn check(cx: &LateContext<'_>, decl: &FnDecl<'_>) {
192196
&format!("this bound is already specified as the supertrait of `{implied_by}`"),
193197
|diag| {
194198
// 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`
199+
// to include the `+` token that is ahead or behind,
200+
// so we don't end up with something like `impl + B` or `impl A + `
196201

197202
let implied_span_extended = if let Some(next_bound) = opaque_ty.bounds.get(index + 1) {
198203
poly_trait.span.to(next_bound.span().shrink_to_lo())
204+
} else if index > 0
205+
&& let Some(prev_bound) = opaque_ty.bounds.get(index - 1)
206+
{
207+
prev_bound.span().shrink_to_hi().to(poly_trait.span.shrink_to_hi())
199208
} else {
200209
poly_trait.span
201210
};
202211

203-
diag.span_suggestion_with_style(
204-
implied_span_extended,
205-
"try removing this bound",
206-
"",
207-
Applicability::MachineApplicable,
208-
SuggestionStyle::ShowAlways
209-
);
212+
let mut sugg = vec![
213+
(implied_span_extended, String::new()),
214+
];
215+
216+
// We also might need to include associated type binding that were specified in the implied bound,
217+
// but omitted in the implied-by bound:
218+
// `fn f() -> impl Deref<Target = u8> + DerefMut`
219+
// If we're going to suggest removing `Deref<..>`, we'll need to put `<Target = u8>` on `DerefMut`
220+
let omitted_assoc_tys: Vec<_> = implied_bindings
221+
.iter()
222+
.filter(|binding| {
223+
implied_by_bindings
224+
.iter()
225+
// TODO: is checking idents enough for stuff like `<Target: Sized> == <Target = u8>`
226+
.find(|b| b.ident == binding.ident)
227+
.is_none()
228+
})
229+
.collect();
230+
231+
if !omitted_assoc_tys.is_empty() {
232+
// `<>` needs to be added if there aren't yet any generic arguments or bindings
233+
let needs_angle_brackets = implied_by_args.is_empty() && implied_by_bindings.is_empty();
234+
let insert_span = match (implied_by_args, implied_by_bindings) {
235+
([.., arg], [.., binding]) => arg.span().max(binding.span).shrink_to_hi(),
236+
([.., arg], []) => arg.span().shrink_to_hi(),
237+
([], [.., binding]) => binding.span.shrink_to_hi(),
238+
([], []) => implied_by_span.shrink_to_hi(),
239+
};
240+
241+
let mut associated_tys_sugg = if needs_angle_brackets {
242+
"<".to_owned()
243+
} else {
244+
// If angle brackets aren't needed (i.e., there are already generic arguments or bindings),
245+
// we need to add a comma:
246+
// `impl A<B, C >`
247+
// ^ if we insert `Assoc=i32` without a comma here, that'd be invalid syntax:
248+
// `impl A<B, C Assoc=i32>`
249+
", ".to_owned()
250+
};
251+
252+
for (index, binding) in omitted_assoc_tys.into_iter().enumerate() {
253+
if index > 0 {
254+
associated_tys_sugg += ", ";
255+
}
256+
associated_tys_sugg += &snippet(cx, binding.span, "..");
257+
}
258+
if needs_angle_brackets {
259+
associated_tys_sugg += ">";
260+
}
261+
sugg.push((insert_span, associated_tys_sugg));
262+
}
263+
264+
diag.multipart_suggestion_with_style("try removing this bound", sugg, Applicability::MachineApplicable, SuggestionStyle::ShowAlways);
210265
}
211266
);
212267
}

‎tests/ui/implied_bounds_in_impls.fixed

Lines changed: 39 additions & 0 deletions
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

Lines changed: 39 additions & 0 deletions
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

Lines changed: 49 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -143,5 +143,53 @@ LL - fn default_generic_param2() -> impl PartialOrd + PartialEq + Debug {}
143143
LL + fn default_generic_param2() -> impl PartialOrd + Debug {}
144144
|
145145

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

0 commit comments

Comments
 (0)