From 726214596486f59064747469dd86f19cf00ba6a3 Mon Sep 17 00:00:00 2001 From: y21 <30553356+y21@users.noreply.github.com> Date: Sun, 3 Sep 2023 22:21:03 +0200 Subject: [PATCH 1/3] [`implied_bounds_in_impl`]: fix suggestion for assoc types --- clippy_lints/src/implied_bounds_in_impls.rs | 79 +++++++++++++++++---- tests/ui/implied_bounds_in_impls.fixed | 39 ++++++++++ tests/ui/implied_bounds_in_impls.rs | 39 ++++++++++ tests/ui/implied_bounds_in_impls.stderr | 50 ++++++++++++- 4 files changed, 194 insertions(+), 13 deletions(-) diff --git a/clippy_lints/src/implied_bounds_in_impls.rs b/clippy_lints/src/implied_bounds_in_impls.rs index 64bdb8ba84e23..165dc2548e129 100644 --- a/clippy_lints/src/implied_bounds_in_impls.rs +++ b/clippy_lints/src/implied_bounds_in_impls.rs @@ -149,7 +149,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 +162,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,7 +182,7 @@ fn check(cx: &LateContext<'_>, decl: &FnDecl<'_>) { def_id, ) { - Some(span) + Some((span, implied_by_args, implied_by_bindings)) } else { None } @@ -192,21 +196,72 @@ fn check(cx: &LateContext<'_>, decl: &FnDecl<'_>) { &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` + // 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 }; - diag.span_suggestion_with_style( - implied_span_extended, - "try removing this bound", - "", - Applicability::MachineApplicable, - SuggestionStyle::ShowAlways - ); + 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() + // TODO: is checking idents enough for stuff like ` == ` + .find(|b| b.ident == binding.ident) + .is_none() + }) + .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); } ); } 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 d6a4ae889fe89..9f381efba0a4c 100644 --- a/tests/ui/implied_bounds_in_impls.stderr +++ b/tests/ui/implied_bounds_in_impls.stderr @@ -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 + 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 From 32e25118a69e0235f14dd971714bc653e8788d8c Mon Sep 17 00:00:00 2001 From: y21 <30553356+y21@users.noreply.github.com> Date: Sun, 3 Sep 2023 22:53:37 +0200 Subject: [PATCH 2/3] extract lint emitting into separate fn --- clippy_lints/src/implied_bounds_in_impls.rs | 170 +++++++++++--------- 1 file changed, 96 insertions(+), 74 deletions(-) diff --git a/clippy_lints/src/implied_bounds_in_impls.rs b/clippy_lints/src/implied_bounds_in_impls.rs index 165dc2548e129..120ab5a034e8f 100644 --- a/clippy_lints/src/implied_bounds_in_impls.rs +++ b/clippy_lints/src/implied_bounds_in_impls.rs @@ -50,6 +50,93 @@ 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, + implied_bindings: &[rustc_hir::TypeBinding<'_>], + 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 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. @@ -189,80 +276,15 @@ fn check(cx: &LateContext<'_>, decl: &FnDecl<'_>) { }) }) { - 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 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() - // TODO: is checking idents enough for stuff like ` == ` - .find(|b| b.ident == binding.ident) - .is_none() - }) - .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); - } + emit_lint( + cx, + poly_trait, + opaque_ty, + index, + implied_bindings, + implied_by_bindings, + implied_by_args, + implied_by_span ); } } From 30846b16a0feaf25e4a78d468d1b56920e775aae Mon Sep 17 00:00:00 2001 From: y21 <30553356+y21@users.noreply.github.com> Date: Tue, 5 Sep 2023 21:55:08 +0200 Subject: [PATCH 3/3] add comments in code to clarify and fix typo --- clippy_lints/src/implied_bounds_in_impls.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/clippy_lints/src/implied_bounds_in_impls.rs b/clippy_lints/src/implied_bounds_in_impls.rs index 120ab5a034e8f..ec9044bba5cc3 100644 --- a/clippy_lints/src/implied_bounds_in_impls.rs +++ b/clippy_lints/src/implied_bounds_in_impls.rs @@ -56,7 +56,9 @@ fn emit_lint( 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, @@ -69,7 +71,7 @@ fn emit_lint( 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 + // 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 + `