From d6be597fbc35dcc795599784a27ad7f8f567f731 Mon Sep 17 00:00:00 2001 From: y21 <30553356+y21@users.noreply.github.com> Date: Tue, 1 Oct 2024 21:16:50 +0000 Subject: [PATCH] Use SpanlessEq for in `trait_bounds` lints --- clippy_lints/src/trait_bounds.rs | 107 ++++++++---------- clippy_utils/src/hir_utils.rs | 119 +++++++++++++++++--- tests/ui/trait_duplication_in_bounds.fixed | 31 +++++ tests/ui/trait_duplication_in_bounds.rs | 31 +++++ tests/ui/trait_duplication_in_bounds.stderr | 32 ++++-- 5 files changed, 239 insertions(+), 81 deletions(-) diff --git a/clippy_lints/src/trait_bounds.rs b/clippy_lints/src/trait_bounds.rs index 00277593622a..9a15cf2562de 100644 --- a/clippy_lints/src/trait_bounds.rs +++ b/clippy_lints/src/trait_bounds.rs @@ -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); -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(&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 { - 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()); diff --git a/clippy_utils/src/hir_utils.rs b/clippy_utils/src/hir_utils.rs index 06a5cb1a84f0..9de855c53b3e 100644 --- a/clippy_utils/src/hir_utils.rs +++ b/clippy_utils/src/hir_utils.rs @@ -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>>, + 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, _) => { + // >::assoc:: + // ^^^^^^^^^^^^^^^^ ^^^^^^^^^^ segments: [module, Trait, assoc] + 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); + } } }, } diff --git a/tests/ui/trait_duplication_in_bounds.fixed b/tests/ui/trait_duplication_in_bounds.fixed index 7e2663d734f6..779431303aed 100644 --- a/tests/ui/trait_duplication_in_bounds.fixed +++ b/tests/ui/trait_duplication_in_bounds.fixed @@ -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(obj: &dyn Derived

) { Base::<()>::is_base(obj); } +// #13476 +trait Value {} +fn const_generic + Value<1>>() {} + +// #11067 and #9626 +fn assoc_tys_generics<'a, 'b, T, U>() +where + T: IntoIterator + IntoIterator, + U: From<&'a str> + From<&'b [u16]>, +{ +} + +// #13476 +#[const_trait] +trait ConstTrait {} +const fn const_trait_bounds_good() {} + +const fn const_trait_bounds_bad() {} +//~^ trait_duplication_in_bounds + +fn projections() +where + U: ToOwned, + V: ToOwned, + T: IntoIterator, + //~^ trait_duplication_in_bounds + V: IntoIterator + IntoIterator, +{ +} + fn main() { let _x: fn(_) = f::<()>; let _x: fn(_) = f::; diff --git a/tests/ui/trait_duplication_in_bounds.rs b/tests/ui/trait_duplication_in_bounds.rs index fede1671a436..3e974dc0a8f2 100644 --- a/tests/ui/trait_duplication_in_bounds.rs +++ b/tests/ui/trait_duplication_in_bounds.rs @@ -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(obj: &dyn Derived

) { Base::<()>::is_base(obj); } +// #13476 +trait Value {} +fn const_generic + Value<1>>() {} + +// #11067 and #9626 +fn assoc_tys_generics<'a, 'b, T, U>() +where + T: IntoIterator + IntoIterator, + U: From<&'a str> + From<&'b [u16]>, +{ +} + +// #13476 +#[const_trait] +trait ConstTrait {} +const fn const_trait_bounds_good() {} + +const fn const_trait_bounds_bad() {} +//~^ trait_duplication_in_bounds + +fn projections() +where + U: ToOwned, + V: ToOwned, + T: IntoIterator + IntoIterator, + //~^ trait_duplication_in_bounds + V: IntoIterator + IntoIterator, +{ +} + fn main() { let _x: fn(_) = f::<()>; let _x: fn(_) = f::; diff --git a/tests/ui/trait_duplication_in_bounds.stderr b/tests/ui/trait_duplication_in_bounds.stderr index 78861fc16e81..0dd508e47450 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 - --> tests/ui/trait_duplication_in_bounds.rs:6:15 + --> tests/ui/trait_duplication_in_bounds.rs:7:15 | LL | fn bad_foo(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 { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ 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 + GenericTrait + GenericTrait>(arg0: T) { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `GenericTrait + GenericTrait` 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(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() {} + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `~const ConstTrait` + +error: these where clauses contain repeated elements + --> tests/ui/trait_duplication_in_bounds.rs:172:8 + | +LL | T: IntoIterator + IntoIterator, + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `IntoIterator` + +error: aborting due to 11 previous errors