Skip to content

Commit

Permalink
more work
Browse files Browse the repository at this point in the history
  • Loading branch information
@y21
y21 committed Nov 25, 2024
1 parent 9301d67 commit e272d9d
Show file tree
Hide file tree
Showing 4 changed files with 350 additions and 100 deletions.
195 changes: 125 additions & 70 deletions clippy_lints/src/could_be_assoc_type_bounds.rs
View file
Original file line number Diff line number Diff line change
@@ -1,34 +1,56 @@
use clippy_config::Conf;
use clippy_utils::diagnostics::span_lint_and_then;
use clippy_utils::msrvs::{self, Msrv};
use clippy_utils::source::SpanRangeExt;
use clippy_utils::source::{SourceText, SpanRangeExt};
use clippy_utils::{SpanlessEq, over};
use itertools::Itertools;
use rustc_data_structures::fx::FxIndexMap;
use rustc_errors::{Applicability, MultiSpan};
use rustc_hir::def::{DefKind, Res};
use rustc_hir::def_id::{DefId, LocalDefId};
use rustc_hir::{self as hir, AssocItemConstraintKind, PathSegment, QPath};
use rustc_hir::{self as hir, AssocItemConstraintKind, HirId, PathSegment, PredicateOrigin, QPath};
use rustc_lint::{LateContext, LateLintPass};
use rustc_session::impl_lint_pass;
use rustc_span::Span;
use rustc_span::symbol::Ident;

declare_clippy_lint! {
/// ### What it does
/// TODO
/// Checks for trait bounds that constrain another trait bound's associated type,
/// which could be expressed with [associated type bounds] directly.
///
/// ### Why is this bad?
/// TODO
/// Removing extra bounds and type parameters reduces noise and makes trait bounds easier to read.
///
/// ### Example
/// TODO
/// ```no_run
/// fn example1<I>()
/// where
/// I: Iterator,
/// <I as Iterator>::Item: Copy
/// {}
///
/// fn example2<I: Iterator<Item = T>, T: Copy>() {}
///
/// fn example3<I, T>() -> impl IntoIterator<Item = impl Copy> {}
/// ```
///
/// Use instead:
/// TODO
/// ```no_run
/// fn example1<I>()
/// where
/// I: Iterator<Item: Copy>,
/// {}
///
/// fn example2<I: Iterator<Item: Copy>>() {}
///
/// fn example3<I, T>() -> impl IntoIterator<Item: Copy> {}
/// ```
///
/// [associated type bounds]: https://blog.rust-lang.org/2024/06/13/Rust-1.79.0.html#bounds-in-associated-type-position
#[clippy::version = "1.84.0"]
pub COULD_BE_ASSOC_TYPE_BOUNDS, // TODO: better name
pub COULD_BE_ASSOC_TYPE_BOUNDS,
complexity,
"todo"
"trait bounds that could be expressed using associated type bounds"
}

#[derive(Debug)]
Expand Down Expand Up @@ -69,7 +91,7 @@ struct TyParamState {
/// (Only used in `ItemState::process`)
constrained_assoc_type: Option<(usize, usize)>,

/// The index of this parameter into `hir::Generics`
/// The index of this parameter into `hir::Generics::params`
param_index: usize,
}

Expand All @@ -85,19 +107,8 @@ struct ItemState {
item_def_id: LocalDefId,
}

fn hir_ty_param(ty: &hir::Ty<'_>) -> Option<LocalDefId> {
if let hir::TyKind::Path(QPath::Resolved(None, path)) = ty.kind
&& let Res::Def(DefKind::TyParam, ty_param) = path.res
{
Some(ty_param.as_local().expect("type parameters are always crate local"))
} else {
None
}
}

