From 478555d468d54105a0202220b7632b08a992b9be Mon Sep 17 00:00:00 2001 From: John Kelly Date: Sun, 30 Apr 2023 13:16:04 +0100 Subject: [PATCH 1/9] wip --- clippy_lints/src/trait_bounds.rs | 45 +++++++++++++++++++++++-- tests/ui/trait_duplication_in_bounds.rs | 8 +++++ 2 files changed, 50 insertions(+), 3 deletions(-) diff --git a/clippy_lints/src/trait_bounds.rs b/clippy_lints/src/trait_bounds.rs index b5f11b4acae0..13651c978e9d 100644 --- a/clippy_lints/src/trait_bounds.rs +++ b/clippy_lints/src/trait_bounds.rs @@ -9,7 +9,7 @@ use rustc_data_structures::unhash::UnhashMap; use rustc_errors::Applicability; use rustc_hir::def::Res; use rustc_hir::{ - GenericArg, GenericBound, Generics, Item, ItemKind, Node, Path, PathSegment, PredicateOrigin, QPath, + GenericArg, GenericBound, Generics, Item, ItemKind, MutTy, Node, Path, PathSegment, PredicateOrigin, QPath, TraitBoundModifier, TraitItem, TraitRef, Ty, TyKind, WherePredicate, }; use rustc_lint::{LateContext, LateLintPass}; @@ -37,12 +37,12 @@ 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 unnecessarily in trait bounds, use `+` instead of using `T: _, T: _`" } declare_clippy_lint! { /// ### What it does - /// Checks for cases where generics are being used and multiple + /// Checks for cases where generics or trait objects are being used and multiple /// syntax specifications for trait bounds are used simultaneously. /// /// ### Why is this bad? @@ -167,6 +167,45 @@ impl<'tcx> LateLintPass<'tcx> for TraitBounds { } } } + + fn check_ty(&mut self, cx: &LateContext<'tcx>, ty: &'tcx Ty<'tcx>) { + let TyKind::Ref( + .., + MutTy { + ty: Ty { + kind: TyKind::TraitObject( + bounds, + .. + ), + .. + }, + .. + } + ) = ty.kind else { return; }; + + if bounds.len() < 2 { + return; + } + + let mut seen_def_ids = FxHashSet::default(); + + for bound in bounds.iter() { + let Some(def_id) = bound.trait_ref.trait_def_id() else { continue; }; + + let already_seen = !seen_def_ids.insert(def_id); + + if already_seen { + span_lint_and_help( + cx, + TRAIT_DUPLICATION_IN_BOUNDS, + bound.span, + "this trait bound is already specified in trait declaration", + None, + "consider removing this trait bound", + ); + } + } + } } impl TraitBounds { diff --git a/tests/ui/trait_duplication_in_bounds.rs b/tests/ui/trait_duplication_in_bounds.rs index a7a1caf2880d..f4f4a9fcb088 100644 --- a/tests/ui/trait_duplication_in_bounds.rs +++ b/tests/ui/trait_duplication_in_bounds.rs @@ -109,4 +109,12 @@ fn qualified_path(arg0: T) { unimplemented!(); } +fn good_trait_object(arg0: &(dyn Any + Send)) { + unimplemented!(); +} + +fn bad_trait_object(arg0: &(dyn Any + Send + Send)) { + unimplemented!(); +} + fn main() {} From 1eff408ca44ae15ffd6592714487f168a5549bcc Mon Sep 17 00:00:00 2001 From: John Kelly Date: Sun, 30 Apr 2023 13:45:45 +0100 Subject: [PATCH 2/9] WIP --- clippy_lints/src/trait_bounds.rs | 5 +++-- tests/ui/trait_duplication_in_bounds.fixed | 10 +++++++++ tests/ui/trait_duplication_in_bounds.rs | 2 ++ tests/ui/trait_duplication_in_bounds.stderr | 25 +++++++++++++-------- 4 files changed, 31 insertions(+), 11 deletions(-) diff --git a/clippy_lints/src/trait_bounds.rs b/clippy_lints/src/trait_bounds.rs index 13651c978e9d..1af2c5942437 100644 --- a/clippy_lints/src/trait_bounds.rs +++ b/clippy_lints/src/trait_bounds.rs @@ -195,13 +195,14 @@ impl<'tcx> LateLintPass<'tcx> for TraitBounds { let already_seen = !seen_def_ids.insert(def_id); if already_seen { - span_lint_and_help( + span_lint_and_sugg( cx, TRAIT_DUPLICATION_IN_BOUNDS, bound.span, "this trait bound is already specified in trait declaration", - None, "consider removing this trait bound", + "".to_string(), + Applicability::MaybeIncorrect, ); } } diff --git a/tests/ui/trait_duplication_in_bounds.fixed b/tests/ui/trait_duplication_in_bounds.fixed index eef8024b131c..fdac0e4cb1e8 100644 --- a/tests/ui/trait_duplication_in_bounds.fixed +++ b/tests/ui/trait_duplication_in_bounds.fixed @@ -2,6 +2,8 @@ #![deny(clippy::trait_duplication_in_bounds)] #![allow(unused)] +use std::any::Any; + fn bad_foo(arg0: T, argo1: U) { unimplemented!(); } @@ -109,4 +111,12 @@ fn qualified_path(arg0: T) { unimplemented!(); } +fn good_trait_object(arg0: &(dyn Any + Send)) { + unimplemented!(); +} + +fn bad_trait_object(arg0: &(dyn Any + Send)) { + unimplemented!(); +} + fn main() {} diff --git a/tests/ui/trait_duplication_in_bounds.rs b/tests/ui/trait_duplication_in_bounds.rs index f4f4a9fcb088..a0300da55558 100644 --- a/tests/ui/trait_duplication_in_bounds.rs +++ b/tests/ui/trait_duplication_in_bounds.rs @@ -2,6 +2,8 @@ #![deny(clippy::trait_duplication_in_bounds)] #![allow(unused)] +use std::any::Any; + fn bad_foo(arg0: T, argo1: U) { unimplemented!(); } diff --git a/tests/ui/trait_duplication_in_bounds.stderr b/tests/ui/trait_duplication_in_bounds.stderr index af800ba78880..a6508eb86f30 100644 --- a/tests/ui/trait_duplication_in_bounds.stderr +++ b/tests/ui/trait_duplication_in_bounds.stderr @@ -1,5 +1,5 @@ error: these bounds contain repeated elements - --> $DIR/trait_duplication_in_bounds.rs:5:15 + --> $DIR/trait_duplication_in_bounds.rs:7:15 | LL | fn bad_foo(arg0: T, argo1: U) { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Clone + Copy` @@ -11,46 +11,53 @@ LL | #![deny(clippy::trait_duplication_in_bounds)] | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: these where clauses contain repeated elements - --> $DIR/trait_duplication_in_bounds.rs:11:8 + --> $DIR/trait_duplication_in_bounds.rs:13:8 | LL | T: Clone + Clone + Clone + Copy, | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Clone + Copy` error: these bounds contain repeated elements - --> $DIR/trait_duplication_in_bounds.rs:39:26 + --> $DIR/trait_duplication_in_bounds.rs:41:26 | LL | trait BadSelfTraitBound: Clone + Clone + Clone { | ^^^^^^^^^^^^^^^^^^^^^ help: try: `Clone` error: these where clauses contain repeated elements - --> $DIR/trait_duplication_in_bounds.rs:46:15 + --> $DIR/trait_duplication_in_bounds.rs:48:15 | LL | Self: Clone + Clone + Clone; | ^^^^^^^^^^^^^^^^^^^^^ help: try: `Clone` error: these bounds contain repeated elements - --> $DIR/trait_duplication_in_bounds.rs:60:24 + --> $DIR/trait_duplication_in_bounds.rs:62:24 | LL | trait BadTraitBound { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Clone + Copy` error: these where clauses contain repeated elements - --> $DIR/trait_duplication_in_bounds.rs:67:12 + --> $DIR/trait_duplication_in_bounds.rs:69:12 | LL | T: Clone + Clone + Clone + Copy, | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Clone + Copy` error: these bounds contain repeated elements - --> $DIR/trait_duplication_in_bounds.rs:100:19 + --> $DIR/trait_duplication_in_bounds.rs:102:19 | LL | fn bad_generic + GenericTrait + GenericTrait>(arg0: T) { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `GenericTrait + GenericTrait` error: these bounds contain repeated elements - --> $DIR/trait_duplication_in_bounds.rs:108:22 + --> $DIR/trait_duplication_in_bounds.rs:110:22 | LL | fn qualified_path(arg0: T) { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `std::clone::Clone + foo::Clone` -error: aborting due to 8 previous errors +error: this trait bound is already specified in trait declaration + --> $DIR/trait_duplication_in_bounds.rs:118:46 + | +LL | fn bad_trait_object(arg0: &(dyn Any + Send + Send)) { + | ^^^^ help: consider removing this trait bound + | + +error: aborting due to 9 previous errors From 0fb3f3b25667c2e9c56a7fd83019cafec7ada1cf Mon Sep 17 00:00:00 2001 From: John Kelly Date: Sun, 30 Apr 2023 14:10:26 +0100 Subject: [PATCH 3/9] WIP --- clippy_lints/src/trait_bounds.rs | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/clippy_lints/src/trait_bounds.rs b/clippy_lints/src/trait_bounds.rs index 1af2c5942437..4c70bae75290 100644 --- a/clippy_lints/src/trait_bounds.rs +++ b/clippy_lints/src/trait_bounds.rs @@ -177,6 +177,7 @@ impl<'tcx> LateLintPass<'tcx> for TraitBounds { bounds, .. ), + span, .. }, .. @@ -189,6 +190,12 @@ impl<'tcx> LateLintPass<'tcx> for TraitBounds { let mut seen_def_ids = FxHashSet::default(); + let traits = bounds + .iter() + .filter_map(|b| snippet_opt(cx, b.span)) + .collect::>(); + let traits = traits.join(" + "); + for bound in bounds.iter() { let Some(def_id) = bound.trait_ref.trait_def_id() else { continue; }; @@ -198,10 +205,10 @@ impl<'tcx> LateLintPass<'tcx> for TraitBounds { span_lint_and_sugg( cx, TRAIT_DUPLICATION_IN_BOUNDS, - bound.span, + *span, "this trait bound is already specified in trait declaration", "consider removing this trait bound", - "".to_string(), + traits.clone(), Applicability::MaybeIncorrect, ); } From 8db21e9a9c63496271b5db11af665ada036fac4d Mon Sep 17 00:00:00 2001 From: John Kelly Date: Sun, 30 Apr 2023 14:14:47 +0100 Subject: [PATCH 4/9] WIP --- clippy_lints/src/trait_bounds.rs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/clippy_lints/src/trait_bounds.rs b/clippy_lints/src/trait_bounds.rs index 4c70bae75290..2c1c63e808bc 100644 --- a/clippy_lints/src/trait_bounds.rs +++ b/clippy_lints/src/trait_bounds.rs @@ -177,7 +177,6 @@ impl<'tcx> LateLintPass<'tcx> for TraitBounds { bounds, .. ), - span, .. }, .. @@ -188,6 +187,12 @@ impl<'tcx> LateLintPass<'tcx> for TraitBounds { return; } + let mut bounds_span = Span::default(); + + for bound in bounds.iter() { + bounds_span = bounds_span.to(bound.span); + } + let mut seen_def_ids = FxHashSet::default(); let traits = bounds @@ -205,7 +210,7 @@ impl<'tcx> LateLintPass<'tcx> for TraitBounds { span_lint_and_sugg( cx, TRAIT_DUPLICATION_IN_BOUNDS, - *span, + bounds_span, "this trait bound is already specified in trait declaration", "consider removing this trait bound", traits.clone(), From b9788fef298815c2b8366a42ef0c8113170d11ed Mon Sep 17 00:00:00 2001 From: John Kelly Date: Sun, 30 Apr 2023 14:34:46 +0100 Subject: [PATCH 5/9] Working --- clippy_lints/src/trait_bounds.rs | 45 ++++++++++++--------- tests/ui/trait_duplication_in_bounds.stderr | 5 +-- 2 files changed, 27 insertions(+), 23 deletions(-) diff --git a/clippy_lints/src/trait_bounds.rs b/clippy_lints/src/trait_bounds.rs index 2c1c63e808bc..8de0f92109b3 100644 --- a/clippy_lints/src/trait_bounds.rs +++ b/clippy_lints/src/trait_bounds.rs @@ -187,37 +187,42 @@ impl<'tcx> LateLintPass<'tcx> for TraitBounds { return; } - let mut bounds_span = Span::default(); + let mut bounds_span = bounds[0].span; - for bound in bounds.iter() { + for bound in bounds.iter().skip(1) { bounds_span = bounds_span.to(bound.span); } let mut seen_def_ids = FxHashSet::default(); - - let traits = bounds - .iter() - .filter_map(|b| snippet_opt(cx, b.span)) - .collect::>(); - let traits = traits.join(" + "); + let mut fixed_traits = Vec::new(); for bound in bounds.iter() { let Some(def_id) = bound.trait_ref.trait_def_id() else { continue; }; - let already_seen = !seen_def_ids.insert(def_id); - - if already_seen { - span_lint_and_sugg( - cx, - TRAIT_DUPLICATION_IN_BOUNDS, - bounds_span, - "this trait bound is already specified in trait declaration", - "consider removing this trait bound", - traits.clone(), - Applicability::MaybeIncorrect, - ); + let new_trait = seen_def_ids.insert(def_id); + + if new_trait { + fixed_traits.push(bound); } } + + let fixed_trait_snippet = fixed_traits + .iter() + .filter_map(|b| snippet_opt(cx, b.span)) + .collect::>() + .join(" + "); + + if bounds.len() != fixed_traits.len() { + span_lint_and_sugg( + cx, + TRAIT_DUPLICATION_IN_BOUNDS, + bounds_span, + "this trait bound is already specified in trait declaration", + "try", + fixed_trait_snippet, + Applicability::MaybeIncorrect, + ); + } } } diff --git a/tests/ui/trait_duplication_in_bounds.stderr b/tests/ui/trait_duplication_in_bounds.stderr index a6508eb86f30..539b6114ca3a 100644 --- a/tests/ui/trait_duplication_in_bounds.stderr +++ b/tests/ui/trait_duplication_in_bounds.stderr @@ -53,11 +53,10 @@ LL | fn qualified_path(arg0: T) { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `std::clone::Clone + foo::Clone` error: this trait bound is already specified in trait declaration - --> $DIR/trait_duplication_in_bounds.rs:118:46 + --> $DIR/trait_duplication_in_bounds.rs:118:33 | LL | fn bad_trait_object(arg0: &(dyn Any + Send + Send)) { - | ^^^^ help: consider removing this trait bound - | + | ^^^^^^^^^^^^^^^^^ help: try: `Any + Send` error: aborting due to 9 previous errors From cbd0135bd2d2e546ca4c2c2147798d1f7c547128 Mon Sep 17 00:00:00 2001 From: John Kelly Date: Tue, 2 May 2023 18:15:02 +0100 Subject: [PATCH 6/9] Update trait_bounds.rs --- clippy_lints/src/trait_bounds.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/clippy_lints/src/trait_bounds.rs b/clippy_lints/src/trait_bounds.rs index 8de0f92109b3..ac61fb157b48 100644 --- a/clippy_lints/src/trait_bounds.rs +++ b/clippy_lints/src/trait_bounds.rs @@ -206,13 +206,13 @@ impl<'tcx> LateLintPass<'tcx> for TraitBounds { } } - let fixed_trait_snippet = fixed_traits - .iter() - .filter_map(|b| snippet_opt(cx, b.span)) - .collect::>() - .join(" + "); - if bounds.len() != fixed_traits.len() { + let fixed_trait_snippet = fixed_traits + .iter() + .filter_map(|b| snippet_opt(cx, b.span)) + .collect::>() + .join(" + "); + span_lint_and_sugg( cx, TRAIT_DUPLICATION_IN_BOUNDS, From 122793b4947c2691efcb2ab6450c251a25311aaf Mon Sep 17 00:00:00 2001 From: John Kelly Date: Tue, 2 May 2023 18:21:23 +0100 Subject: [PATCH 7/9] Update trait_bounds.rs --- clippy_lints/src/trait_bounds.rs | 81 ++++++++++++++------------------ 1 file changed, 35 insertions(+), 46 deletions(-) diff --git a/clippy_lints/src/trait_bounds.rs b/clippy_lints/src/trait_bounds.rs index ac61fb157b48..5acd44dccaf5 100644 --- a/clippy_lints/src/trait_bounds.rs +++ b/clippy_lints/src/trait_bounds.rs @@ -9,7 +9,7 @@ use rustc_data_structures::unhash::UnhashMap; use rustc_errors::Applicability; use rustc_hir::def::Res; use rustc_hir::{ - GenericArg, GenericBound, Generics, Item, ItemKind, MutTy, Node, Path, PathSegment, PredicateOrigin, QPath, + GenericArg, GenericBound, Generics, Item, ItemKind, Node, Path, PathSegment, PredicateOrigin, QPath, TraitBoundModifier, TraitItem, TraitRef, Ty, TyKind, WherePredicate, }; use rustc_lint::{LateContext, LateLintPass}; @@ -169,60 +169,49 @@ impl<'tcx> LateLintPass<'tcx> for TraitBounds { } fn check_ty(&mut self, cx: &LateContext<'tcx>, ty: &'tcx Ty<'tcx>) { - let TyKind::Ref( - .., - MutTy { - ty: Ty { - kind: TyKind::TraitObject( - bounds, - .. - ), - .. - }, - .. - } - ) = ty.kind else { return; }; + if_chain! { + if let TyKind::Ref(.., mut_ty) = &ty.kind; + if let TyKind::TraitObject(bounds, ..) = mut_ty.ty.kind; + if bounds.len() > 2; + then { + let mut bounds_span = bounds[0].span; - if bounds.len() < 2 { - return; - } + for bound in bounds.iter().skip(1) { + bounds_span = bounds_span.to(bound.span); + } - let mut bounds_span = bounds[0].span; + let mut seen_def_ids = FxHashSet::default(); + let mut fixed_traits = Vec::new(); - for bound in bounds.iter().skip(1) { - bounds_span = bounds_span.to(bound.span); - } + for bound in bounds.iter() { + let Some(def_id) = bound.trait_ref.trait_def_id() else { continue; }; - let mut seen_def_ids = FxHashSet::default(); - let mut fixed_traits = Vec::new(); + let new_trait = seen_def_ids.insert(def_id); - for bound in bounds.iter() { - let Some(def_id) = bound.trait_ref.trait_def_id() else { continue; }; + if new_trait { + fixed_traits.push(bound); + } + } - let new_trait = seen_def_ids.insert(def_id); + if bounds.len() != fixed_traits.len() { + let fixed_trait_snippet = fixed_traits + .iter() + .filter_map(|b| snippet_opt(cx, b.span)) + .collect::>() + .join(" + "); - if new_trait { - fixed_traits.push(bound); + span_lint_and_sugg( + cx, + TRAIT_DUPLICATION_IN_BOUNDS, + bounds_span, + "this trait bound is already specified in trait declaration", + "try", + fixed_trait_snippet, + Applicability::MaybeIncorrect, + ); + } } } - - if bounds.len() != fixed_traits.len() { - let fixed_trait_snippet = fixed_traits - .iter() - .filter_map(|b| snippet_opt(cx, b.span)) - .collect::>() - .join(" + "); - - span_lint_and_sugg( - cx, - TRAIT_DUPLICATION_IN_BOUNDS, - bounds_span, - "this trait bound is already specified in trait declaration", - "try", - fixed_trait_snippet, - Applicability::MaybeIncorrect, - ); - } } } From 5c8a00923bc435cec37820c32768871b9e8a9ca1 Mon Sep 17 00:00:00 2001 From: John Kelly Date: Sun, 7 May 2023 10:10:44 +0100 Subject: [PATCH 8/9] Comments --- clippy_lints/src/trait_bounds.rs | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/clippy_lints/src/trait_bounds.rs b/clippy_lints/src/trait_bounds.rs index 5acd44dccaf5..c4e4410e9fd7 100644 --- a/clippy_lints/src/trait_bounds.rs +++ b/clippy_lints/src/trait_bounds.rs @@ -174,15 +174,11 @@ impl<'tcx> LateLintPass<'tcx> for TraitBounds { if let TyKind::TraitObject(bounds, ..) = mut_ty.ty.kind; if bounds.len() > 2; then { - let mut bounds_span = bounds[0].span; - - for bound in bounds.iter().skip(1) { - bounds_span = bounds_span.to(bound.span); - } - let mut seen_def_ids = FxHashSet::default(); let mut fixed_traits = Vec::new(); + // Iterate the bounds and add them to our seen hash + // If we haven't yet seen it, add it to the fixed traits for bound in bounds.iter() { let Some(def_id) = bound.trait_ref.trait_def_id() else { continue; }; @@ -193,7 +189,15 @@ impl<'tcx> LateLintPass<'tcx> for TraitBounds { } } + // If the number added to fixed (which are not duplicates) isn't the same as the number found, + // there must be 1 or more duplicates if bounds.len() != fixed_traits.len() { + let mut bounds_span = bounds[0].span; + + for bound in bounds.iter().skip(1) { + bounds_span = bounds_span.to(bound.span); + } + let fixed_trait_snippet = fixed_traits .iter() .filter_map(|b| snippet_opt(cx, b.span)) From b169bdb732c1a0751a32c4ede1c16a3bba3ffb09 Mon Sep 17 00:00:00 2001 From: John Kelly Date: Tue, 9 May 2023 10:06:38 +0100 Subject: [PATCH 9/9] Comments --- clippy_lints/src/trait_bounds.rs | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/clippy_lints/src/trait_bounds.rs b/clippy_lints/src/trait_bounds.rs index c4e4410e9fd7..4ccda15068bb 100644 --- a/clippy_lints/src/trait_bounds.rs +++ b/clippy_lints/src/trait_bounds.rs @@ -174,8 +174,13 @@ impl<'tcx> LateLintPass<'tcx> for TraitBounds { if let TyKind::TraitObject(bounds, ..) = mut_ty.ty.kind; if bounds.len() > 2; then { + + // Build up a hash of every trait we've seen + // When we see a trait for the first time, add it to unique_traits + // so we can later use it to build a string of all traits exactly once, without duplicates + let mut seen_def_ids = FxHashSet::default(); - let mut fixed_traits = Vec::new(); + let mut unique_traits = Vec::new(); // Iterate the bounds and add them to our seen hash // If we haven't yet seen it, add it to the fixed traits @@ -185,20 +190,20 @@ impl<'tcx> LateLintPass<'tcx> for TraitBounds { let new_trait = seen_def_ids.insert(def_id); if new_trait { - fixed_traits.push(bound); + unique_traits.push(bound); } } - // If the number added to fixed (which are not duplicates) isn't the same as the number found, + // If the number of unique traits isn't the same as the number of traits in the bounds, // there must be 1 or more duplicates - if bounds.len() != fixed_traits.len() { + if bounds.len() != unique_traits.len() { let mut bounds_span = bounds[0].span; for bound in bounds.iter().skip(1) { bounds_span = bounds_span.to(bound.span); } - let fixed_trait_snippet = fixed_traits + let fixed_trait_snippet = unique_traits .iter() .filter_map(|b| snippet_opt(cx, b.span)) .collect::>()