From 19ef04ff5df281f248ace29741b1054abf224752 Mon Sep 17 00:00:00 2001 From: Jason Newcomb Date: Tue, 30 Aug 2022 00:33:56 -0400 Subject: [PATCH] Fix the order of `trait_duplication_in_bounds` * Emit the lint in source order * Make suggestions with multiple traits be in source order rather than alphabetical --- clippy_lints/src/trait_bounds.rs | 34 ++++++++++++++----- tests/ui/trait_duplication_in_bounds.fixed | 4 +-- tests/ui/trait_duplication_in_bounds.stderr | 4 +-- ...ait_duplication_in_bounds_unfixable.stderr | 8 ++--- 4 files changed, 33 insertions(+), 17 deletions(-) diff --git a/clippy_lints/src/trait_bounds.rs b/clippy_lints/src/trait_bounds.rs index 0434720f79b57..2ffa022b04f7a 100644 --- a/clippy_lints/src/trait_bounds.rs +++ b/clippy_lints/src/trait_bounds.rs @@ -15,6 +15,7 @@ use rustc_hir::{ use rustc_lint::{LateContext, LateLintPass}; use rustc_session::{declare_tool_lint, impl_lint_pass}; use rustc_span::{BytePos, Span}; +use std::collections::hash_map::Entry; declare_clippy_lint! { /// ### What it does @@ -255,7 +256,7 @@ fn check_trait_bound_duplication(cx: &LateContext<'_>, gen: &'_ Generics<'_>) { then { return Some( rollup_traits(cx, bound_predicate.bounds, "these where clauses contain repeated elements") - .into_keys().map(|trait_ref| (path.res, trait_ref))) + .into_iter().map(|(trait_ref, _)| (path.res, trait_ref))) } } None @@ -295,8 +296,13 @@ fn check_trait_bound_duplication(cx: &LateContext<'_>, gen: &'_ Generics<'_>) { } } -#[derive(PartialEq, Eq, Hash, Debug)] +#[derive(Clone, PartialEq, Eq, Hash, Debug)] struct ComparableTraitRef(Res, Vec); +impl Default for ComparableTraitRef { + fn default() -> Self { + Self(Res::Err, Vec::new()) + } +} fn get_trait_info_from_bound<'a>(bound: &'a GenericBound<'_>) -> Option<(Res, &'a [PathSegment<'a>], Span)> { if let GenericBound::Trait(t, tbm) = bound { @@ -339,7 +345,7 @@ fn into_comparable_trait_ref(trait_ref: &TraitRef<'_>) -> ComparableTraitRef { ) } -fn rollup_traits(cx: &LateContext<'_>, bounds: &[GenericBound<'_>], msg: &str) -> FxHashMap { +fn rollup_traits(cx: &LateContext<'_>, bounds: &[GenericBound<'_>], msg: &str) -> Vec<(ComparableTraitRef, Span)> { let mut map = FxHashMap::default(); let mut repeated_res = false; @@ -351,23 +357,33 @@ fn rollup_traits(cx: &LateContext<'_>, bounds: &[GenericBound<'_>], msg: &str) - } }; + let mut i = 0usize; 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; + match map.entry(comparable_bound) { + Entry::Occupied(_) => repeated_res = true, + Entry::Vacant(e) => { + e.insert((span_direct, i)); + i += 1; + }, } } + // Put bounds in source order + let mut comparable_bounds = vec![Default::default(); map.len()]; + for (k, (v, i)) in map { + comparable_bounds[i] = (k, v); + } + 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)) + let traits = comparable_bounds.iter() + .filter_map(|&(_, span)| snippet_opt(cx, span)) .collect::>(); - traits.sort_unstable(); let traits = traits.join(" + "); span_lint_and_sugg( @@ -382,5 +398,5 @@ fn rollup_traits(cx: &LateContext<'_>, bounds: &[GenericBound<'_>], msg: &str) - } } - map + comparable_bounds } diff --git a/tests/ui/trait_duplication_in_bounds.fixed b/tests/ui/trait_duplication_in_bounds.fixed index b4e6bf0ea1c2b..4ce5d42178225 100644 --- a/tests/ui/trait_duplication_in_bounds.fixed +++ b/tests/ui/trait_duplication_in_bounds.fixed @@ -97,7 +97,7 @@ fn good_generic + GenericTrait>(arg0: T) { unimplemented!(); } -fn bad_generic + GenericTrait>(arg0: T) { +fn bad_generic + GenericTrait>(arg0: T) { unimplemented!(); } @@ -105,7 +105,7 @@ mod foo { pub trait Clone {} } -fn qualified_path(arg0: T) { +fn qualified_path(arg0: T) { unimplemented!(); } diff --git a/tests/ui/trait_duplication_in_bounds.stderr b/tests/ui/trait_duplication_in_bounds.stderr index 86c593811a74f..af800ba78880c 100644 --- a/tests/ui/trait_duplication_in_bounds.stderr +++ b/tests/ui/trait_duplication_in_bounds.stderr @@ -44,13 +44,13 @@ error: these bounds contain repeated elements --> $DIR/trait_duplication_in_bounds.rs:100:19 | LL | fn bad_generic + GenericTrait + GenericTrait>(arg0: T) { - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `GenericTrait + GenericTrait` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `GenericTrait + GenericTrait` error: these bounds contain repeated elements --> $DIR/trait_duplication_in_bounds.rs:108:22 | LL | fn qualified_path(arg0: T) { - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Clone + foo::Clone` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `std::clone::Clone + foo::Clone` error: aborting due to 8 previous errors diff --git a/tests/ui/trait_duplication_in_bounds_unfixable.stderr b/tests/ui/trait_duplication_in_bounds_unfixable.stderr index aa44114eb6c5f..fbd9abb005f1f 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:23 + --> $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 @@ -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:15 + --> $DIR/trait_duplication_in_bounds_unfixable.rs:6:23 | LL | fn bad_foo(arg0: T, arg1: Z) - | ^^^^^ + | ^^^^^^^ | = help: consider removing this trait bound