impl ItemState {
fn new(item_def_id: LocalDefId, generics: &hir::Generics<'_>) -> Self {
// TODO: what about implicit `Self` type parameter in traits?
Self {
item_def_id,
ty_params: generics
Expand All @@ -119,6 +130,7 @@ impl ItemState {
self.ty_params.iter_mut().find(|t| t.def_id == def_id)
}

/// If the type is a type parameter bound by this item, return its state
fn param_state_of_ty(&mut self, ty: &hir::Ty<'_>) -> Option<&mut TyParamState> {
hir_ty_param(ty).and_then(|def_id| self.param_state(def_id))
}
Expand Down Expand Up @@ -202,6 +214,7 @@ impl ItemState {
.first()
.zip(trait_refs.last())
.map(|(first, last)| first.span().to(last.span()))
.filter(|s| !s.from_expansion())
else {
continue;
};
Expand All @@ -222,9 +235,6 @@ impl ItemState {
)
})
{
// TODO: check that all spans are from the same ctxt? or just make sure that we can
// safely unwrap
// further down and that the source is available
trait_bound.emissions.push(Emission::ProjectionBound {
predicate_index,
bounds_span,
Expand Down Expand Up @@ -252,6 +262,7 @@ impl ItemState {
///
/// See the documentation for `TraitBound::emissions` for why we don't just immediately emit a
/// warning in `ItemState::fill_emissions` and instead delay them for here.
#[expect(clippy::too_many_lines)]
fn lint_emissions(&mut self, cx: &LateContext<'_>, trait_bounds: &[TraitBound<'_>], generics: &hir::Generics<'_>) {
for &TraitBound {
self_ty: _,
Expand All @@ -261,7 +272,13 @@ impl ItemState {
ref emissions,
} in trait_bounds.iter().filter(|b| !b.emissions.is_empty())
{
let message = if emissions.len() > 1 {
let is_impl_trait_desugar = emissions
.iter()
.all(|e| e.predicate(generics).origin == PredicateOrigin::ImplTrait);

let message = if is_impl_trait_desugar {
"this nested `impl Trait` type could be expressed using associated type bounds"
} else if emissions.len() > 1 {
"these trait bounds only exist to constrain another bound's associated type"
} else {
"this trait bound only exists to constrain another bound's associated type"
Expand All @@ -271,22 +288,22 @@ impl ItemState {
spans.push_span_label(span, "merge them with this bound");

span_lint_and_then(cx, COULD_BE_ASSOC_TYPE_BOUNDS, spans, message, |diag| {
let mut suggestions = Vec::with_capacity(emissions.len());
let mut suggestions = Vec::new();
let mut removed_generic_param = false;

// Group the emissions so we can merge all bounds from all predicates for
// any single associated type at the same time
let emissions = group_emissions_by_assoc_type(emissions, trait_path_segment, self);

for emission in emissions {
// Only explicitly remove predicate that live in the where clause.
for emission in emissions.iter().flat_map(|(_, e)| e) {
// Only explicitly remove predicates that live in the where clause.
// Trait bounds in the generic parameter list don't need to be removed here as we remove the whole
// generic parameter including all bounds in one go further down.
if emission.predicate(generics).origin == hir::PredicateOrigin::WhereClause {
if emission.predicate(generics).origin == PredicateOrigin::WhereClause {
suggestions.push((emission.predicate_span_including_comma(generics), String::new()));
}
}

// TODO: REALLY dont use + iterate a hashmap here and isntead just sort?
let grouped_emissions = emissions
.iter()
.into_group_map_by(|e| e.assoc_ty(trait_path_segment, self));

if let Some(args) = trait_path_segment.args
&& let Some(args_span) = args.span()
{
Expand All @@ -296,10 +313,7 @@ impl ItemState {
// type, or add the bounds to the existing constraint
let mut new_bounds = String::new();

for (assoc, emissions) in grouped_emissions {
// TODO: check if emissions.len() > some threshold and then dont lint as it might be more
// unreadable

for (assoc, emissions) in emissions {
if let Some(constraint) = args.constraints.iter().find(|c| c.ident == assoc) {
// Associated type is already bound in the generics. Don't introduce
// a new associated type constraint and instead append bounds to the
Expand Down Expand Up @@ -327,17 +341,18 @@ impl ItemState {
.with_hi(generics.span.hi() - rustc_span::BytePos(1)),
};
suggestions.push((removal_span, String::new()));
removed_generic_param = true;
}

// Replace ` = P` of `Iterator<Item = P>` with `: Bounds`
let mut sugg = String::from(": ");
Emission::append_bounds(&emissions, cx, &mut sugg, false);
append_emission_bounds(&emissions, cx, &mut sugg, false);
suggestions.push((constraint.span.with_lo(constraint.ident.span.hi()), sugg));
}
},
AssocItemConstraintKind::Bound { bounds } => {
let mut suggestion = String::new();
Emission::append_bounds(&emissions, cx, &mut suggestion, !bounds.is_empty());
append_emission_bounds(&emissions, cx, &mut suggestion, !bounds.is_empty());
suggestions.push((constraint.span.shrink_to_hi(), suggestion));
},
}
Expand All @@ -346,37 +361,53 @@ impl ItemState {
new_bounds += assoc.as_str();
new_bounds += ": ";

Emission::append_bounds(&emissions, cx, &mut new_bounds, false);
append_emission_bounds(&emissions, cx, &mut new_bounds, false);
}
}

if !new_bounds.is_empty() {
suggestions.push((insert_span, new_bounds));
if let Some((_, sugg)) = suggestions.iter_mut().find(|(sp, _)| sp.hi() == insert_span.lo()) {
// This is a hack to work around a rustfix bug: it considers replacements like
// `[(10:20, "foo"), (20:20, "bar")]` to have overlapping parts when
// they aren't overlapping. So we just extend the existing part in that case instead of
// pushing a new one.
*sugg += &new_bounds;
} else {
suggestions.push((insert_span, new_bounds));
}
}
} else {
// Generic arguments not present
let mut new_bounds = String::from("<");

for (i, (assoc, emissions)) in grouped_emissions.iter().enumerate() {
for (i, (assoc, emissions)) in emissions.iter().enumerate() {
if i > 0 {
new_bounds += ", ";
}

new_bounds += assoc.as_str();
new_bounds += ": ";

Emission::append_bounds(emissions, cx, &mut new_bounds, false);
append_emission_bounds(emissions, cx, &mut new_bounds, false);
}

new_bounds += ">";
suggestions.push((trait_path_segment.ident.span.shrink_to_hi(), new_bounds));
}

diag.multipart_suggestion_verbose(
"remove the extra trait bounds add them directly to this trait bound using associated type bounds",
if is_impl_trait_desugar {
"replace this `impl Trait` type with associated type bounds"
} else {
"remove any extra trait bounds add them directly to this trait bound using associated type bounds"
},
suggestions,
// Possibly requires changes at callsites if a generic parameter is removed.
Applicability::Unspecified,
if removed_generic_param {
// Possibly requires changes at callsites
Applicability::Unspecified
} else {
Applicability::MachineApplicable
},
);
});
}
Expand Down Expand Up @@ -432,19 +463,6 @@ enum Emission {
}

impl Emission {
fn append_bounds(emissions: &[&Emission], cx: &LateContext<'_>, out: &mut String, prepend_plus_at_start: bool) {
for (i, emission) in emissions.iter().enumerate() {
if i > 0 || prepend_plus_at_start {
*out += " + ";
}

emission
.bounds_span()
.with_source_text(cx, |s| out.push_str(s))
.unwrap(); // TODO: dont unwrap
}
}

fn predicate_index(&self) -> usize {
match *self {
Emission::ProjectionBound { predicate_index, .. } | Emission::TyParamBound { predicate_index, .. } => {
Expand Down Expand Up @@ -484,7 +502,7 @@ impl Emission {
}
}

fn assoc_ty(&self, trait_path_segment: &PathSegment<'_>, item_state: &ItemState) -> Ident {
fn expect_assoc_ty(&self, trait_path_segment: &PathSegment<'_>, item_state: &ItemState) -> Ident {
match *self {
Emission::ProjectionBound { assoc_type, .. } => assoc_type,
Emission::TyParamBound { ty_param_index, .. } => {
Expand All @@ -496,6 +514,39 @@ impl Emission {
}
}

fn group_emissions_by_assoc_type<'e>(
emissions: &'e [Emission],
trait_path_segment: &PathSegment<'_>,
item: &ItemState,
) -> FxIndexMap<Ident, Vec<&'e Emission>> {
emissions.iter().fold(FxIndexMap::default(), |mut emissions, emission| {
emissions
.entry(emission.expect_assoc_ty(trait_path_segment, item))
.or_default()
.push(emission);
emissions
})
}

fn append_emission_bounds(
emissions: &[&Emission],
cx: &LateContext<'_>,
out: &mut String,
prepend_plus_at_start: bool,
) {
for (i, emission) in emissions.iter().enumerate() {
if i > 0 || prepend_plus_at_start {
*out += " + ";
}

*out += emission
.bounds_span()
.get_source_text(cx)
.as_ref()
.map_or("..", SourceText::as_str);
}
}

/// A trait bound in an item's generics with an associated item constraint
#[derive(Debug)]
struct TraitBound<'tcx> {
Expand Down Expand Up @@ -525,11 +576,6 @@ struct TraitBound<'tcx> {
/// where T: Iterator<Item = U>,
/// <T as Iterator>::Item: Copy + Sized,
/// U: Clone {}
///
/// Lastly, we also only want to lint if there are <= 3 bounds for an associated type as it
/// might be more readable when split out, which is easily doable by buffering them (the number
/// was chosen fairly arbitrary) (TODO: actually implement this restriction, does it make
/// sense as a config too?)
emissions: Vec<Emission>,
}

Expand Down Expand Up @@ -563,6 +609,16 @@ fn hir_generics_of_item<'tcx>(item: &'tcx hir::Item<'tcx>) -> &'tcx hir::Generic
}
}

fn hir_ty_param(ty: &hir::Ty<'_>) -> Option<LocalDefId> {
if let hir::TyKind::Path(QPath::Resolved(None, path)) = ty.kind
&& let Res::Def(DefKind::TyParam, ty_param) = path.res
{
Some(ty_param.as_local().expect("type parameters are always crate local"))
} else {
None
}
}

impl<'tcx> LateLintPass<'tcx> for ManualAssocTypeBounds {
fn check_item(&mut self, _: &LateContext<'tcx>, item: &'tcx hir::Item<'tcx>) {
if self.msrv.meets(msrvs::ASSOCIATED_TYPE_BOUNDS) {
Expand All @@ -580,23 +636,22 @@ impl<'tcx> LateLintPass<'tcx> for ManualAssocTypeBounds {
fn check_impl_item_post(&mut self, cx: &LateContext<'tcx>, item: &'tcx hir::ImplItem<'tcx>) {
if self.msrv.meets(msrvs::ASSOCIATED_TYPE_BOUNDS) {
let state = self.states.pop().unwrap();
assert_eq!(state.item_def_id, item.owner_id.def_id);
debug_assert_eq!(state.item_def_id, item.owner_id.def_id);

state.postprocess_generics(cx, item.generics);
}
}

fn check_item_post(&mut self, cx: &LateContext<'tcx>, item: &'tcx hir::Item<'tcx>) {
// TODO: does all of this work correctly with #[msrv] attributes on the function?
if self.msrv.meets(msrvs::ASSOCIATED_TYPE_BOUNDS) {
let state = self.states.pop().unwrap();
assert_eq!(state.item_def_id, item.owner_id.def_id);
debug_assert_eq!(state.item_def_id, item.owner_id.def_id);

state.postprocess_generics(cx, hir_generics_of_item(item));
}
}

fn check_path(&mut self, cx: &LateContext<'tcx>, path: &hir::Path<'tcx>, _: rustc_hir::HirId) {
fn check_path(&mut self, cx: &LateContext<'tcx>, path: &hir::Path<'tcx>, _: HirId) {
if let Res::Def(DefKind::TyParam, ty_param_def_id) = path.res
&& 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()
Expand Down
Loading

0 comments on commit e272d9d

Please sign in to comment.