|
| 1 | +use clippy_utils::diagnostics::span_lint_and_then; |
| 2 | +use clippy_utils::source::snippet; |
| 3 | +use rustc_hir::def_id::LocalDefId; |
| 4 | +use rustc_hir::intravisit::FnKind; |
| 5 | +use rustc_hir::{Body, FnDecl, FnRetTy, GenericArg, GenericBound, ItemKind, TraitBoundModifier, TyKind}; |
| 6 | +use rustc_hir_analysis::hir_ty_to_ty; |
| 7 | +use rustc_lint::{LateContext, LateLintPass}; |
| 8 | +use rustc_errors::{Applicability, SuggestionStyle}; |
| 9 | +use rustc_middle::ty::{self, ClauseKind, TyCtxt}; |
| 10 | +use rustc_session::{declare_lint_pass, declare_tool_lint}; |
| 11 | +use rustc_span::Span; |
| 12 | + |
| 13 | +declare_clippy_lint! { |
| 14 | + /// ### What it does |
| 15 | + /// Looks for bounds in `impl Trait` in return position that are implied by other bounds. |
| 16 | + /// This can happen when a trait is specified that another trait already has as a supertrait |
| 17 | + /// (e.g. `fn() -> impl Deref + DerefMut<Target = i32>` has an unnecessary `Deref` bound, |
| 18 | + /// because `Deref` is a supertrait of `DerefMut`) |
| 19 | + /// |
| 20 | + /// ### Why is this bad? |
| 21 | + /// Specifying more bounds than necessary adds needless complexity for the reader. |
| 22 | + /// |
| 23 | + /// ### Limitations |
| 24 | + /// This lint does not check for implied bounds transitively. Meaning that |
| 25 | + /// it does't check for implied bounds from supertraits of supertraits |
| 26 | + /// (e.g. `trait A {} trait B: A {} trait C: B {}`, then having an `fn() -> impl A + C`) |
| 27 | + /// |
| 28 | + /// ### Example |
| 29 | + /// ```rust |
| 30 | + /// # use std::ops::{Deref,DerefMut}; |
| 31 | + /// fn f() -> impl Deref<Target = i32> + DerefMut<Target = i32> { |
| 32 | + /// // ^^^^^^^^^^^^^^^^^^^ unnecessary bound, already implied by the `DerefMut` trait bound |
| 33 | + /// Box::new(123) |
| 34 | + /// } |
| 35 | + /// ``` |
| 36 | + /// Use instead: |
| 37 | + /// ```rust |
| 38 | + /// # use std::ops::{Deref,DerefMut}; |
| 39 | + /// fn f() -> impl DerefMut<Target = i32> { |
| 40 | + /// Box::new(123) |
| 41 | + /// } |
| 42 | + /// ``` |
| 43 | + #[clippy::version = "1.73.0"] |
| 44 | + pub IMPLIED_BOUNDS_IN_IMPLS, |
| 45 | + complexity, |
| 46 | + "specifying bounds that are implied by other bounds in `impl Trait` type" |
| 47 | +} |
| 48 | +declare_lint_pass!(ImpliedBoundsInImpls => [IMPLIED_BOUNDS_IN_IMPLS]); |
| 49 | + |
| 50 | +/// This function tries to, for all type parameters in a supertype predicate `GenericTrait<U>`, |
| 51 | +/// check if the substituted type in the implied-by bound matches with what's subtituted in the |
| 52 | +/// implied type. |
| 53 | +/// |
| 54 | +/// Consider this example. |
| 55 | +/// ```rust,ignore |
| 56 | +/// trait GenericTrait<T> {} |
| 57 | +/// trait GenericSubTrait<T, U, V>: GenericTrait<U> {} |
| 58 | +/// ^ trait_predicate_args: [Self#0, U#2] |
| 59 | +/// impl GenericTrait<i32> for () {} |
| 60 | +/// impl GenericSubTrait<(), i32, ()> for () {} |
| 61 | +/// impl GenericSubTrait<(), [u8; 8], ()> for () {} |
| 62 | +/// |
| 63 | +/// fn f() -> impl GenericTrait<i32> + GenericSubTrait<(), [u8; 8], ()> { |
| 64 | +/// ^^^ implied_args ^^^^^^^^^^^^^^^ implied_by_args |
| 65 | +/// (we are interested in `[u8; 8]` specifically, as that |
| 66 | +/// is what `U` in `GenericTrait<U>` is substituted with) |
| 67 | +/// () |
| 68 | +/// } |
| 69 | +/// ``` |
| 70 | +/// Here i32 != [u8; 8], so this will return false. |
| 71 | +fn is_same_generics( |
| 72 | + tcx: TyCtxt<'_>, |
| 73 | + trait_predicate_args: &[ty::GenericArg<'_>], |
| 74 | + implied_by_args: &[GenericArg<'_>], |
| 75 | + implied_args: &[GenericArg<'_>], |
| 76 | +) -> bool { |
| 77 | + trait_predicate_args |
| 78 | + .iter() |
| 79 | + .enumerate() |
| 80 | + .skip(1) // skip `Self` implicit arg |
| 81 | + .all(|(arg_index, arg)| { |
| 82 | + if let Some(ty) = arg.as_type() |
| 83 | + && let &ty::Param(ty::ParamTy{ index, .. }) = ty.kind() |
| 84 | + // Since `trait_predicate_args` and type params in traits start with `Self=0` |
| 85 | + // and generic argument lists `GenericTrait<i32>` don't have `Self`, |
| 86 | + // we need to subtract 1 from the index. |
| 87 | + && let GenericArg::Type(ty_a) = implied_by_args[index as usize - 1] |
| 88 | + && let GenericArg::Type(ty_b) = implied_args[arg_index - 1] |
| 89 | + { |
| 90 | + hir_ty_to_ty(tcx, ty_a) == hir_ty_to_ty(tcx, ty_b) |
| 91 | + } else { |
| 92 | + false |
| 93 | + } |
| 94 | + }) |
| 95 | +} |
| 96 | + |
| 97 | +impl LateLintPass<'_> for ImpliedBoundsInImpls { |
| 98 | + fn check_fn( |
| 99 | + &mut self, |
| 100 | + cx: &LateContext<'_>, |
| 101 | + _: FnKind<'_>, |
| 102 | + decl: &FnDecl<'_>, |
| 103 | + _: &Body<'_>, |
| 104 | + _: Span, |
| 105 | + _: LocalDefId, |
| 106 | + ) { |
| 107 | + if let FnRetTy::Return(ty) = decl.output |
| 108 | + &&let TyKind::OpaqueDef(item_id, ..) = ty.kind |
| 109 | + && let item = cx.tcx.hir().item(item_id) |
| 110 | + && let ItemKind::OpaqueTy(opaque_ty) = item.kind |
| 111 | + // Very often there is only a single bound, e.g. `impl Deref<..>`, in which case |
| 112 | + // we can avoid doing a bunch of stuff unnecessarily. |
| 113 | + && opaque_ty.bounds.len() > 1 |
| 114 | + { |
| 115 | + // Get all the (implied) trait predicates in the bounds. |
| 116 | + // For `impl Deref + DerefMut` this will contain [`Deref`]. |
| 117 | + // The implied `Deref` comes from `DerefMut` because `trait DerefMut: Deref {}`. |
| 118 | + // N.B. (G)ATs are fine to disregard, because they must be the same for all of its supertraits. |
| 119 | + // Example: |
| 120 | + // `impl Deref<Target = i32> + DerefMut<Target = u32>` is not allowed. |
| 121 | + // `DerefMut::Target` needs to match `Deref::Target`. |
| 122 | + let implied_bounds: Vec<_> = opaque_ty.bounds.iter().filter_map(|bound| { |
| 123 | + if let GenericBound::Trait(poly_trait, TraitBoundModifier::None) = bound |
| 124 | + && let [.., path] = poly_trait.trait_ref.path.segments |
| 125 | + && poly_trait.bound_generic_params.is_empty() |
| 126 | + && let Some(trait_def_id) = path.res.opt_def_id() |
| 127 | + && let predicates = cx.tcx.super_predicates_of(trait_def_id).predicates |
| 128 | + && !predicates.is_empty() // If the trait has no supertrait, there is nothing to add. |
| 129 | + { |
| 130 | + Some((bound.span(), path.args.map_or([].as_slice(), |a| a.args), predicates)) |
| 131 | + } else { |
| 132 | + None |
| 133 | + } |
| 134 | + }).collect(); |
| 135 | + |
| 136 | + // Lint all bounds in the `impl Trait` type that are also in the `implied_bounds` vec. |
| 137 | + // This involves some extra logic when generic arguments are present, since |
| 138 | + // simply comparing trait `DefId`s won't be enough. We also need to compare the generics. |
| 139 | + for (index, bound) in opaque_ty.bounds.iter().enumerate() { |
| 140 | + if let GenericBound::Trait(poly_trait, TraitBoundModifier::None) = bound |
| 141 | + && let [.., path] = poly_trait.trait_ref.path.segments |
| 142 | + && let implied_args = path.args.map_or([].as_slice(), |a| a.args) |
| 143 | + && let Some(def_id) = poly_trait.trait_ref.path.res.opt_def_id() |
| 144 | + && let Some(implied_by_span) = implied_bounds.iter().find_map(|&(span, implied_by_args, preds)| { |
| 145 | + preds.iter().find_map(|(clause, _)| { |
| 146 | + if let ClauseKind::Trait(tr) = clause.kind().skip_binder() |
| 147 | + && tr.def_id() == def_id |
| 148 | + && is_same_generics(cx.tcx, tr.trait_ref.args, implied_by_args, implied_args) |
| 149 | + { |
| 150 | + Some(span) |
| 151 | + } else { |
| 152 | + None |
| 153 | + } |
| 154 | + }) |
| 155 | + }) |
| 156 | + { |
| 157 | + let implied_by = snippet(cx, implied_by_span, ".."); |
| 158 | + span_lint_and_then( |
| 159 | + cx, IMPLIED_BOUNDS_IN_IMPLS, |
| 160 | + poly_trait.span, |
| 161 | + &format!("this bound is already specified as the supertrait of `{}`", implied_by), |
| 162 | + |diag| { |
| 163 | + // If we suggest removing a bound, we may also need extend the span |
| 164 | + // to include the `+` token, so we don't end up with something like `impl + B` |
| 165 | + |
| 166 | + let implied_span_extended = if let Some(next_bound) = opaque_ty.bounds.get(index + 1) { |
| 167 | + poly_trait.span.to(next_bound.span().shrink_to_lo()) |
| 168 | + } else { |
| 169 | + poly_trait.span |
| 170 | + }; |
| 171 | + |
| 172 | + diag.span_suggestion_with_style( |
| 173 | + implied_span_extended, |
| 174 | + "try removing this bound", |
| 175 | + "", |
| 176 | + Applicability::MachineApplicable, |
| 177 | + SuggestionStyle::ShowAlways |
| 178 | + ); |
| 179 | + } |
| 180 | + ); |
| 181 | + } |
| 182 | + } |
| 183 | + } |
| 184 | + } |
| 185 | +} |
0 commit comments