add lint: could_be_assoc_type_bounds
#13738
Conversation
|
r? @Alexendoo rustbot has assigned @Alexendoo. Use |
rustbot
added
the
S-waiting-on-review
| { | ||
| } | ||
|
|
||
| fn ty_param<T: Iterator<Item: Clone>, >() {} |
There was a problem hiding this comment.
One case where the lint relies on rustfmt to cleanup. Naively removing both the leading or trailing comma would create overlapping spans when trying to remove two bounds next to each other (removing the middle comma in T: Copy, U: Copy twice), so that's why it only tries to remove trailing commas
|
☔ The latest upstream changes (presumably a5e46a6) made this pull request unmergeable. Please resolve the merge conflicts. |
|
☔ The latest upstream changes (possibly d28d234) made this pull request unmergeable. Please resolve the merge conflicts. |
rustbot
added
S-waiting-on-author
y21
added
S-waiting-on-review
| && let ty_param_def_id = ty_param_def_id.expect_local() | ||
| && let ty_param_bound_at = cx.tcx.parent(ty_param_def_id.to_def_id()).expect_local() |
There was a problem hiding this comment.
I can't think of any situation where these would panic, but to be safe we can use as_local
| if emission.predicate(generics).origin == PredicateOrigin::WhereClause { | ||
| if exactly_one_where_bound { | ||
| suggestions.push((generics.where_clause_span, String::new())); | ||
| } else { | ||
| suggestions.push((emission.predicate_span_including_comma(generics), String::new())); | ||
| } | ||
| } |
There was a problem hiding this comment.
A shame we can't use https://doc.rust-lang.org/nightly/nightly-rustc/rustc_hir/hir/struct.Generics.html#method.span_for_predicate_removal here because it might have overlaps
Makes me wonder if we should add a fn remove_span_overlaps(&mut [Span]) for when it's intentional, it would be fine where we're just removing things
| error: this trait bound only exists to constrain another bound's associated type | ||
| --> tests/ui/could_be_assoc_type_bounds.rs:45:24 | ||
| | | ||
| LL | fn impl_trait_generic<T: Copy>(_: impl Iterator<Item = T>) {} | ||
| | ^^^^^^ ------------------ merge it with this bound | ||
| | | ||
| help: remove any extra trait bounds add them directly to this trait bound using associated type bounds | ||
| | | ||
| LL - fn impl_trait_generic<T: Copy>(_: impl Iterator<Item = T>) {} | ||
| LL + fn impl_trait_generic(_: impl Iterator<Item: Copy>) {} |
There was a problem hiding this comment.
The where bound removal cases are a clear win but removing generic params is trickier since it allows e.g. impl_trait_generic::<u8>([].into_iter())
If we do lint them it should honour avoid-breaking-exported-api, but we may want to be conservative here
| fn hir_generics_of_item<'tcx>(item: &'tcx hir::Item<'tcx>) -> &'tcx hir::Generics<'tcx> { | ||
| match item.kind { | ||
| hir::ItemKind::Enum(_, generics) | ||
| | hir::ItemKind::Fn(_, generics, _) | ||
| | hir::ItemKind::Const(_, generics, _) | ||
| | hir::ItemKind::Impl(&hir::Impl { generics, .. }) | ||
| | hir::ItemKind::Struct(_, generics) | ||
| | hir::ItemKind::Trait(_, _, generics, ..) | ||
| | hir::ItemKind::TraitAlias(generics, _) | ||
| | hir::ItemKind::TyAlias(_, generics) | ||
| | hir::ItemKind::Union(_, generics) => generics, | ||
| _ => hir::Generics::empty(), | ||
| } | ||
| } |
There was a problem hiding this comment.
Can use item.kind.generics() here
|
Ping @y21 from triage. Do you plan to return to working on this? |
5 participants
Closes #12925.
The issue explains the two patterns it looks for, but to quickly summarize:
can both be
This ended up far more involved than I originally expected and it's a bit annoying just how much code is required to have a proper machine applicable suggestion that can handle all the ways these bounds can be written.
TBH I'm not sure if it's worth all the complexity, but oh well (especially considering that something like
<T as Trait>::Assoc: Copy, <T as Trait>::Assoc: Clone, which requires buffering the lint, is probably rather rare). The current suggestion still relies a bit on rustfmt to clean up, like a trailing comma because I didn't want to make it even more complicated than it already is, but I think it should be fine.There's the question if we want to lint
impl Traittypes (<T: Iterator<Item = impl Copy>>-><T: Iterator<Item: Copy>>). My initial implementation naturally linted them beacuse they're desugared to the pattern we already detect, but calling it a complexity issue is a stretch IMO as it barely saves a few characters so for now I just ignored them, and the lint description/messages would need some special casing.changelog: new lint:
could_be_assoc_type_bounds