Auto merge of rust-lang#5761 - ThibsG:TypeRepetitionThreshold, r=flip…

…1995 Improvements for `type_repetition_in_bounds` lint Some improvements for `type_repetition_in_bounds`: - add a configurable threshold to trigger the lint (rust-lang#4380). The lint won't trigger anymore if there are more bounds (strictly) than `conf.max_trait_bounds` on this type. - take generic args into account over bounded type (rust-lang#4323) - don't lint for predicates generated in macros (rust-lang#4326) Fixes rust-lang#4380, Fixes rust-lang#4323, Fixes rust-lang#4326, Closes rust-lang#3764 changelog: Fix multiple FPs in `type_repetition_in_bounds` and add a configuration option Note: the rust-lang#3764 has already been fixed but not closed
flip1995 · Jul 3, 2020 · fff8e72 · fff8e72
2 parents c493090 + 2d5930a
commit fff8e72
Show file tree

Hide file tree

Showing 7 changed files with 107 additions and 21 deletions.
diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs
@@ -996,7 +996,8 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
     store.register_late_pass(|| box checked_conversions::CheckedConversions);
     store.register_late_pass(|| box integer_division::IntegerDivision);
     store.register_late_pass(|| box inherent_to_string::InherentToString);
-    store.register_late_pass(|| box trait_bounds::TraitBounds);
+    let max_trait_bounds = conf.max_trait_bounds;
+    store.register_late_pass(move || box trait_bounds::TraitBounds::new(max_trait_bounds));
     store.register_late_pass(|| box comparison_chain::ComparisonChain);
     store.register_late_pass(|| box mut_key::MutableKeyType);
     store.register_late_pass(|| box modulo_arithmetic::ModuloArithmetic);
@@ -1033,7 +1034,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
     let array_size_threshold = conf.array_size_threshold;
     store.register_late_pass(move || box large_stack_arrays::LargeStackArrays::new(array_size_threshold));
     store.register_late_pass(move || box large_const_arrays::LargeConstArrays::new(array_size_threshold));
-    store.register_late_pass(move || box floating_point_arithmetic::FloatingPointArithmetic);
+    store.register_late_pass(|| box floating_point_arithmetic::FloatingPointArithmetic);
     store.register_early_pass(|| box as_conversions::AsConversions);
     store.register_early_pass(|| box utils::internal_lints::ProduceIce);
     store.register_late_pass(|| box let_underscore::LetUnderscore);

diff --git a/clippy_lints/src/trait_bounds.rs b/clippy_lints/src/trait_bounds.rs
@@ -1,19 +1,19 @@
 use crate::utils::{in_macro, snippet, snippet_with_applicability, span_lint_and_help, SpanlessHash};
+use if_chain::if_chain;
 use rustc_data_structures::fx::FxHashMap;
 use rustc_errors::Applicability;
 use rustc_hir::{GenericBound, Generics, WherePredicate};
 use rustc_lint::{LateContext, LateLintPass};
 use rustc_session::{declare_tool_lint, impl_lint_pass};
 
-#[derive(Copy, Clone)]
-pub struct TraitBounds;
-
 declare_clippy_lint! {
     /// **What it does:** This lint warns about unnecessary type repetitions in trait bounds
     ///
     /// **Why is this bad?** Repeating the type for every bound makes the code
     /// less readable than combining the bounds
     ///
+    /// **Known problems:** None.
+    ///
     /// **Example:**
     /// ```rust
     /// pub fn foo<T>(t: T) where T: Copy, T: Clone {}
@@ -29,6 +29,18 @@ declare_clippy_lint! {
     "Types are repeated unnecessary in trait bounds use `+` instead of using `T: _, T: _`"
 }
 
+#[derive(Copy, Clone)]
+pub struct TraitBounds {
+    max_trait_bounds: u64,
+}
+
+impl TraitBounds {
+    #[must_use]
+    pub fn new(max_trait_bounds: u64) -> Self {
+        Self { max_trait_bounds }
+    }
+}
+
 impl_lint_pass!(TraitBounds => [TYPE_REPETITION_IN_BOUNDS]);
 
 impl<'tcx> LateLintPass<'tcx> for TraitBounds {
@@ -44,9 +56,14 @@ impl<'tcx> LateLintPass<'tcx> for TraitBounds {
         let mut map = FxHashMap::default();
         let mut applicability = Applicability::MaybeIncorrect;
         for bound in gen.where_clause.predicates {
-            if let WherePredicate::BoundPredicate(ref p) = bound {
+            if_chain! {
+                if let WherePredicate::BoundPredicate(ref p) = bound;
+                if p.bounds.len() as u64 <= self.max_trait_bounds;
+                if !in_macro(p.span);
                 let h = hash(&p.bounded_ty);
-                if let Some(ref v) = map.insert(h, p.bounds.iter().collect::<Vec<_>>()) {
+                if let Some(ref v) = map.insert(h, p.bounds.iter().collect::<Vec<_>>());
+
+                then {
                     let mut hint_string = format!(
                         "consider combining the bounds: `{}:",
                         snippet(cx, p.bounded_ty.span, "_")

diff --git a/clippy_lints/src/utils/conf.rs b/clippy_lints/src/utils/conf.rs
@@ -156,6 +156,8 @@ define_Conf! {
     (array_size_threshold, "array_size_threshold": u64, 512_000),
     /// Lint: VEC_BOX. The size of the boxed type in bytes, where boxing in a `Vec` is allowed
     (vec_box_size_threshold, "vec_box_size_threshold": u64, 4096),
+    /// Lint: TYPE_REPETITION_IN_BOUNDS. The maximum number of bounds a trait can have to be linted
+    (max_trait_bounds, "max_trait_bounds": u64, 3),
     /// Lint: STRUCT_EXCESSIVE_BOOLS. The maximum number of bools a struct can have
     (max_struct_bools, "max_struct_bools": u64, 3),
     /// Lint: FN_PARAMS_EXCESSIVE_BOOLS. The maximum number of bools function parameters can have

diff --git a/clippy_lints/src/utils/hir_utils.rs b/clippy_lints/src/utils/hir_utils.rs
@@ -703,6 +703,7 @@ impl<'a, 'tcx> SpanlessHash<'a, 'tcx> {
                     }
                     for segment in path.segments {
                         segment.ident.name.hash(&mut self.s);
+                        self.hash_generic_args(segment.generic_args().args);
                     }
                 },
                 QPath::TypeRelative(ref ty, ref segment) => {
@@ -711,13 +712,7 @@ impl<'a, 'tcx> SpanlessHash<'a, 'tcx> {
                 },
             },
             TyKind::OpaqueDef(_, arg_list) => {
-                for arg in *arg_list {
-                    match arg {
-                        GenericArg::Lifetime(ref l) => self.hash_lifetime(l),
-                        GenericArg::Type(ref ty) => self.hash_ty(&ty),
-                        GenericArg::Const(ref ca) => self.hash_body(ca.value.body),
-                    }
-                }
+                self.hash_generic_args(arg_list);
             },
             TyKind::TraitObject(_, lifetime) => {
                 self.hash_lifetime(lifetime);
@@ -735,4 +730,14 @@ impl<'a, 'tcx> SpanlessHash<'a, 'tcx> {
         self.hash_expr(&self.cx.tcx.hir().body(body_id).value);
         self.maybe_typeck_tables = old_maybe_typeck_tables;
     }
+
+    fn hash_generic_args(&mut self, arg_list: &[GenericArg<'_>]) {
+        for arg in arg_list {
+            match arg {
+                GenericArg::Lifetime(ref l) => self.hash_lifetime(l),
+                GenericArg::Type(ref ty) => self.hash_ty(&ty),
+                GenericArg::Const(ref ca) => self.hash_body(ca.value.body),
+            }
+        }
+    }
 }
diff --git a/tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr b/tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr
@@ -1,4 +1,4 @@
-error: error reading Clippy's configuration file `$DIR/clippy.toml`: unknown field `foobar`, expected one of `blacklisted-names`, `cognitive-complexity-threshold`, `cyclomatic-complexity-threshold`, `doc-valid-idents`, `too-many-arguments-threshold`, `type-complexity-threshold`, `single-char-binding-names-threshold`, `too-large-for-stack`, `enum-variant-name-threshold`, `enum-variant-size-threshold`, `verbose-bit-mask-threshold`, `literal-representation-threshold`, `trivial-copy-size-limit`, `too-many-lines-threshold`, `array-size-threshold`, `vec-box-size-threshold`, `max-struct-bools`, `max-fn-params-bools`, `warn-on-all-wildcard-imports`, `third-party` at line 5 column 1
+error: error reading Clippy's configuration file `$DIR/clippy.toml`: unknown field `foobar`, expected one of `blacklisted-names`, `cognitive-complexity-threshold`, `cyclomatic-complexity-threshold`, `doc-valid-idents`, `too-many-arguments-threshold`, `type-complexity-threshold`, `single-char-binding-names-threshold`, `too-large-for-stack`, `enum-variant-name-threshold`, `enum-variant-size-threshold`, `verbose-bit-mask-threshold`, `literal-representation-threshold`, `trivial-copy-size-limit`, `too-many-lines-threshold`, `array-size-threshold`, `vec-box-size-threshold`, `max-trait-bounds`, `max-struct-bools`, `max-fn-params-bools`, `warn-on-all-wildcard-imports`, `third-party` at line 5 column 1
 
 error: aborting due to previous error
 
diff --git a/tests/ui/type_repetition_in_bounds.rs b/tests/ui/type_repetition_in_bounds.rs
@@ -1,4 +1,6 @@
-#[deny(clippy::type_repetition_in_bounds)]
+#![deny(clippy::type_repetition_in_bounds)]
+
+use std::ops::{Add, AddAssign, Div, DivAssign, Mul, MulAssign, Sub, SubAssign};
 
 pub fn foo<T>(_t: T)
 where
@@ -16,4 +18,55 @@ where
     unimplemented!();
 }
 
+// Threshold test (see #4380)
+trait LintBounds
+where
+    Self: Clone,
+    Self: Copy + Default + Ord,
+    Self: Add<Output = Self> + AddAssign + Sub<Output = Self> + SubAssign,
+    Self: Mul<Output = Self> + MulAssign + Div<Output = Self> + DivAssign,
+{
+}
+
+trait LotsOfBounds
+where
+    Self: Clone + Copy + Default + Ord,
+    Self: Add<Output = Self> + AddAssign + Sub<Output = Self> + SubAssign,
+    Self: Mul<Output = Self> + MulAssign + Div<Output = Self> + DivAssign,
+{
+}
+
+// Generic distinction (see #4323)
+mod issue4323 {
+    pub struct Foo<A>(A);
+    pub struct Bar<A, B> {
+        a: Foo<A>,
+        b: Foo<B>,
+    }
+
+    impl<A, B> Unpin for Bar<A, B>
+    where
+        Foo<A>: Unpin,
+        Foo<B>: Unpin,
+    {
+    }
+}
+
+// Extern macros shouldn't lint (see #4326)
+extern crate serde;
+mod issue4326 {
+    use serde::{Deserialize, Serialize};
+
+    trait Foo {}
+    impl Foo for String {}
+
+    #[derive(Debug, Serialize, Deserialize)]
+    struct Bar<S>
+    where
+        S: Foo,
+    {
+        foo: S,
+    }
+}
+
 fn main() {}
diff --git a/tests/ui/type_repetition_in_bounds.stderr b/tests/ui/type_repetition_in_bounds.stderr
@@ -1,15 +1,23 @@
 error: this type has already been used as a bound predicate
-  --> $DIR/type_repetition_in_bounds.rs:6:5
+  --> $DIR/type_repetition_in_bounds.rs:8:5
    |
 LL |     T: Clone,
    |     ^^^^^^^^
    |
 note: the lint level is defined here
-  --> $DIR/type_repetition_in_bounds.rs:1:8
+  --> $DIR/type_repetition_in_bounds.rs:1:9
    |
-LL | #[deny(clippy::type_repetition_in_bounds)]
-   |        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+LL | #![deny(clippy::type_repetition_in_bounds)]
+   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    = help: consider combining the bounds: `T: Copy + Clone`
 
-error: aborting due to previous error
+error: this type has already been used as a bound predicate
+  --> $DIR/type_repetition_in_bounds.rs:25:5
+   |
+LL |     Self: Copy + Default + Ord,
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = help: consider combining the bounds: `Self: Clone + Copy + Default + Ord`
+
+error: aborting due to 2 previous errors