diff --git a/clippy_lints/src/implied_bounds_in_impls.rs b/clippy_lints/src/implied_bounds_in_impls.rs index 64bdb8ba84e23..ec9044bba5cc3 100644 --- a/clippy_lints/src/implied_bounds_in_impls.rs +++ b/clippy_lints/src/implied_bounds_in_impls.rs @@ -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 + }; + + 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 + DerefMut` + // If we're going to suggest removing `Deref<..>`, we'll need to put `` 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` + // ^ if we insert `Assoc=i32` without a comma here, that'd be invalid syntax: + // `impl A` + ", ".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. @@ -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 } @@ -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 @@ -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 ); } } diff --git a/tests/ui/implied_bounds_in_impls.fixed b/tests/ui/implied_bounds_in_impls.fixed index 69fcc8921d641..a50fa0ccf6e7f 100644 --- a/tests/ui/implied_bounds_in_impls.fixed +++ b/tests/ui/implied_bounds_in_impls.fixed @@ -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 { + 0..5 + } + + // Removing the `Clone` bound should include the `+` behind it in its remove suggestion + fn f() -> impl Copy { + 1 + } + + trait Trait1 { + type U; + } + impl Trait1 for () { + type U = i64; + } + trait Trait2: Trait1 {} + impl Trait2 for () {} + + // When the other trait has generics, it shouldn't add another pair of `<>` + fn f2() -> impl Trait2 {} + + trait Trait3 { + type X; + type Y; + } + trait Trait4: Trait3 {} + impl Trait3 for () { + type X = i32; + type Y = i128; + } + impl Trait4 for () {} + + // Associated type `X` is specified, but `Y` is not, so only that associated type should be moved + // over + fn f3() -> impl Trait4 {} +} + fn main() {} diff --git a/tests/ui/implied_bounds_in_impls.rs b/tests/ui/implied_bounds_in_impls.rs index 5504feae613e5..e74ed4425b81e 100644 --- a/tests/ui/implied_bounds_in_impls.rs +++ b/tests/ui/implied_bounds_in_impls.rs @@ -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 + DoubleEndedIterator { + 0..5 + } + + // Removing the `Clone` bound should include the `+` behind it in its remove suggestion + fn f() -> impl Copy + Clone { + 1 + } + + trait Trait1 { + type U; + } + impl Trait1 for () { + type U = i64; + } + trait Trait2: Trait1 {} + impl Trait2 for () {} + + // When the other trait has generics, it shouldn't add another pair of `<>` + fn f2() -> impl Trait1 + Trait2 {} + + trait Trait3 { + type X; + type Y; + } + trait Trait4: Trait3 {} + impl Trait3 for () { + type X = i32; + type Y = i128; + } + impl Trait4 for () {} + + // Associated type `X` is specified, but `Y` is not, so only that associated type should be moved + // over + fn f3() -> impl Trait3 + Trait4 {} +} + fn main() {} diff --git a/tests/ui/implied_bounds_in_impls.stderr b/tests/ui/implied_bounds_in_impls.stderr index 7245126982435..72dc2a183a383 100644 --- a/tests/ui/implied_bounds_in_impls.stderr +++ b/tests/ui/implied_bounds_in_impls.stderr @@ -144,5 +144,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 + DoubleEndedIterator { + | ^^^^^^^^^^^^^^^^^^^^ + | +help: try removing this bound + | +LL - fn my_iter() -> impl Iterator + DoubleEndedIterator { +LL + fn my_iter() -> impl DoubleEndedIterator { + | + +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` + --> $DIR/implied_bounds_in_impls.rs:107:21 + | +LL | fn f2() -> impl Trait1 + Trait2 {} + | ^^^^^^^^^^^^^^^^^^^^ + | +help: try removing this bound + | +LL - fn f2() -> impl Trait1 + Trait2 {} +LL + fn f2() -> impl Trait2 {} + | + +error: this bound is already specified as the supertrait of `Trait4` + --> $DIR/implied_bounds_in_impls.rs:122:21 + | +LL | fn f3() -> impl Trait3 + Trait4 {} + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: try removing this bound + | +LL - fn f3() -> impl Trait3 + Trait4 {} +LL + fn f3() -> impl Trait4 {} + | + +error: aborting due to 16 previous errors