Auto merge of rust-lang#3766 - xd009642:issue-3764, r=flip1995

trait bounds lint - repeated types

This PR is to tackle rust-lang/rust-clippy#3764 it's still a WIP and doesn't work but this is an initial stab. It builds though I haven't added any tests as I'm not sure where lint tests should go?

Unfortunately, it seems id isn't tied to the type itself but I guess where it is in the AST? Looking at https://manishearth.github.io/rust-internals-docs/syntax/ast/struct.Ty.html I can't see any members that would let me tell if a type was repeated in multiple trait bounds.

There may be other issues with how I've implemented this so any assistance is appreciated!

changelog: Add new lint: `type_repetition_in_bounds`

CHANGELOG.md

@@ -1138,6 +1138,7 @@ Released 2018-09-13
 [`trivially_copy_pass_by_ref`]: https://rust-lang.github.io/rust-clippy/master/index.html#trivially_copy_pass_by_ref
 [`try_err`]: https://rust-lang.github.io/rust-clippy/master/index.html#try_err
 [`type_complexity`]: https://rust-lang.github.io/rust-clippy/master/index.html#type_complexity
+[`type_repetition_in_bounds`]: https://rust-lang.github.io/rust-clippy/master/index.html#type_repetition_in_bounds
 [`unicode_not_nfc`]: https://rust-lang.github.io/rust-clippy/master/index.html#unicode_not_nfc
 [`unimplemented`]: https://rust-lang.github.io/rust-clippy/master/index.html#unimplemented
 [`unit_arg`]: https://rust-lang.github.io/rust-clippy/master/index.html#unit_arg

README.md

