]>
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}; | |
c620b35d | 4 | use rustc_hir::def_id::DefId; |
781aab86 | 5 | use rustc_hir::{ |
1f0639a9 FG |
6 | AssocItemConstraint, GenericArg, GenericBound, GenericBounds, ItemKind, PredicateOrigin, TraitBoundModifier, |
7 | TyKind, WherePredicate, | |
781aab86 | 8 | }; |
e8be2606 | 9 | use rustc_hir_analysis::lower_ty; |
781aab86 FG |
10 | use rustc_lint::{LateContext, LateLintPass}; |
11 | use rustc_middle::ty::{self, ClauseKind, Generics, Ty, TyCtxt}; | |
4b012472 | 12 | use rustc_session::declare_lint_pass; |
781aab86 FG |
13 | use rustc_span::Span; |
14 | ||
15 | declare_clippy_lint! { | |
16 | /// ### What it does | |
17 | /// Looks for bounds in `impl Trait` in return position that are implied by other bounds. | |
18 | /// This can happen when a trait is specified that another trait already has as a supertrait | |
19 | /// (e.g. `fn() -> impl Deref + DerefMut<Target = i32>` has an unnecessary `Deref` bound, | |
20 | /// because `Deref` is a supertrait of `DerefMut`) | |
21 | /// | |
22 | /// ### Why is this bad? | |
23 | /// Specifying more bounds than necessary adds needless complexity for the reader. | |
24 | /// | |
25 | /// ### Limitations | |
26 | /// This lint does not check for implied bounds transitively. Meaning that | |
31ef2f64 | 27 | /// it doesn't check for implied bounds from supertraits of supertraits |
781aab86 FG |
28 | /// (e.g. `trait A {} trait B: A {} trait C: B {}`, then having an `fn() -> impl A + C`) |
29 | /// | |
30 | /// ### Example | |
ed00b5ec | 31 | /// ```no_run |
781aab86 FG |
32 | /// # use std::ops::{Deref,DerefMut}; |
33 | /// fn f() -> impl Deref<Target = i32> + DerefMut<Target = i32> { | |
34 | /// // ^^^^^^^^^^^^^^^^^^^ unnecessary bound, already implied by the `DerefMut` trait bound | |
35 | /// Box::new(123) | |
36 | /// } | |
37 | /// ``` | |
38 | /// Use instead: | |
ed00b5ec | 39 | /// ```no_run |
781aab86 FG |
40 | /// # use std::ops::{Deref,DerefMut}; |
41 | /// fn f() -> impl DerefMut<Target = i32> { | |
42 | /// Box::new(123) | |
43 | /// } | |
44 | /// ``` | |
4b012472 | 45 | #[clippy::version = "1.74.0"] |
781aab86 | 46 | pub IMPLIED_BOUNDS_IN_IMPLS, |
4b012472 | 47 | complexity, |
781aab86 FG |
48 | "specifying bounds that are implied by other bounds in `impl Trait` type" |
49 | } | |
50 | declare_lint_pass!(ImpliedBoundsInImpls => [IMPLIED_BOUNDS_IN_IMPLS]); | |
51 | ||
781aab86 FG |
52 | fn emit_lint( |
53 | cx: &LateContext<'_>, | |
54 | poly_trait: &rustc_hir::PolyTraitRef<'_>, | |
c620b35d | 55 | bounds: GenericBounds<'_>, |
781aab86 | 56 | index: usize, |
31ef2f64 FG |
57 | // The constraints that were implied, used for suggestion purposes since removing a bound with |
58 | // associated types means we might need to then move it to a different bound. | |
59 | implied_constraints: &[AssocItemConstraint<'_>], | |
c620b35d | 60 | bound: &ImplTraitBound<'_>, |
781aab86 | 61 | ) { |
c620b35d | 62 | let implied_by = snippet(cx, bound.span, ".."); |
781aab86 FG |
63 | |
64 | span_lint_and_then( | |
65 | cx, | |
66 | IMPLIED_BOUNDS_IN_IMPLS, | |
67 | poly_trait.span, | |
e8be2606 | 68 | format!("this bound is already specified as the supertrait of `{implied_by}`"), |
781aab86 FG |
69 | |diag| { |
70 | // If we suggest removing a bound, we may also need to extend the span | |
71 | // to include the `+` token that is ahead or behind, | |
72 | // so we don't end up with something like `impl + B` or `impl A + ` | |
73 | ||
c620b35d | 74 | let implied_span_extended = if let Some(next_bound) = bounds.get(index + 1) { |
781aab86 FG |
75 | poly_trait.span.to(next_bound.span().shrink_to_lo()) |
76 | } else if index > 0 | |
c620b35d | 77 | && let Some(prev_bound) = bounds.get(index - 1) |
781aab86 FG |
78 | { |
79 | prev_bound.span().shrink_to_hi().to(poly_trait.span.shrink_to_hi()) | |
80 | } else { | |
81 | poly_trait.span | |
82 | }; | |
83 | ||
84 | let mut sugg = vec![(implied_span_extended, String::new())]; | |
85 | ||
1f0639a9 FG |
86 | // We also might need to include associated item constraints that were specified in the implied |
87 | // bound, but omitted in the implied-by bound: | |
781aab86 FG |
88 | // `fn f() -> impl Deref<Target = u8> + DerefMut` |
89 | // If we're going to suggest removing `Deref<..>`, we'll need to put `<Target = u8>` on `DerefMut` | |
31ef2f64 | 90 | let omitted_constraints: Vec<_> = implied_constraints |
781aab86 | 91 | .iter() |
31ef2f64 | 92 | .filter(|constraint| !bound.constraints.iter().any(|c| c.ident == constraint.ident)) |
781aab86 FG |
93 | .collect(); |
94 | ||
31ef2f64 FG |
95 | if !omitted_constraints.is_empty() { |
96 | // `<>` needs to be added if there aren't yet any generic arguments or constraints | |
97 | let needs_angle_brackets = bound.args.is_empty() && bound.constraints.is_empty(); | |
98 | let insert_span = match (bound.args, bound.constraints) { | |
99 | ([.., arg], [.., constraint]) => arg.span().max(constraint.span).shrink_to_hi(), | |
781aab86 | 100 | ([.., arg], []) => arg.span().shrink_to_hi(), |
31ef2f64 | 101 | ([], [.., constraint]) => constraint.span.shrink_to_hi(), |
c620b35d | 102 | ([], []) => bound.span.shrink_to_hi(), |
781aab86 FG |
103 | }; |
104 | ||
31ef2f64 | 105 | let mut constraints_sugg = if needs_angle_brackets { |
781aab86 FG |
106 | "<".to_owned() |
107 | } else { | |
31ef2f64 | 108 | // If angle brackets aren't needed (i.e., there are already generic arguments or constraints), |
781aab86 FG |
109 | // we need to add a comma: |
110 | // `impl A<B, C >` | |
111 | // ^ if we insert `Assoc=i32` without a comma here, that'd be invalid syntax: | |
112 | // `impl A<B, C Assoc=i32>` | |
113 | ", ".to_owned() | |
114 | }; | |
115 | ||
31ef2f64 | 116 | for (index, constraint) in omitted_constraints.into_iter().enumerate() { |
781aab86 | 117 | if index > 0 { |
31ef2f64 | 118 | constraints_sugg += ", "; |
781aab86 | 119 | } |
31ef2f64 | 120 | constraints_sugg += &snippet(cx, constraint.span, ".."); |
781aab86 FG |
121 | } |
122 | if needs_angle_brackets { | |
31ef2f64 | 123 | constraints_sugg += ">"; |
781aab86 | 124 | } |
31ef2f64 | 125 | sugg.push((insert_span, constraints_sugg)); |
781aab86 FG |
126 | } |
127 | ||
128 | diag.multipart_suggestion_with_style( | |
129 | "try removing this bound", | |
130 | sugg, | |
131 | Applicability::MachineApplicable, | |
132 | SuggestionStyle::ShowAlways, | |
133 | ); | |
134 | }, | |
135 | ); | |
136 | } | |
137 | ||
138 | /// Tries to "resolve" a type. | |
139 | /// The index passed to this function must start with `Self=0`, i.e. it must be a valid | |
140 | /// type parameter index. | |
141 | /// If the index is out of bounds, it means that the generic parameter has a default type. | |
142 | fn try_resolve_type<'tcx>( | |
143 | tcx: TyCtxt<'tcx>, | |
144 | args: &'tcx [GenericArg<'tcx>], | |
145 | generics: &'tcx Generics, | |
146 | index: usize, | |
147 | ) -> Option<Ty<'tcx>> { | |
148 | match args.get(index - 1) { | |
e8be2606 | 149 | Some(GenericArg::Type(ty)) => Some(lower_ty(tcx, ty)), |
781aab86 | 150 | Some(_) => None, |
31ef2f64 | 151 | None => Some(tcx.type_of(generics.own_params[index].def_id).skip_binder()), |
781aab86 FG |
152 | } |
153 | } | |
154 | ||
155 | /// This function tries to, for all generic type parameters in a supertrait predicate `trait ...<U>: | |
156 | /// GenericTrait<U>`, check if the substituted type in the implied-by bound matches with what's | |
c0240ec0 | 157 | /// substituted in the implied bound. |
781aab86 FG |
158 | /// |
159 | /// Consider this example. | |
160 | /// ```rust,ignore | |
161 | /// trait GenericTrait<T> {} | |
162 | /// trait GenericSubTrait<T, U, V>: GenericTrait<U> {} | |
163 | /// ^^^^^^^^^^^^^^^ trait_predicate_args: [Self#0, U#2] | |
164 | /// (the Self#0 is implicit: `<Self as GenericTrait<U>>`) | |
165 | /// impl GenericTrait<i32> for () {} | |
166 | /// impl GenericSubTrait<(), i32, ()> for () {} | |
167 | /// impl GenericSubTrait<(), i64, ()> for () {} | |
168 | /// | |
169 | /// fn f() -> impl GenericTrait<i32> + GenericSubTrait<(), i64, ()> { | |
170 | /// ^^^ implied_args ^^^^^^^^^^^ implied_by_args | |
171 | /// (we are interested in `i64` specifically, as that | |
172 | /// is what `U` in `GenericTrait<U>` is substituted with) | |
173 | /// } | |
174 | /// ``` | |
175 | /// Here i32 != i64, so this will return false. | |
176 | fn is_same_generics<'tcx>( | |
177 | tcx: TyCtxt<'tcx>, | |
178 | trait_predicate_args: &'tcx [ty::GenericArg<'tcx>], | |
179 | implied_by_args: &'tcx [GenericArg<'tcx>], | |
180 | implied_args: &'tcx [GenericArg<'tcx>], | |
181 | implied_by_def_id: DefId, | |
182 | implied_def_id: DefId, | |
183 | ) -> bool { | |
184 | // Get the generics of the two traits to be able to get default generic parameter. | |
185 | let implied_by_generics = tcx.generics_of(implied_by_def_id); | |
186 | let implied_generics = tcx.generics_of(implied_def_id); | |
187 | ||
188 | trait_predicate_args | |
189 | .iter() | |
190 | .enumerate() | |
191 | .skip(1) // skip `Self` implicit arg | |
192 | .all(|(arg_index, arg)| { | |
4b012472 FG |
193 | if [ |
194 | implied_by_generics.host_effect_index, | |
195 | implied_generics.host_effect_index, | |
196 | ] | |
197 | .contains(&Some(arg_index)) | |
198 | { | |
199 | // skip host effect params in determining whether generics are same | |
200 | return true; | |
201 | } | |
781aab86 FG |
202 | if let Some(ty) = arg.as_type() { |
203 | if let &ty::Param(ty::ParamTy { index, .. }) = ty.kind() | |
204 | // `index == 0` means that it's referring to `Self`, | |
205 | // in which case we don't try to substitute it | |
206 | && index != 0 | |
207 | && let Some(ty_a) = try_resolve_type(tcx, implied_by_args, implied_by_generics, index as usize) | |
208 | && let Some(ty_b) = try_resolve_type(tcx, implied_args, implied_generics, arg_index) | |
209 | { | |
210 | ty_a == ty_b | |
211 | } else if let Some(ty_b) = try_resolve_type(tcx, implied_args, implied_generics, arg_index) { | |
212 | ty == ty_b | |
213 | } else { | |
214 | false | |
215 | } | |
216 | } else { | |
217 | false | |
218 | } | |
219 | }) | |
220 | } | |
221 | ||
c620b35d FG |
222 | struct ImplTraitBound<'tcx> { |
223 | /// The span of the bound in the `impl Trait` type | |
224 | span: Span, | |
225 | /// The predicates defined in the trait referenced by this bound. This also contains the actual | |
226 | /// supertrait bounds | |
227 | predicates: &'tcx [(ty::Clause<'tcx>, Span)], | |
228 | /// The `DefId` of the trait being referenced by this bound | |
229 | trait_def_id: DefId, | |
230 | /// The generic arguments on the `impl Trait` bound | |
231 | args: &'tcx [GenericArg<'tcx>], | |
31ef2f64 FG |
232 | /// The associated item constraints of this bound |
233 | constraints: &'tcx [AssocItemConstraint<'tcx>], | |
c620b35d | 234 | } |
781aab86 | 235 | |
c620b35d FG |
236 | /// Given an `impl Trait` type, gets all the supertraits from each bound ("implied bounds"). |
237 | /// | |
238 | /// For `impl Deref + DerefMut + Eq` this returns `[Deref, PartialEq]`. | |
239 | /// The `Deref` comes from `DerefMut` because `trait DerefMut: Deref {}`, and `PartialEq` comes from | |
240 | /// `Eq`. | |
241 | fn collect_supertrait_bounds<'tcx>(cx: &LateContext<'tcx>, bounds: GenericBounds<'tcx>) -> Vec<ImplTraitBound<'tcx>> { | |
242 | bounds | |
243 | .iter() | |
244 | .filter_map(|bound| { | |
781aab86 FG |
245 | if let GenericBound::Trait(poly_trait, TraitBoundModifier::None) = bound |
246 | && let [.., path] = poly_trait.trait_ref.path.segments | |
c620b35d FG |
247 | && poly_trait.bound_generic_params.is_empty() |
248 | && let Some(trait_def_id) = path.res.opt_def_id() | |
1f0639a9 | 249 | && let predicates = cx.tcx.explicit_super_predicates_of(trait_def_id).predicates |
c620b35d FG |
250 | // If the trait has no supertrait, there is no need to collect anything from that bound |
251 | && !predicates.is_empty() | |
252 | { | |
253 | Some(ImplTraitBound { | |
254 | predicates, | |
255 | args: path.args.map_or([].as_slice(), |p| p.args), | |
31ef2f64 | 256 | constraints: path.args.map_or([].as_slice(), |p| p.constraints), |
c620b35d FG |
257 | trait_def_id, |
258 | span: bound.span(), | |
259 | }) | |
260 | } else { | |
261 | None | |
262 | } | |
263 | }) | |
264 | .collect() | |
265 | } | |
781aab86 | 266 | |
c620b35d FG |
267 | /// Given a bound in an `impl Trait` type, looks for a trait in the set of supertraits (previously |
268 | /// collected in [`collect_supertrait_bounds`]) that matches (same trait and generic arguments). | |
269 | fn find_bound_in_supertraits<'a, 'tcx>( | |
270 | cx: &LateContext<'tcx>, | |
271 | trait_def_id: DefId, | |
272 | args: &'tcx [GenericArg<'tcx>], | |
273 | bounds: &'a [ImplTraitBound<'tcx>], | |
274 | ) -> Option<&'a ImplTraitBound<'tcx>> { | |
275 | bounds.iter().find(|bound| { | |
276 | bound.predicates.iter().any(|(clause, _)| { | |
277 | if let ClauseKind::Trait(tr) = clause.kind().skip_binder() | |
278 | && tr.def_id() == trait_def_id | |
781aab86 | 279 | { |
c620b35d FG |
280 | is_same_generics( |
281 | cx.tcx, | |
282 | tr.trait_ref.args, | |
283 | bound.args, | |
284 | args, | |
285 | bound.trait_def_id, | |
286 | trait_def_id, | |
287 | ) | |
288 | } else { | |
289 | false | |
781aab86 | 290 | } |
c620b35d FG |
291 | }) |
292 | }) | |
293 | } | |
294 | ||
295 | fn check<'tcx>(cx: &LateContext<'tcx>, bounds: GenericBounds<'tcx>) { | |
296 | if bounds.len() == 1 { | |
297 | // Very often there is only a single bound, e.g. `impl Deref<..>`, in which case | |
298 | // we can avoid doing a bunch of stuff unnecessarily; there will trivially be | |
299 | // no duplicate bounds | |
300 | return; | |
301 | } | |
302 | ||
303 | let supertraits = collect_supertrait_bounds(cx, bounds); | |
304 | ||
305 | // Lint all bounds in the `impl Trait` type that we've previously also seen in the set of | |
306 | // supertraits of each of the bounds. | |
307 | // This involves some extra logic when generic arguments are present, since | |
308 | // simply comparing trait `DefId`s won't be enough. We also need to compare the generics. | |
309 | for (index, bound) in bounds.iter().enumerate() { | |
310 | if let GenericBound::Trait(poly_trait, TraitBoundModifier::None) = bound | |
311 | && let [.., path] = poly_trait.trait_ref.path.segments | |
312 | && let implied_args = path.args.map_or([].as_slice(), |a| a.args) | |
31ef2f64 | 313 | && let implied_constraints = path.args.map_or([].as_slice(), |a| a.constraints) |
c620b35d FG |
314 | && let Some(def_id) = poly_trait.trait_ref.path.res.opt_def_id() |
315 | && let Some(bound) = find_bound_in_supertraits(cx, def_id, implied_args, &supertraits) | |
316 | // If the implied bound has a type binding that also exists in the implied-by trait, | |
317 | // then we shouldn't lint. See #11880 for an example. | |
318 | && let assocs = cx.tcx.associated_items(bound.trait_def_id) | |
31ef2f64 | 319 | && !implied_constraints.iter().any(|constraint| { |
c620b35d | 320 | assocs |
31ef2f64 | 321 | .filter_by_name_unhygienic(constraint.ident.name) |
c620b35d FG |
322 | .next() |
323 | .is_some_and(|assoc| assoc.kind == ty::AssocKind::Type) | |
324 | }) | |
325 | { | |
31ef2f64 | 326 | emit_lint(cx, poly_trait, bounds, index, implied_constraints, bound); |
781aab86 FG |
327 | } |
328 | } | |
329 | } | |
330 | ||
c620b35d FG |
331 | impl<'tcx> LateLintPass<'tcx> for ImpliedBoundsInImpls { |
332 | fn check_generics(&mut self, cx: &LateContext<'tcx>, generics: &rustc_hir::Generics<'tcx>) { | |
333 | for predicate in generics.predicates { | |
334 | if let WherePredicate::BoundPredicate(predicate) = predicate | |
335 | // In theory, the origin doesn't really matter, | |
336 | // we *could* also lint on explicit where clauses written out by the user, | |
337 | // not just impl trait desugared ones, but that contradicts with the lint name... | |
338 | && let PredicateOrigin::ImplTrait = predicate.origin | |
339 | { | |
340 | check(cx, predicate.bounds); | |
341 | } | |
781aab86 FG |
342 | } |
343 | } | |
c620b35d FG |
344 | |
345 | fn check_ty(&mut self, cx: &LateContext<'_>, ty: &rustc_hir::Ty<'_>) { | |
346 | if let TyKind::OpaqueDef(item_id, ..) = ty.kind | |
347 | && let item = cx.tcx.hir().item(item_id) | |
348 | && let ItemKind::OpaqueTy(opaque_ty) = item.kind | |
349 | { | |
350 | check(cx, opaque_ty.bounds); | |
781aab86 FG |
351 | } |
352 | } | |
353 | } |