From 42bd6d7af3b12366b6d063c2f5220323aee62efc Mon Sep 17 00:00:00 2001 From: y21 <30553356+y21@users.noreply.github.com> Date: Sun, 20 Aug 2023 01:20:29 +0200 Subject: [PATCH 1/4] new lint: `implied_bounds_in_impl` --- CHANGELOG.md | 1 + clippy_lints/src/declared_lints.rs | 1 + clippy_lints/src/implied_bounds_in_impl.rs | 96 ++++++++++++++++++++++ clippy_lints/src/lib.rs | 2 + tests/ui/implied_bounds_in_impl.rs | 45 ++++++++++ tests/ui/implied_bounds_in_impl.stderr | 16 ++++ 6 files changed, 161 insertions(+) create mode 100644 clippy_lints/src/implied_bounds_in_impl.rs create mode 100644 tests/ui/implied_bounds_in_impl.rs create mode 100644 tests/ui/implied_bounds_in_impl.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index 4e33cb7b4570..1ae44c502dbb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4985,6 +4985,7 @@ Released 2018-09-13 [`implicit_return`]: https://rust-lang.github.io/rust-clippy/master/index.html#implicit_return [`implicit_saturating_add`]: https://rust-lang.github.io/rust-clippy/master/index.html#implicit_saturating_add [`implicit_saturating_sub`]: https://rust-lang.github.io/rust-clippy/master/index.html#implicit_saturating_sub +[`implied_bounds_in_impl`]: https://rust-lang.github.io/rust-clippy/master/index.html#implied_bounds_in_impl [`impossible_comparisons`]: https://rust-lang.github.io/rust-clippy/master/index.html#impossible_comparisons [`imprecise_flops`]: https://rust-lang.github.io/rust-clippy/master/index.html#imprecise_flops [`inconsistent_digit_grouping`]: https://rust-lang.github.io/rust-clippy/master/index.html#inconsistent_digit_grouping diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 1be9720fbbf8..ea8d804b423d 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -209,6 +209,7 @@ crate::implicit_return::IMPLICIT_RETURN_INFO, crate::implicit_saturating_add::IMPLICIT_SATURATING_ADD_INFO, crate::implicit_saturating_sub::IMPLICIT_SATURATING_SUB_INFO, + crate::implied_bounds_in_impl::IMPLIED_BOUNDS_IN_IMPL_INFO, crate::inconsistent_struct_constructor::INCONSISTENT_STRUCT_CONSTRUCTOR_INFO, crate::incorrect_impls::INCORRECT_CLONE_IMPL_ON_COPY_TYPE_INFO, crate::incorrect_impls::INCORRECT_PARTIAL_ORD_IMPL_ON_ORD_TYPE_INFO, diff --git a/clippy_lints/src/implied_bounds_in_impl.rs b/clippy_lints/src/implied_bounds_in_impl.rs new file mode 100644 index 000000000000..c83153e5350f --- /dev/null +++ b/clippy_lints/src/implied_bounds_in_impl.rs @@ -0,0 +1,96 @@ +use clippy_utils::diagnostics::span_lint; +use rustc_hir::def_id::LocalDefId; +use rustc_hir::intravisit::FnKind; +use rustc_hir::{Body, FnDecl, FnRetTy, GenericArgs, GenericBound, ItemKind, TraitBoundModifier, TyKind}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_middle::ty::ClauseKind; +use rustc_session::{declare_lint_pass, declare_tool_lint}; +use rustc_span::Span; + +declare_clippy_lint! { + /// ### What it does + /// Looks for bounds in `impl Trait` in return position that are implied by other bounds. + /// This is usually the case when a supertrait is explicitly specified, when it is already implied + /// by a subtrait (e.g. `DerefMut: Deref`, so specifying `Deref` is unnecessary when `DerefMut` is specified). + /// + /// ### Why is this bad? + /// Unnecessary complexity. + /// + /// ### Known problems + /// This lint currently does not work with generic traits (i.e. will not lint even if redundant). + /// + /// ### Example + /// ```rust + /// fn f() -> impl Deref + DerefMut { + /// // ^^^^^^^^^^^^^^^^^^^ unnecessary bound, already implied by the `DerefMut` trait bound + /// Box::new(123) + /// } + /// ``` + /// Use instead: + /// ```rust + /// fn f() -> impl DerefMut { + /// Box::new(123) + /// } + /// ``` + #[clippy::version = "1.73.0"] + pub IMPLIED_BOUNDS_IN_IMPL, + complexity, + "specifying bounds that are implied by other bounds in `impl Trait` type" +} +declare_lint_pass!(ImpliedBoundsInImpl => [IMPLIED_BOUNDS_IN_IMPL]); + +impl LateLintPass<'_> for ImpliedBoundsInImpl { + fn check_fn( + &mut self, + cx: &LateContext<'_>, + _: FnKind<'_>, + decl: &FnDecl<'_>, + _: &Body<'_>, + _: Span, + _: LocalDefId, + ) { + if let FnRetTy::Return(ty) = decl.output { + if let TyKind::OpaqueDef(item_id, ..) = ty.kind + && let item = cx.tcx.hir().item(item_id) + && let ItemKind::OpaqueTy(opaque_ty) = item.kind + { + // Get all `DefId`s of (implied) trait predicates in all the bounds. + // For `impl Deref + DerefMut` this will contain [`Deref`]. + // The implied `Deref` comes from `DerefMut` because `trait DerefMut: Deref {}`. + + // N.B. Generic args on trait bounds are currently ignored and (G)ATs are fine to disregard, + // because they must be the same for all of its supertraits. Example: + // `impl Deref + DerefMut` is not allowed. + // `DerefMut::Target` needs to match `Deref::Target` + let implied_bounds = opaque_ty.bounds.iter().flat_map(|bound| { + if let GenericBound::Trait(poly_trait, TraitBoundModifier::None) = bound + && let [.., path] = poly_trait.trait_ref.path.segments + && poly_trait.bound_generic_params.is_empty() + && path.args.map_or(true, GenericArgs::is_empty) + && let Some(trait_def_id) = path.res.opt_def_id() + { + cx.tcx.implied_predicates_of(trait_def_id).predicates + } else { + &[] + } + }).collect::>(); + + // Lint all bounds in the `impl Trait` type that are also in the `implied_bounds` vec. + for bound in opaque_ty.bounds { + if let GenericBound::Trait(poly_trait, TraitBoundModifier::None) = bound + && let Some(def_id) = poly_trait.trait_ref.path.res.opt_def_id() + && implied_bounds.iter().any(|(clause, _)| { + if let ClauseKind::Trait(tr) = clause.kind().skip_binder() { + tr.def_id() == def_id + } else { + false + } + }) + { + span_lint(cx, IMPLIED_BOUNDS_IN_IMPL, poly_trait.span, "this bound is implied by another bound and can be removed"); + } + } + } + } + } +} diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index f50019f3cf76..d4fcddc66c5c 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -152,6 +152,7 @@ mod implicit_return; mod implicit_saturating_add; mod implicit_saturating_sub; +mod implied_bounds_in_impl; mod inconsistent_struct_constructor; mod incorrect_impls; mod index_refutable_slice; @@ -1097,6 +1098,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_late_pass(|_| Box::new(redundant_locals::RedundantLocals)); store.register_late_pass(|_| Box::new(ignored_unit_patterns::IgnoredUnitPatterns)); store.register_late_pass(|_| Box::::default()); + store.register_late_pass(|_| Box::new(implied_bounds_in_impl::ImpliedBoundsInImpl)); // add lints here, do not remove this comment, it's used in `new_lint` } diff --git a/tests/ui/implied_bounds_in_impl.rs b/tests/ui/implied_bounds_in_impl.rs new file mode 100644 index 000000000000..516f21fc795b --- /dev/null +++ b/tests/ui/implied_bounds_in_impl.rs @@ -0,0 +1,45 @@ +#![warn(clippy::implied_bounds_in_impl)] +#![allow(dead_code)] + +use std::ops::{Deref, DerefMut}; + +trait Trait1 {} +// T is intentionally at a different position in Trait2 than in Trait1, +// since that also needs to be taken into account when making this lint work with generics +trait Trait2: Trait1 {} +impl Trait1 for () {} +impl Trait1 for () {} +impl Trait2 for () {} +impl Trait2 for () {} + +// Deref implied by DerefMut +fn deref_derefmut(x: T) -> impl Deref + DerefMut { + Box::new(x) +} + +// Note: no test for different associated types needed since that isn't allowed in the first place. +// E.g. `-> impl Deref + DerefMut` is a compile error. + +// DefIds of the traits match, but the generics do not, so it's *not* redundant. +// `Trait2: Trait` holds, but not `Trait2<_, String>: Trait1`. +// (Generic traits are currently not linted anyway but once/if ever implemented this should not +// warn.) +fn different_generics() -> impl Trait1 + Trait2 { + /* () */ +} + +trait NonGenericTrait1 {} +trait NonGenericTrait2: NonGenericTrait1 {} +impl NonGenericTrait1 for i32 {} +impl NonGenericTrait2 for i32 {} + +// Only one bound. Nothing to lint. +fn normal1() -> impl NonGenericTrait1 { + 1 +} + +fn normal2() -> impl NonGenericTrait1 + NonGenericTrait2 { + 1 +} + +fn main() {} diff --git a/tests/ui/implied_bounds_in_impl.stderr b/tests/ui/implied_bounds_in_impl.stderr new file mode 100644 index 000000000000..2709f727acc9 --- /dev/null +++ b/tests/ui/implied_bounds_in_impl.stderr @@ -0,0 +1,16 @@ +error: this bound is implied by another bound and can be removed + --> $DIR/implied_bounds_in_impl.rs:16:36 + | +LL | fn deref_derefmut(x: T) -> impl Deref + DerefMut { + | ^^^^^^^^^^^^^^^^^ + | + = note: `-D clippy::implied-bounds-in-impl` implied by `-D warnings` + +error: this bound is implied by another bound and can be removed + --> $DIR/implied_bounds_in_impl.rs:41:22 + | +LL | fn normal2() -> impl NonGenericTrait1 + NonGenericTrait2 { + | ^^^^^^^^^^^^^^^^ + +error: aborting due to 2 previous errors + From 2ebff58969988d1b45b19d0dd50a8058aa26eb37 Mon Sep 17 00:00:00 2001 From: y21 <30553356+y21@users.noreply.github.com> Date: Sun, 20 Aug 2023 05:25:35 +0200 Subject: [PATCH 2/4] make generics work fix compile error in doc example --- clippy_lints/src/implied_bounds_in_impl.rs | 99 ++++++++++++++++++---- tests/ui/implied_bounds_in_impl.rs | 54 ++++++------ tests/ui/implied_bounds_in_impl.stderr | 40 +++++++-- 3 files changed, 145 insertions(+), 48 deletions(-) diff --git a/clippy_lints/src/implied_bounds_in_impl.rs b/clippy_lints/src/implied_bounds_in_impl.rs index c83153e5350f..3f9f5daa6a46 100644 --- a/clippy_lints/src/implied_bounds_in_impl.rs +++ b/clippy_lints/src/implied_bounds_in_impl.rs @@ -1,9 +1,10 @@ use clippy_utils::diagnostics::span_lint; use rustc_hir::def_id::LocalDefId; use rustc_hir::intravisit::FnKind; -use rustc_hir::{Body, FnDecl, FnRetTy, GenericArgs, GenericBound, ItemKind, TraitBoundModifier, TyKind}; +use rustc_hir::{Body, FnDecl, FnRetTy, GenericArg, GenericBound, ItemKind, TraitBoundModifier, TyKind}; +use rustc_hir_analysis::hir_ty_to_ty; use rustc_lint::{LateContext, LateLintPass}; -use rustc_middle::ty::ClauseKind; +use rustc_middle::ty::{self, ClauseKind, TyCtxt}; use rustc_session::{declare_lint_pass, declare_tool_lint}; use rustc_span::Span; @@ -17,10 +18,11 @@ /// Unnecessary complexity. /// /// ### Known problems - /// This lint currently does not work with generic traits (i.e. will not lint even if redundant). + /// This lint does not transitively look for implied bounds past the first supertrait. /// /// ### Example /// ```rust + /// # use std::ops::{Deref,DerefMut}; /// fn f() -> impl Deref + DerefMut { /// // ^^^^^^^^^^^^^^^^^^^ unnecessary bound, already implied by the `DerefMut` trait bound /// Box::new(123) @@ -28,6 +30,7 @@ /// ``` /// Use instead: /// ```rust + /// # use std::ops::{Deref,DerefMut}; /// fn f() -> impl DerefMut { /// Box::new(123) /// } @@ -39,6 +42,53 @@ } declare_lint_pass!(ImpliedBoundsInImpl => [IMPLIED_BOUNDS_IN_IMPL]); +/// This function tries to, for all type parameters in a supertype predicate `GenericTrait`, +/// check if the substituted type in the implied-by bound matches with what's subtituted in the +/// implied type. +/// +/// Consider this example function. +/// ```rust,ignore +/// trait GenericTrait {} +/// trait GenericSubTrait: GenericTrait {} +/// ^ trait_predicate_args: [Self#0, U#2] +/// impl GenericTrait for () {} +/// impl GenericSubTrait<(), i32, ()> for () {} +/// impl GenericSubTrait<(), [u8; 8], ()> for () {} +/// +/// fn f() -> impl GenericTrait + GenericSubTrait<(), [u8; 8], ()> { +/// ^^^ implied_args ^^^^^^^^^^^^^^^ implied_by_args +/// (we are interested in `[u8; 8]` specifically, as that +/// is what `U` in `GenericTrait` is substituted with) +/// () +/// } +/// ``` +/// Here i32 != [u8; 8], so this will return false. +fn is_same_generics( + tcx: TyCtxt<'_>, + trait_predicate_args: &[ty::GenericArg<'_>], + implied_by_args: &[GenericArg<'_>], + implied_args: &[GenericArg<'_>], +) -> bool { + trait_predicate_args + .iter() + .enumerate() + .skip(1) // skip `Self` implicit arg + .all(|(arg_index, arg)| { + if let Some(ty) = arg.as_type() + && let &ty::Param(ty::ParamTy{ index, .. }) = ty.kind() + // Since `trait_predicate_args` and type params in traits start with `Self=0` + // and generic argument lists `GenericTrait` don't have `Self`, + // we need to subtract 1 from the index. + && let GenericArg::Type(ty_a) = implied_by_args[index as usize - 1] + && let GenericArg::Type(ty_b) = implied_args[arg_index - 1] + { + hir_ty_to_ty(tcx, ty_a) == hir_ty_to_ty(tcx, ty_b) + } else { + false + } + }) +} + impl LateLintPass<'_> for ImpliedBoundsInImpl { fn check_fn( &mut self, @@ -54,37 +104,52 @@ fn check_fn( && let item = cx.tcx.hir().item(item_id) && let ItemKind::OpaqueTy(opaque_ty) = item.kind { - // Get all `DefId`s of (implied) trait predicates in all the bounds. + // Very often there is only a single bound, e.g. `impl Deref<..>`, in which case + // we can avoid doing a bunch of stuff unnecessarily. + if opaque_ty.bounds.is_empty() { + return; + } + + // Get all the (implied) trait predicates in the bounds. // For `impl Deref + DerefMut` this will contain [`Deref`]. // The implied `Deref` comes from `DerefMut` because `trait DerefMut: Deref {}`. - // N.B. Generic args on trait bounds are currently ignored and (G)ATs are fine to disregard, - // because they must be the same for all of its supertraits. Example: + // N.B. (G)ATs are fine to disregard, because they must be the same for all of its supertraits. + // Example: // `impl Deref + DerefMut` is not allowed. // `DerefMut::Target` needs to match `Deref::Target` - let implied_bounds = opaque_ty.bounds.iter().flat_map(|bound| { + let implied_bounds: Vec<_> = opaque_ty.bounds.iter().filter_map(|bound| { if let GenericBound::Trait(poly_trait, TraitBoundModifier::None) = bound && let [.., path] = poly_trait.trait_ref.path.segments && poly_trait.bound_generic_params.is_empty() - && path.args.map_or(true, GenericArgs::is_empty) && let Some(trait_def_id) = path.res.opt_def_id() + && let predicates = cx.tcx.super_predicates_of(trait_def_id).predicates + && !predicates.is_empty() // If the trait has no supertrait, there is nothing to add. { - cx.tcx.implied_predicates_of(trait_def_id).predicates + Some((path.args.map_or([].as_slice(), |a| a.args), predicates)) } else { - &[] + None } - }).collect::>(); + }).collect(); // Lint all bounds in the `impl Trait` type that are also in the `implied_bounds` vec. + // 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 bound in opaque_ty.bounds { 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 Some(def_id) = poly_trait.trait_ref.path.res.opt_def_id() - && implied_bounds.iter().any(|(clause, _)| { - if let ClauseKind::Trait(tr) = clause.kind().skip_binder() { - tr.def_id() == def_id - } else { - false - } + && implied_bounds.iter().any(|(implied_by_args, preds)| { + preds.iter().any(|(clause, _)| { + if let ClauseKind::Trait(tr) = clause.kind().skip_binder() + && tr.def_id() == def_id + { + is_same_generics(cx.tcx, tr.trait_ref.args, implied_by_args, implied_args) + } else { + false + } + }) }) { span_lint(cx, IMPLIED_BOUNDS_IN_IMPL, poly_trait.span, "this bound is implied by another bound and can be removed"); diff --git a/tests/ui/implied_bounds_in_impl.rs b/tests/ui/implied_bounds_in_impl.rs index 516f21fc795b..0bc4b193d533 100644 --- a/tests/ui/implied_bounds_in_impl.rs +++ b/tests/ui/implied_bounds_in_impl.rs @@ -3,43 +3,45 @@ use std::ops::{Deref, DerefMut}; -trait Trait1 {} -// T is intentionally at a different position in Trait2 than in Trait1, -// since that also needs to be taken into account when making this lint work with generics -trait Trait2: Trait1 {} -impl Trait1 for () {} -impl Trait1 for () {} -impl Trait2 for () {} -impl Trait2 for () {} +// Only one bound, nothing to lint. +fn normal_deref(x: T) -> impl Deref { + Box::new(x) +} // Deref implied by DerefMut fn deref_derefmut(x: T) -> impl Deref + DerefMut { Box::new(x) } -// Note: no test for different associated types needed since that isn't allowed in the first place. -// E.g. `-> impl Deref + DerefMut` is a compile error. +trait GenericTrait {} +trait GenericTrait2 {} +// U is intentionally at a different "index" in GenericSubtrait than `T` is in GenericTrait +trait GenericSubtrait: GenericTrait + GenericTrait2 {} -// DefIds of the traits match, but the generics do not, so it's *not* redundant. -// `Trait2: Trait` holds, but not `Trait2<_, String>: Trait1`. -// (Generic traits are currently not linted anyway but once/if ever implemented this should not -// warn.) -fn different_generics() -> impl Trait1 + Trait2 { - /* () */ +impl GenericTrait for () {} +impl GenericTrait for () {} +impl GenericTrait2 for () {} +impl GenericSubtrait<(), i32, V> for () {} +impl GenericSubtrait<(), i64, V> for () {} + +fn generics_implied() -> impl GenericTrait + GenericSubtrait<(), T, ()> +where + (): GenericSubtrait<(), T, ()>, +{ } -trait NonGenericTrait1 {} -trait NonGenericTrait2: NonGenericTrait1 {} -impl NonGenericTrait1 for i32 {} -impl NonGenericTrait2 for i32 {} +fn generics_implied_multi() -> impl GenericTrait + GenericTrait2 + GenericSubtrait<(), i32, V> {} -// Only one bound. Nothing to lint. -fn normal1() -> impl NonGenericTrait1 { - 1 +fn generics_implied_multi2() -> impl GenericTrait + GenericTrait2 + GenericSubtrait<(), T, V> +where + (): GenericSubtrait<(), T, V> + GenericTrait, +{ } -fn normal2() -> impl NonGenericTrait1 + NonGenericTrait2 { - 1 -} +// i32 != i64, GenericSubtrait<_, i64, _> does not imply GenericTrait, don't lint +fn generics_different() -> impl GenericTrait + GenericSubtrait<(), i64, ()> {} + +// i32 == i32, GenericSubtrait<_, i32, _> does imply GenericTrait, lint +fn generics_same() -> impl GenericTrait + GenericSubtrait<(), i32, ()> {} fn main() {} diff --git a/tests/ui/implied_bounds_in_impl.stderr b/tests/ui/implied_bounds_in_impl.stderr index 2709f727acc9..92d412e784f3 100644 --- a/tests/ui/implied_bounds_in_impl.stderr +++ b/tests/ui/implied_bounds_in_impl.stderr @@ -1,5 +1,5 @@ error: this bound is implied by another bound and can be removed - --> $DIR/implied_bounds_in_impl.rs:16:36 + --> $DIR/implied_bounds_in_impl.rs:12:36 | LL | fn deref_derefmut(x: T) -> impl Deref + DerefMut { | ^^^^^^^^^^^^^^^^^ @@ -7,10 +7,40 @@ LL | fn deref_derefmut(x: T) -> impl Deref + DerefMut = note: `-D clippy::implied-bounds-in-impl` implied by `-D warnings` error: this bound is implied by another bound and can be removed - --> $DIR/implied_bounds_in_impl.rs:41:22 + --> $DIR/implied_bounds_in_impl.rs:27:34 | -LL | fn normal2() -> impl NonGenericTrait1 + NonGenericTrait2 { - | ^^^^^^^^^^^^^^^^ +LL | fn generics_implied() -> impl GenericTrait + GenericSubtrait<(), T, ()> + | ^^^^^^^^^^^^^^^ -error: aborting due to 2 previous errors +error: this bound is implied by another bound and can be removed + --> $DIR/implied_bounds_in_impl.rs:33:40 + | +LL | fn generics_implied_multi() -> impl GenericTrait + GenericTrait2 + GenericSubtrait<(), i32, V> {} + | ^^^^^^^^^^^^^^^^^ + +error: this bound is implied by another bound and can be removed + --> $DIR/implied_bounds_in_impl.rs:33:60 + | +LL | fn generics_implied_multi() -> impl GenericTrait + GenericTrait2 + GenericSubtrait<(), i32, V> {} + | ^^^^^^^^^^^^^^^^ + +error: this bound is implied by another bound and can be removed + --> $DIR/implied_bounds_in_impl.rs:35:44 + | +LL | fn generics_implied_multi2() -> impl GenericTrait + GenericTrait2 + GenericSubtrait<(), T, V> + | ^^^^^^^^^^^^^^^ + +error: this bound is implied by another bound and can be removed + --> $DIR/implied_bounds_in_impl.rs:35:62 + | +LL | fn generics_implied_multi2() -> impl GenericTrait + GenericTrait2 + GenericSubtrait<(), T, V> + | ^^^^^^^^^^^^^^^^ + +error: this bound is implied by another bound and can be removed + --> $DIR/implied_bounds_in_impl.rs:45:28 + | +LL | fn generics_same() -> impl GenericTrait + GenericSubtrait<(), i32, ()> {} + | ^^^^^^^^^^^^^^^^^ + +error: aborting due to 7 previous errors From 09506f49c13630765f636cf8a1ac58f95658dd68 Mon Sep 17 00:00:00 2001 From: y21 <30553356+y21@users.noreply.github.com> Date: Wed, 23 Aug 2023 15:48:26 +0200 Subject: [PATCH 3/4] rename lint, docs, improve diagnostics --- CHANGELOG.md | 2 +- clippy_lints/src/declared_lints.rs | 2 +- clippy_lints/src/implied_bounds_in_impl.rs | 161 --------------- clippy_lints/src/implied_bounds_in_impls.rs | 185 ++++++++++++++++++ clippy_lints/src/lib.rs | 4 +- tests/ui/implied_bounds_in_impl.stderr | 46 ----- tests/ui/implied_bounds_in_impls.fixed | 70 +++++++ ..._in_impl.rs => implied_bounds_in_impls.rs} | 31 ++- tests/ui/implied_bounds_in_impls.stderr | 99 ++++++++++ 9 files changed, 385 insertions(+), 215 deletions(-) delete mode 100644 clippy_lints/src/implied_bounds_in_impl.rs create mode 100644 clippy_lints/src/implied_bounds_in_impls.rs delete mode 100644 tests/ui/implied_bounds_in_impl.stderr create mode 100644 tests/ui/implied_bounds_in_impls.fixed rename tests/ui/{implied_bounds_in_impl.rs => implied_bounds_in_impls.rs} (62%) create mode 100644 tests/ui/implied_bounds_in_impls.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index 1ae44c502dbb..583bc6754fb1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4985,7 +4985,7 @@ Released 2018-09-13 [`implicit_return`]: https://rust-lang.github.io/rust-clippy/master/index.html#implicit_return [`implicit_saturating_add`]: https://rust-lang.github.io/rust-clippy/master/index.html#implicit_saturating_add [`implicit_saturating_sub`]: https://rust-lang.github.io/rust-clippy/master/index.html#implicit_saturating_sub -[`implied_bounds_in_impl`]: https://rust-lang.github.io/rust-clippy/master/index.html#implied_bounds_in_impl +[`implied_bounds_in_impls`]: https://rust-lang.github.io/rust-clippy/master/index.html#implied_bounds_in_impls [`impossible_comparisons`]: https://rust-lang.github.io/rust-clippy/master/index.html#impossible_comparisons [`imprecise_flops`]: https://rust-lang.github.io/rust-clippy/master/index.html#imprecise_flops [`inconsistent_digit_grouping`]: https://rust-lang.github.io/rust-clippy/master/index.html#inconsistent_digit_grouping diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index ea8d804b423d..f73f1ed18f8b 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -209,7 +209,7 @@ crate::implicit_return::IMPLICIT_RETURN_INFO, crate::implicit_saturating_add::IMPLICIT_SATURATING_ADD_INFO, crate::implicit_saturating_sub::IMPLICIT_SATURATING_SUB_INFO, - crate::implied_bounds_in_impl::IMPLIED_BOUNDS_IN_IMPL_INFO, + crate::implied_bounds_in_impls::IMPLIED_BOUNDS_IN_IMPLS_INFO, crate::inconsistent_struct_constructor::INCONSISTENT_STRUCT_CONSTRUCTOR_INFO, crate::incorrect_impls::INCORRECT_CLONE_IMPL_ON_COPY_TYPE_INFO, crate::incorrect_impls::INCORRECT_PARTIAL_ORD_IMPL_ON_ORD_TYPE_INFO, diff --git a/clippy_lints/src/implied_bounds_in_impl.rs b/clippy_lints/src/implied_bounds_in_impl.rs deleted file mode 100644 index 3f9f5daa6a46..000000000000 --- a/clippy_lints/src/implied_bounds_in_impl.rs +++ /dev/null @@ -1,161 +0,0 @@ -use clippy_utils::diagnostics::span_lint; -use rustc_hir::def_id::LocalDefId; -use rustc_hir::intravisit::FnKind; -use rustc_hir::{Body, FnDecl, FnRetTy, GenericArg, GenericBound, ItemKind, TraitBoundModifier, TyKind}; -use rustc_hir_analysis::hir_ty_to_ty; -use rustc_lint::{LateContext, LateLintPass}; -use rustc_middle::ty::{self, ClauseKind, TyCtxt}; -use rustc_session::{declare_lint_pass, declare_tool_lint}; -use rustc_span::Span; - -declare_clippy_lint! { - /// ### What it does - /// Looks for bounds in `impl Trait` in return position that are implied by other bounds. - /// This is usually the case when a supertrait is explicitly specified, when it is already implied - /// by a subtrait (e.g. `DerefMut: Deref`, so specifying `Deref` is unnecessary when `DerefMut` is specified). - /// - /// ### Why is this bad? - /// Unnecessary complexity. - /// - /// ### Known problems - /// This lint does not transitively look for implied bounds past the first supertrait. - /// - /// ### Example - /// ```rust - /// # use std::ops::{Deref,DerefMut}; - /// fn f() -> impl Deref + DerefMut { - /// // ^^^^^^^^^^^^^^^^^^^ unnecessary bound, already implied by the `DerefMut` trait bound - /// Box::new(123) - /// } - /// ``` - /// Use instead: - /// ```rust - /// # use std::ops::{Deref,DerefMut}; - /// fn f() -> impl DerefMut { - /// Box::new(123) - /// } - /// ``` - #[clippy::version = "1.73.0"] - pub IMPLIED_BOUNDS_IN_IMPL, - complexity, - "specifying bounds that are implied by other bounds in `impl Trait` type" -} -declare_lint_pass!(ImpliedBoundsInImpl => [IMPLIED_BOUNDS_IN_IMPL]); - -/// This function tries to, for all type parameters in a supertype predicate `GenericTrait`, -/// check if the substituted type in the implied-by bound matches with what's subtituted in the -/// implied type. -/// -/// Consider this example function. -/// ```rust,ignore -/// trait GenericTrait {} -/// trait GenericSubTrait: GenericTrait {} -/// ^ trait_predicate_args: [Self#0, U#2] -/// impl GenericTrait for () {} -/// impl GenericSubTrait<(), i32, ()> for () {} -/// impl GenericSubTrait<(), [u8; 8], ()> for () {} -/// -/// fn f() -> impl GenericTrait + GenericSubTrait<(), [u8; 8], ()> { -/// ^^^ implied_args ^^^^^^^^^^^^^^^ implied_by_args -/// (we are interested in `[u8; 8]` specifically, as that -/// is what `U` in `GenericTrait` is substituted with) -/// () -/// } -/// ``` -/// Here i32 != [u8; 8], so this will return false. -fn is_same_generics( - tcx: TyCtxt<'_>, - trait_predicate_args: &[ty::GenericArg<'_>], - implied_by_args: &[GenericArg<'_>], - implied_args: &[GenericArg<'_>], -) -> bool { - trait_predicate_args - .iter() - .enumerate() - .skip(1) // skip `Self` implicit arg - .all(|(arg_index, arg)| { - if let Some(ty) = arg.as_type() - && let &ty::Param(ty::ParamTy{ index, .. }) = ty.kind() - // Since `trait_predicate_args` and type params in traits start with `Self=0` - // and generic argument lists `GenericTrait` don't have `Self`, - // we need to subtract 1 from the index. - && let GenericArg::Type(ty_a) = implied_by_args[index as usize - 1] - && let GenericArg::Type(ty_b) = implied_args[arg_index - 1] - { - hir_ty_to_ty(tcx, ty_a) == hir_ty_to_ty(tcx, ty_b) - } else { - false - } - }) -} - -impl LateLintPass<'_> for ImpliedBoundsInImpl { - fn check_fn( - &mut self, - cx: &LateContext<'_>, - _: FnKind<'_>, - decl: &FnDecl<'_>, - _: &Body<'_>, - _: Span, - _: LocalDefId, - ) { - if let FnRetTy::Return(ty) = decl.output { - if let TyKind::OpaqueDef(item_id, ..) = ty.kind - && let item = cx.tcx.hir().item(item_id) - && let ItemKind::OpaqueTy(opaque_ty) = item.kind - { - // Very often there is only a single bound, e.g. `impl Deref<..>`, in which case - // we can avoid doing a bunch of stuff unnecessarily. - if opaque_ty.bounds.is_empty() { - return; - } - - // Get all the (implied) trait predicates in the bounds. - // For `impl Deref + DerefMut` this will contain [`Deref`]. - // The implied `Deref` comes from `DerefMut` because `trait DerefMut: Deref {}`. - - // N.B. (G)ATs are fine to disregard, because they must be the same for all of its supertraits. - // Example: - // `impl Deref + DerefMut` is not allowed. - // `DerefMut::Target` needs to match `Deref::Target` - let implied_bounds: Vec<_> = opaque_ty.bounds.iter().filter_map(|bound| { - if let GenericBound::Trait(poly_trait, TraitBoundModifier::None) = bound - && let [.., path] = poly_trait.trait_ref.path.segments - && poly_trait.bound_generic_params.is_empty() - && let Some(trait_def_id) = path.res.opt_def_id() - && let predicates = cx.tcx.super_predicates_of(trait_def_id).predicates - && !predicates.is_empty() // If the trait has no supertrait, there is nothing to add. - { - Some((path.args.map_or([].as_slice(), |a| a.args), predicates)) - } else { - None - } - }).collect(); - - // Lint all bounds in the `impl Trait` type that are also in the `implied_bounds` vec. - // 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 bound in opaque_ty.bounds { - 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 Some(def_id) = poly_trait.trait_ref.path.res.opt_def_id() - && implied_bounds.iter().any(|(implied_by_args, preds)| { - preds.iter().any(|(clause, _)| { - if let ClauseKind::Trait(tr) = clause.kind().skip_binder() - && tr.def_id() == def_id - { - is_same_generics(cx.tcx, tr.trait_ref.args, implied_by_args, implied_args) - } else { - false - } - }) - }) - { - span_lint(cx, IMPLIED_BOUNDS_IN_IMPL, poly_trait.span, "this bound is implied by another bound and can be removed"); - } - } - } - } - } -} diff --git a/clippy_lints/src/implied_bounds_in_impls.rs b/clippy_lints/src/implied_bounds_in_impls.rs new file mode 100644 index 000000000000..7ba467a3e06c --- /dev/null +++ b/clippy_lints/src/implied_bounds_in_impls.rs @@ -0,0 +1,185 @@ +use clippy_utils::diagnostics::span_lint_and_then; +use clippy_utils::source::snippet; +use rustc_hir::def_id::LocalDefId; +use rustc_hir::intravisit::FnKind; +use rustc_hir::{Body, FnDecl, FnRetTy, GenericArg, GenericBound, ItemKind, TraitBoundModifier, TyKind}; +use rustc_hir_analysis::hir_ty_to_ty; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_errors::{Applicability, SuggestionStyle}; +use rustc_middle::ty::{self, ClauseKind, TyCtxt}; +use rustc_session::{declare_lint_pass, declare_tool_lint}; +use rustc_span::Span; + +declare_clippy_lint! { + /// ### What it does + /// Looks for bounds in `impl Trait` in return position that are implied by other bounds. + /// This can happen when a trait is specified that another trait already has as a supertrait + /// (e.g. `fn() -> impl Deref + DerefMut` has an unnecessary `Deref` bound, + /// because `Deref` is a supertrait of `DerefMut`) + /// + /// ### Why is this bad? + /// Specifying more bounds than necessary adds needless complexity for the reader. + /// + /// ### Limitations + /// This lint does not check for implied bounds transitively. Meaning that + /// it does't check for implied bounds from supertraits of supertraits + /// (e.g. `trait A {} trait B: A {} trait C: B {}`, then having an `fn() -> impl A + C`) + /// + /// ### Example + /// ```rust + /// # use std::ops::{Deref,DerefMut}; + /// fn f() -> impl Deref + DerefMut { + /// // ^^^^^^^^^^^^^^^^^^^ unnecessary bound, already implied by the `DerefMut` trait bound + /// Box::new(123) + /// } + /// ``` + /// Use instead: + /// ```rust + /// # use std::ops::{Deref,DerefMut}; + /// fn f() -> impl DerefMut { + /// Box::new(123) + /// } + /// ``` + #[clippy::version = "1.73.0"] + pub IMPLIED_BOUNDS_IN_IMPLS, + complexity, + "specifying bounds that are implied by other bounds in `impl Trait` type" +} +declare_lint_pass!(ImpliedBoundsInImpls => [IMPLIED_BOUNDS_IN_IMPLS]); + +/// This function tries to, for all type parameters in a supertype predicate `GenericTrait`, +/// check if the substituted type in the implied-by bound matches with what's subtituted in the +/// implied type. +/// +/// Consider this example. +/// ```rust,ignore +/// trait GenericTrait {} +/// trait GenericSubTrait: GenericTrait {} +/// ^ trait_predicate_args: [Self#0, U#2] +/// impl GenericTrait for () {} +/// impl GenericSubTrait<(), i32, ()> for () {} +/// impl GenericSubTrait<(), [u8; 8], ()> for () {} +/// +/// fn f() -> impl GenericTrait + GenericSubTrait<(), [u8; 8], ()> { +/// ^^^ implied_args ^^^^^^^^^^^^^^^ implied_by_args +/// (we are interested in `[u8; 8]` specifically, as that +/// is what `U` in `GenericTrait` is substituted with) +/// () +/// } +/// ``` +/// Here i32 != [u8; 8], so this will return false. +fn is_same_generics( + tcx: TyCtxt<'_>, + trait_predicate_args: &[ty::GenericArg<'_>], + implied_by_args: &[GenericArg<'_>], + implied_args: &[GenericArg<'_>], +) -> bool { + trait_predicate_args + .iter() + .enumerate() + .skip(1) // skip `Self` implicit arg + .all(|(arg_index, arg)| { + if let Some(ty) = arg.as_type() + && let &ty::Param(ty::ParamTy{ index, .. }) = ty.kind() + // Since `trait_predicate_args` and type params in traits start with `Self=0` + // and generic argument lists `GenericTrait` don't have `Self`, + // we need to subtract 1 from the index. + && let GenericArg::Type(ty_a) = implied_by_args[index as usize - 1] + && let GenericArg::Type(ty_b) = implied_args[arg_index - 1] + { + hir_ty_to_ty(tcx, ty_a) == hir_ty_to_ty(tcx, ty_b) + } else { + false + } + }) +} + +impl LateLintPass<'_> for ImpliedBoundsInImpls { + fn check_fn( + &mut self, + cx: &LateContext<'_>, + _: FnKind<'_>, + decl: &FnDecl<'_>, + _: &Body<'_>, + _: Span, + _: LocalDefId, + ) { + 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 + // 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 + { + // Get all the (implied) trait predicates in the bounds. + // For `impl Deref + DerefMut` this will contain [`Deref`]. + // The implied `Deref` comes from `DerefMut` because `trait DerefMut: Deref {}`. + // N.B. (G)ATs are fine to disregard, because they must be the same for all of its supertraits. + // Example: + // `impl Deref + DerefMut` is not allowed. + // `DerefMut::Target` needs to match `Deref::Target`. + let implied_bounds: Vec<_> = opaque_ty.bounds.iter().filter_map(|bound| { + if let GenericBound::Trait(poly_trait, TraitBoundModifier::None) = bound + && let [.., path] = poly_trait.trait_ref.path.segments + && poly_trait.bound_generic_params.is_empty() + && let Some(trait_def_id) = path.res.opt_def_id() + && let predicates = cx.tcx.super_predicates_of(trait_def_id).predicates + && !predicates.is_empty() // If the trait has no supertrait, there is nothing to add. + { + Some((bound.span(), path.args.map_or([].as_slice(), |a| a.args), predicates)) + } else { + None + } + }).collect(); + + // Lint all bounds in the `impl Trait` type that are also in the `implied_bounds` vec. + // 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 Some(def_id) = poly_trait.trait_ref.path.res.opt_def_id() + && let Some(implied_by_span) = implied_bounds.iter().find_map(|&(span, implied_by_args, preds)| { + preds.iter().find_map(|(clause, _)| { + if let ClauseKind::Trait(tr) = clause.kind().skip_binder() + && tr.def_id() == def_id + && is_same_generics(cx.tcx, tr.trait_ref.args, implied_by_args, implied_args) + { + Some(span) + } else { + None + } + }) + }) + { + let implied_by = snippet(cx, implied_by_span, ".."); + span_lint_and_then( + cx, IMPLIED_BOUNDS_IN_IMPLS, + poly_trait.span, + &format!("this bound is already specified as the supertrait of `{}`", implied_by), + |diag| { + // If we suggest removing a bound, we may also need extend the span + // to include the `+` token, so we don't end up with something like `impl + B` + + let implied_span_extended = if let Some(next_bound) = opaque_ty.bounds.get(index + 1) { + poly_trait.span.to(next_bound.span().shrink_to_lo()) + } else { + poly_trait.span + }; + + diag.span_suggestion_with_style( + implied_span_extended, + "try removing this bound", + "", + Applicability::MachineApplicable, + SuggestionStyle::ShowAlways + ); + } + ); + } + } + } + } +} diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index d4fcddc66c5c..ab71bfbdc587 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -152,7 +152,7 @@ mod implicit_return; mod implicit_saturating_add; mod implicit_saturating_sub; -mod implied_bounds_in_impl; +mod implied_bounds_in_impls; mod inconsistent_struct_constructor; mod incorrect_impls; mod index_refutable_slice; @@ -1098,7 +1098,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_late_pass(|_| Box::new(redundant_locals::RedundantLocals)); store.register_late_pass(|_| Box::new(ignored_unit_patterns::IgnoredUnitPatterns)); store.register_late_pass(|_| Box::::default()); - store.register_late_pass(|_| Box::new(implied_bounds_in_impl::ImpliedBoundsInImpl)); + store.register_late_pass(|_| Box::new(implied_bounds_in_impls::ImpliedBoundsInImpls)); // add lints here, do not remove this comment, it's used in `new_lint` } diff --git a/tests/ui/implied_bounds_in_impl.stderr b/tests/ui/implied_bounds_in_impl.stderr deleted file mode 100644 index 92d412e784f3..000000000000 --- a/tests/ui/implied_bounds_in_impl.stderr +++ /dev/null @@ -1,46 +0,0 @@ -error: this bound is implied by another bound and can be removed - --> $DIR/implied_bounds_in_impl.rs:12:36 - | -LL | fn deref_derefmut(x: T) -> impl Deref + DerefMut { - | ^^^^^^^^^^^^^^^^^ - | - = note: `-D clippy::implied-bounds-in-impl` implied by `-D warnings` - -error: this bound is implied by another bound and can be removed - --> $DIR/implied_bounds_in_impl.rs:27:34 - | -LL | fn generics_implied() -> impl GenericTrait + GenericSubtrait<(), T, ()> - | ^^^^^^^^^^^^^^^ - -error: this bound is implied by another bound and can be removed - --> $DIR/implied_bounds_in_impl.rs:33:40 - | -LL | fn generics_implied_multi() -> impl GenericTrait + GenericTrait2 + GenericSubtrait<(), i32, V> {} - | ^^^^^^^^^^^^^^^^^ - -error: this bound is implied by another bound and can be removed - --> $DIR/implied_bounds_in_impl.rs:33:60 - | -LL | fn generics_implied_multi() -> impl GenericTrait + GenericTrait2 + GenericSubtrait<(), i32, V> {} - | ^^^^^^^^^^^^^^^^ - -error: this bound is implied by another bound and can be removed - --> $DIR/implied_bounds_in_impl.rs:35:44 - | -LL | fn generics_implied_multi2() -> impl GenericTrait + GenericTrait2 + GenericSubtrait<(), T, V> - | ^^^^^^^^^^^^^^^ - -error: this bound is implied by another bound and can be removed - --> $DIR/implied_bounds_in_impl.rs:35:62 - | -LL | fn generics_implied_multi2() -> impl GenericTrait + GenericTrait2 + GenericSubtrait<(), T, V> - | ^^^^^^^^^^^^^^^^ - -error: this bound is implied by another bound and can be removed - --> $DIR/implied_bounds_in_impl.rs:45:28 - | -LL | fn generics_same() -> impl GenericTrait + GenericSubtrait<(), i32, ()> {} - | ^^^^^^^^^^^^^^^^^ - -error: aborting due to 7 previous errors - diff --git a/tests/ui/implied_bounds_in_impls.fixed b/tests/ui/implied_bounds_in_impls.fixed new file mode 100644 index 000000000000..72239afae4a3 --- /dev/null +++ b/tests/ui/implied_bounds_in_impls.fixed @@ -0,0 +1,70 @@ +#![warn(clippy::implied_bounds_in_impls)] +#![allow(dead_code)] +#![feature(return_position_impl_trait_in_trait)] + +use std::ops::{Deref, DerefMut}; + +// Only one bound, nothing to lint. +fn normal_deref(x: T) -> impl Deref { + Box::new(x) +} + +// Deref implied by DerefMut +fn deref_derefmut(x: T) -> impl DerefMut { + Box::new(x) +} + +trait GenericTrait {} +trait GenericTrait2 {} +// U is intentionally at a different "index" in GenericSubtrait than `T` is in GenericTrait, +// so this can be a good test to make sure that the calculations are right (no off-by-one errors, +// ...) +trait GenericSubtrait: GenericTrait + GenericTrait2 {} + +impl GenericTrait for () {} +impl GenericTrait for () {} +impl GenericTrait2 for () {} +impl GenericSubtrait<(), i32, V> for () {} +impl GenericSubtrait<(), i64, V> for () {} + +fn generics_implied() -> impl GenericSubtrait +where + (): GenericSubtrait, +{ +} + +fn generics_implied_multi() -> impl GenericSubtrait<(), i32, V> {} + +fn generics_implied_multi2() -> impl GenericSubtrait<(), T, V> +where + (): GenericSubtrait<(), T, V> + GenericTrait, +{ +} + +// i32 != i64, GenericSubtrait<_, i64, _> does not imply GenericTrait, don't lint +fn generics_different() -> impl GenericTrait + GenericSubtrait<(), i64, ()> {} + +// i32 == i32, GenericSubtrait<_, i32, _> does imply GenericTrait, lint +fn generics_same() -> impl GenericSubtrait<(), i32, ()> {} + +trait SomeTrait { + // Check that it works in trait declarations. + fn f() -> impl DerefMut { + Box::new(0) + } +} +struct SomeStruct; +impl SomeStruct { + // Check that it works in inherent impl blocks. + fn f() -> impl DerefMut { + Box::new(123) + } +} +impl SomeTrait for SomeStruct { + // Check that it works in trait impls. + fn f() -> impl DerefMut { + Box::new(42) + } +} + +fn main() {} diff --git a/tests/ui/implied_bounds_in_impl.rs b/tests/ui/implied_bounds_in_impls.rs similarity index 62% rename from tests/ui/implied_bounds_in_impl.rs rename to tests/ui/implied_bounds_in_impls.rs index 0bc4b193d533..d59241d9ad13 100644 --- a/tests/ui/implied_bounds_in_impl.rs +++ b/tests/ui/implied_bounds_in_impls.rs @@ -1,5 +1,6 @@ -#![warn(clippy::implied_bounds_in_impl)] +#![warn(clippy::implied_bounds_in_impls)] #![allow(dead_code)] +#![feature(return_position_impl_trait_in_trait)] use std::ops::{Deref, DerefMut}; @@ -15,7 +16,9 @@ fn deref_derefmut(x: T) -> impl Deref + DerefMut { trait GenericTrait {} trait GenericTrait2 {} -// U is intentionally at a different "index" in GenericSubtrait than `T` is in GenericTrait +// U is intentionally at a different "index" in GenericSubtrait than `T` is in GenericTrait, +// so this can be a good test to make sure that the calculations are right (no off-by-one errors, +// ...) trait GenericSubtrait: GenericTrait + GenericTrait2 {} impl GenericTrait for () {} @@ -24,9 +27,9 @@ impl GenericTrait2 for () {} impl GenericSubtrait<(), i32, V> for () {} impl GenericSubtrait<(), i64, V> for () {} -fn generics_implied() -> impl GenericTrait + GenericSubtrait<(), T, ()> +fn generics_implied() -> impl GenericTrait + GenericSubtrait where - (): GenericSubtrait<(), T, ()>, + (): GenericSubtrait, { } @@ -44,4 +47,24 @@ fn generics_different() -> impl GenericTrait + GenericSubtrait<(), i64, ()> // i32 == i32, GenericSubtrait<_, i32, _> does imply GenericTrait, lint fn generics_same() -> impl GenericTrait + GenericSubtrait<(), i32, ()> {} +trait SomeTrait { + // Check that it works in trait declarations. + fn f() -> impl Deref + DerefMut { + Box::new(0) + } +} +struct SomeStruct; +impl SomeStruct { + // Check that it works in inherent impl blocks. + fn f() -> impl DerefMut { + Box::new(123) + } +} +impl SomeTrait for SomeStruct { + // Check that it works in trait impls. + fn f() -> impl DerefMut { + Box::new(42) + } +} + fn main() {} diff --git a/tests/ui/implied_bounds_in_impls.stderr b/tests/ui/implied_bounds_in_impls.stderr new file mode 100644 index 000000000000..86ce64a47fd1 --- /dev/null +++ b/tests/ui/implied_bounds_in_impls.stderr @@ -0,0 +1,99 @@ +error: this bound is already specified as the supertrait of `DerefMut` + --> $DIR/implied_bounds_in_impls.rs:13:36 + | +LL | fn deref_derefmut(x: T) -> impl Deref + DerefMut { + | ^^^^^^^^^^^^^^^^^ + | + = note: `-D clippy::implied-bounds-in-impls` implied by `-D warnings` +help: try removing this bound + | +LL - fn deref_derefmut(x: T) -> impl Deref + DerefMut { +LL + fn deref_derefmut(x: T) -> impl DerefMut { + | + +error: this bound is already specified as the supertrait of `GenericSubtrait` + --> $DIR/implied_bounds_in_impls.rs:30:37 + | +LL | fn generics_implied() -> impl GenericTrait + GenericSubtrait + | ^^^^^^^^^^^^^^^ + | +help: try removing this bound + | +LL - fn generics_implied() -> impl GenericTrait + GenericSubtrait +LL + fn generics_implied() -> impl GenericSubtrait + | + +error: this bound is already specified as the supertrait of `GenericSubtrait<(), i32, V>` + --> $DIR/implied_bounds_in_impls.rs:36:40 + | +LL | fn generics_implied_multi() -> impl GenericTrait + GenericTrait2 + GenericSubtrait<(), i32, V> {} + | ^^^^^^^^^^^^^^^^^ + | +help: try removing this bound + | +LL - fn generics_implied_multi() -> impl GenericTrait + GenericTrait2 + GenericSubtrait<(), i32, V> {} +LL + fn generics_implied_multi() -> impl GenericTrait2 + GenericSubtrait<(), i32, V> {} + | + +error: this bound is already specified as the supertrait of `GenericSubtrait<(), i32, V>` + --> $DIR/implied_bounds_in_impls.rs:36:60 + | +LL | fn generics_implied_multi() -> impl GenericTrait + GenericTrait2 + GenericSubtrait<(), i32, V> {} + | ^^^^^^^^^^^^^^^^ + | +help: try removing this bound + | +LL - fn generics_implied_multi() -> impl GenericTrait + GenericTrait2 + GenericSubtrait<(), i32, V> {} +LL + fn generics_implied_multi() -> impl GenericTrait + GenericSubtrait<(), i32, V> {} + | + +error: this bound is already specified as the supertrait of `GenericSubtrait<(), T, V>` + --> $DIR/implied_bounds_in_impls.rs:38:44 + | +LL | fn generics_implied_multi2() -> impl GenericTrait + GenericTrait2 + GenericSubtrait<(), T, V> + | ^^^^^^^^^^^^^^^ + | +help: try removing this bound + | +LL - fn generics_implied_multi2() -> impl GenericTrait + GenericTrait2 + GenericSubtrait<(), T, V> +LL + fn generics_implied_multi2() -> impl GenericTrait2 + GenericSubtrait<(), T, V> + | + +error: this bound is already specified as the supertrait of `GenericSubtrait<(), T, V>` + --> $DIR/implied_bounds_in_impls.rs:38:62 + | +LL | fn generics_implied_multi2() -> impl GenericTrait + GenericTrait2 + GenericSubtrait<(), T, V> + | ^^^^^^^^^^^^^^^^ + | +help: try removing this bound + | +LL - fn generics_implied_multi2() -> impl GenericTrait + GenericTrait2 + GenericSubtrait<(), T, V> +LL + fn generics_implied_multi2() -> impl GenericTrait + GenericSubtrait<(), T, V> + | + +error: this bound is already specified as the supertrait of `GenericSubtrait<(), i32, ()>` + --> $DIR/implied_bounds_in_impls.rs:48:28 + | +LL | fn generics_same() -> impl GenericTrait + GenericSubtrait<(), i32, ()> {} + | ^^^^^^^^^^^^^^^^^ + | +help: try removing this bound + | +LL - fn generics_same() -> impl GenericTrait + GenericSubtrait<(), i32, ()> {} +LL + fn generics_same() -> impl GenericSubtrait<(), i32, ()> {} + | + +error: this bound is already specified as the supertrait of `DerefMut` + --> $DIR/implied_bounds_in_impls.rs:52:20 + | +LL | fn f() -> impl Deref + DerefMut { + | ^^^^^ + | +help: try removing this bound + | +LL - fn f() -> impl Deref + DerefMut { +LL + fn f() -> impl DerefMut { + | + +error: aborting due to 8 previous errors + From 12275713d5e1540ddde6a6854b106c537270e9de Mon Sep 17 00:00:00 2001 From: y21 <30553356+y21@users.noreply.github.com> Date: Wed, 23 Aug 2023 16:48:12 +0200 Subject: [PATCH 4/4] support inherent impls and trait impls --- clippy_lints/src/implied_bounds_in_impls.rs | 173 +++++++++++--------- tests/ui/implied_bounds_in_impls.fixed | 4 +- tests/ui/implied_bounds_in_impls.rs | 8 +- tests/ui/implied_bounds_in_impls.stderr | 26 ++- 4 files changed, 124 insertions(+), 87 deletions(-) diff --git a/clippy_lints/src/implied_bounds_in_impls.rs b/clippy_lints/src/implied_bounds_in_impls.rs index 7ba467a3e06c..c6d1acad3a0f 100644 --- a/clippy_lints/src/implied_bounds_in_impls.rs +++ b/clippy_lints/src/implied_bounds_in_impls.rs @@ -1,11 +1,14 @@ use clippy_utils::diagnostics::span_lint_and_then; use clippy_utils::source::snippet; +use rustc_errors::{Applicability, SuggestionStyle}; use rustc_hir::def_id::LocalDefId; use rustc_hir::intravisit::FnKind; -use rustc_hir::{Body, FnDecl, FnRetTy, GenericArg, GenericBound, ItemKind, TraitBoundModifier, TyKind}; +use rustc_hir::{ + Body, FnDecl, FnRetTy, GenericArg, GenericBound, ImplItem, ImplItemKind, ItemKind, TraitBoundModifier, TraitItem, + TraitItemKind, TyKind, +}; use rustc_hir_analysis::hir_ty_to_ty; use rustc_lint::{LateContext, LateLintPass}; -use rustc_errors::{Applicability, SuggestionStyle}; use rustc_middle::ty::{self, ClauseKind, TyCtxt}; use rustc_session::{declare_lint_pass, declare_tool_lint}; use rustc_span::Span; @@ -94,6 +97,86 @@ fn is_same_generics( }) } +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 + // 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 + { + // Get all the (implied) trait predicates in the bounds. + // For `impl Deref + DerefMut` this will contain [`Deref`]. + // The implied `Deref` comes from `DerefMut` because `trait DerefMut: Deref {}`. + // N.B. (G)ATs are fine to disregard, because they must be the same for all of its supertraits. + // Example: + // `impl Deref + DerefMut` is not allowed. + // `DerefMut::Target` needs to match `Deref::Target`. + let implied_bounds: Vec<_> = opaque_ty.bounds.iter().filter_map(|bound| { + if let GenericBound::Trait(poly_trait, TraitBoundModifier::None) = bound + && let [.., path] = poly_trait.trait_ref.path.segments + && poly_trait.bound_generic_params.is_empty() + && let Some(trait_def_id) = path.res.opt_def_id() + && let predicates = cx.tcx.super_predicates_of(trait_def_id).predicates + && !predicates.is_empty() // If the trait has no supertrait, there is nothing to add. + { + Some((bound.span(), path.args.map_or([].as_slice(), |a| a.args), predicates)) + } else { + None + } + }).collect(); + + // Lint all bounds in the `impl Trait` type that are also in the `implied_bounds` vec. + // 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 Some(def_id) = poly_trait.trait_ref.path.res.opt_def_id() + && let Some(implied_by_span) = implied_bounds.iter().find_map(|&(span, implied_by_args, preds)| { + preds.iter().find_map(|(clause, _)| { + if let ClauseKind::Trait(tr) = clause.kind().skip_binder() + && tr.def_id() == def_id + && is_same_generics(cx.tcx, tr.trait_ref.args, implied_by_args, implied_args) + { + Some(span) + } else { + None + } + }) + }) + { + let implied_by = snippet(cx, implied_by_span, ".."); + span_lint_and_then( + cx, IMPLIED_BOUNDS_IN_IMPLS, + poly_trait.span, + &format!("this bound is already specified as the supertrait of `{implied_by}`"), + |diag| { + // If we suggest removing a bound, we may also need extend the span + // to include the `+` token, so we don't end up with something like `impl + B` + + let implied_span_extended = if let Some(next_bound) = opaque_ty.bounds.get(index + 1) { + poly_trait.span.to(next_bound.span().shrink_to_lo()) + } else { + poly_trait.span + }; + + diag.span_suggestion_with_style( + implied_span_extended, + "try removing this bound", + "", + Applicability::MachineApplicable, + SuggestionStyle::ShowAlways + ); + } + ); + } + } + } +} + impl LateLintPass<'_> for ImpliedBoundsInImpls { fn check_fn( &mut self, @@ -104,82 +187,16 @@ fn check_fn( _: Span, _: LocalDefId, ) { - 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 - // 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 - { - // Get all the (implied) trait predicates in the bounds. - // For `impl Deref + DerefMut` this will contain [`Deref`]. - // The implied `Deref` comes from `DerefMut` because `trait DerefMut: Deref {}`. - // N.B. (G)ATs are fine to disregard, because they must be the same for all of its supertraits. - // Example: - // `impl Deref + DerefMut` is not allowed. - // `DerefMut::Target` needs to match `Deref::Target`. - let implied_bounds: Vec<_> = opaque_ty.bounds.iter().filter_map(|bound| { - if let GenericBound::Trait(poly_trait, TraitBoundModifier::None) = bound - && let [.., path] = poly_trait.trait_ref.path.segments - && poly_trait.bound_generic_params.is_empty() - && let Some(trait_def_id) = path.res.opt_def_id() - && let predicates = cx.tcx.super_predicates_of(trait_def_id).predicates - && !predicates.is_empty() // If the trait has no supertrait, there is nothing to add. - { - Some((bound.span(), path.args.map_or([].as_slice(), |a| a.args), predicates)) - } else { - None - } - }).collect(); - - // Lint all bounds in the `impl Trait` type that are also in the `implied_bounds` vec. - // 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 Some(def_id) = poly_trait.trait_ref.path.res.opt_def_id() - && let Some(implied_by_span) = implied_bounds.iter().find_map(|&(span, implied_by_args, preds)| { - preds.iter().find_map(|(clause, _)| { - if let ClauseKind::Trait(tr) = clause.kind().skip_binder() - && tr.def_id() == def_id - && is_same_generics(cx.tcx, tr.trait_ref.args, implied_by_args, implied_args) - { - Some(span) - } else { - None - } - }) - }) - { - let implied_by = snippet(cx, implied_by_span, ".."); - span_lint_and_then( - cx, IMPLIED_BOUNDS_IN_IMPLS, - poly_trait.span, - &format!("this bound is already specified as the supertrait of `{}`", implied_by), - |diag| { - // If we suggest removing a bound, we may also need extend the span - // to include the `+` token, so we don't end up with something like `impl + B` - - let implied_span_extended = if let Some(next_bound) = opaque_ty.bounds.get(index + 1) { - poly_trait.span.to(next_bound.span().shrink_to_lo()) - } else { - poly_trait.span - }; - - diag.span_suggestion_with_style( - implied_span_extended, - "try removing this bound", - "", - Applicability::MachineApplicable, - SuggestionStyle::ShowAlways - ); - } - ); - } - } + 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); } } } diff --git a/tests/ui/implied_bounds_in_impls.fixed b/tests/ui/implied_bounds_in_impls.fixed index 72239afae4a3..952c2b70619b 100644 --- a/tests/ui/implied_bounds_in_impls.fixed +++ b/tests/ui/implied_bounds_in_impls.fixed @@ -49,9 +49,7 @@ fn generics_same() -> impl GenericSubtrait<(), i32, ()> {} trait SomeTrait { // Check that it works in trait declarations. - fn f() -> impl DerefMut { - Box::new(0) - } + fn f() -> impl DerefMut; } struct SomeStruct; impl SomeStruct { diff --git a/tests/ui/implied_bounds_in_impls.rs b/tests/ui/implied_bounds_in_impls.rs index d59241d9ad13..475f2621bbf6 100644 --- a/tests/ui/implied_bounds_in_impls.rs +++ b/tests/ui/implied_bounds_in_impls.rs @@ -49,20 +49,18 @@ fn generics_same() -> impl GenericTrait + GenericSubtrait<(), i32, ()> {} trait SomeTrait { // Check that it works in trait declarations. - fn f() -> impl Deref + DerefMut { - Box::new(0) - } + fn f() -> impl Deref + DerefMut; } struct SomeStruct; impl SomeStruct { // Check that it works in inherent impl blocks. - fn f() -> impl DerefMut { + fn f() -> impl Deref + DerefMut { Box::new(123) } } impl SomeTrait for SomeStruct { // Check that it works in trait impls. - fn f() -> impl DerefMut { + fn f() -> impl Deref + DerefMut { Box::new(42) } } diff --git a/tests/ui/implied_bounds_in_impls.stderr b/tests/ui/implied_bounds_in_impls.stderr index 86ce64a47fd1..8dffc674444d 100644 --- a/tests/ui/implied_bounds_in_impls.stderr +++ b/tests/ui/implied_bounds_in_impls.stderr @@ -86,6 +86,18 @@ LL + fn generics_same() -> impl GenericSubtrait<(), i32, ()> {} error: this bound is already specified as the supertrait of `DerefMut` --> $DIR/implied_bounds_in_impls.rs:52: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` + --> $DIR/implied_bounds_in_impls.rs:57:20 + | LL | fn f() -> impl Deref + DerefMut { | ^^^^^ | @@ -95,5 +107,17 @@ LL - fn f() -> impl Deref + DerefMut { LL + fn f() -> impl DerefMut { | -error: aborting due to 8 previous errors +error: this bound is already specified as the supertrait of `DerefMut` + --> $DIR/implied_bounds_in_impls.rs:63:20 + | +LL | fn f() -> impl Deref + DerefMut { + | ^^^^^ + | +help: try removing this bound + | +LL - fn f() -> impl Deref + DerefMut { +LL + fn f() -> impl DerefMut { + | + +error: aborting due to 10 previous errors