@@ -6,7 +6,7 @@
 
 A collection of lints to catch common mistakes and improve your [Rust](https://github.com/rust-lang/rust) code.
 
-[There are 308 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html)
+[There are 309 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html)
 
 We have a bunch of lint categories to allow you to choose how much Clippy is supposed to ~~annoy~~ help you:
 

clippy_lints/src/lib.rs

@@ -263,6 +263,7 @@ pub mod strings;
 pub mod suspicious_trait_impl;
 pub mod swap;
 pub mod temporary_assignment;
+pub mod trait_bounds;
 pub mod transmute;
 pub mod transmuting_null;
 pub mod trivially_copy_pass_by_ref;
@@ -588,6 +589,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) {
     reg.register_late_lint_pass(box checked_conversions::CheckedConversions);
     reg.register_late_lint_pass(box integer_division::IntegerDivision);
     reg.register_late_lint_pass(box inherent_to_string::InherentToString);
+    reg.register_late_lint_pass(box trait_bounds::TraitBounds);
 
     reg.register_lint_group("clippy::restriction", Some("clippy_restriction"), vec![
         arithmetic::FLOAT_ARITHMETIC,
@@ -858,6 +860,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) {
         swap::ALMOST_SWAPPED,
         swap::MANUAL_SWAP,
         temporary_assignment::TEMPORARY_ASSIGNMENT,
+        trait_bounds::TYPE_REPETITION_IN_BOUNDS,
         transmute::CROSSPOINTER_TRANSMUTE,
         transmute::TRANSMUTE_BYTES_TO_STR,
         transmute::TRANSMUTE_INT_TO_BOOL,
@@ -1039,6 +1042,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) {
         reference::REF_IN_DEREF,
         swap::MANUAL_SWAP,
         temporary_assignment::TEMPORARY_ASSIGNMENT,
+        trait_bounds::TYPE_REPETITION_IN_BOUNDS,
         transmute::CROSSPOINTER_TRANSMUTE,
         transmute::TRANSMUTE_BYTES_TO_STR,
         transmute::TRANSMUTE_INT_TO_BOOL,

clippy_lints/src/trait_bounds.rs

@@ -0,0 +1,77 @@
+use crate::utils::{in_macro, snippet, span_help_and_lint, SpanlessHash};
+use rustc::hir::*;
+use rustc::lint::{LateContext, LateLintPass, LintArray, LintPass};
+use rustc::{declare_tool_lint, impl_lint_pass};
+use rustc_data_structures::fx::FxHashMap;
+
+#[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
+    ///
+    /// **Example:**
+    /// ```rust
+    /// pub fn foo<T>(t: T) where T: Copy, T: Clone
+    /// ```
+    ///
+    /// Could be written as:
+    ///
+    /// ```rust
+    /// pub fn foo<T>(t: T) where T: Copy + Clone
+    /// ```
+    pub TYPE_REPETITION_IN_BOUNDS,
+    complexity,
+    "Types are repeated unnecessary in trait bounds use `+` instead of using `T: _, T: _`"
+}
+
+impl_lint_pass!(TraitBounds => [TYPE_REPETITION_IN_BOUNDS]);
+
+impl<'a, 'tcx> LateLintPass<'a, 'tcx> for TraitBounds {
+    fn check_generics(&mut self, cx: &LateContext<'a, 'tcx>, gen: &'tcx Generics) {
+        if in_macro(gen.span) {
+            return;
+        }
+        let hash = |ty| -> u64 {
+            let mut hasher = SpanlessHash::new(cx, cx.tables);
+            hasher.hash_ty(ty);
+            hasher.finish()
+        };
+        let mut map = FxHashMap::default();
+        for bound in &gen.where_clause.predicates {
+            if let WherePredicate::BoundPredicate(ref p) = bound {
+                let h = hash(&p.bounded_ty);
+                if let Some(ref v) = map.insert(h, p.bounds.iter().collect::<Vec<_>>()) {
+                    let mut hint_string = format!(
+                        "consider combining the bounds: `{}:",
+                        snippet(cx, p.bounded_ty.span, "_")
+                    );
+                    for b in v.iter() {
+                        if let GenericBound::Trait(ref poly_trait_ref, _) = b {
+                            let path = &poly_trait_ref.trait_ref.path;
+                            hint_string.push_str(&format!(" {} +", path));
+                        }
+                    }
+                    for b in p.bounds.iter() {
+                        if let GenericBound::Trait(ref poly_trait_ref, _) = b {
+                            let path = &poly_trait_ref.trait_ref.path;
+                            hint_string.push_str(&format!(" {} +", path));
+                        }
+                    }
+                    hint_string.truncate(hint_string.len() - 2);
+                    hint_string.push('`');
+                    span_help_and_lint(
+                        cx,
+                        TYPE_REPETITION_IN_BOUNDS,
+                        p.span,
+                        "this type has already been used as a bound predicate",
+                        &hint_string,
+                    );
+                }
+            }
+        }
+    }
+}

clippy_lints/src/utils/hir_utils.rs

@@ -438,9 +438,9 @@ impl<'a, 'tcx> SpanlessHash<'a, 'tcx> {
                 self.hash_expr(fun);
                 self.hash_exprs(args);
             },
-            ExprKind::Cast(ref e, ref _ty) | ExprKind::Type(ref e, ref _ty) => {
+            ExprKind::Cast(ref e, ref ty) | ExprKind::Type(ref e, ref ty) => {
                 self.hash_expr(e);
-                // TODO: _ty
+                self.hash_ty(ty);
             },
             ExprKind::Closure(cap, _, eid, _, _) => {
                 match cap {
@@ -512,7 +512,10 @@ impl<'a, 'tcx> SpanlessHash<'a, 'tcx> {
                     self.hash_expr(e);
                 }
             },
-            ExprKind::Tup(ref v) | ExprKind::Array(ref v) => {
+            ExprKind::Tup(ref tup) => {
+                self.hash_exprs(tup);
+            },
+            ExprKind::Array(ref v) => {
                 self.hash_exprs(v);
             },
             ExprKind::Unary(lop, ref le) => {
@@ -574,4 +577,100 @@ impl<'a, 'tcx> SpanlessHash<'a, 'tcx> {
             },
         }
     }
+
+    pub fn hash_lifetime(&mut self, lifetime: &Lifetime) {
+        std::mem::discriminant(&lifetime.name).hash(&mut self.s);
+        if let LifetimeName::Param(ref name) = lifetime.name {
+            std::mem::discriminant(name).hash(&mut self.s);
+            match name {
+                ParamName::Plain(ref ident) => {
+                    ident.name.hash(&mut self.s);
+                },
+                ParamName::Fresh(ref size) => {
+                    size.hash(&mut self.s);
+                },
+                ParamName::Error => {},
+            }
+        }
+    }
+
+    pub fn hash_ty(&mut self, ty: &Ty) {
+        self.hash_tykind(&ty.node);
+    }
+
+    pub fn hash_tykind(&mut self, ty: &TyKind) {
+        std::mem::discriminant(ty).hash(&mut self.s);
+        match ty {
+            TyKind::Slice(ty) => {
+                self.hash_ty(ty);
+            },
+            TyKind::Array(ty, anon_const) => {
+                self.hash_ty(ty);
+                self.hash_expr(&self.cx.tcx.hir().body(anon_const.body).value);
+            },
+            TyKind::Ptr(mut_ty) => {
+                self.hash_ty(&mut_ty.ty);
+                mut_ty.mutbl.hash(&mut self.s);
+            },
+            TyKind::Rptr(lifetime, mut_ty) => {
+                self.hash_lifetime(lifetime);
+                self.hash_ty(&mut_ty.ty);
+                mut_ty.mutbl.hash(&mut self.s);
+            },
+            TyKind::BareFn(bfn) => {
+                bfn.unsafety.hash(&mut self.s);
+                bfn.abi.hash(&mut self.s);
+                for arg in &bfn.decl.inputs {
+                    self.hash_ty(&arg);
+                }
+                match bfn.decl.output {
+                    FunctionRetTy::DefaultReturn(_) => {
+                        ().hash(&mut self.s);
+                    },
+                    FunctionRetTy::Return(ref ty) => {
+                        self.hash_ty(ty);
+                    },
+                }
+                bfn.decl.c_variadic.hash(&mut self.s);
+            },
+            TyKind::Tup(ty_list) => {
+                for ty in ty_list {
+                    self.hash_ty(ty);
+                }
+            },
+            TyKind::Path(qpath) => match qpath {
+                QPath::Resolved(ref maybe_ty, ref path) => {
+                    if let Some(ref ty) = maybe_ty {
+                        self.hash_ty(ty);
+                    }
+                    for segment in &path.segments {
+                        segment.ident.name.hash(&mut self.s);
+                    }
+                },
+                QPath::TypeRelative(ref ty, ref segment) => {
+                    self.hash_ty(ty);
+                    segment.ident.name.hash(&mut self.s);
+                },
+            },
+            TyKind::Def(_, 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_expr(&self.cx.tcx.hir().body(ca.value.body).value);
+                        },
+                    }
+                }
+            },
+            TyKind::TraitObject(_, lifetime) => {
+                self.hash_lifetime(lifetime);
+            },
+            TyKind::Typeof(anon_const) => {
+                self.hash_expr(&self.cx.tcx.hir().body(anon_const.body).value);
+            },
+            TyKind::CVarArgs(lifetime) => self.hash_lifetime(lifetime),
+            TyKind::Err | TyKind::Infer | TyKind::Never => {},
+        }
+    }
 }

