diff --git a/CHANGELOG.md b/CHANGELOG.md index ff5aebbc892b..089897811a59 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/README.md b/README.md index 1e649d8982fe..38651f72eb36 100644 --- a/README.md +++ b/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: diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 6d90d315f005..908bbeb5e19f 100644 --- a/clippy_lints/src/lib.rs +++ b/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, diff --git a/clippy_lints/src/trait_bounds.rs b/clippy_lints/src/trait_bounds.rs new file mode 100644 index 000000000000..8a719c0dd041 --- /dev/null +++ b/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) where T: Copy, T: Clone + /// ``` + /// + /// Could be written as: + /// + /// ```rust + /// pub fn foo(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::>()) { + 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, + ); + } + } + } + } +} diff --git a/clippy_lints/src/utils/hir_utils.rs b/clippy_lints/src/utils/hir_utils.rs index e9b5cee430e7..6fc5939a2160 100644 --- a/clippy_lints/src/utils/hir_utils.rs +++ b/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 => {}, + } + } } diff --git a/src/lintlist/mod.rs b/src/lintlist/mod.rs index 2d9e800b6e43..49bce5a6cef4 100644 --- a/src/lintlist/mod.rs +++ b/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", diff --git a/tests/ui/float_cmp.rs b/tests/ui/float_cmp.rs index 1ec8af3e3871..207c1bcbbc67 100644 --- a/tests/ui/float_cmp.rs +++ b/tests/ui/float_cmp.rs @@ -8,8 +8,7 @@ const ONE: f32 = ZERO + 1.0; fn twice(x: T) -> T where - T: Add, - T: Copy, + T: Add + Copy, { x + x } diff --git a/tests/ui/float_cmp.stderr b/tests/ui/float_cmp.stderr index 5dc5fbf0f6e8..116e3e90e637 100644 --- a/tests/ui/float_cmp.stderr +++ b/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); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ diff --git a/tests/ui/type_repetition_in_bounds.rs b/tests/ui/type_repetition_in_bounds.rs new file mode 100644 index 000000000000..8b538be762b0 --- /dev/null +++ b/tests/ui/type_repetition_in_bounds.rs @@ -0,0 +1,19 @@ +#[deny(clippy::type_repetition_in_bounds)] + +pub fn foo(_t: T) +where + T: Copy, + T: Clone, +{ + unimplemented!(); +} + +pub fn bar(_t: T, _u: U) +where + T: Copy, + U: Clone, +{ + unimplemented!(); +} + +fn main() {} diff --git a/tests/ui/type_repetition_in_bounds.stderr b/tests/ui/type_repetition_in_bounds.stderr new file mode 100644 index 000000000000..a72f512b0129 --- /dev/null +++ b/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 +