Auto merge of #8703 - aldhsu:add_repeated_where_clause_or_trait_bound, r=flip1995

Add `repeated_where_clause_or_trait_bound` lint

I thought I would try and scratch my own itch for #8674.

1. Is comparing the `Res` the correct way for ensuring we have the same trait?
2. Is there a way to get the spans for the bounds and clauses for suggestions?
I tried to use `GenericParam::bounds_span_for_suggestions` but it only gave me an empty span at the end of the spans.
I tried `WhereClause::span_for_predicates_or_empty_place` and it included the comma.
3. Is there a simpler way to get the trait names? I have used the spans of the traits because I didn't see a way to get it off the `Res` or `Def`.

changelog: Add ``[`repeated_where_clause_or_trait_bound`]`` lint.

4 files changed, 340 insertions, 15 deletions

diff --git a/clippy_lints/src/trait_bounds.rs b/clippy_lints/src/trait_bounds.rs
index 2741179074b..0a42a31fb8c 100644
--- a/clippy_lints/src/trait_bounds.rs
+++ b/clippy_lints/src/trait_bounds.rs
@@ -1,5 +1,5 @@
-use clippy_utils::diagnostics::span_lint_and_help;
-use clippy_utils::source::{snippet, snippet_with_applicability};
+use clippy_utils::diagnostics::{span_lint_and_help, span_lint_and_sugg};
+use clippy_utils::source::{snippet, snippet_opt, snippet_with_applicability};
 use clippy_utils::{SpanlessEq, SpanlessHash};
 use core::hash::{Hash, Hasher};
 use if_chain::if_chain;
@@ -9,8 +9,8 @@ use rustc_data_structures::unhash::UnhashMap;
 use rustc_errors::Applicability;
 use rustc_hir::def::Res;
 use rustc_hir::{
-    GenericBound, Generics, Item, ItemKind, Node, Path, PathSegment, PredicateOrigin, QPath, TraitBoundModifier,
-    TraitItem, Ty, TyKind, WherePredicate,
+    GenericArg, GenericBound, Generics, Item, ItemKind, Node, Path, PathSegment, PredicateOrigin, QPath,
+    TraitBoundModifier, TraitItem, TraitRef, Ty, TyKind, WherePredicate,
 };
 use rustc_lint::{LateContext, LateLintPass};
 use rustc_session::{declare_tool_lint, impl_lint_pass};
@@ -36,7 +36,7 @@ declare_clippy_lint! {
     #[clippy::version = "1.38.0"]
     pub TYPE_REPETITION_IN_BOUNDS,
     nursery,
-    "Types are repeated unnecessary in trait bounds use `+` instead of using `T: _, T: _`"
+    "types are repeated unnecessary in trait bounds use `+` instead of using `T: _, T: _`"
 }
 
 declare_clippy_lint! {
@@ -63,10 +63,26 @@ declare_clippy_lint! {
     ///
     /// fn func<T>(arg: T) where T: Clone + Default {}
     /// ```
+    ///
+    /// ```rust
+    /// fn foo<T: Default + Default>(bar: T) {}
+    /// ```
+    /// Use instead:
+    /// ```rust
+    /// fn foo<T: Default>(bar: T) {}
+    /// ```
+    ///
+    /// ```rust
+    /// fn foo<T>(bar: T) where T: Default + Default {}
+    /// ```
+    /// Use instead:
+    /// ```rust
+    /// fn foo<T>(bar: T) where T: Default {}
+    /// ```
     #[clippy::version = "1.47.0"]
     pub TRAIT_DUPLICATION_IN_BOUNDS,
     nursery,
-    "Check if the same trait bounds are specified twice during a function declaration"
+    "check if the same trait bounds are specified more than once during a generic declaration"
 }
 
 #[derive(Copy, Clone)]
@@ -87,6 +103,19 @@ impl<'tcx> LateLintPass<'tcx> for TraitBounds {
     fn check_generics(&mut self, cx: &LateContext<'tcx>, gen: &'tcx Generics<'_>) {
         self.check_type_repetition(cx, gen);
         check_trait_bound_duplication(cx, gen);
+        check_bounds_or_where_duplication(cx, gen);
+    }
+
+    fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx Item<'tcx>) {
+        // special handling for self trait bounds as these are not considered generics
+        // ie. trait Foo: Display {}
+        if let Item {
+            kind: ItemKind::Trait(_, _, _, bounds, ..),
+            ..
+        } = item
+        {
+            rollup_traits(cx, bounds, "these bounds contain repeated elements");
+        }
     }
 
     fn check_trait_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx TraitItem<'tcx>) {
@@ -241,6 +270,26 @@ fn check_trait_bound_duplication(cx: &LateContext<'_>, gen: &'_ Generics<'_>) {
     }
 }
 
