From 8878d674b1d030cbf87605fce2ebbd05cc8f3670 Mon Sep 17 00:00:00 2001 From: Allen Hsu Date: Wed, 13 Apr 2022 00:21:08 +1000 Subject: [PATCH] Lint for repeated traits within trait bounds or where clauses. --- clippy_lints/src/trait_bounds.rs | 129 +++++++++++++++++++- tests/compile-test.rs | 1 + tests/ui/trait_duplication_in_bounds.rs | 111 +++++++++++++++++ tests/ui/trait_duplication_in_bounds.stderr | 114 +++++++++++++++-- 4 files changed, 340 insertions(+), 15 deletions(-) diff --git a/clippy_lints/src/trait_bounds.rs b/clippy_lints/src/trait_bounds.rs index 2741179074bd..0a42a31fb8cf 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(arg: T) where T: Clone + Default {} /// ``` + /// + /// ```rust + /// fn foo(bar: T) {} + /// ``` + /// Use instead: + /// ```rust + /// fn foo(bar: T) {} + /// ``` + /// + /// ```rust + /// fn foo(bar: T) where T: Default + Default {} + /// ``` + /// Use instead: + /// ```rust + /// fn foo(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); + +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::>(); + 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 41048298349e..52cf751c8d0b 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 a21d4c5d637d..a5751c58aab8 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 { // This should not lint fn impl_trait(_: impl AsRef, _: impl AsRef) {} +mod repeated_where_clauses_or_trait_bounds { + fn bad_foo(arg0: T, argo1: U) { + unimplemented!(); + } + + fn bad_bar(arg0: T, arg1: U) + where + T: Clone + Clone + Clone + Copy, + U: Clone + Copy, + { + unimplemented!(); + } + + fn good_bar(arg0: T, arg1: U) { + unimplemented!(); + } + + fn good_foo(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 { + fn f(); + } + + trait GoodWhereClause { + fn f() + where + T: Clone + Copy, + U: Clone + Copy; + } + + trait BadTraitBound { + fn f(); + } + + trait BadWhereClause { + fn f() + where + T: Clone + Clone + Clone + Copy, + U: Clone + Copy; + } + + struct GoodStructBound { + t: T, + u: U, + } + + impl GoodTraitBound for GoodStructBound { + // this should not warn + fn f() {} + } + + struct GoodStructWhereClause; + + impl GoodTraitBound 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 {} + + // This should not warn but currently does see #8757 + fn good_generic + GenericTrait>(arg0: T) { + unimplemented!(); + } + + fn bad_generic + GenericTrait + GenericTrait>(arg0: T) { + unimplemented!(); + } + + mod foo { + pub trait Clone {} + } + + fn qualified_path(arg0: T) { + unimplemented!(); + } +} + fn main() {} diff --git a/tests/ui/trait_duplication_in_bounds.stderr b/tests/ui/trait_duplication_in_bounds.stderr index 6f8c8e47dfbf..7ef04e52708f 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(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(arg0: T, arg1: Z) | ^^^^^^^ @@ -20,7 +20,7 @@ LL | fn bad_foo(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, | ^^^^^^^^^^^^^^^^^^^^ | = 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(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(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 { + | ^^^^^ + | + = help: consider removing this trait bound + +error: these bounds contain repeated elements + --> $DIR/trait_duplication_in_bounds.rs:158:28 + | +LL | trait BadTraitBound { + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ 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 + GenericTrait>(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 + GenericTrait + GenericTrait>(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 + GenericTrait + GenericTrait>(arg0: T) { + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `GenericTrait + GenericTrait` + +error: this trait bound is already specified in the where clause + --> $DIR/trait_duplication_in_bounds.rs:207:26 + | +LL | fn qualified_path(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(arg0: T) { + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Clone + foo::Clone` + +error: aborting due to 22 previous errors