mikros/rust
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Auto merge of #13493 - y21:trait_duplication_in_bounds_fix, r=Manishearth

Browse Source 
Compare trait references in `trait_duplication_in_bounds` correctly

Fixes #13476
Fixes #11067
Fixes #9915
Fixes #9626

Currently, the `trait_duplication_in_bounds` lints has a helper type for a trait reference that can be used for comparison and hashing, represented as `{trait: Res, generic_args: Vec<Res>}`. However, there are a lot of issues with this. For one, a `Res` can't represent e.g. references, slices, or lots of other types, as well as const generics and associated type equality. In those cases, the lint simply ignores them and has no way of checking if they're actually the same.

So, instead of using `Res` for this, use `SpanlessEq` and `SpanlessHash` for comparisons with the trait path for checking if there are duplicates.

However, using `SpanlessEq` as is alone lead to a false negative in the test. `std::clone::Clone` + `foo::Clone` wasn't recognized as a duplicate, because it has different segments. So this also adds a new "mode" to SpanlessEq which compares by final resolution. (I've been wondering if this can't just be the default but it's quite a large scale change as it affects a lot of lints and I haven't yet looked at all uses of it to see if there are lints that really do care about having exactly the same path segments).

Maybe an alternative would be to turn the hir types/consts into middle types/consts and compare them instead but I'm not sure there's really a good way to do that

changelog: none
This commit is contained in:
bors 2024-10-03 21:16:22 +00:00
commit a01975b152
5 changed files with 239 additions and 81 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

107
clippy_lints/src/trait_bounds.rs
View File