+#[derive(PartialEq, Eq, Hash, Debug)]
+struct ComparableTraitRef(Res, Vec<Res>);
+
+fn check_bounds_or_where_duplication(cx: &LateContext<'_>, gen: &'_ Generics<'_>) {
+    if gen.span.from_expansion() {
+        return;
+    }
+
+    for predicate in gen.predicates {
+        if let WherePredicate::BoundPredicate(ref bound_predicate) = predicate {
+            let msg = if predicate.in_where_clause() {
+                "these where clauses contain repeated elements"
+            } else {
+                "these bounds contain repeated elements"
+            };
+            rollup_traits(cx, bound_predicate.bounds, msg);
+        }
+    }
+}
+
 fn get_trait_info_from_bound<'a>(bound: &'a GenericBound<'_>) -> Option<(Res, &'a [PathSegment<'a>], Span)> {
     if let GenericBound::Trait(t, tbm) = bound {
         let trait_path = t.trait_ref.path;
@@ -257,3 +306,71 @@ fn get_trait_info_from_bound<'a>(bound: &'a GenericBound<'_>) -> Option<(Res, &'
         None
     }
 }
+
+// FIXME: ComparableTraitRef does not support nested bounds needed for associated_type_bounds
+fn into_comparable_trait_ref(trait_ref: &TraitRef<'_>) -> ComparableTraitRef {
+    ComparableTraitRef(
+        trait_ref.path.res,
+        trait_ref
+            .path
+            .segments
+            .iter()
+            .filter_map(|segment| {
+                // get trait bound type arguments
+                Some(segment.args?.args.iter().filter_map(|arg| {
+                    if_chain! {
+                        if let GenericArg::Type(ty) = arg;
+                        if let TyKind::Path(QPath::Resolved(_, path)) = ty.kind;
+                        then { return Some(path.res) }
+                    }
+                    None
+                }))
+            })
+            .flatten()
+            .collect(),
+    )
+}
+
+fn rollup_traits(cx: &LateContext<'_>, bounds: &[GenericBound<'_>], msg: &str) {
+    let mut map = FxHashMap::default();
+    let mut repeated_res = false;
+
+    let only_comparable_trait_refs = |bound: &GenericBound<'_>| {
+        if let GenericBound::Trait(t, _) = bound {
+            Some((into_comparable_trait_ref(&t.trait_ref), t.span))
+        } else {
+            None
+        }
+    };
+
+    for bound in bounds.iter().filter_map(only_comparable_trait_refs) {
+        let (comparable_bound, span_direct) = bound;
+        if map.insert(comparable_bound, span_direct).is_some() {
+            repeated_res = true;
+        }
+    }
+
+    if_chain! {
+        if repeated_res;
+        if let [first_trait, .., last_trait] = bounds;
+        then {
+            let all_trait_span = first_trait.span().to(last_trait.span());
+
+            let mut traits = map.values()
+                .filter_map(|span| snippet_opt(cx, *span))
+                .collect::<Vec<_>>();
+            traits.sort_unstable();
+            let traits = traits.join(" + ");
+
+            span_lint_and_sugg(
+                cx,
+                TRAIT_DUPLICATION_IN_BOUNDS,
+                all_trait_span,
+                msg,
+                "try",
+                traits,
+                Applicability::MachineApplicable
+            );
+        }
+    }
+}
diff --git a/tests/compile-test.rs b/tests/compile-test.rs
index 41048298349..52cf751c8d0 100644
--- a/tests/compile-test.rs
+++ b/tests/compile-test.rs
@@ -397,6 +397,7 @@ const RUSTFIX_COVERAGE_KNOWN_EXCEPTIONS: &[&str] = &[
     "single_component_path_imports_nested_first.rs",
     "string_add.rs",
     "toplevel_ref_arg_non_rustfix.rs",
+    "trait_duplication_in_bounds.rs",
     "unit_arg.rs",
     "unnecessary_clone.rs",
     "unnecessary_lazy_eval_unfixable.rs",
diff --git a/tests/ui/trait_duplication_in_bounds.rs b/tests/ui/trait_duplication_in_bounds.rs
index a21d4c5d637..a5751c58aab 100644
--- a/tests/ui/trait_duplication_in_bounds.rs
+++ b/tests/ui/trait_duplication_in_bounds.rs
@@ -1,4 +1,5 @@
 #![deny(clippy::trait_duplication_in_bounds)]
+#![allow(unused)]
 
 use std::collections::BTreeMap;
 use std::ops::{Add, AddAssign, Div, DivAssign, Mul, MulAssign, Sub, SubAssign};
@@ -98,4 +99,114 @@ trait FooIter: Iterator<Item = Foo> {
 // This should not lint
 fn impl_trait(_: impl AsRef<str>, _: impl AsRef<str>) {}
 
+mod repeated_where_clauses_or_trait_bounds {
+    fn bad_foo<T: Clone + Clone + Clone + Copy, U: Clone + Copy>(arg0: T, argo1: U) {
+        unimplemented!();
+    }
+
+    fn bad_bar<T, U>(arg0: T, arg1: U)
+    where
+        T: Clone + Clone + Clone + Copy,
+        U: Clone + Copy,
+    {
+        unimplemented!();
+    }
+
+    fn good_bar<T: Clone + Copy, U: Clone + Copy>(arg0: T, arg1: U) {
+        unimplemented!();
+    }
+
+    fn good_foo<T, U>(arg0: T, arg1: U)
+    where
+        T: Clone + Copy,
+        U: Clone + Copy,
+    {
+        unimplemented!();
+    }
+
+    trait GoodSelfTraitBound: Clone + Copy {
+        fn f();
+    }
+
+    trait GoodSelfWhereClause {
+        fn f()
+        where
+            Self: Clone + Copy;
+    }
+
+    trait BadSelfTraitBound: Clone + Clone + Clone {
+        fn f();
+    }
+
+    trait BadSelfWhereClause {
+        fn f()
+        where
+            Self: Clone + Clone + Clone;
+    }
+
+    trait GoodTraitBound<T: Clone + Copy, U: Clone + Copy> {
+        fn f();
+    }
+
+    trait GoodWhereClause<T, U> {
+        fn f()
+        where
+            T: Clone + Copy,
+            U: Clone + Copy;
+    }
+
+    trait BadTraitBound<T: Clone + Clone + Clone + Copy, U: Clone + Copy> {
+        fn f();
+    }
+
+    trait BadWhereClause<T, U> {
+        fn f()
+        where
+            T: Clone + Clone + Clone + Copy,
+            U: Clone + Copy;
+    }
+
+    struct GoodStructBound<T: Clone + Copy, U: Clone + Copy> {
+        t: T,
+        u: U,
+    }
+
+    impl<T: Clone + Copy, U: Clone + Copy> GoodTraitBound<T, U> for GoodStructBound<T, U> {
+        // this should not warn
+        fn f() {}
+    }
+
+    struct GoodStructWhereClause;
+
+    impl<T, U> GoodTraitBound<T, U> for GoodStructWhereClause
+    where
+        T: Clone + Copy,
+        U: Clone + Copy,
+    {
+        // this should not warn
+        fn f() {}
+    }
+
+    fn no_error_separate_arg_bounds(program: impl AsRef<()>, dir: impl AsRef<()>, args: &[impl AsRef<()>]) {}
+
+    trait GenericTrait<T> {}
+
+    // This should not warn but currently does see #8757
+    fn good_generic<T: GenericTrait<u64> + GenericTrait<u32>>(arg0: T) {
+        unimplemented!();
+    }
+
+    fn bad_generic<T: GenericTrait<u64> + GenericTrait<u32> + GenericTrait<u64>>(arg0: T) {
+        unimplemented!();
+    }
+
+    mod foo {
+        pub trait Clone {}
+    }
+
+    fn qualified_path<T: std::clone::Clone + Clone + foo::Clone>(arg0: T) {
+        unimplemented!();
+    }
+}
+
 fn main() {}
diff --git a/tests/ui/trait_duplication_in_bounds.stderr b/tests/ui/trait_duplication_in_bounds.stderr
index 6f8c8e47dfb..7ef04e52708 100644
--- a/tests/ui/trait_duplication_in_bounds.stderr
+++ b/tests/ui/trait_duplication_in_bounds.stderr
@@ -1,5 +1,5 @@
 error: this trait bound is already specified in the where clause
-  --> $DIR/trait_duplication_in_bounds.rs:6:15
+  --> $DIR/trait_duplication_in_bounds.rs:7:15
    |
 LL | fn bad_foo<T: Clone + Default, Z: Copy>(arg0: T, arg1: Z)
    |               ^^^^^
@@ -12,7 +12,7 @@ LL | #![deny(clippy::trait_duplication_in_bounds)]
    = help: consider removing this trait bound
 
 error: this trait bound is already specified in the where clause
-  --> $DIR/trait_duplication_in_bounds.rs:6:23
+  --> $DIR/trait_duplication_in_bounds.rs:7:23
    |
 LL | fn bad_foo<T: Clone + Default, Z: Copy>(arg0: T, arg1: Z)
    |                       ^^^^^^^
@@ -20,7 +20,7 @@ LL | fn bad_foo<T: Clone + Default, Z: Copy>(arg0: T, arg1: Z)
    = help: consider removing this trait bound
 
 error: this trait bound is already specified in trait declaration
-  --> $DIR/trait_duplication_in_bounds.rs:35:15
+  --> $DIR/trait_duplication_in_bounds.rs:36:15
    |
 LL |         Self: Default;
    |               ^^^^^^^
@@ -28,7 +28,7 @@ LL |         Self: Default;
    = help: consider removing this trait bound
 
 error: this trait bound is already specified in trait declaration
-  --> $DIR/trait_duplication_in_bounds.rs:49:15
+  --> $DIR/trait_duplication_in_bounds.rs:50:15
    |
 LL |         Self: Default + Clone;
    |               ^^^^^^^
@@ -36,7 +36,7 @@ LL |         Self: Default + Clone;
    = help: consider removing this trait bound
 
 error: this trait bound is already specified in trait declaration
-  --> $DIR/trait_duplication_in_bounds.rs:55:15
+  --> $DIR/trait_duplication_in_bounds.rs:56:15
    |
 LL |         Self: Default + Clone;
    |               ^^^^^^^
@@ -44,7 +44,7 @@ LL |         Self: Default + Clone;
    = help: consider removing this trait bound
 
 error: this trait bound is already specified in trait declaration
-  --> $DIR/trait_duplication_in_bounds.rs:55:25
+  --> $DIR/trait_duplication_in_bounds.rs:56:25
    |
 LL |         Self: Default + Clone;
    |                         ^^^^^
@@ -52,7 +52,7 @@ LL |         Self: Default + Clone;
    = help: consider removing this trait bound
 
 error: this trait bound is already specified in trait declaration
-  --> $DIR/trait_duplication_in_bounds.rs:58:15
+  --> $DIR/trait_duplication_in_bounds.rs:59:15
    |
 LL |         Self: Default;
    |               ^^^^^^^
@@ -60,12 +60,108 @@ LL |         Self: Default;
    = help: consider removing this trait bound
 
 error: this trait bound is already specified in trait declaration
-  --> $DIR/trait_duplication_in_bounds.rs:93:15
+  --> $DIR/trait_duplication_in_bounds.rs:94:15
    |
 LL |         Self: Iterator<Item = Foo>,
    |               ^^^^^^^^^^^^^^^^^^^^
    |
    = help: consider removing this trait bound
 
-error: aborting due to 8 previous errors
+error: this trait bound is already specified in the where clause
+  --> $DIR/trait_duplication_in_bounds.rs:103:19
+   |
+LL |     fn bad_foo<T: Clone + Clone + Clone + Copy, U: Clone + Copy>(arg0: T, argo1: U) {
+   |                   ^^^^^
+   |
+   = help: consider removing this trait bound
+
+error: these bounds contain repeated elements
+  --> $DIR/trait_duplication_in_bounds.rs:103:19
+   |
+LL |     fn bad_foo<T: Clone + Clone + Clone + Copy, U: Clone + Copy>(arg0: T, argo1: U) {
+   |                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Clone + Copy`
+
+error: this trait bound is already specified in the where clause
+  --> $DIR/trait_duplication_in_bounds.rs:109:12
+   |
+LL |         T: Clone + Clone + Clone + Copy,
+   |            ^^^^^
+   |
+   = help: consider removing this trait bound
+
+error: these where clauses contain repeated elements
+  --> $DIR/trait_duplication_in_bounds.rs:109:12
+   |
+LL |         T: Clone + Clone + Clone + Copy,
+   |            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Clone + Copy`
+
+error: these bounds contain repeated elements
+  --> $DIR/trait_duplication_in_bounds.rs:137:30
+   |
+LL |     trait BadSelfTraitBound: Clone + Clone + Clone {
+   |                              ^^^^^^^^^^^^^^^^^^^^^ help: try: `Clone`
+
+error: these where clauses contain repeated elements
+  --> $DIR/trait_duplication_in_bounds.rs:144:19
+   |
+LL |             Self: Clone + Clone + Clone;
+   |                   ^^^^^^^^^^^^^^^^^^^^^ help: try: `Clone`
+
+error: this trait bound is already specified in the where clause
+  --> $DIR/trait_duplication_in_bounds.rs:158:28
+   |
+LL |     trait BadTraitBound<T: Clone + Clone + Clone + Copy, U: Clone + Copy> {
+   |                            ^^^^^
+   |
+   = help: consider removing this trait bound
+
+error: these bounds contain repeated elements
+  --> $DIR/trait_duplication_in_bounds.rs:158:28
+   |
+LL |     trait BadTraitBound<T: Clone + Clone + Clone + Copy, U: Clone + Copy> {
+   |                            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Clone + Copy`
+
+error: these where clauses contain repeated elements
+  --> $DIR/trait_duplication_in_bounds.rs:165:16
+   |
+LL |             T: Clone + Clone + Clone + Copy,
+   |                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Clone + Copy`
+
+error: this trait bound is already specified in the where clause
+  --> $DIR/trait_duplication_in_bounds.rs:195:24
+   |
+LL |     fn good_generic<T: GenericTrait<u64> + GenericTrait<u32>>(arg0: T) {
+   |                        ^^^^^^^^^^^^^^^^^
+   |
+   = help: consider removing this trait bound
+
+error: this trait bound is already specified in the where clause
+  --> $DIR/trait_duplication_in_bounds.rs:199:23
+   |
+LL |     fn bad_generic<T: GenericTrait<u64> + GenericTrait<u32> + GenericTrait<u64>>(arg0: T) {
+   |                       ^^^^^^^^^^^^^^^^^
+   |
+   = help: consider removing this trait bound
+
+error: these bounds contain repeated elements
+  --> $DIR/trait_duplication_in_bounds.rs:199:23
+   |
+LL |     fn bad_generic<T: GenericTrait<u64> + GenericTrait<u32> + GenericTrait<u64>>(arg0: T) {
+   |                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `GenericTrait<u32> + GenericTrait<u64>`
+
+error: this trait bound is already specified in the where clause
+  --> $DIR/trait_duplication_in_bounds.rs:207:26
+   |
+LL |     fn qualified_path<T: std::clone::Clone + Clone + foo::Clone>(arg0: T) {
+   |                          ^^^^^^^^^^^^^^^^^
+   |
+   = help: consider removing this trait bound
+
+error: these bounds contain repeated elements
+  --> $DIR/trait_duplication_in_bounds.rs:207:26
+   |
+LL |     fn qualified_path<T: std::clone::Clone + Clone + foo::Clone>(arg0: T) {
+   |                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Clone + foo::Clone`
+
+error: aborting due to 22 previous errors