From ec29b0d6b819bc88c4a411a8104e684032324b9e Mon Sep 17 00:00:00 2001 From: y21 <30553356+y21@users.noreply.github.com> Date: Sat, 17 Feb 2024 21:35:16 +0100 Subject: [PATCH] lint implied bounds in *all* opaque `impl Trait` types --- clippy_lints/src/implied_bounds_in_impls.rs | 107 ++++++++------------ tests/ui/implied_bounds_in_impls.fixed | 23 +++++ tests/ui/implied_bounds_in_impls.rs | 23 +++++ tests/ui/implied_bounds_in_impls.stderr | 76 ++++++++++---- 4 files changed, 145 insertions(+), 84 deletions(-) diff --git a/clippy_lints/src/implied_bounds_in_impls.rs b/clippy_lints/src/implied_bounds_in_impls.rs index 174db30a7cd..9efe3993ca5 100644 --- a/clippy_lints/src/implied_bounds_in_impls.rs +++ b/clippy_lints/src/implied_bounds_in_impls.rs @@ -2,16 +2,11 @@ use clippy_utils::source::snippet; use rustc_errors::{Applicability, SuggestionStyle}; use rustc_hir::def_id::DefId; -use rustc_hir::intravisit::FnKind; -use rustc_hir::{ - Body, FnDecl, FnRetTy, GenericArg, GenericBound, ImplItem, ImplItemKind, ItemKind, OpaqueTy, TraitBoundModifier, - TraitItem, TraitItemKind, TyKind, TypeBinding, -}; +use rustc_hir::{GenericArg, GenericBound, GenericBounds, ItemKind, TraitBoundModifier, TyKind, TypeBinding}; use rustc_hir_analysis::hir_ty_to_ty; use rustc_lint::{LateContext, LateLintPass}; use rustc_middle::ty::{self, ClauseKind, Generics, Ty, TyCtxt}; use rustc_session::declare_lint_pass; -use rustc_span::def_id::LocalDefId; use rustc_span::Span; declare_clippy_lint! { @@ -54,7 +49,7 @@ fn emit_lint( cx: &LateContext<'_>, poly_trait: &rustc_hir::PolyTraitRef<'_>, - opaque_ty: &rustc_hir::OpaqueTy<'_>, + bounds: GenericBounds<'_>, index: usize, // The bindings that were implied, used for suggestion purposes since removing a bound with associated types // means we might need to then move it to a different bound @@ -73,10 +68,10 @@ fn emit_lint( // to include the `+` token that is ahead or behind, // so we don't end up with something like `impl + B` or `impl A + ` - let implied_span_extended = if let Some(next_bound) = opaque_ty.bounds.get(index + 1) { + let implied_span_extended = if let Some(next_bound) = bounds.get(index + 1) { poly_trait.span.to(next_bound.span().shrink_to_lo()) } else if index > 0 - && let Some(prev_bound) = opaque_ty.bounds.get(index - 1) + && let Some(prev_bound) = bounds.get(index - 1) { prev_bound.span().shrink_to_hi().to(poly_trait.span.shrink_to_hi()) } else { @@ -240,9 +235,8 @@ struct ImplTraitBound<'tcx> { /// For `impl Deref + DerefMut + Eq` this returns `[Deref, PartialEq]`. /// The `Deref` comes from `DerefMut` because `trait DerefMut: Deref {}`, and `PartialEq` comes from /// `Eq`. -fn collect_supertrait_bounds<'tcx>(cx: &LateContext<'tcx>, opaque_ty: &OpaqueTy<'tcx>) -> Vec> { - opaque_ty - .bounds +fn collect_supertrait_bounds<'tcx>(cx: &LateContext<'tcx>, bounds: GenericBounds<'tcx>) -> Vec> { + bounds .iter() .filter_map(|bound| { if let GenericBound::Trait(poly_trait, TraitBoundModifier::None) = bound @@ -295,64 +289,49 @@ fn find_bound_in_supertraits<'a, 'tcx>( }) } -fn check(cx: &LateContext<'_>, decl: &FnDecl<'_>) { - if let FnRetTy::Return(ty) = decl.output - && let TyKind::OpaqueDef(item_id, ..) = ty.kind - && let item = cx.tcx.hir().item(item_id) - && let ItemKind::OpaqueTy(opaque_ty) = item.kind +fn check<'tcx>(cx: &LateContext<'tcx>, bounds: GenericBounds<'tcx>) { + if bounds.len() == 1 { // Very often there is only a single bound, e.g. `impl Deref<..>`, in which case - // we can avoid doing a bunch of stuff unnecessarily. - && opaque_ty.bounds.len() > 1 - { - let supertraits = collect_supertrait_bounds(cx, opaque_ty); + // we can avoid doing a bunch of stuff unnecessarily; there will trivially be + // no duplicate bounds + return; + } - // Lint all bounds in the `impl Trait` type that we've previously also seen in the set of - // supertraits of each of the bounds. - // This involves some extra logic when generic arguments are present, since - // simply comparing trait `DefId`s won't be enough. We also need to compare the generics. - for (index, bound) in opaque_ty.bounds.iter().enumerate() { - if let GenericBound::Trait(poly_trait, TraitBoundModifier::None) = bound - && let [.., path] = poly_trait.trait_ref.path.segments - && let implied_args = path.args.map_or([].as_slice(), |a| a.args) - && let implied_bindings = path.args.map_or([].as_slice(), |a| a.bindings) - && let Some(def_id) = poly_trait.trait_ref.path.res.opt_def_id() - && let Some(bound) = find_bound_in_supertraits(cx, def_id, implied_args, &supertraits) - // If the implied bound has a type binding that also exists in the implied-by trait, - // then we shouldn't lint. See #11880 for an example. - && let assocs = cx.tcx.associated_items(bound.trait_def_id) - && !implied_bindings.iter().any(|binding| { - assocs - .filter_by_name_unhygienic(binding.ident.name) - .next() - .is_some_and(|assoc| assoc.kind == ty::AssocKind::Type) - }) - { - emit_lint(cx, poly_trait, opaque_ty, index, implied_bindings, bound); - } + let supertraits = collect_supertrait_bounds(cx, bounds); + + // Lint all bounds in the `impl Trait` type that we've previously also seen in the set of + // supertraits of each of the bounds. + // This involves some extra logic when generic arguments are present, since + // simply comparing trait `DefId`s won't be enough. We also need to compare the generics. + for (index, bound) in bounds.iter().enumerate() { + if let GenericBound::Trait(poly_trait, TraitBoundModifier::None) = bound + && let [.., path] = poly_trait.trait_ref.path.segments + && let implied_args = path.args.map_or([].as_slice(), |a| a.args) + && let implied_bindings = path.args.map_or([].as_slice(), |a| a.bindings) + && let Some(def_id) = poly_trait.trait_ref.path.res.opt_def_id() + && let Some(bound) = find_bound_in_supertraits(cx, def_id, implied_args, &supertraits) + // If the implied bound has a type binding that also exists in the implied-by trait, + // then we shouldn't lint. See #11880 for an example. + && let assocs = cx.tcx.associated_items(bound.trait_def_id) + && !implied_bindings.iter().any(|binding| { + assocs + .filter_by_name_unhygienic(binding.ident.name) + .next() + .is_some_and(|assoc| assoc.kind == ty::AssocKind::Type) + }) + { + emit_lint(cx, poly_trait, bounds, index, implied_bindings, bound); } } } -impl LateLintPass<'_> for ImpliedBoundsInImpls { - fn check_fn( - &mut self, - cx: &LateContext<'_>, - _: FnKind<'_>, - decl: &FnDecl<'_>, - _: &Body<'_>, - _: Span, - _: LocalDefId, - ) { - check(cx, decl); - } - fn check_trait_item(&mut self, cx: &LateContext<'_>, item: &TraitItem<'_>) { - if let TraitItemKind::Fn(sig, ..) = &item.kind { - check(cx, sig.decl); - } - } - fn check_impl_item(&mut self, cx: &LateContext<'_>, item: &ImplItem<'_>) { - if let ImplItemKind::Fn(sig, ..) = &item.kind { - check(cx, sig.decl); +impl<'tcx> LateLintPass<'tcx> for ImpliedBoundsInImpls { + fn check_ty(&mut self, cx: &LateContext<'_>, ty: &rustc_hir::Ty<'_>) { + if let TyKind::OpaqueDef(item_id, ..) = ty.kind + && let item = cx.tcx.hir().item(item_id) + && let ItemKind::OpaqueTy(opaque_ty) = item.kind + { + check(cx, opaque_ty.bounds); } } } diff --git a/tests/ui/implied_bounds_in_impls.fixed b/tests/ui/implied_bounds_in_impls.fixed index 18f29e900c1..d65fc97ebbf 100644 --- a/tests/ui/implied_bounds_in_impls.fixed +++ b/tests/ui/implied_bounds_in_impls.fixed @@ -1,5 +1,6 @@ #![warn(clippy::implied_bounds_in_impls)] #![allow(dead_code)] +#![feature(impl_trait_in_assoc_type, type_alias_impl_trait)] use std::ops::{Deref, DerefMut}; @@ -150,4 +151,26 @@ fn issue11880() { fn f5() -> impl Y {} } +fn apit(_: impl Deref + DerefMut) {} + +trait Rpitit { + fn f() -> impl DerefMut; +} + +trait Atpit { + type Assoc; + fn define() -> Self::Assoc; +} +impl Atpit for () { + type Assoc = impl DerefMut; + fn define() -> Self::Assoc { + &mut [] as &mut [()] + } +} + +type Tait = impl DerefMut; +fn define() -> Tait { + &mut [] as &mut [()] +} + fn main() {} diff --git a/tests/ui/implied_bounds_in_impls.rs b/tests/ui/implied_bounds_in_impls.rs index b7951e7dfba..52076c9f998 100644 --- a/tests/ui/implied_bounds_in_impls.rs +++ b/tests/ui/implied_bounds_in_impls.rs @@ -1,5 +1,6 @@ #![warn(clippy::implied_bounds_in_impls)] #![allow(dead_code)] +#![feature(impl_trait_in_assoc_type, type_alias_impl_trait)] use std::ops::{Deref, DerefMut}; @@ -150,4 +151,26 @@ fn f4() -> impl X + Y {} fn f5() -> impl X + Y {} } +fn apit(_: impl Deref + DerefMut) {} + +trait Rpitit { + fn f() -> impl Deref + DerefMut; +} + +trait Atpit { + type Assoc; + fn define() -> Self::Assoc; +} +impl Atpit for () { + type Assoc = impl Deref + DerefMut; + fn define() -> Self::Assoc { + &mut [] as &mut [()] + } +} + +type Tait = impl Deref + DerefMut; +fn define() -> Tait { + &mut [] as &mut [()] +} + fn main() {} diff --git a/tests/ui/implied_bounds_in_impls.stderr b/tests/ui/implied_bounds_in_impls.stderr index fa1938f77d9..9505deee351 100644 --- a/tests/ui/implied_bounds_in_impls.stderr +++ b/tests/ui/implied_bounds_in_impls.stderr @@ -1,5 +1,5 @@ error: this bound is already specified as the supertrait of `DerefMut` - --> tests/ui/implied_bounds_in_impls.rs:12:36 + --> tests/ui/implied_bounds_in_impls.rs:13:36 | LL | fn deref_derefmut(x: T) -> impl Deref + DerefMut { | ^^^^^^^^^^^^^^^^^ @@ -13,7 +13,7 @@ LL + fn deref_derefmut(x: T) -> impl DerefMut { | error: this bound is already specified as the supertrait of `GenericSubtrait` - --> tests/ui/implied_bounds_in_impls.rs:29:37 + --> tests/ui/implied_bounds_in_impls.rs:30:37 | LL | fn generics_implied() -> impl GenericTrait + GenericSubtrait | ^^^^^^^^^^^^^^^ @@ -25,7 +25,7 @@ LL + fn generics_implied() -> impl GenericSubtrait | error: this bound is already specified as the supertrait of `GenericSubtrait<(), i32, V>` - --> tests/ui/implied_bounds_in_impls.rs:35:40 + --> tests/ui/implied_bounds_in_impls.rs:36:40 | LL | fn generics_implied_multi() -> impl GenericTrait + GenericTrait2 + GenericSubtrait<(), i32, V> {} | ^^^^^^^^^^^^^^^^^ @@ -37,7 +37,7 @@ LL + fn generics_implied_multi() -> impl GenericTrait2 + GenericSubtrait<( | error: this bound is already specified as the supertrait of `GenericSubtrait<(), i32, V>` - --> tests/ui/implied_bounds_in_impls.rs:35:60 + --> tests/ui/implied_bounds_in_impls.rs:36:60 | LL | fn generics_implied_multi() -> impl GenericTrait + GenericTrait2 + GenericSubtrait<(), i32, V> {} | ^^^^^^^^^^^^^^^^ @@ -49,7 +49,7 @@ LL + fn generics_implied_multi() -> impl GenericTrait + GenericSubtrait< | error: this bound is already specified as the supertrait of `GenericSubtrait<(), T, V>` - --> tests/ui/implied_bounds_in_impls.rs:37:44 + --> tests/ui/implied_bounds_in_impls.rs:38:44 | LL | fn generics_implied_multi2() -> impl GenericTrait + GenericTrait2 + GenericSubtrait<(), T, V> | ^^^^^^^^^^^^^^^ @@ -61,7 +61,7 @@ LL + fn generics_implied_multi2() -> impl GenericTrait2 + GenericSubtra | error: this bound is already specified as the supertrait of `GenericSubtrait<(), T, V>` - --> tests/ui/implied_bounds_in_impls.rs:37:62 + --> tests/ui/implied_bounds_in_impls.rs:38:62 | LL | fn generics_implied_multi2() -> impl GenericTrait + GenericTrait2 + GenericSubtrait<(), T, V> | ^^^^^^^^^^^^^^^^ @@ -73,7 +73,7 @@ LL + fn generics_implied_multi2() -> impl GenericTrait + GenericSubtrai | error: this bound is already specified as the supertrait of `GenericSubtrait<(), i32, ()>` - --> tests/ui/implied_bounds_in_impls.rs:47:28 + --> tests/ui/implied_bounds_in_impls.rs:48:28 | LL | fn generics_same() -> impl GenericTrait + GenericSubtrait<(), i32, ()> {} | ^^^^^^^^^^^^^^^^^ @@ -85,7 +85,7 @@ LL + fn generics_same() -> impl GenericSubtrait<(), i32, ()> {} | error: this bound is already specified as the supertrait of `DerefMut` - --> tests/ui/implied_bounds_in_impls.rs:51:20 + --> tests/ui/implied_bounds_in_impls.rs:52:20 | LL | fn f() -> impl Deref + DerefMut; | ^^^^^ @@ -97,7 +97,7 @@ LL + fn f() -> impl DerefMut; | error: this bound is already specified as the supertrait of `DerefMut` - --> tests/ui/implied_bounds_in_impls.rs:56:20 + --> tests/ui/implied_bounds_in_impls.rs:57:20 | LL | fn f() -> impl Deref + DerefMut { | ^^^^^ @@ -109,7 +109,7 @@ LL + fn f() -> impl DerefMut { | error: this bound is already specified as the supertrait of `DerefMut` - --> tests/ui/implied_bounds_in_impls.rs:62:20 + --> tests/ui/implied_bounds_in_impls.rs:63:20 | LL | fn f() -> impl Deref + DerefMut { | ^^^^^ @@ -121,7 +121,7 @@ LL + fn f() -> impl DerefMut { | error: this bound is already specified as the supertrait of `PartialOrd` - --> tests/ui/implied_bounds_in_impls.rs:73:41 + --> tests/ui/implied_bounds_in_impls.rs:74:41 | LL | fn default_generic_param1() -> impl PartialEq + PartialOrd + Debug {} | ^^^^^^^^^ @@ -133,7 +133,7 @@ LL + fn default_generic_param1() -> impl PartialOrd + Debug {} | error: this bound is already specified as the supertrait of `PartialOrd` - --> tests/ui/implied_bounds_in_impls.rs:74:54 + --> tests/ui/implied_bounds_in_impls.rs:75:54 | LL | fn default_generic_param2() -> impl PartialOrd + PartialEq + Debug {} | ^^^^^^^^^ @@ -145,7 +145,7 @@ LL + fn default_generic_param2() -> impl PartialOrd + Debug {} | error: this bound is already specified as the supertrait of `DoubleEndedIterator` - --> tests/ui/implied_bounds_in_impls.rs:87:26 + --> tests/ui/implied_bounds_in_impls.rs:88:26 | LL | fn my_iter() -> impl Iterator + DoubleEndedIterator { | ^^^^^^^^^^^^^^^^^^^^ @@ -157,7 +157,7 @@ LL + fn my_iter() -> impl DoubleEndedIterator { | error: this bound is already specified as the supertrait of `Copy` - --> tests/ui/implied_bounds_in_impls.rs:92:27 + --> tests/ui/implied_bounds_in_impls.rs:93:27 | LL | fn f() -> impl Copy + Clone { | ^^^^^ @@ -169,7 +169,7 @@ LL + fn f() -> impl Copy { | error: this bound is already specified as the supertrait of `Trait2` - --> tests/ui/implied_bounds_in_impls.rs:106:21 + --> tests/ui/implied_bounds_in_impls.rs:107:21 | LL | fn f2() -> impl Trait1 + Trait2 {} | ^^^^^^^^^^^^^^^^^^^^ @@ -181,7 +181,7 @@ LL + fn f2() -> impl Trait2 {} | error: this bound is already specified as the supertrait of `Trait4` - --> tests/ui/implied_bounds_in_impls.rs:121:21 + --> tests/ui/implied_bounds_in_impls.rs:122:21 | LL | fn f3() -> impl Trait3 + Trait4 {} | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -193,7 +193,7 @@ LL + fn f3() -> impl Trait4 {} | error: this bound is already specified as the supertrait of `Y` - --> tests/ui/implied_bounds_in_impls.rs:148:21 + --> tests/ui/implied_bounds_in_impls.rs:149:21 | LL | fn f3() -> impl X + Y {} | ^ @@ -205,7 +205,7 @@ LL + fn f3() -> impl Y {} | error: this bound is already specified as the supertrait of `Y` - --> tests/ui/implied_bounds_in_impls.rs:149:21 + --> tests/ui/implied_bounds_in_impls.rs:150:21 | LL | fn f4() -> impl X + Y {} | ^ @@ -217,7 +217,7 @@ LL + fn f4() -> impl Y {} | error: this bound is already specified as the supertrait of `Y` - --> tests/ui/implied_bounds_in_impls.rs:150:21 + --> tests/ui/implied_bounds_in_impls.rs:151:21 | LL | fn f5() -> impl X + Y {} | ^^^^^^^^^^^^^ @@ -228,5 +228,41 @@ LL - fn f5() -> impl X + Y {} LL + fn f5() -> impl Y {} | -error: aborting due to 19 previous errors +error: this bound is already specified as the supertrait of `DerefMut` + --> tests/ui/implied_bounds_in_impls.rs:157:20 + | +LL | fn f() -> impl Deref + DerefMut; + | ^^^^^ + | +help: try removing this bound + | +LL - fn f() -> impl Deref + DerefMut; +LL + fn f() -> impl DerefMut; + | + +error: this bound is already specified as the supertrait of `DerefMut` + --> tests/ui/implied_bounds_in_impls.rs:165:23 + | +LL | type Assoc = impl Deref + DerefMut; + | ^^^^^ + | +help: try removing this bound + | +LL - type Assoc = impl Deref + DerefMut; +LL + type Assoc = impl DerefMut; + | + +error: this bound is already specified as the supertrait of `DerefMut` + --> tests/ui/implied_bounds_in_impls.rs:171:18 + | +LL | type Tait = impl Deref + DerefMut; + | ^^^^^ + | +help: try removing this bound + | +LL - type Tait = impl Deref + DerefMut; +LL + type Tait = impl DerefMut; + | + +error: aborting due to 22 previous errors