dnrops
/
rust-clippy
mirror of https://github.com/rust-lang/rust-clippy.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Use SpanlessEq for in `trait_bounds` lints

This commit is contained in:
y21 2024-10-01 21:16:50 +00:00
parent db1bda3df1
commit d6be597fbc
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