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

rustdoc: fix & clean up handling of cross-crate higher-ranked parameters #116388

Merged
Show file tree
Hide file tree
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
7 changes: 3 additions & 4 deletions src/librustdoc/clean/auto_trait.rs
Original file line number Diff line number Diff line change
Expand Up @@ -551,8 +551,8 @@ where
WherePredicate::RegionPredicate { lifetime, bounds } => {
lifetime_to_bounds.entry(lifetime).or_default().extend(bounds);
}
WherePredicate::EqPredicate { lhs, rhs, bound_params } => {
match *lhs {
WherePredicate::EqPredicate { lhs, rhs } => {
match lhs {
Type::QPath(box QPathData {
ref assoc,
ref self_type,
Expand Down Expand Up @@ -590,14 +590,13 @@ where
GenericArgs::AngleBracketed { ref mut bindings, .. } => {
bindings.push(TypeBinding {
assoc: assoc.clone(),
kind: TypeBindingKind::Equality { term: *rhs },
kind: TypeBindingKind::Equality { term: rhs },
});
}
GenericArgs::Parenthesized { .. } => {
existing_predicates.push(WherePredicate::EqPredicate {
lhs: lhs.clone(),
rhs,
bound_params,
});
continue; // If something other than a Fn ends up
// with parentheses, leave it alone
Expand Down
17 changes: 5 additions & 12 deletions src/librustdoc/clean/inline.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,9 @@ use rustc_span::hygiene::MacroKind;
use rustc_span::symbol::{kw, sym, Symbol};

use crate::clean::{
self, clean_fn_decl_from_did_and_sig, clean_generics, clean_impl_item, clean_middle_assoc_item,
clean_middle_field, clean_middle_ty, clean_trait_ref_with_bindings, clean_ty,
clean_ty_alias_inner_type, clean_ty_generics, clean_variant_def, utils, Attributes,
self, clean_bound_vars, clean_fn_decl_from_did_and_sig, clean_generics, clean_impl_item,
clean_middle_assoc_item, clean_middle_field, clean_middle_ty, clean_trait_ref_with_bindings,
clean_ty, clean_ty_alias_inner_type, clean_ty_generics, clean_variant_def, utils, Attributes,
AttributesExt, ImplKind, ItemId, Type,
};
use crate::core::DocContext;
Expand Down Expand Up @@ -239,20 +239,13 @@ pub(crate) fn build_external_trait(cx: &mut DocContext<'_>, did: DefId) -> clean

fn build_external_function<'tcx>(cx: &mut DocContext<'tcx>, did: DefId) -> Box<clean::Function> {
let sig = cx.tcx.fn_sig(did).instantiate_identity();

let late_bound_regions = sig.bound_vars().into_iter().filter_map(|var| match var {
ty::BoundVariableKind::Region(ty::BrNamed(_, name)) if name != kw::UnderscoreLifetime => {
Some(clean::GenericParamDef::lifetime(name))
}
_ => None,
});

let predicates = cx.tcx.explicit_predicates_of(did);

let (generics, decl) = clean::enter_impl_trait(cx, |cx| {
// NOTE: generics need to be cleaned before the decl!
let mut generics = clean_ty_generics(cx, cx.tcx.generics_of(did), predicates);
// FIXME: This does not place parameters in source order (late-bound ones come last)
generics.params.extend(late_bound_regions);
generics.params.extend(clean_bound_vars(sig.bound_vars()));
let decl = clean_fn_decl_from_did_and_sig(cx, Some(did), sig);
(generics, decl)
});
Expand Down
111 changes: 53 additions & 58 deletions src/librustdoc/clean/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -232,20 +232,11 @@ fn clean_poly_trait_ref_with_bindings<'tcx>(
poly_trait_ref: ty::PolyTraitRef<'tcx>,
bindings: ThinVec<TypeBinding>,
) -> GenericBound {
// collect any late bound regions
let late_bound_regions: Vec<_> = cx
.tcx
.collect_referenced_late_bound_regions(&poly_trait_ref)
.into_iter()
.filter_map(|br| match br {
ty::BrNamed(_, name) if br.is_named() => Some(GenericParamDef::lifetime(name)),
_ => None,
})
.collect();
Copy link
Contributor

Choose a reason for hiding this comment

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

If nothing else, I'm glad to see all this duplicate code factored out into a function. ❤️


let trait_ = clean_trait_ref_with_bindings(cx, poly_trait_ref, bindings);
GenericBound::TraitBound(
PolyTrait { trait_, generic_params: late_bound_regions },
PolyTrait {
trait_: clean_trait_ref_with_bindings(cx, poly_trait_ref, bindings),
generic_params: clean_bound_vars(poly_trait_ref.bound_vars()),
},
hir::TraitBoundModifier::None,
)
}
Expand Down Expand Up @@ -338,9 +329,8 @@ fn clean_where_predicate<'tcx>(
},

hir::WherePredicate::EqPredicate(ref wrp) => WherePredicate::EqPredicate {
lhs: Box::new(clean_ty(wrp.lhs_ty, cx)),
rhs: Box::new(clean_ty(wrp.rhs_ty, cx).into()),
bound_params: Vec::new(),
lhs: clean_ty(wrp.lhs_ty, cx),
rhs: clean_ty(wrp.rhs_ty, cx).into(),
},
})
}
Expand Down Expand Up @@ -436,20 +426,9 @@ fn clean_projection_predicate<'tcx>(
pred: ty::Binder<'tcx, ty::ProjectionPredicate<'tcx>>,
cx: &mut DocContext<'tcx>,
) -> WherePredicate {
let late_bound_regions = cx
.tcx
.collect_referenced_late_bound_regions(&pred)
.into_iter()
.filter_map(|br| match br {
ty::BrNamed(_, name) if br.is_named() => Some(GenericParamDef::lifetime(name)),
_ => None,
})
.collect();

WherePredicate::EqPredicate {
lhs: Box::new(clean_projection(pred.map_bound(|p| p.projection_ty), cx, None)),
rhs: Box::new(clean_middle_term(pred.map_bound(|p| p.term), cx)),
bound_params: late_bound_regions,
lhs: clean_projection(pred.map_bound(|p| p.projection_ty), cx, None),
rhs: clean_middle_term(pred.map_bound(|p| p.term), cx),
}
}

Expand Down Expand Up @@ -703,8 +682,8 @@ pub(crate) fn clean_generics<'tcx>(
}
}
}
WherePredicate::EqPredicate { lhs, rhs, bound_params } => {
eq_predicates.push(WherePredicate::EqPredicate { lhs, rhs, bound_params });
WherePredicate::EqPredicate { lhs, rhs } => {
eq_predicates.push(WherePredicate::EqPredicate { lhs, rhs });
}
}
}
Expand Down Expand Up @@ -798,11 +777,9 @@ fn clean_ty_generics<'tcx>(
})
.collect::<ThinVec<GenericParamDef>>();

