Auto merge of rust-lang#12308 - y21:more_implied_bounds, r=xFrednet

Look for `implied_bounds_in_impls` in more positions

With this, we lint `impl Trait` implied bounds in more positions:
- Type alias impl trait
- Associated type position impl trait
- Argument position impl trait
  - these are not opaque types, but instead are desugared to `where` clauses, so we need extra logic for finding them (`check_generics`), however the rest of the logic is the same

Before this, we'd only lint RPIT `impl Trait`s.
"Hide whitespaces" and reviewing commits individually might make this easier

changelog: [`implied_bounds_in_impls`]: start linting implied bounds in APIT, ATPIT, TAIT

Author: bors

clippy_lints/src/implied_bounds_in_impls.rs

@@ -2,16 +2,14 @@ use clippy_utils::diagnostics::span_lint_and_then;
 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,
+    GenericArg, GenericBound, GenericBounds, ItemKind, PredicateOrigin, TraitBoundModifier, TyKind, TypeBinding,
+    WherePredicate,
 };
 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 +52,7 @@ declare_lint_pass!(ImpliedBoundsInImpls => [IMPLIED_BOUNDS_IN_IMPLS]);
 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 +71,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 +238,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<ImplTraitBound<'tcx>> {
-    opaque_ty
-        .bounds
+fn collect_supertrait_bounds<'tcx>(cx: &LateContext<'tcx>, bounds: GenericBounds<'tcx>) -> Vec<ImplTraitBound<'tcx>> {
+    bounds
         .iter()
         .filter_map(|bound| {
             if let GenericBound::Trait(poly_trait, TraitBoundModifier::None) = bound
@@ -295,64 +292,62 @@ 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);
+impl<'tcx> LateLintPass<'tcx> for ImpliedBoundsInImpls {
+    fn check_generics(&mut self, cx: &LateContext<'tcx>, generics: &rustc_hir::Generics<'tcx>) {
+        for predicate in generics.predicates {
+            if let WherePredicate::BoundPredicate(predicate) = predicate
+                // In theory, the origin doesn't really matter,
+                // we *could* also lint on explicit where clauses written out by the user,
+                // not just impl trait desugared ones, but that contradicts with the lint name...
+                && let PredicateOrigin::ImplTrait = predicate.origin
+            {
+                check(cx, predicate.bounds);
+            }
         }
     }
-    fn check_impl_item(&mut self, cx: &LateContext<'_>, item: &ImplItem<'_>) {
-        if let ImplItemKind::Fn(sig, ..) = &item.kind {
-            check(cx, sig.decl);
+
+    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);
         }
     }
 }

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<T = u32, U = String> {}
 }
 
+fn apit(_: impl 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() {}

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 issue11880() {
     fn f5() -> impl X<U = String> + Y<T = u32> {}
 }
 
+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() {}