]>
Commit | Line | Data |
---|---|---|
781aab86 FG |
1 | use clippy_utils::diagnostics::span_lint_and_then; |
2 | use clippy_utils::source::snippet; | |
3 | use rustc_errors::{Applicability, SuggestionStyle}; | |
4 | use rustc_hir::def_id::{DefId, LocalDefId}; | |
5 | use rustc_hir::intravisit::FnKind; | |
6 | use rustc_hir::{ | |
7 | Body, FnDecl, FnRetTy, GenericArg, GenericBound, ImplItem, ImplItemKind, ItemKind, TraitBoundModifier, TraitItem, | |
8 | TraitItemKind, TyKind, | |
9 | }; | |
10 | use rustc_hir_analysis::hir_ty_to_ty; | |
11 | use rustc_lint::{LateContext, LateLintPass}; | |
12 | use rustc_middle::ty::{self, ClauseKind, Generics, Ty, TyCtxt}; | |
13 | use rustc_session::{declare_lint_pass, declare_tool_lint}; | |
14 | use rustc_span::Span; | |
15 | ||
16 | declare_clippy_lint! { | |
17 | /// ### What it does | |
18 | /// Looks for bounds in `impl Trait` in return position that are implied by other bounds. | |
19 | /// This can happen when a trait is specified that another trait already has as a supertrait | |
20 | /// (e.g. `fn() -> impl Deref + DerefMut<Target = i32>` has an unnecessary `Deref` bound, | |
21 | /// because `Deref` is a supertrait of `DerefMut`) | |
22 | /// | |
23 | /// ### Why is this bad? | |
24 | /// Specifying more bounds than necessary adds needless complexity for the reader. | |
25 | /// | |
26 | /// ### Limitations | |
27 | /// This lint does not check for implied bounds transitively. Meaning that | |
28 | /// it does't check for implied bounds from supertraits of supertraits | |
29 | /// (e.g. `trait A {} trait B: A {} trait C: B {}`, then having an `fn() -> impl A + C`) | |
30 | /// | |
31 | /// ### Example | |
ed00b5ec | 32 | /// ```no_run |
781aab86 FG |
33 | /// # use std::ops::{Deref,DerefMut}; |
34 | /// fn f() -> impl Deref<Target = i32> + DerefMut<Target = i32> { | |
35 | /// // ^^^^^^^^^^^^^^^^^^^ unnecessary bound, already implied by the `DerefMut` trait bound | |
36 | /// Box::new(123) | |
37 | /// } | |
38 | /// ``` | |
39 | /// Use instead: | |
ed00b5ec | 40 | /// ```no_run |
781aab86 FG |
41 | /// # use std::ops::{Deref,DerefMut}; |
42 | /// fn f() -> impl DerefMut<Target = i32> { | |
43 | /// Box::new(123) | |
44 | /// } | |
45 | /// ``` | |
46 | #[clippy::version = "1.73.0"] | |
47 | pub IMPLIED_BOUNDS_IN_IMPLS, | |
48 | nursery, | |
49 | "specifying bounds that are implied by other bounds in `impl Trait` type" | |
50 | } | |
51 | declare_lint_pass!(ImpliedBoundsInImpls => [IMPLIED_BOUNDS_IN_IMPLS]); | |
52 | ||
53 | #[allow(clippy::too_many_arguments)] | |
54 | fn emit_lint( | |
55 | cx: &LateContext<'_>, | |
56 | poly_trait: &rustc_hir::PolyTraitRef<'_>, | |
57 | opaque_ty: &rustc_hir::OpaqueTy<'_>, | |
58 | index: usize, | |
59 | // The bindings that were implied | |
60 | implied_bindings: &[rustc_hir::TypeBinding<'_>], | |
61 | // The original bindings that `implied_bindings` are implied from | |
62 | implied_by_bindings: &[rustc_hir::TypeBinding<'_>], | |
63 | implied_by_args: &[GenericArg<'_>], | |
64 | implied_by_span: Span, | |
65 | ) { | |
66 | let implied_by = snippet(cx, implied_by_span, ".."); | |
67 | ||
68 | span_lint_and_then( | |
69 | cx, | |
70 | IMPLIED_BOUNDS_IN_IMPLS, | |
71 | poly_trait.span, | |
72 | &format!("this bound is already specified as the supertrait of `{implied_by}`"), | |
73 | |diag| { | |
74 | // If we suggest removing a bound, we may also need to extend the span | |
75 | // to include the `+` token that is ahead or behind, | |
76 | // so we don't end up with something like `impl + B` or `impl A + ` | |
77 | ||
78 | let implied_span_extended = if let Some(next_bound) = opaque_ty.bounds.get(index + 1) { | |
79 | poly_trait.span.to(next_bound.span().shrink_to_lo()) | |
80 | } else if index > 0 | |
81 | && let Some(prev_bound) = opaque_ty.bounds.get(index - 1) | |
82 | { | |
83 | prev_bound.span().shrink_to_hi().to(poly_trait.span.shrink_to_hi()) | |
84 | } else { | |
85 | poly_trait.span | |
86 | }; | |
87 | ||
88 | let mut sugg = vec![(implied_span_extended, String::new())]; | |
89 | ||
90 | // We also might need to include associated type binding that were specified in the implied bound, | |
91 | // but omitted in the implied-by bound: | |
92 | // `fn f() -> impl Deref<Target = u8> + DerefMut` | |
93 | // If we're going to suggest removing `Deref<..>`, we'll need to put `<Target = u8>` on `DerefMut` | |
94 | let omitted_assoc_tys: Vec<_> = implied_bindings | |
95 | .iter() | |
96 | .filter(|binding| !implied_by_bindings.iter().any(|b| b.ident == binding.ident)) | |
97 | .collect(); | |
98 | ||
99 | if !omitted_assoc_tys.is_empty() { | |
100 | // `<>` needs to be added if there aren't yet any generic arguments or bindings | |
101 | let needs_angle_brackets = implied_by_args.is_empty() && implied_by_bindings.is_empty(); | |
102 | let insert_span = match (implied_by_args, implied_by_bindings) { | |
103 | ([.., arg], [.., binding]) => arg.span().max(binding.span).shrink_to_hi(), | |
104 | ([.., arg], []) => arg.span().shrink_to_hi(), | |
105 | ([], [.., binding]) => binding.span.shrink_to_hi(), | |
106 | ([], []) => implied_by_span.shrink_to_hi(), | |
107 | }; | |
108 | ||
109 | let mut associated_tys_sugg = if needs_angle_brackets { | |
110 | "<".to_owned() | |
111 | } else { | |
112 | // If angle brackets aren't needed (i.e., there are already generic arguments or bindings), | |
113 | // we need to add a comma: | |
114 | // `impl A<B, C >` | |
115 | // ^ if we insert `Assoc=i32` without a comma here, that'd be invalid syntax: | |
116 | // `impl A<B, C Assoc=i32>` | |
117 | ", ".to_owned() | |
118 | }; | |
119 | ||
120 | for (index, binding) in omitted_assoc_tys.into_iter().enumerate() { | |
121 | if index > 0 { | |
122 | associated_tys_sugg += ", "; | |
123 | } | |
124 | associated_tys_sugg += &snippet(cx, binding.span, ".."); | |
125 | } | |
126 | if needs_angle_brackets { | |
127 | associated_tys_sugg += ">"; | |
128 | } | |
129 | sugg.push((insert_span, associated_tys_sugg)); | |
130 | } | |
131 | ||
132 | diag.multipart_suggestion_with_style( | |
133 | "try removing this bound", | |
134 | sugg, | |
135 | Applicability::MachineApplicable, | |
136 | SuggestionStyle::ShowAlways, | |
137 | ); | |
138 | }, | |
139 | ); | |
140 | } | |
141 | ||
142 | /// Tries to "resolve" a type. | |
143 | /// The index passed to this function must start with `Self=0`, i.e. it must be a valid | |
144 | /// type parameter index. | |
145 | /// If the index is out of bounds, it means that the generic parameter has a default type. | |
146 | fn try_resolve_type<'tcx>( | |
147 | tcx: TyCtxt<'tcx>, | |
148 | args: &'tcx [GenericArg<'tcx>], | |
149 | generics: &'tcx Generics, | |
150 | index: usize, | |
151 | ) -> Option<Ty<'tcx>> { | |
152 | match args.get(index - 1) { | |
153 | Some(GenericArg::Type(ty)) => Some(hir_ty_to_ty(tcx, ty)), | |
154 | Some(_) => None, | |
155 | None => Some(tcx.type_of(generics.params[index].def_id).skip_binder()), | |
156 | } | |
157 | } | |
158 | ||
159 | /// This function tries to, for all generic type parameters in a supertrait predicate `trait ...<U>: | |
160 | /// GenericTrait<U>`, check if the substituted type in the implied-by bound matches with what's | |
161 | /// subtituted in the implied bound. | |
162 | /// | |
163 | /// Consider this example. | |
164 | /// ```rust,ignore | |
165 | /// trait GenericTrait<T> {} | |
166 | /// trait GenericSubTrait<T, U, V>: GenericTrait<U> {} | |
167 | /// ^^^^^^^^^^^^^^^ trait_predicate_args: [Self#0, U#2] | |
168 | /// (the Self#0 is implicit: `<Self as GenericTrait<U>>`) | |
169 | /// impl GenericTrait<i32> for () {} | |
170 | /// impl GenericSubTrait<(), i32, ()> for () {} | |
171 | /// impl GenericSubTrait<(), i64, ()> for () {} | |
172 | /// | |
173 | /// fn f() -> impl GenericTrait<i32> + GenericSubTrait<(), i64, ()> { | |
174 | /// ^^^ implied_args ^^^^^^^^^^^ implied_by_args | |
175 | /// (we are interested in `i64` specifically, as that | |
176 | /// is what `U` in `GenericTrait<U>` is substituted with) | |
177 | /// } | |
178 | /// ``` | |
179 | /// Here i32 != i64, so this will return false. | |
180 | fn is_same_generics<'tcx>( | |
181 | tcx: TyCtxt<'tcx>, | |
182 | trait_predicate_args: &'tcx [ty::GenericArg<'tcx>], | |
183 | implied_by_args: &'tcx [GenericArg<'tcx>], | |
184 | implied_args: &'tcx [GenericArg<'tcx>], | |
185 | implied_by_def_id: DefId, | |
186 | implied_def_id: DefId, | |
187 | ) -> bool { | |
188 | // Get the generics of the two traits to be able to get default generic parameter. | |
189 | let implied_by_generics = tcx.generics_of(implied_by_def_id); | |
190 | let implied_generics = tcx.generics_of(implied_def_id); | |
191 | ||
192 | trait_predicate_args | |
193 | .iter() | |
194 | .enumerate() | |
195 | .skip(1) // skip `Self` implicit arg | |
196 | .all(|(arg_index, arg)| { | |
197 | if let Some(ty) = arg.as_type() { | |
198 | if let &ty::Param(ty::ParamTy { index, .. }) = ty.kind() | |
199 | // `index == 0` means that it's referring to `Self`, | |
200 | // in which case we don't try to substitute it | |
201 | && index != 0 | |
202 | && let Some(ty_a) = try_resolve_type(tcx, implied_by_args, implied_by_generics, index as usize) | |
203 | && let Some(ty_b) = try_resolve_type(tcx, implied_args, implied_generics, arg_index) | |
204 | { | |
205 | ty_a == ty_b | |
206 | } else if let Some(ty_b) = try_resolve_type(tcx, implied_args, implied_generics, arg_index) { | |
207 | ty == ty_b | |
208 | } else { | |
209 | false | |
210 | } | |
211 | } else { | |
212 | false | |
213 | } | |
214 | }) | |
215 | } | |
216 | ||
217 | fn check(cx: &LateContext<'_>, decl: &FnDecl<'_>) { | |
218 | if let FnRetTy::Return(ty) = decl.output | |
219 | &&let TyKind::OpaqueDef(item_id, ..) = ty.kind | |
220 | && let item = cx.tcx.hir().item(item_id) | |
221 | && let ItemKind::OpaqueTy(opaque_ty) = item.kind | |
222 | // Very often there is only a single bound, e.g. `impl Deref<..>`, in which case | |
223 | // we can avoid doing a bunch of stuff unnecessarily. | |
224 | && opaque_ty.bounds.len() > 1 | |
225 | { | |
226 | // Get all the (implied) trait predicates in the bounds. | |
227 | // For `impl Deref + DerefMut` this will contain [`Deref`]. | |
228 | // The implied `Deref` comes from `DerefMut` because `trait DerefMut: Deref {}`. | |
229 | // N.B. (G)ATs are fine to disregard, because they must be the same for all of its supertraits. | |
230 | // Example: | |
231 | // `impl Deref<Target = i32> + DerefMut<Target = u32>` is not allowed. | |
232 | // `DerefMut::Target` needs to match `Deref::Target`. | |
ed00b5ec FG |
233 | let implied_bounds: Vec<_> = opaque_ty |
234 | .bounds | |
235 | .iter() | |
236 | .filter_map(|bound| { | |
237 | if let GenericBound::Trait(poly_trait, TraitBoundModifier::None) = bound | |
238 | && let [.., path] = poly_trait.trait_ref.path.segments | |
239 | && poly_trait.bound_generic_params.is_empty() | |
240 | && let Some(trait_def_id) = path.res.opt_def_id() | |
241 | && let predicates = cx.tcx.super_predicates_of(trait_def_id).predicates | |
242 | && !predicates.is_empty() | |
243 | // If the trait has no supertrait, there is nothing to add. | |
244 | { | |
245 | Some((bound.span(), path, predicates, trait_def_id)) | |
246 | } else { | |
247 | None | |
248 | } | |
249 | }) | |
250 | .collect(); | |
781aab86 FG |
251 | |
252 | // Lint all bounds in the `impl Trait` type that are also in the `implied_bounds` vec. | |
253 | // This involves some extra logic when generic arguments are present, since | |
254 | // simply comparing trait `DefId`s won't be enough. We also need to compare the generics. | |
255 | for (index, bound) in opaque_ty.bounds.iter().enumerate() { | |
256 | if let GenericBound::Trait(poly_trait, TraitBoundModifier::None) = bound | |
257 | && let [.., path] = poly_trait.trait_ref.path.segments | |
258 | && let implied_args = path.args.map_or([].as_slice(), |a| a.args) | |
259 | && let implied_bindings = path.args.map_or([].as_slice(), |a| a.bindings) | |
260 | && let Some(def_id) = poly_trait.trait_ref.path.res.opt_def_id() | |
ed00b5ec FG |
261 | && let Some((implied_by_span, implied_by_args, implied_by_bindings)) = |
262 | implied_bounds | |
263 | .iter() | |
264 | .find_map(|&(span, implied_by_path, preds, implied_by_def_id)| { | |
265 | let implied_by_args = implied_by_path.args.map_or([].as_slice(), |a| a.args); | |
266 | let implied_by_bindings = implied_by_path.args.map_or([].as_slice(), |a| a.bindings); | |
781aab86 | 267 | |
ed00b5ec FG |
268 | preds.iter().find_map(|(clause, _)| { |
269 | if let ClauseKind::Trait(tr) = clause.kind().skip_binder() | |
270 | && tr.def_id() == def_id | |
271 | && is_same_generics( | |
272 | cx.tcx, | |
273 | tr.trait_ref.args, | |
274 | implied_by_args, | |
275 | implied_args, | |
276 | implied_by_def_id, | |
277 | def_id, | |
278 | ) | |
279 | { | |
280 | Some((span, implied_by_args, implied_by_bindings)) | |
281 | } else { | |
282 | None | |
283 | } | |
284 | }) | |
781aab86 | 285 | }) |
781aab86 FG |
286 | { |
287 | emit_lint( | |
288 | cx, | |
289 | poly_trait, | |
290 | opaque_ty, | |
291 | index, | |
292 | implied_bindings, | |
293 | implied_by_bindings, | |
294 | implied_by_args, | |
ed00b5ec | 295 | implied_by_span, |
781aab86 FG |
296 | ); |
297 | } | |
298 | } | |
299 | } | |
300 | } | |
301 | ||
302 | impl LateLintPass<'_> for ImpliedBoundsInImpls { | |
303 | fn check_fn( | |
304 | &mut self, | |
305 | cx: &LateContext<'_>, | |
306 | _: FnKind<'_>, | |
307 | decl: &FnDecl<'_>, | |
308 | _: &Body<'_>, | |
309 | _: Span, | |
310 | _: LocalDefId, | |
311 | ) { | |
312 | check(cx, decl); | |
313 | } | |
314 | fn check_trait_item(&mut self, cx: &LateContext<'_>, item: &TraitItem<'_>) { | |
315 | if let TraitItemKind::Fn(sig, ..) = &item.kind { | |
316 | check(cx, sig.decl); | |
317 | } | |
318 | } | |
319 | fn check_impl_item(&mut self, cx: &LateContext<'_>, item: &ImplItem<'_>) { | |
320 | if let ImplItemKind::Fn(sig, ..) = &item.kind { | |
321 | check(cx, sig.decl); | |
322 | } | |
323 | } | |
324 | } |