Skip to content

[implied_bounds_in_impls]: include (previously omitted) associated types in suggestion #11459

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Sep 9, 2023
Merged

[implied_bounds_in_impls]: include (previously omitted) associated types in suggestion #11459

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7262145
[`implied_bounds_in_impl`]: fix suggestion for assoc types
y21 Sep 3, 2023
32e2511
extract lint emitting into separate fn
y21 Sep 3, 2023
30846b1
add comments in code to clarify and fix typo
y21 Sep 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
133 changes: 106 additions & 27 deletions clippy_lints/src/implied_bounds_in_impls.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,95 @@ declare_clippy_lint! {
}
declare_lint_pass!(ImpliedBoundsInImpls => [IMPLIED_BOUNDS_IN_IMPLS]);

#[allow(clippy::too_many_arguments)]
fn emit_lint(
cx: &LateContext<'_>,
poly_trait: &rustc_hir::PolyTraitRef<'_>,
opaque_ty: &rustc_hir::OpaqueTy<'_>,
index: usize,
// The bindings that were implied
implied_bindings: &[rustc_hir::TypeBinding<'_>],
// The original bindings that `implied_bindings` are implied from
implied_by_bindings: &[rustc_hir::TypeBinding<'_>],
implied_by_args: &[GenericArg<'_>],
implied_by_span: Span,
) {
let implied_by = snippet(cx, implied_by_span, "..");

span_lint_and_then(
cx,
IMPLIED_BOUNDS_IN_IMPLS,
poly_trait.span,
&format!("this bound is already specified as the supertrait of `{implied_by}`"),
|diag| {
// If we suggest removing a bound, we may also need to extend the span
// to include the `+` token that is ahead or behind,
// so we don't end up with something like `impl + B` or `impl A + `

let implied_span_extended = if let Some(next_bound) = opaque_ty.bounds.get(index + 1) {
poly_trait.span.to(next_bound.span().shrink_to_lo())
} else if index > 0
&& let Some(prev_bound) = opaque_ty.bounds.get(index - 1)
{
prev_bound.span().shrink_to_hi().to(poly_trait.span.shrink_to_hi())
} else {
poly_trait.span
};
Comment on lines +78 to +86
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm kinda worried about this check, it's kinda inconsistent (one checking index and the other not), but it may not matter if we make sure it doesn't fail.

Could you add a test with 3 bounds? Maybe some with generics and some without them?

Copy link
Member Author

@y21 y21 Sep 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason why index - 1 has a check and the other doesn't is mainly that - 1 (I assume you're referring to this) can panic with debug assertions due to overflow if it's the first bound, while + 1 in this context can't.
I mean, a slice with usize::MAX elements (which is needed here for an overflow to occur) is not only realistically impossible because nobody has that much ram but also theoretically impossible, since afaik allocations in rust can never be bigger than isize::MAX.

Only if there's neither an element before or after it (i.e. only one bound) will it not extend the span


let mut sugg = vec![(implied_span_extended, String::new())];

// We also might need to include associated type binding that were specified in the implied bound,
// but omitted in the implied-by bound:
// `fn f() -> impl Deref<Target = u8> + DerefMut`
// If we're going to suggest removing `Deref<..>`, we'll need to put `<Target = u8>` on `DerefMut`
let omitted_assoc_tys: Vec<_> = implied_bindings
.iter()
.filter(|binding| !implied_by_bindings.iter().any(|b| b.ident == binding.ident))
.collect();

if !omitted_assoc_tys.is_empty() {
// `<>` needs to be added if there aren't yet any generic arguments or bindings
let needs_angle_brackets = implied_by_args.is_empty() && implied_by_bindings.is_empty();
let insert_span = match (implied_by_args, implied_by_bindings) {
([.., arg], [.., binding]) => arg.span().max(binding.span).shrink_to_hi(),
([.., arg], []) => arg.span().shrink_to_hi(),
([], [.., binding]) => binding.span.shrink_to_hi(),
([], []) => implied_by_span.shrink_to_hi(),
};

let mut associated_tys_sugg = if needs_angle_brackets {
"<".to_owned()
} else {
// If angle brackets aren't needed (i.e., there are already generic arguments or bindings),
// we need to add a comma:
// `impl A<B, C >`
// ^ if we insert `Assoc=i32` without a comma here, that'd be invalid syntax:
// `impl A<B, C Assoc=i32>`
", ".to_owned()
};

for (index, binding) in omitted_assoc_tys.into_iter().enumerate() {
if index > 0 {
associated_tys_sugg += ", ";
}
associated_tys_sugg += &snippet(cx, binding.span, "..");
}
if needs_angle_brackets {
associated_tys_sugg += ">";
}
sugg.push((insert_span, associated_tys_sugg));
}

diag.multipart_suggestion_with_style(
"try removing this bound",
sugg,
Applicability::MachineApplicable,
SuggestionStyle::ShowAlways,
);
},
);
}

/// Tries to "resolve" a type.
/// The index passed to this function must start with `Self=0`, i.e. it must be a valid
/// type parameter index.
Expand Down Expand Up @@ -149,7 +238,7 @@ fn check(cx: &LateContext<'_>, decl: &FnDecl<'_>) {
&& let predicates = cx.tcx.super_predicates_of(trait_def_id).predicates
&& !predicates.is_empty() // If the trait has no supertrait, there is nothing to add.
{
Some((bound.span(), path.args.map_or([].as_slice(), |a| a.args), predicates, trait_def_id))
Some((bound.span(), path, predicates, trait_def_id))
} else {
None
}
Expand All @@ -162,10 +251,14 @@ fn check(cx: &LateContext<'_>, decl: &FnDecl<'_>) {
if let GenericBound::Trait(poly_trait, TraitBoundModifier::None) = bound
&& let [.., path] = poly_trait.trait_ref.path.segments
&& let implied_args = path.args.map_or([].as_slice(), |a| a.args)
&& 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(implied_by_span) = implied_bounds
&& let Some((implied_by_span, implied_by_args, implied_by_bindings)) = implied_bounds
.iter()
.find_map(|&(span, implied_by_args, preds, implied_by_def_id)| {
.find_map(|&(span, implied_by_path, preds, implied_by_def_id)| {
let implied_by_args = implied_by_path.args.map_or([].as_slice(), |a| a.args);
let implied_by_bindings = implied_by_path.args.map_or([].as_slice(), |a| a.bindings);

preds.iter().find_map(|(clause, _)| {
if let ClauseKind::Trait(tr) = clause.kind().skip_binder()
&& tr.def_id() == def_id
Expand All @@ -178,36 +271,22 @@ fn check(cx: &LateContext<'_>, decl: &FnDecl<'_>) {
def_id,
)
{
Some(span)
Some((span, implied_by_args, implied_by_bindings))
} else {
None
}
})
})
{
let implied_by = snippet(cx, implied_by_span, "..");
span_lint_and_then(
cx, IMPLIED_BOUNDS_IN_IMPLS,
poly_trait.span,
&format!("this bound is already specified as the supertrait of `{implied_by}`"),
|diag| {
// If we suggest removing a bound, we may also need extend the span
// to include the `+` token, so we don't end up with something like `impl + B`

let implied_span_extended = if let Some(next_bound) = opaque_ty.bounds.get(index + 1) {
poly_trait.span.to(next_bound.span().shrink_to_lo())
} else {
poly_trait.span
};

diag.span_suggestion_with_style(
implied_span_extended,
"try removing this bound",
"",
Applicability::MachineApplicable,
SuggestionStyle::ShowAlways
);
}
emit_lint(
cx,
poly_trait,
opaque_ty,
index,
implied_bindings,
implied_by_bindings,
implied_by_args,
implied_by_span
);
}
}
Expand Down
39 changes: 39 additions & 0 deletions tests/ui/implied_bounds_in_impls.fixed
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,4 +83,43 @@ mod issue11422 {
fn f() -> impl Trait1<()> + Trait2 {}
}

mod issue11435 {
// Associated type needs to be included on DoubleEndedIterator in the suggestion
fn my_iter() -> impl DoubleEndedIterator<Item = u32> {
0..5
}

// Removing the `Clone` bound should include the `+` behind it in its remove suggestion
fn f() -> impl Copy {
1
}

trait Trait1<T> {
type U;
}
impl Trait1<i32> for () {
type U = i64;
}
trait Trait2<T>: Trait1<T> {}
impl Trait2<i32> for () {}

// When the other trait has generics, it shouldn't add another pair of `<>`
fn f2() -> impl Trait2<i32, U = i64> {}

trait Trait3<T, U, V> {
type X;
type Y;
}
trait Trait4<T>: Trait3<T, i16, i64> {}
impl Trait3<i8, i16, i64> for () {
type X = i32;
type Y = i128;
}
impl Trait4<i8> for () {}

// Associated type `X` is specified, but `Y` is not, so only that associated type should be moved
// over
fn f3() -> impl Trait4<i8, X = i32, Y = i128> {}
}

fn main() {}
39 changes: 39 additions & 0 deletions tests/ui/implied_bounds_in_impls.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,4 +83,43 @@ mod issue11422 {
fn f() -> impl Trait1<()> + Trait2 {}
}

mod issue11435 {
// Associated type needs to be included on DoubleEndedIterator in the suggestion
fn my_iter() -> impl Iterator<Item = u32> + DoubleEndedIterator {
0..5
}

// Removing the `Clone` bound should include the `+` behind it in its remove suggestion
fn f() -> impl Copy + Clone {
1
}

trait Trait1<T> {
type U;
}
impl Trait1<i32> for () {
type U = i64;
}
trait Trait2<T>: Trait1<T> {}
impl Trait2<i32> for () {}

// When the other trait has generics, it shouldn't add another pair of `<>`
fn f2() -> impl Trait1<i32, U = i64> + Trait2<i32> {}

trait Trait3<T, U, V> {
type X;
type Y;
}
trait Trait4<T>: Trait3<T, i16, i64> {}
impl Trait3<i8, i16, i64> for () {
type X = i32;
type Y = i128;
}
impl Trait4<i8> for () {}

// Associated type `X` is specified, but `Y` is not, so only that associated type should be moved
// over
fn f3() -> impl Trait3<i8, i16, i64, X = i32, Y = i128> + Trait4<i8, X = i32> {}
}

fn main() {}
50 changes: 49 additions & 1 deletion tests/ui/implied_bounds_in_impls.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,5 +143,53 @@ LL - fn default_generic_param2() -> impl PartialOrd + PartialEq + Debug {}
LL + fn default_generic_param2() -> impl PartialOrd + Debug {}
|

error: aborting due to 12 previous errors
error: this bound is already specified as the supertrait of `DoubleEndedIterator`
--> $DIR/implied_bounds_in_impls.rs:88:26
|
LL | fn my_iter() -> impl Iterator<Item = u32> + DoubleEndedIterator {
| ^^^^^^^^^^^^^^^^^^^^
|
help: try removing this bound
|
LL - fn my_iter() -> impl Iterator<Item = u32> + DoubleEndedIterator {
LL + fn my_iter() -> impl DoubleEndedIterator<Item = u32> {
|

error: this bound is already specified as the supertrait of `Copy`
--> $DIR/implied_bounds_in_impls.rs:93:27
|
LL | fn f() -> impl Copy + Clone {
| ^^^^^
|
help: try removing this bound
|
LL - fn f() -> impl Copy + Clone {
LL + fn f() -> impl Copy {
|

error: this bound is already specified as the supertrait of `Trait2<i32>`
--> $DIR/implied_bounds_in_impls.rs:107:21
|
LL | fn f2() -> impl Trait1<i32, U = i64> + Trait2<i32> {}
| ^^^^^^^^^^^^^^^^^^^^
|
help: try removing this bound
|
LL - fn f2() -> impl Trait1<i32, U = i64> + Trait2<i32> {}
LL + fn f2() -> impl Trait2<i32, U = i64> {}
|

error: this bound is already specified as the supertrait of `Trait4<i8, X = i32>`
--> $DIR/implied_bounds_in_impls.rs:122:21
|
LL | fn f3() -> impl Trait3<i8, i16, i64, X = i32, Y = i128> + Trait4<i8, X = i32> {}
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
help: try removing this bound
|
LL - fn f3() -> impl Trait3<i8, i16, i64, X = i32, Y = i128> + Trait4<i8, X = i32> {}
LL + fn f3() -> impl Trait4<i8, X = i32, Y = i128> {}
|

error: aborting due to 16 previous errors