Auto merge of rust-lang#11459 - y21:issue11435, r=blyxyas

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

Fixes rust-lang#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

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<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.
@@ -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
                 );
             }
         }

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<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() {}

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<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() {}

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<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