new lint: implied_bounds_in_impl

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

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,

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<Target = i32> + DerefMut<Target = i32> {
+    /// //             ^^^^^^^^^^^^^^^^^^^ unnecessary bound, already implied by the `DerefMut` trait bound
+    ///     Box::new(123)
+    /// }
+    /// ```
+    /// Use instead:
+    /// ```rust
+    /// fn f() -> impl DerefMut<Target = i32> {
+    ///     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<Target = i32> + DerefMut<Target = u32>` 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::<Vec<_>>();
+
+                // 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");
+                    }
+                }
+            }
+        }
+    }
+}

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::<reserve_after_initialization::ReserveAfterInitialization>::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`
 }
 

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> {}
+// 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<U, T>: Trait1<T> {}
+impl Trait1<i32> for () {}
+impl Trait1<String> for () {}
+impl Trait2<u32, i32> for () {}
+impl Trait2<u32, String> for () {}
+
+// Deref implied by DerefMut
+fn deref_derefmut<T>(x: T) -> impl Deref<Target = T> + DerefMut<Target = T> {
+    Box::new(x)
+}
+
+// Note: no test for different associated types needed since that isn't allowed in the first place.
+// E.g. `-> impl Deref<Target = T> + DerefMut<Target = U>` 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<i32>`.
+// (Generic traits are currently not linted anyway but once/if ever implemented this should not
+// warn.)
+fn different_generics() -> impl Trait1<i32> + Trait2<u32, String> {
+    /* () */
+}
+
+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() {}

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<T>(x: T) -> impl Deref<Target = T> + DerefMut<Target = T> {
+   |                                    ^^^^^^^^^^^^^^^^^
+   |
+   = 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
+