diff --git a/CHANGELOG.md b/CHANGELOG.md index 4e33cb7b457..1ae44c502db 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 1be9720fbbf..ea8d804b423 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 00000000000..c83153e5350 --- /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 f50019f3cf7..d4fcddc66c5 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 00000000000..516f21fc795 --- /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 00000000000..2709f727acc --- /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 +