// param index -> [(trait DefId, associated type name & generics, term, higher-ranked params)]
let mut impl_trait_proj = FxHashMap::<
u32,
Vec<(DefId, PathSegment, ty::Binder<'_, ty::Term<'_>>, Vec<GenericParamDef>)>,
>::default();
// param index -> [(trait DefId, associated type name & generics, term)]
let mut impl_trait_proj =
FxHashMap::<u32, Vec<(DefId, PathSegment, ty::Binder<'_, ty::Term<'_>>)>>::default();

let where_predicates = preds
.predicates
Expand Down Expand Up @@ -854,11 +831,6 @@ fn clean_ty_generics<'tcx>(
trait_did,
name,
proj.map_bound(|p| p.term),
pred.get_bound_params()
.into_iter()
.flatten()
.cloned()
.collect(),
));
}

Expand Down Expand Up @@ -894,9 +866,9 @@ fn clean_ty_generics<'tcx>(

let crate::core::ImplTraitParam::ParamIndex(idx) = param else { unreachable!() };
if let Some(proj) = impl_trait_proj.remove(&idx) {
for (trait_did, name, rhs, bound_params) in proj {
for (trait_did, name, rhs) in proj {
let rhs = clean_middle_term(rhs, cx);
simplify::merge_bounds(cx, &mut bounds, bound_params, trait_did, name, &rhs);
simplify::merge_bounds(cx, &mut bounds, trait_did, name, &rhs);
}
}

Expand Down Expand Up @@ -1357,23 +1329,13 @@ pub(crate) fn clean_middle_assoc_item<'tcx>(
}
ty::AssocKind::Fn => {
let sig = tcx.fn_sig(assoc_item.def_id).instantiate_identity();

let late_bound_regions = sig.bound_vars().into_iter().filter_map(|var| match var {
ty::BoundVariableKind::Region(ty::BrNamed(_, name))
if name != kw::UnderscoreLifetime =>
{
Some(GenericParamDef::lifetime(name))
}
_ => None,
});

let mut generics = clean_ty_generics(
cx,
tcx.generics_of(assoc_item.def_id),
tcx.explicit_predicates_of(assoc_item.def_id),
);
// FIXME: This does not place parameters in source order (late-bound ones come last)
generics.params.extend(late_bound_regions);
generics.params.extend(clean_bound_vars(sig.bound_vars()));

let mut decl = clean_fn_decl_from_did_and_sig(cx, Some(assoc_item.def_id), sig);

Expand Down Expand Up @@ -2109,9 +2071,11 @@ pub(crate) fn clean_middle_ty<'tcx>(
// FIXME: should we merge the outer and inner binders somehow?
Copy link
Contributor

Choose a reason for hiding this comment

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

Does anybody know what kind of code would motivate fixing this? Is it actually possible to make rustdoc misrender something because the outer and inner binders aren't being merged?

Copy link
Member Author

@fmease fmease Oct 3, 2023

Choose a reason for hiding this comment

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

I'm not super familiar with some of the details surrounding bound vars yet. I can't tell you all possible cases where the type and the signature have bound vars at the same time.

I can give you one example though which rustdoc renders just fine:

pub fn f(_: impl for<'a> Trait<for<'b> fn(&'a (), &'b ())>) {  }
pub trait Trait<T> {}

Here, bound_ty.bound_vars() is ['a] and sig.bound_vars() is ['b]. It would be incorrect to concatenate ['a] and ['b]. We don't want to render for<'a> … for<'a, 'b> fn(…).

In this case, the bound_ty.bound_vars() is ['a] because clean_poly_trait_ref_with_bindings passes this list of bound vars down to external_path to … to ty_args_to_args which calls clean_middle_ty. I'm not sure if that's the intended usage of map_bound but I guess it is since oli-obk implemented that as far as I know.
bound_ty.skip_binder() and just using sig.bound_vars() should therefore be fine.

Copy link
Member Author

@fmease fmease Oct 3, 2023

Choose a reason for hiding this comment

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

In my limited understanding, I disagree with the FIXME, it would be super odd to merge different binder lists, rustc does it like one or two times in exceptional cases. There can be a semantic difference between for<'a> … for<'b> … and for<'a, 'b> … and rustc needs to shift the de Bruijn indices and the var indices for correctness after merging such lists.

A few months ago, I chatted a bit with compiler-errors about ty::Binder<_> and bound vars in rustdoc and from what I've learned rustdoc (still) doesn't properly track bound vars. This was relevant for #112463 where we don't have enough information to be able to deal with (escaping) late bound vars. I haven't understood everything yet and thus my explanation sucks.

I'm pretty sure from what I've learned so far, proper bound var handling would prevent ICEs in rustdoc like in PR #108503 (which you're acquainted with) and probably other ones like https://github.com/rust-lang/rust/issues?q=is%3Aissue+is%3Aopen+label%3AT-rustdoc%20label%3AI-ICE%20label%3AA-auto-traits%20label%3AA-synthetic-impls.

let sig = bound_ty.skip_binder().fn_sig(cx.tcx);
let decl = clean_fn_decl_from_did_and_sig(cx, None, sig);
let generic_params = clean_bound_vars(sig.bound_vars());

BareFunction(Box::new(BareFunctionDecl {
unsafety: sig.unsafety(),
generic_params: Vec::new(),
generic_params,
Copy link
Member Author

Choose a reason for hiding this comment

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

This change allows us to render the for<> parameter list of cross-crate fn ptr tys.

decl,
abi: sig.abi(),
}))
Expand Down Expand Up @@ -2187,8 +2151,8 @@ pub(crate) fn clean_middle_ty<'tcx>(

let late_bound_regions: FxIndexSet<_> = obj
.iter()
.flat_map(|pb| pb.bound_vars())
.filter_map(|br| match br {
.flat_map(|pred| pred.bound_vars())
.filter_map(|var| match var {
ty::BoundVariableKind::Region(ty::BrNamed(_, name))
if name != kw::UnderscoreLifetime =>
{
Expand Down Expand Up @@ -2262,6 +2226,11 @@ pub(crate) fn clean_middle_ty<'tcx>(
}
}

ty::Bound(_, ref ty) => match ty.kind {
ty::BoundTyKind::Param(_, name) => Generic(name),
ty::BoundTyKind::Anon => panic!("unexpected anonymous bound type variable"),
},

ty::Alias(ty::Opaque, ty::AliasTy { def_id, args, .. }) => {
// If it's already in the same alias, don't get an infinite loop.
if cx.current_type_aliases.contains_key(&def_id) {
Expand Down Expand Up @@ -2290,7 +2259,6 @@ pub(crate) fn clean_middle_ty<'tcx>(

ty::Closure(..) => panic!("Closure"),
ty::Generator(..) => panic!("Generator"),
ty::Bound(..) => panic!("Bound"),
ty::Placeholder(..) => panic!("Placeholder"),
ty::GeneratorWitness(..) => panic!("GeneratorWitness"),
ty::Infer(..) => panic!("Infer"),
Expand Down Expand Up @@ -3121,3 +3089,30 @@ fn clean_type_binding<'tcx>(
},
}
}

fn clean_bound_vars<'tcx>(
bound_vars: &'tcx ty::List<ty::BoundVariableKind>,
) -> Vec<GenericParamDef> {
Comment on lines +3093 to +3095
Copy link
Member Author

@fmease fmease Oct 3, 2023

Choose a reason for hiding this comment

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

Theoretically, I could make this generic by letting it take a <T> bound_ty: ty::Binder<'tcx, T> instead but I've decided against it for simplicity and to avoid code size explosion due to excessive monomorphization. I could use a non-generic inner function though, if you'd like.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm happy with this function not being generic. It wouldn't save very much code at the call site if it was.

bound_vars
.into_iter()
.filter_map(|var| match var {
ty::BoundVariableKind::Region(ty::BrNamed(_, name))
if name != kw::UnderscoreLifetime =>
{
Some(GenericParamDef::lifetime(name))
}
ty::BoundVariableKind::Ty(ty::BoundTyKind::Param(did, name)) => Some(GenericParamDef {
name,
kind: GenericParamDefKind::Type {
did,
bounds: Vec::new(),
default: None,
synthetic: false,
},
}),
// FIXME(non_lifetime_binders): Support higher-ranked const parameters.
ty::BoundVariableKind::Const => None,
_ => None,
})
.collect()
}
19 changes: 4 additions & 15 deletions src/librustdoc/clean/simplify.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,18 +40,18 @@ pub(crate) fn where_clauses(cx: &DocContext<'_>, clauses: Vec<WP>) -> ThinVec<WP
WP::RegionPredicate { lifetime, bounds } => {
lifetimes.push((lifetime, bounds));
}
WP::EqPredicate { lhs, rhs, bound_params } => equalities.push((lhs, rhs, bound_params)),
WP::EqPredicate { lhs, rhs } => equalities.push((lhs, rhs)),
}
}

// Look for equality predicates on associated types that can be merged into
// general bound predicates.
equalities.retain(|(lhs, rhs, bound_params)| {
equalities.retain(|(lhs, rhs)| {
let Some((ty, trait_did, name)) = lhs.projection() else {
return true;
};
let Some((bounds, _)) = tybounds.get_mut(ty) else { return true };
merge_bounds(cx, bounds, bound_params.clone(), trait_did, name, rhs)
merge_bounds(cx, bounds, trait_did, name, rhs)
});

// And finally, let's reassemble everything
Expand All @@ -64,18 +64,13 @@ pub(crate) fn where_clauses(cx: &DocContext<'_>, clauses: Vec<WP>) -> ThinVec<WP
bounds,
bound_params,
}));
clauses.extend(equalities.into_iter().map(|(lhs, rhs, bound_params)| WP::EqPredicate {
lhs,
rhs,
bound_params,
}));
clauses.extend(equalities.into_iter().map(|(lhs, rhs)| WP::EqPredicate { lhs, rhs }));
clauses
}

pub(crate) fn merge_bounds(
cx: &clean::DocContext<'_>,
bounds: &mut Vec<clean::GenericBound>,
mut bound_params: Vec<clean::GenericParamDef>,
trait_did: DefId,
assoc: clean::PathSegment,
rhs: &clean::Term,
Expand All @@ -93,12 +88,6 @@ pub(crate) fn merge_bounds(
}
let last = trait_ref.trait_.segments.last_mut().expect("segments were empty");

trait_ref.generic_params.append(&mut bound_params);
// Sort parameters (likely) originating from a hashset alphabetically to
// produce predictable output (and to allow for full deduplication).
trait_ref.generic_params.sort_unstable_by(|p, q| p.name.as_str().cmp(q.name.as_str()));
trait_ref.generic_params.dedup_by_key(|p| p.name);

match last.args {
PP::AngleBracketed { ref mut bindings, .. } => {
bindings.push(clean::TypeBinding {
Expand Down
11 changes: 1 addition & 10 deletions src/librustdoc/clean/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1289,7 +1289,7 @@ impl Lifetime {
pub(crate) enum WherePredicate {
BoundPredicate { ty: Type, bounds: Vec<GenericBound>, bound_params: Vec<GenericParamDef> },
RegionPredicate { lifetime: Lifetime, bounds: Vec<GenericBound> },
EqPredicate { lhs: Box<Type>, rhs: Box<Term>, bound_params: Vec<GenericParamDef> },
EqPredicate { lhs: Type, rhs: Term },
}

impl WherePredicate {
Expand All @@ -1300,15 +1300,6 @@ impl WherePredicate {
_ => None,
}
}

pub(crate) fn get_bound_params(&self) -> Option<&[GenericParamDef]> {
match self {
Self::BoundPredicate { bound_params, .. } | Self::EqPredicate { bound_params, .. } => {
Some(bound_params)
}
_ => None,
}
}
}

#[derive(Clone, PartialEq, Eq, Debug, Hash)]
Expand Down
3 changes: 1 addition & 2 deletions src/librustdoc/html/format.rs
Original file line number Diff line number Diff line change
Expand Up @@ -326,8 +326,7 @@ pub(crate) fn print_where_clause<'a, 'tcx: 'a>(
bounds_display.truncate(bounds_display.len() - " + ".len());
write!(f, "{}: {bounds_display}", lifetime.print())
}
// FIXME(fmease): Render bound params.
clean::WherePredicate::EqPredicate { lhs, rhs, bound_params: _ } => {
clean::WherePredicate::EqPredicate { lhs, rhs } => {
if f.alternate() {
write!(f, "{:#} == {:#}", lhs.print(cx), rhs.print(cx))
} else {
Expand Down
5 changes: 2 additions & 3 deletions src/librustdoc/json/conversions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -506,9 +506,8 @@ impl FromWithTcx<clean::WherePredicate> for WherePredicate {
lifetime: convert_lifetime(lifetime),
bounds: bounds.into_tcx(tcx),
},
// FIXME(fmease): Convert bound parameters as well.
EqPredicate { lhs, rhs, bound_params: _ } => {
WherePredicate::EqPredicate { lhs: (*lhs).into_tcx(tcx), rhs: (*rhs).into_tcx(tcx) }
EqPredicate { lhs, rhs } => {
WherePredicate::EqPredicate { lhs: lhs.into_tcx(tcx), rhs: rhs.into_tcx(tcx) }
}
}
}
Expand Down
1 change: 1 addition & 0 deletions tests/rustdoc/inline_cross/auxiliary/fn-type.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
pub type F = for<'z, 'a, '_unused> fn(&'z for<'b> fn(&'b str), &'a ()) -> &'a ();
2 changes: 1 addition & 1 deletion tests/rustdoc/inline_cross/auxiliary/impl_trait_aux.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ pub fn func4<T: Iterator<Item = impl Clone>>(_x: T) {}

pub fn func5(
_f: impl for<'any> Fn(&'any str, &'any str) -> bool + for<'r> Other<T<'r> = ()>,
_a: impl for<'alpha, 'beta> Auxiliary<'alpha, Item<'beta> = fn(&'beta ())>,
_a: impl for<'beta, 'alpha, '_gamma> Auxiliary<'alpha, Item<'beta> = fn(&'beta ())>,
) {}

pub trait Other {
Expand Down
10 changes: 10 additions & 0 deletions tests/rustdoc/inline_cross/auxiliary/non_lifetime_binders.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
#![feature(non_lifetime_binders)]

pub trait Trait<T> {}

pub fn f(_: impl for<T> Trait<T>) {}

pub fn g<T>(_: T)
where
T: for<U> Trait<U>,
{}
Loading
Loading