@ -5,18 +5,17 @@ use clippy_utils::source::{SpanRangeExt, snippet, snippet_with_applicability};
use clippy_utils::{SpanlessEq, SpanlessHash, is_from_proc_macro};
use core::hash::{Hash, Hasher};
use itertools::Itertools;
use rustc_data_structures::fx::{FxHashMap, FxHashSet};
use rustc_data_structures::fx::{FxHashMap, FxHashSet, FxIndexMap, IndexEntry};
use rustc_data_structures::unhash::UnhashMap;
use rustc_errors::Applicability;
use rustc_hir::def::Res;
use rustc_hir::{
GenericArg, GenericBound, Generics, Item, ItemKind, LangItem, Node, Path, PathSegment, PredicateOrigin, QPath,
GenericBound, Generics, Item, ItemKind, LangItem, Node, Path, PathSegment, PredicateOrigin, QPath,
TraitBoundModifier, TraitItem, TraitRef, Ty, TyKind, WherePredicate,
};
use rustc_lint::{LateContext, LateLintPass};
use rustc_session::impl_lint_pass;
use rustc_span::{BytePos, Span};
use std::collections::hash_map::Entry;
declare_clippy_lint! {
/// ### What it does
@ -153,7 +152,10 @@ impl<'tcx> LateLintPass<'tcx> for TraitBounds {
.filter_map(get_trait_info_from_bound)
.for_each(|(trait_item_res, trait_item_segments, span)| {
if let Some(self_segments) = self_bounds_map.get(&trait_item_res) {
if SpanlessEq::new(cx).eq_path_segments(self_segments, trait_item_segments) {
if SpanlessEq::new(cx)
.paths_by_resolution()
.eq_path_segments(self_segments, trait_item_segments)
{
span_lint_and_help(
cx,
TRAIT_DUPLICATION_IN_BOUNDS,
@ -302,7 +304,7 @@ impl TraitBounds {
}
}
fn check_trait_bound_duplication(cx: &LateContext<'_>, generics: &'_ Generics<'_>) {
fn check_trait_bound_duplication<'tcx>(cx: &LateContext<'tcx>, generics: &'_ Generics<'tcx>) {
if generics.span.from_expansion() {
return;
}
@ -314,6 +316,7 @@ fn check_trait_bound_duplication(cx: &LateContext<'_>, generics: &'_ Generics<'_
// |
// collects each of these where clauses into a set keyed by generic name and comparable trait
// eg. (T, Clone)
#[expect(clippy::mutable_key_type)]
let where_predicates = generics
.predicates
.iter()
@ -367,11 +370,27 @@ fn check_trait_bound_duplication(cx: &LateContext<'_>, generics: &'_ Generics<'_
}
}
#[derive(Clone, PartialEq, Eq, Hash, Debug)]
struct ComparableTraitRef(Res, Vec<Res>);
impl Default for ComparableTraitRef {
fn default() -> Self {
Self(Res::Err, Vec::new())
struct ComparableTraitRef<'a, 'tcx> {
cx: &'a LateContext<'tcx>,
trait_ref: &'tcx TraitRef<'tcx>,
modifier: TraitBoundModifier,
}
impl PartialEq for ComparableTraitRef<'_, '_> {
fn eq(&self, other: &Self) -> bool {
self.modifier == other.modifier
&& SpanlessEq::new(self.cx)
.paths_by_resolution()
.eq_path(self.trait_ref.path, other.trait_ref.path)
}
}
impl Eq for ComparableTraitRef<'_, '_> {}
impl Hash for ComparableTraitRef<'_, '_> {
fn hash<H: Hasher>(&self, state: &mut H) {
let mut s = SpanlessHash::new(self.cx).paths_by_resolution();
s.hash_path(self.trait_ref.path);
state.write_u64(s.finish());
self.modifier.hash(state);
}
}
@ -392,69 +411,41 @@ fn get_trait_info_from_bound<'a>(bound: &'a GenericBound<'_>) -> Option<(Res, &'
}
}
fn get_ty_res(ty: Ty<'_>) -> Option<Res> {
match ty.kind {
TyKind::Path(QPath::Resolved(_, path)) => Some(path.res),
TyKind::Path(QPath::TypeRelative(ty, _)) => get_ty_res(*ty),
_ => 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 let GenericArg::Type(ty) = arg {
return get_ty_res(**ty);
}
None
}))
})
.flatten()
.collect(),
)
}
fn rollup_traits(
cx: &LateContext<'_>,
bounds: &[GenericBound<'_>],
fn rollup_traits<'cx, 'tcx>(
cx: &'cx LateContext<'tcx>,
bounds: &'tcx [GenericBound<'tcx>],
msg: &'static str,
) -> Vec<(ComparableTraitRef, Span)> {
let mut map = FxHashMap::default();
) -> Vec<(ComparableTraitRef<'cx, 'tcx>, Span)> {
// Source order is needed for joining spans
let mut map = FxIndexMap::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))
let only_comparable_trait_refs = |bound: &'tcx GenericBound<'tcx>| {
if let GenericBound::Trait(t, modifier) = bound {
Some((
ComparableTraitRef {
cx,
trait_ref: &t.trait_ref,
modifier: *modifier,
},
t.span,
))
} else {
None
}
};
let mut i = 0usize;
for bound in bounds.iter().filter_map(only_comparable_trait_refs) {
let (comparable_bound, span_direct) = bound;
match map.entry(comparable_bound) {
Entry::Occupied(_) => repeated_res = true,
Entry::Vacant(e) => {
e.insert((span_direct, i));
i += 1;
IndexEntry::Occupied(_) => repeated_res = true,
IndexEntry::Vacant(e) => {
e.insert(span_direct);
},
}
}
// 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);
}
let comparable_bounds: Vec<_> = map.into_iter().collect();
if repeated_res && let [first_trait, .., last_trait] = bounds {
let all_trait_span = first_trait.span().to(last_trait.span());

119
clippy_utils/src/hir_utils.rs
View File

@ -5,7 +5,7 @@ use crate::tokenize_with_text;
use rustc_ast::ast::InlineAsmTemplatePiece;
use rustc_data_structures::fx::FxHasher;
use rustc_hir::MatchSource::TryDesugar;
use rustc_hir::def::Res;
use rustc_hir::def::{DefKind, Res};
use rustc_hir::{
ArrayLen, AssocItemConstraint, BinOpKind, BindingMode, Block, BodyId, Closure, ConstArg, ConstArgKind, Expr,
ExprField, ExprKind, FnRetTy, GenericArg, GenericArgs, HirId, HirIdMap, InlineAsmOperand, LetExpr, Lifetime,
@ -17,11 +17,33 @@ use rustc_middle::ty::TypeckResults;
use rustc_span::{BytePos, ExpnKind, MacroKind, Symbol, SyntaxContext, sym};
use std::hash::{Hash, Hasher};
use std::ops::Range;
use std::slice;
/// Callback that is called when two expressions are not equal in the sense of `SpanlessEq`, but
/// other conditions would make them equal.
type SpanlessEqCallback<'a> = dyn FnMut(&Expr<'_>, &Expr<'_>) -> bool + 'a;
/// Determines how paths are hashed and compared for equality.
#[derive(Copy, Clone, Debug, Default)]
pub enum PathCheck {
/// Paths must match exactly and are hashed by their exact HIR tree.
///
/// Thus, `std::iter::Iterator` and `Iterator` are not considered equal even though they refer
/// to the same item.
#[default]
Exact,
/// Paths are compared and hashed based on their resolution.
///
/// They can appear different in the HIR tree but are still considered equal
/// and have equal hashes as long as they refer to the same item.
///
/// Note that this is currently only partially implemented specifically for paths that are
/// resolved before type-checking, i.e. the final segment must have a non-error resolution.
/// If a path with an error resolution is encountered, it falls back to the default exact
/// matching behavior.
Resolution,
}
/// Type used to check whether two ast are the same. This is different from the
/// operator `==` on ast types as this operator would compare true equality with
/// ID and span.
@ -33,6 +55,7 @@ pub struct SpanlessEq<'a, 'tcx> {
maybe_typeck_results: Option<(&'tcx TypeckResults<'tcx>, &'tcx TypeckResults<'tcx>)>,
allow_side_effects: bool,
expr_fallback: Option<Box<SpanlessEqCallback<'a>>>,
path_check: PathCheck,
}
impl<'a, 'tcx> SpanlessEq<'a, 'tcx> {
@ -42,6 +65,7 @@ impl<'a, 'tcx> SpanlessEq<'a, 'tcx> {
maybe_typeck_results: cx.maybe_typeck_results().map(|x| (x, x)),
allow_side_effects: true,
expr_fallback: None,
path_check: PathCheck::default(),
}
}
@ -54,6 +78,16 @@ impl<'a, 'tcx> SpanlessEq<'a, 'tcx> {
}
}
/// Check paths by their resolution instead of exact equality. See [`PathCheck`] for more
/// details.
#[must_use]
pub fn paths_by_resolution(self) -> Self {
Self {
path_check: PathCheck::Resolution,
..self
}
}
#[must_use]
pub fn expr_fallback(self, expr_fallback: impl FnMut(&Expr<'_>, &Expr<'_>) -> bool + 'a) -> Self {
Self {
@ -498,7 +532,7 @@ impl HirEqInterExpr<'_, '_, '_> {
match (left.res, right.res) {
(Res::Local(l), Res::Local(r)) => l == r || self.locals.get(&l) == Some(&r),
(Res::Local(_), _) | (_, Res::Local(_)) => false,
_ => over(left.segments, right.segments, |l, r| self.eq_path_segment(l, r)),
_ => self.eq_path_segments(left.segments, right.segments),
}
}
@ -511,17 +545,39 @@ impl HirEqInterExpr<'_, '_, '_> {
}
}
pub fn eq_path_segments(&mut self, left: &[PathSegment<'_>], right: &[PathSegment<'_>]) -> bool {
left.len() == right.len() && left.iter().zip(right).all(|(l, r)| self.eq_path_segment(l, r))
pub fn eq_path_segments<'tcx>(
&mut self,
mut left: &'tcx [PathSegment<'tcx>],
mut right: &'tcx [PathSegment<'tcx>],
) -> bool {
if let PathCheck::Resolution = self.inner.path_check
&& let Some(left_seg) = generic_path_segments(left)
&& let Some(right_seg) = generic_path_segments(right)
{
// If we compare by resolution, then only check the last segments that could possibly have generic
// arguments
left = left_seg;
right = right_seg;
}
over(left, right, |l, r| self.eq_path_segment(l, r))
}
pub fn eq_path_segment(&mut self, left: &PathSegment<'_>, right: &PathSegment<'_>) -> bool {
// The == of idents doesn't work with different contexts,
// we have to be explicit about hygiene
left.ident.name == right.ident.name
&& both(left.args.as_ref(), right.args.as_ref(), |l, r| {
self.eq_path_parameters(l, r)
})
if !self.eq_path_parameters(left.args(), right.args()) {
return false;
}
if let PathCheck::Resolution = self.inner.path_check
&& left.res != Res::Err
&& right.res != Res::Err
{
left.res == right.res
} else {
// The == of idents doesn't work with different contexts,
// we have to be explicit about hygiene
left.ident.name == right.ident.name
}
}
pub fn eq_ty(&mut self, left: &Ty<'_>, right: &Ty<'_>) -> bool {
@ -684,6 +740,21 @@ pub fn eq_expr_value(cx: &LateContext<'_>, left: &Expr<'_>, right: &Expr<'_>) ->
SpanlessEq::new(cx).deny_side_effects().eq_expr(left, right)
}
/// Returns the segments of a path that might have generic parameters.
/// Usually just the last segment for free items, except for when the path resolves to an associated
/// item, in which case it is the last two
fn generic_path_segments<'tcx>(segments: &'tcx [PathSegment<'tcx>]) -> Option<&'tcx [PathSegment<'tcx>]> {
match segments.last()?.res {
Res::Def(DefKind::AssocConst | DefKind::AssocFn | DefKind::AssocTy, _) => {
// <Ty as module::Trait<T>>::assoc::<U>
// ^^^^^^^^^^^^^^^^ ^^^^^^^^^^ segments: [module, Trait<T>, assoc<U>]
Some(&segments[segments.len().checked_sub(2)?..])
},
Res::Err => None,
_ => Some(slice::from_ref(segments.last()?)),
}
}
/// Type used to hash an ast element. This is different from the `Hash` trait
/// on ast types as this
/// trait would consider IDs and spans.
@ -694,6 +765,7 @@ pub struct SpanlessHash<'a, 'tcx> {
cx: &'a LateContext<'tcx>,
maybe_typeck_results: Option<&'tcx TypeckResults<'tcx>>,
s: FxHasher,
path_check: PathCheck,
}
impl<'a, 'tcx> SpanlessHash<'a, 'tcx> {
@ -701,10 +773,21 @@ impl<'a, 'tcx> SpanlessHash<'a, 'tcx> {
Self {
cx,
maybe_typeck_results: cx.maybe_typeck_results(),
path_check: PathCheck::default(),
s: FxHasher::default(),
}
}
/// Check paths by their resolution instead of exact equality. See [`PathCheck`] for more
/// details.
#[must_use]
pub fn paths_by_resolution(self) -> Self {
Self {
path_check: PathCheck::Resolution,
..self
}
}
pub fn finish(self) -> u64 {
self.s.finish()
}
@ -1042,9 +1125,19 @@ impl<'a, 'tcx> SpanlessHash<'a, 'tcx> {
// even though the binding names are different and they have different `HirId`s.
Res::Local(_) => 1_usize.hash(&mut self.s),
_ => {
for seg in path.segments {
self.hash_name(seg.ident.name);
self.hash_generic_args(seg.args().args);
if let PathCheck::Resolution = self.path_check
&& let [.., last] = path.segments
&& let Some(segments) = generic_path_segments(path.segments)
{
for seg in segments {
self.hash_generic_args(seg.args().args);
}
last.res.hash(&mut self.s);
} else {
for seg in path.segments {
self.hash_name(seg.ident.name);
self.hash_generic_args(seg.args().args);
}
}
},
}

31
tests/ui/trait_duplication_in_bounds.fixed
View File

@ -1,5 +1,6 @@
#![deny(clippy::trait_duplication_in_bounds)]
#![allow(unused)]
#![feature(const_trait_impl)]
use std::any::Any;
@ -144,6 +145,36 @@ fn f<P: Proj>(obj: &dyn Derived<P>) {
Base::<()>::is_base(obj);
}
// #13476
trait Value<const N: usize> {}
fn const_generic<T: Value<0> + Value<1>>() {}
// #11067 and #9626
fn assoc_tys_generics<'a, 'b, T, U>()
where
T: IntoIterator<Item = ()> + IntoIterator<Item = i32>,
U: From<&'a str> + From<&'b [u16]>,
{
}
// #13476
#[const_trait]
trait ConstTrait {}
const fn const_trait_bounds_good<T: ConstTrait + ~const ConstTrait>() {}
const fn const_trait_bounds_bad<T: ~const ConstTrait>() {}
//~^ trait_duplication_in_bounds
fn projections<T, U, V>()
where
U: ToOwned,
V: ToOwned,
T: IntoIterator<Item = U::Owned>,
//~^ trait_duplication_in_bounds
V: IntoIterator<Item = U::Owned> + IntoIterator<Item = V::Owned>,
{
}
fn main() {
let _x: fn(_) = f::<()>;
let _x: fn(_) = f::<i32>;

31
tests/ui/trait_duplication_in_bounds.rs
View File

@ -1,5 +1,6 @@
#![deny(clippy::trait_duplication_in_bounds)]
#![allow(unused)]
#![feature(const_trait_impl)]
use std::any::Any;
@ -144,6 +145,36 @@ fn f<P: Proj>(obj: &dyn Derived<P>) {
Base::<()>::is_base(obj);
}
// #13476
trait Value<const N: usize> {}
fn const_generic<T: Value<0> + Value<1>>() {}
// #11067 and #9626
fn assoc_tys_generics<'a, 'b, T, U>()
where
T: IntoIterator<Item = ()> + IntoIterator<Item = i32>,
U: From<&'a str> + From<&'b [u16]>,
{
}
// #13476
#[const_trait]
trait ConstTrait {}
const fn const_trait_bounds_good<T: ConstTrait + ~const ConstTrait>() {}
const fn const_trait_bounds_bad<T: ~const ConstTrait + ~const ConstTrait>() {}
//~^ trait_duplication_in_bounds
fn projections<T, U, V>()
where
U: ToOwned,
V: ToOwned,
T: IntoIterator<Item = U::Owned> + IntoIterator<Item = U::Owned>,
//~^ trait_duplication_in_bounds
V: IntoIterator<Item = U::Owned> + IntoIterator<Item = V::Owned>,
{
}
fn main() {
let _x: fn(_) = f::<()>;
let _x: fn(_) = f::<i32>;

32
tests/ui/trait_duplication_in_bounds.stderr
View File

@ -1,5 +1,5 @@
error: these bounds contain repeated elements
--> tests/ui/trait_duplication_in_bounds.rs:6:15
--> tests/ui/trait_duplication_in_bounds.rs:7:15
|
LL | fn bad_foo<T: Clone + Clone + Clone + Copy, U: Clone + Copy>(arg0: T, argo1: U) {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Clone + Copy`
@ -11,52 +11,64 @@ LL | #![deny(clippy::trait_duplication_in_bounds)]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
error: these where clauses contain repeated elements
--> tests/ui/trait_duplication_in_bounds.rs:12:8
--> tests/ui/trait_duplication_in_bounds.rs:13:8
|
LL | T: Clone + Clone + Clone + Copy,
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Clone + Copy`
error: these bounds contain repeated elements
--> tests/ui/trait_duplication_in_bounds.rs:40:26
--> tests/ui/trait_duplication_in_bounds.rs:41:26
|
LL | trait BadSelfTraitBound: Clone + Clone + Clone {
| ^^^^^^^^^^^^^^^^^^^^^ help: try: `Clone`
error: these where clauses contain repeated elements
--> tests/ui/trait_duplication_in_bounds.rs:47:15
--> tests/ui/trait_duplication_in_bounds.rs:48:15
|
LL | Self: Clone + Clone + Clone;
| ^^^^^^^^^^^^^^^^^^^^^ help: try: `Clone`
error: these bounds contain repeated elements
--> tests/ui/trait_duplication_in_bounds.rs:61:24
--> tests/ui/trait_duplication_in_bounds.rs:62:24
|
LL | trait BadTraitBound<T: Clone + Clone + Clone + Copy, U: Clone + Copy> {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Clone + Copy`
error: these where clauses contain repeated elements
--> tests/ui/trait_duplication_in_bounds.rs:68:12
--> tests/ui/trait_duplication_in_bounds.rs:69:12
|
LL | T: Clone + Clone + Clone + Copy,
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Clone + Copy`
error: these bounds contain repeated elements
--> tests/ui/trait_duplication_in_bounds.rs:101:19
--> tests/ui/trait_duplication_in_bounds.rs:102:19
|
LL | fn bad_generic<T: GenericTrait<u64> + GenericTrait<u32> + GenericTrait<u64>>(arg0: T) {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `GenericTrait<u64> + GenericTrait<u32>`
error: these bounds contain repeated elements
--> tests/ui/trait_duplication_in_bounds.rs:109:22
--> tests/ui/trait_duplication_in_bounds.rs:110:22
|
LL | fn qualified_path<T: std::clone::Clone + Clone + foo::Clone>(arg0: T) {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `std::clone::Clone + foo::Clone`
error: this trait bound is already specified in trait declaration
--> tests/ui/trait_duplication_in_bounds.rs:117:33
--> tests/ui/trait_duplication_in_bounds.rs:118:33
|
LL | fn bad_trait_object(arg0: &(dyn Any + Send + Send)) {
| ^^^^^^^^^^^^^^^^^ help: try: `Any + Send`
error: aborting due to 9 previous errors
error: these bounds contain repeated elements
--> tests/ui/trait_duplication_in_bounds.rs:165:36
|
LL | const fn const_trait_bounds_bad<T: ~const ConstTrait + ~const ConstTrait>() {}
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `~const ConstTrait`
error: these where clauses contain repeated elements
--> tests/ui/trait_duplication_in_bounds.rs:172:8
|
LL | T: IntoIterator<Item = U::Owned> + IntoIterator<Item = U::Owned>,
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `IntoIterator<Item = U::Owned>`
error: aborting due to 11 previous errors