Skip to content

Commit

Permalink
Auto merge of rust-lang#9397 - Jarcho:trait_dup_order, r=dswij
Browse files Browse the repository at this point in the history
Fix the emission order of `trait_duplication_in_bounds`

Makes the lint emit in source order rather than whatever order the hash map happens to be in. This is currently blocking the sync into rustc.

changelog: None
  • Loading branch information
bors committed Aug 31, 2022
2 parents 09e4659 + 19ef04f commit f51aade
Show file tree
Hide file tree
Showing 4 changed files with 33 additions and 17 deletions.
34 changes: 25 additions & 9 deletions clippy_lints/src/trait_bounds.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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<Res>);
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 {
Expand Down Expand Up @@ -339,7 +345,7 @@ fn into_comparable_trait_ref(trait_ref: &TraitRef<'_>) -> ComparableTraitRef {
)
}

fn rollup_traits(cx: &LateContext<'_>, bounds: &[GenericBound<'_>], msg: &str) -> FxHashMap<ComparableTraitRef, Span> {
fn rollup_traits(cx: &LateContext<'_>, bounds: &[GenericBound<'_>], msg: &str) -> Vec<(ComparableTraitRef, Span)> {
let mut map = FxHashMap::default();
let mut repeated_res = false;

Expand All @@ -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::<Vec<_>>();
traits.sort_unstable();
let traits = traits.join(" + ");

span_lint_and_sugg(
Expand All @@ -382,5 +398,5 @@ fn rollup_traits(cx: &LateContext<'_>, bounds: &[GenericBound<'_>], msg: &str) -
}
}

map
comparable_bounds
}
4 changes: 2 additions & 2 deletions tests/ui/trait_duplication_in_bounds.fixed
Original file line number Diff line number Diff line change
Expand Up @@ -97,15 +97,15 @@ fn good_generic<T: GenericTrait<u64> + GenericTrait<u32>>(arg0: T) {
unimplemented!();
}

fn bad_generic<T: GenericTrait<u32> + GenericTrait<u64>>(arg0: T) {
fn bad_generic<T: GenericTrait<u64> + GenericTrait<u32>>(arg0: T) {
unimplemented!();
}

mod foo {
pub trait Clone {}
}

fn qualified_path<T: Clone + foo::Clone>(arg0: T) {
fn qualified_path<T: std::clone::Clone + foo::Clone>(arg0: T) {
unimplemented!();
}

Expand Down
4 changes: 2 additions & 2 deletions tests/ui/trait_duplication_in_bounds.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -44,13 +44,13 @@ error: these bounds contain repeated elements
--> $DIR/trait_duplication_in_bounds.rs:100:19
|
LL | fn bad_generic<T: GenericTrait<u64> + GenericTrait<u32> + GenericTrait<u64>>(arg0: T) {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `GenericTrait<u32> + GenericTrait<u64>`
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `GenericTrait<u64> + GenericTrait<u32>`

error: these bounds contain repeated elements
--> $DIR/trait_duplication_in_bounds.rs:108:22
|
LL | fn qualified_path<T: std::clone::Clone + Clone + foo::Clone>(arg0: T) {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Clone + foo::Clone`
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `std::clone::Clone + foo::Clone`

error: aborting due to 8 previous errors

8 changes: 4 additions & 4 deletions tests/ui/trait_duplication_in_bounds_unfixable.stderr
Original file line number Diff line number Diff line change
@@ -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<T: Clone + Default, Z: Copy>(arg0: T, arg1: Z)
| ^^^^^^^
| ^^^^^
|
note: the lint level is defined here
--> $DIR/trait_duplication_in_bounds_unfixable.rs:1:9
Expand All @@ -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<T: Clone + Default, Z: Copy>(arg0: T, arg1: Z)
| ^^^^^
| ^^^^^^^
|
= help: consider removing this trait bound

Expand Down

0 comments on commit f51aade

Please sign in to comment.