Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor LoweringContext::lower_generics_mut #86298

Closed
wants to merge 1 commit into from
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 36 additions & 25 deletions compiler/rustc_ast_lowering/src/item.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1372,37 +1372,48 @@ impl<'hir> LoweringContext<'_, 'hir> {
generics: &Generics,
itctx: ImplTraitContext<'_, 'hir>,
) -> GenericsCtor<'hir> {
// Collect `?Trait` bounds in where clause and move them to parameter definitions.
// Collect `?Trait` bounds in where clause and move them to parameter
// definitions. Currently, the decision to add the predicate for the
// implicit `Sized` bound only examines the generic parameters, not the
// where clauses, to discover any `?Sized` bounds. (e.g.,
// `AstConv::is_unsized`)
let mut add_bounds: NodeMap<Vec<_>> = Default::default();
for pred in &generics.where_clause.predicates {
if let WherePredicate::BoundPredicate(ref bound_pred) = *pred {
'next_bound: for bound in &bound_pred.bounds {
if let GenericBound::Trait(_, TraitBoundModifier::Maybe) = *bound {
// Check if the where clause type is a plain type parameter.
match self
.resolver
.get_partial_res(bound_pred.bounded_ty.id)
.map(|d| (d.base_res(), d.unresolved_segments()))
let bound_pred = match *pred {
WherePredicate::BoundPredicate(ref bound_pred) => bound_pred,
_ => continue,
};
for bound in &bound_pred.bounds {
if !matches!(*bound, GenericBound::Trait(_, TraitBoundModifier::Maybe)) {
continue;
}
// Check if the where clause type is a plain type parameter.
match self
.resolver
.get_partial_res(bound_pred.bounded_ty.id)
.map(|d| (d.base_res(), d.unresolved_segments()))
{
Some((Res::Def(DefKind::TyParam, def_id), 0))
if bound_pred.bound_generic_params.is_empty() =>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The match expression and this test do not depend on bound. Can those be lifted out of the loop?

{
if let Some(param) = generics
.params
.iter()
.find(|p| def_id == self.resolver.local_def_id(p.id).to_def_id())
{
Some((Res::Def(DefKind::TyParam, def_id), 0))
if bound_pred.bound_generic_params.is_empty() =>
{
for param in &generics.params {
if def_id == self.resolver.local_def_id(param.id).to_def_id() {
add_bounds.entry(param.id).or_default().push(bound.clone());
continue 'next_bound;
}
}
}
_ => {}
add_bounds.entry(param.id).or_default().push(bound.clone());
continue;
}
self.diagnostic().span_err(
bound_pred.bounded_ty.span,
"`?Trait` bounds are only permitted at the \
point where a type parameter is declared",
);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This loop can be refactored into a call to generics.params.iter().find. The matches! test should perhaps be an assertion on the found parameter.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you saying that param.id won't match unless it's a type parameter? I guess duplicate LocalDefIds aren't supposed to happen, and we already know that def_id is for a TyParam. I'm also wary of adding an assertion where there wasn't one in the original code.

On the other hand, the matches! will allow skipping of calls to self.resolver.local_def_id for non-type generic parameters, which could have a performance impact, given that this function is kind of quadratic in the number of generic parameters.

_ => {}
}
// Either the `bounded_ty` is not a plain type parameter, or
// it's not found in the generic type parameters list.
self.diagnostic().span_err(
bound_pred.bounded_ty.span,
"`?Trait` bounds are only permitted at the \
point where a type parameter is declared",
);
}
}

Expand Down