Skip to content

Commit

Permalink
Lint for repeated traits within trait bounds or where clauses.
Browse files Browse the repository at this point in the history
  • Loading branch information
Allen Hsu authored and aldhsu committed Jul 12, 2022
1 parent 526f02e commit 8878d67
Show file tree
Hide file tree
Showing 4 changed files with 340 additions and 15 deletions.
129 changes: 123 additions & 6 deletions clippy_lints/src/trait_bounds.rs
Original file line number Diff line number Diff line change
@@ -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;
Expand All @@ -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};
Expand All @@ -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! {
Expand All @@ -63,10 +63,26 @@ declare_clippy_lint! {
///
/// fn func<T>(arg: T) where T: Clone + Default {}
/// ```
///
/// ```rust
/// fn foo<T: Default + Default>(bar: T) {}
/// ```
/// Use instead:
/// ```rust
/// fn foo<T: Default>(bar: T) {}
/// ```
///
/// ```rust
/// fn foo<T>(bar: T) where T: Default + Default {}
/// ```
/// Use instead:
/// ```rust
/// fn foo<T>(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)]
Expand All @@ -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>) {
Expand Down Expand Up @@ -241,6 +270,26 @@ fn check_trait_bound_duplication(cx: &LateContext<'_>, gen: &'_ Generics<'_>) {
}
}

#[derive(PartialEq, Eq, Hash, Debug)]
struct ComparableTraitRef(Res, Vec<Res>);

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

span_lint_and_sugg(
cx,
TRAIT_DUPLICATION_IN_BOUNDS,
all_trait_span,
msg,
"try",
traits,
Applicability::MachineApplicable
);
}
}
}
1 change: 1 addition & 0 deletions tests/compile-test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
111 changes: 111 additions & 0 deletions tests/ui/trait_duplication_in_bounds.rs
Original file line number Diff line number Diff line change
@@ -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};
Expand Down Expand Up @@ -98,4 +99,114 @@ trait FooIter: Iterator<Item = Foo> {
// This should not lint
fn impl_trait(_: impl AsRef<str>, _: impl AsRef<str>) {}

mod repeated_where_clauses_or_trait_bounds {
fn bad_foo<T: Clone + Clone + Clone + Copy, U: Clone + Copy>(arg0: T, argo1: U) {
unimplemented!();
}

fn bad_bar<T, U>(arg0: T, arg1: U)
where
T: Clone + Clone + Clone + Copy,
U: Clone + Copy,
{
unimplemented!();
}

fn good_bar<T: Clone + Copy, U: Clone + Copy>(arg0: T, arg1: U) {
unimplemented!();
}

fn good_foo<T, U>(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<T: Clone + Copy, U: Clone + Copy> {
fn f();
}

trait GoodWhereClause<T, U> {
fn f()
where
T: Clone + Copy,
U: Clone + Copy;
}

trait BadTraitBound<T: Clone + Clone + Clone + Copy, U: Clone + Copy> {
fn f();
}

trait BadWhereClause<T, U> {
fn f()
where
T: Clone + Clone + Clone + Copy,
U: Clone + Copy;
}

struct GoodStructBound<T: Clone + Copy, U: Clone + Copy> {
t: T,
u: U,
}

impl<T: Clone + Copy, U: Clone + Copy> GoodTraitBound<T, U> for GoodStructBound<T, U> {
// this should not warn
fn f() {}
}

struct GoodStructWhereClause;

impl<T, U> GoodTraitBound<T, U> 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<T> {}

// This should not warn but currently does see #8757
fn good_generic<T: GenericTrait<u64> + GenericTrait<u32>>(arg0: T) {
unimplemented!();
}

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

mod foo {
pub trait Clone {}
}

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

fn main() {}
Loading

0 comments on commit 8878d67

Please sign in to comment.