src/lintlist/mod.rs

@@ -6,7 +6,7 @@ pub use lint::Lint;
 pub use lint::LINT_LEVELS;
 
 // begin lint list, do not remove this comment, it’s used in `update_lints`
-pub const ALL_LINTS: [Lint; 308] = [
+pub const ALL_LINTS: [Lint; 309] = [
     Lint {
         name: "absurd_extreme_comparisons",
         group: "correctness",
@@ -1848,6 +1848,13 @@ pub const ALL_LINTS: [Lint; 308] = [
         deprecation: None,
         module: "types",
     },
+    Lint {
+        name: "type_repetition_in_bounds",
+        group: "complexity",
+        desc: "Types are repeated unnecessary in trait bounds use `+` instead of using `T: _, T: _`",
+        deprecation: None,
+        module: "trait_bounds",
+    },
     Lint {
         name: "unicode_not_nfc",
         group: "pedantic",

tests/ui/float_cmp.rs

@@ -8,8 +8,7 @@ const ONE: f32 = ZERO + 1.0;
 
 fn twice<T>(x: T) -> T
 where
-    T: Add<T, Output = T>,
-    T: Copy,
+    T: Add<T, Output = T> + Copy,
 {
     x + x
 }

tests/ui/float_cmp.stderr

@@ -1,36 +1,36 @@
 error: strict comparison of f32 or f64
-  --> $DIR/float_cmp.rs:60:5
+  --> $DIR/float_cmp.rs:59:5
    |
 LL |     ONE as f64 != 2.0;
    |     ^^^^^^^^^^^^^^^^^ help: consider comparing them within some error: `(ONE as f64 - 2.0).abs() > error`
    |
    = note: `-D clippy::float-cmp` implied by `-D warnings`
 note: std::f32::EPSILON and std::f64::EPSILON are available.
-  --> $DIR/float_cmp.rs:60:5
+  --> $DIR/float_cmp.rs:59:5
    |
 LL |     ONE as f64 != 2.0;
    |     ^^^^^^^^^^^^^^^^^
 
 error: strict comparison of f32 or f64
-  --> $DIR/float_cmp.rs:65:5
+  --> $DIR/float_cmp.rs:64:5
    |
 LL |     x == 1.0;
    |     ^^^^^^^^ help: consider comparing them within some error: `(x - 1.0).abs() < error`
    |
 note: std::f32::EPSILON and std::f64::EPSILON are available.
-  --> $DIR/float_cmp.rs:65:5
+  --> $DIR/float_cmp.rs:64:5
    |
 LL |     x == 1.0;
    |     ^^^^^^^^
 
 error: strict comparison of f32 or f64
-  --> $DIR/float_cmp.rs:68:5
+  --> $DIR/float_cmp.rs:67:5
    |
 LL |     twice(x) != twice(ONE as f64);
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider comparing them within some error: `(twice(x) - twice(ONE as f64)).abs() > error`
    |
 note: std::f32::EPSILON and std::f64::EPSILON are available.
-  --> $DIR/float_cmp.rs:68:5
+  --> $DIR/float_cmp.rs:67:5
    |
 LL |     twice(x) != twice(ONE as f64);
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

tests/ui/type_repetition_in_bounds.rs

@@ -0,0 +1,19 @@
+#[deny(clippy::type_repetition_in_bounds)]
+
+pub fn foo<T>(_t: T)
+where
+    T: Copy,
+    T: Clone,
+{
+    unimplemented!();
+}
+
+pub fn bar<T, U>(_t: T, _u: U)
+where
+    T: Copy,
+    U: Clone,
+{
+    unimplemented!();
+}
+
+fn main() {}

tests/ui/type_repetition_in_bounds.stderr

@@ -0,0 +1,15 @@
+error: this type has already been used as a bound predicate
+  --> $DIR/type_repetition_in_bounds.rs:6:5
+   |
+LL |     T: Clone,
+   |     ^^^^^^^^
+   |
+note: lint level defined here
+  --> $DIR/type_repetition_in_bounds.rs:1:8
+   |
+LL | #[deny(clippy::type_repetition_in_bounds)]
+   |        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   = help: consider combining the bounds: `T: Copy + Clone`
+
+error: aborting due to previous error
+