From 171d082433904096a7c563d937c860d52ba9b25f Mon Sep 17 00:00:00 2001 From: Allen Hsu Date: Tue, 12 Jul 2022 22:29:12 +1000 Subject: [PATCH 1/4] Compare where predicates to trait bounds. - only compare where predicates to trait bounds when generating where clause specific message to fix #9151 - use comparable_trait_ref to account for trait bound generics to fix #8757 --- clippy_lints/src/trait_bounds.rs | 71 ++++++++++++++------- tests/ui/trait_duplication_in_bounds.stderr | 50 +-------------- 2 files changed, 50 insertions(+), 71 deletions(-) diff --git a/clippy_lints/src/trait_bounds.rs b/clippy_lints/src/trait_bounds.rs index 0a42a31fb8cf..537e6eafb175 100644 --- a/clippy_lints/src/trait_bounds.rs +++ b/clippy_lints/src/trait_bounds.rs @@ -4,7 +4,7 @@ use clippy_utils::{SpanlessEq, SpanlessHash}; use core::hash::{Hash, Hasher}; use if_chain::if_chain; use itertools::Itertools; -use rustc_data_structures::fx::FxHashMap; +use rustc_data_structures::fx::{FxHashMap, FxHashSet}; use rustc_data_structures::unhash::UnhashMap; use rustc_errors::Applicability; use rustc_hir::def::Res; @@ -238,31 +238,58 @@ fn check_trait_bound_duplication(cx: &LateContext<'_>, gen: &'_ Generics<'_>) { return; } - let mut map = FxHashMap::<_, Vec<_>>::default(); - for predicate in gen.predicates { + // Explanation: + // fn bad_foo(arg0: T, arg1: Z) + // where T: Clone + Default, { unimplemented!(); } + // ^^^^^^^^^^^^^^^^^^ + // | + // collects each of these where clauses into a set keyed by generic name and comparable trait + // eg. (T, Clone) + let where_predicates = gen + .predicates + .iter() + .filter_map(|pred| { + if_chain! { + if pred.in_where_clause(); + if let WherePredicate::BoundPredicate(bound_predicate) = pred; + if let TyKind::Path(QPath::Resolved(_, path)) = bound_predicate.bounded_ty.kind; + then { + return Some(bound_predicate.bounds.iter().filter_map(|t| { + Some((path.res, into_comparable_trait_ref(t.trait_ref()?))) + })) + } + } + None + }) + .flatten() + .collect::>(); + + // Explanation: + // fn bad_foo(arg0: T, arg1: Z) ... + // ^^^^^^^^^^^^^^^^^^ ^^^^^^^ + // | + // compare trait bounds keyed by generic name and comparable trait to collected where + // predicates eg. (T, Clone) + for predicate in gen.predicates.iter().filter(|pred| !pred.in_where_clause()) { if_chain! { - if let WherePredicate::BoundPredicate(ref bound_predicate) = predicate; + if let WherePredicate::BoundPredicate(bound_predicate) = predicate; if bound_predicate.origin != PredicateOrigin::ImplTrait; if !bound_predicate.span.from_expansion(); - if let TyKind::Path(QPath::Resolved(_, Path { segments, .. })) = bound_predicate.bounded_ty.kind; - if let Some(segment) = segments.first(); + if let TyKind::Path(QPath::Resolved(_, path)) = bound_predicate.bounded_ty.kind; then { - for (res_where, _, span_where) in bound_predicate.bounds.iter().filter_map(get_trait_info_from_bound) { - let trait_resolutions_direct = map.entry(segment.ident).or_default(); - if let Some((_, span_direct)) = trait_resolutions_direct - .iter() - .find(|(res_direct, _)| *res_direct == res_where) { - span_lint_and_help( - cx, - TRAIT_DUPLICATION_IN_BOUNDS, - *span_direct, - "this trait bound is already specified in the where clause", - None, - "consider removing this trait bound", - ); - } - else { - trait_resolutions_direct.push((res_where, span_where)); + for t in bound_predicate.bounds { + if let Some(trait_ref) = t.trait_ref() { + let key = (path.res, into_comparable_trait_ref(trait_ref)); + if where_predicates.contains(&key) { + span_lint_and_help( + cx, + TRAIT_DUPLICATION_IN_BOUNDS, + t.span(), + "this trait bound is already specified in the where clause", + None, + "consider removing this trait bound", + ); + } } } } diff --git a/tests/ui/trait_duplication_in_bounds.stderr b/tests/ui/trait_duplication_in_bounds.stderr index 7ef04e52708f..9b603fdea327 100644 --- a/tests/ui/trait_duplication_in_bounds.stderr +++ b/tests/ui/trait_duplication_in_bounds.stderr @@ -67,28 +67,12 @@ LL | Self: Iterator, | = help: consider removing this trait bound -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 | @@ -107,14 +91,6 @@ error: these where clauses contain repeated elements 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 | @@ -127,41 +103,17 @@ error: these where clauses contain repeated elements 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 +error: aborting due to 16 previous errors From b96842d7d7b6fbb8509471fb30dfaa8f5877fcb2 Mon Sep 17 00:00:00 2001 From: Allen Hsu Date: Wed, 13 Jul 2022 21:25:19 +1000 Subject: [PATCH 2/4] Split unfixable lints. --- tests/compile-test.rs | 1 - tests/ui/trait_duplication_in_bounds.fixed | 112 +++++++++ tests/ui/trait_duplication_in_bounds.rs | 218 +++++------------- tests/ui/trait_duplication_in_bounds.stderr | 117 +++------- .../trait_duplication_in_bounds_unfixable.rs | 101 ++++++++ ...ait_duplication_in_bounds_unfixable.stderr | 71 ++++++ 6 files changed, 370 insertions(+), 250 deletions(-) create mode 100644 tests/ui/trait_duplication_in_bounds.fixed create mode 100644 tests/ui/trait_duplication_in_bounds_unfixable.rs create mode 100644 tests/ui/trait_duplication_in_bounds_unfixable.stderr diff --git a/tests/compile-test.rs b/tests/compile-test.rs index 92ac1a2be561..610f1f14563d 100644 --- a/tests/compile-test.rs +++ b/tests/compile-test.rs @@ -394,7 +394,6 @@ 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.fixed b/tests/ui/trait_duplication_in_bounds.fixed new file mode 100644 index 000000000000..b4e6bf0ea1c2 --- /dev/null +++ b/tests/ui/trait_duplication_in_bounds.fixed @@ -0,0 +1,112 @@ +// run-rustfix +#![deny(clippy::trait_duplication_in_bounds)] +#![allow(unused)] + +fn bad_foo(arg0: T, argo1: U) { + unimplemented!(); +} + +fn bad_bar(arg0: T, arg1: U) +where + T: 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 { + fn f(); +} + +trait BadSelfWhereClause { + fn f() + where + Self: 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 + 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 {} + +fn good_generic + GenericTrait>(arg0: T) { + unimplemented!(); +} + +fn bad_generic + 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.rs b/tests/ui/trait_duplication_in_bounds.rs index a5751c58aab8..7f2e96a22e66 100644 --- a/tests/ui/trait_duplication_in_bounds.rs +++ b/tests/ui/trait_duplication_in_bounds.rs @@ -1,212 +1,112 @@ +// run-rustfix #![deny(clippy::trait_duplication_in_bounds)] #![allow(unused)] -use std::collections::BTreeMap; -use std::ops::{Add, AddAssign, Div, DivAssign, Mul, MulAssign, Sub, SubAssign}; +fn bad_foo(arg0: T, argo1: U) { + unimplemented!(); +} -fn bad_foo(arg0: T, arg1: Z) +fn bad_bar(arg0: T, arg1: U) where - T: Clone, - T: Default, + T: Clone + Clone + Clone + Copy, + U: Clone + Copy, { unimplemented!(); } -fn good_bar(arg: T) { +fn good_bar(arg0: T, arg1: U) { unimplemented!(); } -fn good_foo(arg: T) +fn good_foo(arg0: T, arg1: U) where - T: Clone + Default, + T: Clone + Copy, + U: Clone + Copy, { unimplemented!(); } -fn good_foobar(arg: T) -where - T: Clone, -{ - unimplemented!(); +trait GoodSelfTraitBound: Clone + Copy { + fn f(); } -trait T: Default { +trait GoodSelfWhereClause { fn f() where - Self: Default; + Self: Clone + Copy; } -trait U: Default { +trait BadSelfTraitBound: Clone + Clone + Clone { + fn f(); +} + +trait BadSelfWhereClause { fn f() where - Self: Clone; + Self: Clone + Clone + Clone; +} + +trait GoodTraitBound { + fn f(); } -trait ZZ: Default { - fn g(); - fn h(); +trait GoodWhereClause { fn f() where - Self: Default + Clone; + T: Clone + Copy, + U: Clone + Copy; } -trait BadTrait: Default + Clone { +trait BadTraitBound { + fn f(); +} + +trait BadWhereClause { fn f() where - Self: Default + Clone; - fn g() - where - Self: Default; - fn h() - where - Self: Copy; + T: Clone + Clone + Clone + Copy, + U: Clone + Copy; } -#[derive(Default, Clone)] -struct Life; +struct GoodStructBound { + t: T, + u: U, +} -impl T for Life { +impl GoodTraitBound for GoodStructBound { // this should not warn fn f() {} } -impl U for Life { +struct GoodStructWhereClause; + +impl GoodTraitBound for GoodStructWhereClause +where + T: Clone + Copy, + U: Clone + Copy, +{ // this should not warn fn f() {} } -// should not warn -trait Iter: Iterator { - fn into_group_btreemap(self) -> BTreeMap> - where - Self: Iterator + Sized, - K: Ord + Eq, - { - unimplemented!(); - } -} +fn no_error_separate_arg_bounds(program: impl AsRef<()>, dir: impl AsRef<()>, args: &[impl AsRef<()>]) {} -struct Foo; +trait GenericTrait {} -trait FooIter: Iterator { - fn bar() - where - Self: Iterator, - { - } +fn good_generic + GenericTrait>(arg0: T) { + unimplemented!(); } -// 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!(); - } +fn bad_generic + GenericTrait + GenericTrait>(arg0: T) { + unimplemented!(); +} - mod foo { - pub trait Clone {} - } +mod foo { + pub trait Clone {} +} - fn qualified_path(arg0: T) { - unimplemented!(); - } +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 9b603fdea327..86c593811a74 100644 --- a/tests/ui/trait_duplication_in_bounds.stderr +++ b/tests/ui/trait_duplication_in_bounds.stderr @@ -1,119 +1,56 @@ -error: this trait bound is already specified in the where clause - --> $DIR/trait_duplication_in_bounds.rs:7:15 +error: these bounds contain repeated elements + --> $DIR/trait_duplication_in_bounds.rs:5:15 | -LL | fn bad_foo(arg0: T, arg1: Z) - | ^^^^^ +LL | fn bad_foo(arg0: T, argo1: U) { + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Clone + Copy` | note: the lint level is defined here - --> $DIR/trait_duplication_in_bounds.rs:1:9 + --> $DIR/trait_duplication_in_bounds.rs:2:9 | 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:7:23 - | -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:36:15 - | -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:50:15 - | -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:56:15 - | -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:56:25 - | -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:59:15 - | -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:94:15 - | -LL | Self: Iterator, - | ^^^^^^^^^^^^^^^^^^^^ - | - = 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: these where clauses contain repeated elements - --> $DIR/trait_duplication_in_bounds.rs:109:12 + --> $DIR/trait_duplication_in_bounds.rs:11:8 | -LL | T: Clone + Clone + Clone + Copy, - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Clone + Copy` +LL | T: Clone + Clone + Clone + Copy, + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Clone + Copy` error: these bounds contain repeated elements - --> $DIR/trait_duplication_in_bounds.rs:137:30 + --> $DIR/trait_duplication_in_bounds.rs:39:26 | -LL | trait BadSelfTraitBound: Clone + Clone + Clone { - | ^^^^^^^^^^^^^^^^^^^^^ help: try: `Clone` +LL | trait BadSelfTraitBound: Clone + Clone + Clone { + | ^^^^^^^^^^^^^^^^^^^^^ help: try: `Clone` error: these where clauses contain repeated elements - --> $DIR/trait_duplication_in_bounds.rs:144:19 + --> $DIR/trait_duplication_in_bounds.rs:46:15 | -LL | Self: Clone + Clone + Clone; - | ^^^^^^^^^^^^^^^^^^^^^ help: try: `Clone` +LL | Self: Clone + Clone + Clone; + | ^^^^^^^^^^^^^^^^^^^^^ help: try: `Clone` error: these bounds contain repeated elements - --> $DIR/trait_duplication_in_bounds.rs:158:28 + --> $DIR/trait_duplication_in_bounds.rs:60:24 | -LL | trait BadTraitBound { - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Clone + Copy` +LL | trait BadTraitBound { + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Clone + Copy` error: these where clauses contain repeated elements - --> $DIR/trait_duplication_in_bounds.rs:165:16 + --> $DIR/trait_duplication_in_bounds.rs:67:12 | -LL | T: Clone + Clone + Clone + Copy, - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Clone + Copy` +LL | T: Clone + Clone + Clone + Copy, + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Clone + Copy` error: these bounds contain repeated elements - --> $DIR/trait_duplication_in_bounds.rs:199:23 + --> $DIR/trait_duplication_in_bounds.rs:100:19 | -LL | fn bad_generic + GenericTrait + GenericTrait>(arg0: T) { - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `GenericTrait + GenericTrait` +LL | fn bad_generic + GenericTrait + GenericTrait>(arg0: T) { + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `GenericTrait + GenericTrait` error: these bounds contain repeated elements - --> $DIR/trait_duplication_in_bounds.rs:207:26 + --> $DIR/trait_duplication_in_bounds.rs:108:22 | -LL | fn qualified_path(arg0: T) { - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Clone + foo::Clone` +LL | fn qualified_path(arg0: T) { + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Clone + foo::Clone` -error: aborting due to 16 previous errors +error: aborting due to 8 previous errors diff --git a/tests/ui/trait_duplication_in_bounds_unfixable.rs b/tests/ui/trait_duplication_in_bounds_unfixable.rs new file mode 100644 index 000000000000..a21d4c5d637d --- /dev/null +++ b/tests/ui/trait_duplication_in_bounds_unfixable.rs @@ -0,0 +1,101 @@ +#![deny(clippy::trait_duplication_in_bounds)] + +use std::collections::BTreeMap; +use std::ops::{Add, AddAssign, Div, DivAssign, Mul, MulAssign, Sub, SubAssign}; + +fn bad_foo(arg0: T, arg1: Z) +where + T: Clone, + T: Default, +{ + unimplemented!(); +} + +fn good_bar(arg: T) { + unimplemented!(); +} + +fn good_foo(arg: T) +where + T: Clone + Default, +{ + unimplemented!(); +} + +fn good_foobar(arg: T) +where + T: Clone, +{ + unimplemented!(); +} + +trait T: Default { + fn f() + where + Self: Default; +} + +trait U: Default { + fn f() + where + Self: Clone; +} + +trait ZZ: Default { + fn g(); + fn h(); + fn f() + where + Self: Default + Clone; +} + +trait BadTrait: Default + Clone { + fn f() + where + Self: Default + Clone; + fn g() + where + Self: Default; + fn h() + where + Self: Copy; +} + +#[derive(Default, Clone)] +struct Life; + +impl T for Life { + // this should not warn + fn f() {} +} + +impl U for Life { + // this should not warn + fn f() {} +} + +// should not warn +trait Iter: Iterator { + fn into_group_btreemap(self) -> BTreeMap> + where + Self: Iterator + Sized, + K: Ord + Eq, + { + unimplemented!(); + } +} + +struct Foo; + +trait FooIter: Iterator { + fn bar() + where + Self: Iterator, + { + } +} + +// This should not lint +fn impl_trait(_: impl AsRef, _: impl AsRef) {} + +fn main() {} diff --git a/tests/ui/trait_duplication_in_bounds_unfixable.stderr b/tests/ui/trait_duplication_in_bounds_unfixable.stderr new file mode 100644 index 000000000000..fbd9abb005f1 --- /dev/null +++ b/tests/ui/trait_duplication_in_bounds_unfixable.stderr @@ -0,0 +1,71 @@ +error: this trait bound is already specified in the where clause + --> $DIR/trait_duplication_in_bounds_unfixable.rs:6:15 + | +LL | fn bad_foo(arg0: T, arg1: Z) + | ^^^^^ + | +note: the lint level is defined here + --> $DIR/trait_duplication_in_bounds_unfixable.rs:1:9 + | +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_unfixable.rs:6:23 + | +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_unfixable.rs:35:15 + | +LL | Self: Default; + | ^^^^^^^ + | + = help: consider removing this trait bound + +error: this trait bound is already specified in trait declaration + --> $DIR/trait_duplication_in_bounds_unfixable.rs:49:15 + | +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_unfixable.rs:55:15 + | +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_unfixable.rs:55:25 + | +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_unfixable.rs:58:15 + | +LL | Self: Default; + | ^^^^^^^ + | + = help: consider removing this trait bound + +error: this trait bound is already specified in trait declaration + --> $DIR/trait_duplication_in_bounds_unfixable.rs:93:15 + | +LL | Self: Iterator, + | ^^^^^^^^^^^^^^^^^^^^ + | + = help: consider removing this trait bound + +error: aborting due to 8 previous errors + From 3ddc04f4db1f768f74fe9e21a4ef119c186295a0 Mon Sep 17 00:00:00 2001 From: Allen Hsu Date: Wed, 13 Jul 2022 21:39:52 +1000 Subject: [PATCH 3/4] Add extra test cases from #8771, #8757, #9076. --- .../trait_duplication_in_bounds_unfixable.rs | 67 ++++++++++++++++++- 1 file changed, 66 insertions(+), 1 deletion(-) diff --git a/tests/ui/trait_duplication_in_bounds_unfixable.rs b/tests/ui/trait_duplication_in_bounds_unfixable.rs index a21d4c5d637d..5630a0345adb 100644 --- a/tests/ui/trait_duplication_in_bounds_unfixable.rs +++ b/tests/ui/trait_duplication_in_bounds_unfixable.rs @@ -95,7 +95,72 @@ trait FooIter: Iterator { } } -// This should not lint +// The below should not lint and exist to guard against false positives fn impl_trait(_: impl AsRef, _: impl AsRef) {} +pub mod one { + #[derive(Clone, Debug)] + struct MultiProductIter + where + I: Iterator + Clone, + I::Item: Clone, + { + _marker: I, + } + + pub struct MultiProduct(Vec>) + where + I: Iterator + Clone, + I::Item: Clone; + + pub fn multi_cartesian_product(_: H) -> MultiProduct<::IntoIter> + where + H: Iterator, + H::Item: IntoIterator, + ::IntoIter: Clone, + ::Item: Clone, + { + todo!() + } +} + +pub mod two { + use std::iter::Peekable; + + pub struct MergeBy + where + I: Iterator, + J: Iterator, + { + _i: Peekable, + _j: Peekable, + _f: F, + } + + impl Clone for MergeBy + where + I: Iterator, + J: Iterator, + std::iter::Peekable: Clone, + std::iter::Peekable: Clone, + F: Clone, + { + fn clone(&self) -> Self { + Self { + _i: self._i.clone(), + _j: self._j.clone(), + _f: self._f.clone(), + } + } + } +} + +pub trait Trait {} + +pub fn f(_a: impl Trait, _b: impl Trait) {} + +pub trait ImplTrait {} + +impl ImplTrait<(A, B)> for Foo where Foo: ImplTrait + ImplTrait {} + fn main() {} From 8bae517c2d540dcf62bff12ba7404a23a4233d99 Mon Sep 17 00:00:00 2001 From: Allen Hsu Date: Fri, 12 Aug 2022 12:51:58 +1000 Subject: [PATCH 4/4] Lint trait duplication in one pass. --- clippy_lints/src/trait_bounds.rs | 55 +++++++------------ ...ait_duplication_in_bounds_unfixable.stderr | 8 +-- 2 files changed, 23 insertions(+), 40 deletions(-) diff --git a/clippy_lints/src/trait_bounds.rs b/clippy_lints/src/trait_bounds.rs index 537e6eafb175..0434720f79b5 100644 --- a/clippy_lints/src/trait_bounds.rs +++ b/clippy_lints/src/trait_bounds.rs @@ -103,7 +103,6 @@ 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>) { @@ -234,7 +233,7 @@ impl TraitBounds { } fn check_trait_bound_duplication(cx: &LateContext<'_>, gen: &'_ Generics<'_>) { - if gen.span.from_expansion() || gen.params.is_empty() || gen.predicates.is_empty() { + if gen.span.from_expansion() { return; } @@ -254,9 +253,9 @@ fn check_trait_bound_duplication(cx: &LateContext<'_>, gen: &'_ Generics<'_>) { if let WherePredicate::BoundPredicate(bound_predicate) = pred; if let TyKind::Path(QPath::Resolved(_, path)) = bound_predicate.bounded_ty.kind; then { - return Some(bound_predicate.bounds.iter().filter_map(|t| { - Some((path.res, into_comparable_trait_ref(t.trait_ref()?))) - })) + return Some( + rollup_traits(cx, bound_predicate.bounds, "these where clauses contain repeated elements") + .into_keys().map(|trait_ref| (path.res, trait_ref))) } } None @@ -277,19 +276,18 @@ fn check_trait_bound_duplication(cx: &LateContext<'_>, gen: &'_ Generics<'_>) { if !bound_predicate.span.from_expansion(); if let TyKind::Path(QPath::Resolved(_, path)) = bound_predicate.bounded_ty.kind; then { - for t in bound_predicate.bounds { - if let Some(trait_ref) = t.trait_ref() { - let key = (path.res, into_comparable_trait_ref(trait_ref)); - if where_predicates.contains(&key) { - span_lint_and_help( - cx, - TRAIT_DUPLICATION_IN_BOUNDS, - t.span(), - "this trait bound is already specified in the where clause", - None, - "consider removing this trait bound", - ); - } + let traits = rollup_traits(cx, bound_predicate.bounds, "these bounds contain repeated elements"); + for (trait_ref, span) in traits { + let key = (path.res, trait_ref); + if where_predicates.contains(&key) { + span_lint_and_help( + cx, + TRAIT_DUPLICATION_IN_BOUNDS, + span, + "this trait bound is already specified in the where clause", + None, + "consider removing this trait bound", + ); } } } @@ -300,23 +298,6 @@ 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; @@ -358,7 +339,7 @@ fn into_comparable_trait_ref(trait_ref: &TraitRef<'_>) -> ComparableTraitRef { ) } -fn rollup_traits(cx: &LateContext<'_>, bounds: &[GenericBound<'_>], msg: &str) { +fn rollup_traits(cx: &LateContext<'_>, bounds: &[GenericBound<'_>], msg: &str) -> FxHashMap { let mut map = FxHashMap::default(); let mut repeated_res = false; @@ -400,4 +381,6 @@ fn rollup_traits(cx: &LateContext<'_>, bounds: &[GenericBound<'_>], msg: &str) { ); } } + + map } diff --git a/tests/ui/trait_duplication_in_bounds_unfixable.stderr b/tests/ui/trait_duplication_in_bounds_unfixable.stderr index fbd9abb005f1..aa44114eb6c5 100644 --- a/tests/ui/trait_duplication_in_bounds_unfixable.stderr +++ b/tests/ui/trait_duplication_in_bounds_unfixable.stderr @@ -1,8 +1,8 @@ error: this trait bound is already specified in the where clause - --> $DIR/trait_duplication_in_bounds_unfixable.rs:6:15 + --> $DIR/trait_duplication_in_bounds_unfixable.rs:6:23 | LL | fn bad_foo(arg0: T, arg1: Z) - | ^^^^^ + | ^^^^^^^ | note: the lint level is defined here --> $DIR/trait_duplication_in_bounds_unfixable.rs:1:9 @@ -12,10 +12,10 @@ 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_unfixable.rs:6:23 + --> $DIR/trait_duplication_in_bounds_unfixable.rs:6:15 | LL | fn bad_foo(arg0: T, arg1: Z) - | ^^^^^^^ + | ^^^^^ | = help: consider removing this trait bound