Auto merge of rust-lang#12269 - y21:implied_bounds_in_impls_refactor, r=xFrednet

Refactor `implied_bounds_in_impls` lint

Some refactors in `implied_bounds_in_impls` that I wanted to make while working on something else in that file, but I found them "large" enough that I didn't want them in the same PR and instead wanted them reviewed separately (since itd just be distracting).

This just splits up the two phases of "collect all the supertraits from each of the `impl Trait` bounds" and "find those `impl Trait` bounds that are mentioned in one of the previously-collected supertraits" into separate functions. Before, this was all in a single function.

Reviewing it commit by commit might make it easier. I can squash it down later.

changelog: none

Author: bors

clippy_lints/src/implied_bounds_in_impls.rs

@@ -1,16 +1,17 @@
 use clippy_utils::diagnostics::span_lint_and_then;
 use clippy_utils::source::snippet;
 use rustc_errors::{Applicability, SuggestionStyle};
-use rustc_hir::def_id::{DefId, LocalDefId};
+use rustc_hir::def_id::DefId;
 use rustc_hir::intravisit::FnKind;
 use rustc_hir::{
-    Body, FnDecl, FnRetTy, GenericArg, GenericBound, ImplItem, ImplItemKind, ItemKind, TraitBoundModifier, TraitItem,
-    TraitItemKind, TyKind,
+    Body, FnDecl, FnRetTy, GenericArg, GenericBound, ImplItem, ImplItemKind, ItemKind, OpaqueTy, TraitBoundModifier,
+    TraitItem, TraitItemKind, 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! {
@@ -50,20 +51,17 @@ declare_clippy_lint! {
 }
 declare_lint_pass!(ImpliedBoundsInImpls => [IMPLIED_BOUNDS_IN_IMPLS]);
 
-#[allow(clippy::too_many_arguments)]
 fn emit_lint(
     cx: &LateContext<'_>,
     poly_trait: &rustc_hir::PolyTraitRef<'_>,
     opaque_ty: &rustc_hir::OpaqueTy<'_>,
     index: usize,
-    // The bindings that were implied
+    // 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
     implied_bindings: &[rustc_hir::TypeBinding<'_>],
-    // The original bindings that `implied_bindings` are implied from
-    implied_by_bindings: &[rustc_hir::TypeBinding<'_>],
-    implied_by_args: &[GenericArg<'_>],
-    implied_by_span: Span,
+    bound: &ImplTraitBound<'_>,
 ) {
-    let implied_by = snippet(cx, implied_by_span, "..");
+    let implied_by = snippet(cx, bound.span, "..");
 
     span_lint_and_then(
         cx,
@@ -93,17 +91,17 @@ fn emit_lint(
             // If we're going to suggest removing `Deref<..>`, we'll need to put `<Target = u8>` on `DerefMut`
             let omitted_assoc_tys: Vec<_> = implied_bindings
                 .iter()
-                .filter(|binding| !implied_by_bindings.iter().any(|b| b.ident == binding.ident))
+                .filter(|binding| !bound.bindings.iter().any(|b| b.ident == binding.ident))
                 .collect();
 
             if !omitted_assoc_tys.is_empty() {
                 // `<>` needs to be added if there aren't yet any generic arguments or bindings
-                let needs_angle_brackets = implied_by_args.is_empty() && implied_by_bindings.is_empty();
-                let insert_span = match (implied_by_args, implied_by_bindings) {
+                let needs_angle_brackets = bound.args.is_empty() && bound.bindings.is_empty();
+                let insert_span = match (bound.args, bound.bindings) {
                     ([.., arg], [.., binding]) => arg.span().max(binding.span).shrink_to_hi(),
                     ([.., arg], []) => arg.span().shrink_to_hi(),
                     ([], [.., binding]) => binding.span.shrink_to_hi(),
-                    ([], []) => implied_by_span.shrink_to_hi(),
+                    ([], []) => bound.span.shrink_to_hi(),
                 };
 
                 let mut associated_tys_sugg = if needs_angle_brackets {
@@ -223,42 +221,93 @@ fn is_same_generics<'tcx>(
         })
 }
 
+struct ImplTraitBound<'tcx> {
+    /// The span of the bound in the `impl Trait` type
+    span: Span,
+    /// The predicates defined in the trait referenced by this bound. This also contains the actual
+    /// supertrait bounds
+    predicates: &'tcx [(ty::Clause<'tcx>, Span)],
+    /// The `DefId` of the trait being referenced by this bound
+    trait_def_id: DefId,
+    /// The generic arguments on the `impl Trait` bound
+    args: &'tcx [GenericArg<'tcx>],
+    /// The associated types on this bound
+    bindings: &'tcx [TypeBinding<'tcx>],
+}
+
+/// Given an `impl Trait` type, gets all the supertraits from each bound ("implied bounds").
+///
+/// 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
+        .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
+                // If the trait has no supertrait, there is no need to collect anything from that bound
+                && !predicates.is_empty()
+            {
+                Some(ImplTraitBound {
+                    predicates,
+                    args: path.args.map_or([].as_slice(), |p| p.args),
+                    bindings: path.args.map_or([].as_slice(), |p| p.bindings),
+                    trait_def_id,
+                    span: bound.span(),
+                })
+            } else {
+                None
+            }
+        })
+        .collect()
+}
+
+/// Given a bound in an `impl Trait` type, looks for a trait in the set of supertraits (previously
+/// collected in [`collect_supertrait_bounds`]) that matches (same trait and generic arguments).
+fn find_bound_in_supertraits<'a, 'tcx>(
+    cx: &LateContext<'tcx>,
+    trait_def_id: DefId,
+    args: &'tcx [GenericArg<'tcx>],
+    bounds: &'a [ImplTraitBound<'tcx>],
+) -> Option<&'a ImplTraitBound<'tcx>> {
+    bounds.iter().find(|bound| {
+        bound.predicates.iter().any(|(clause, _)| {
+            if let ClauseKind::Trait(tr) = clause.kind().skip_binder()
+                && tr.def_id() == trait_def_id
+            {
+                is_same_generics(
+                    cx.tcx,
+                    tr.trait_ref.args,
+                    bound.args,
+                    args,
+                    bound.trait_def_id,
+                    trait_def_id,
+                )
+            } else {
+                false
+            }
+        })
+    })
+}
+
 fn check(cx: &LateContext<'_>, decl: &FnDecl<'_>) {
     if let FnRetTy::Return(ty) = decl.output
-        &&let TyKind::OpaqueDef(item_id, ..) = ty.kind
+        && 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<Target = i32> + DerefMut<Target = u32>` 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, predicates, trait_def_id))
-                } else {
-                    None
-                }
-            })
-            .collect();
+        let supertraits = collect_supertrait_bounds(cx, opaque_ty);
 
-        // Lint all bounds in the `impl Trait` type that are also in the `implied_bounds` vec.
+        // 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() {
@@ -267,42 +316,9 @@ fn check(cx: &LateContext<'_>, decl: &FnDecl<'_>) {
                 && 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((implied_by_span, implied_by_args, implied_by_bindings)) =
-                    implied_bounds
-                        .iter()
-                        .find_map(|&(span, implied_by_path, preds, implied_by_def_id)| {
-                            let implied_by_args = implied_by_path.args.map_or([].as_slice(), |a| a.args);
-                            let implied_by_bindings = implied_by_path.args.map_or([].as_slice(), |a| a.bindings);
-
-                            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,
-                                        implied_by_def_id,
-                                        def_id,
-                                    )
-                                {
-                                    Some((span, implied_by_args, implied_by_bindings))
-                                } else {
-                                    None
-                                }
-                            })
-                        })
+                && let Some(bound) = find_bound_in_supertraits(cx, def_id, implied_args, &supertraits)
             {
-                emit_lint(
-                    cx,
-                    poly_trait,
-                    opaque_ty,
-                    index,
-                    implied_bindings,
-                    implied_by_bindings,
-                    implied_by_args,
-                    implied_by_span,
-                );
+                emit_lint(cx, poly_trait, opaque_ty, index, implied_bindings, bound);
             }
         }
     }