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 cross-crate impl Sized & impl ?Sized #114059

Merged
merged 2 commits into from
Jul 27, 2023
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
80 changes: 49 additions & 31 deletions src/librustdoc/clean/mod.rs
Copy link
Member Author

@fmease fmease Jul 25, 2023

Choose a reason for hiding this comment

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

This further enlarges the preexisting bound cleaning duplication of clean_ty_generics and clean_middle_opaque_bounds. Let me know if this is okay for the time being, I plan on consolidating these implementations when I overhaul bound cleaning next to fix #113015.

Original file line number Diff line number Diff line change
Expand Up @@ -798,10 +798,10 @@ fn clean_ty_generics<'tcx>(
let where_predicates = preds
.predicates
.iter()
.flat_map(|(p, _)| {
.flat_map(|(pred, _)| {
let mut projection = None;
let param_idx = (|| {
let bound_p = p.kind();
let bound_p = pred.kind();
match bound_p.skip_binder() {
ty::ClauseKind::Trait(pred) => {
if let ty::Param(param) = pred.self_ty().kind() {
Expand All @@ -826,33 +826,26 @@ fn clean_ty_generics<'tcx>(
})();

if let Some(param_idx) = param_idx
&& let Some(b) = impl_trait.get_mut(&param_idx.into())
&& let Some(bounds) = impl_trait.get_mut(&param_idx.into())
{
let p: WherePredicate = clean_predicate(*p, cx)?;
let pred = clean_predicate(*pred, cx)?;

b.extend(
p.get_bounds()
bounds.extend(
pred.get_bounds()
.into_iter()
.flatten()
.cloned()
.filter(|b| !b.is_sized_bound(cx)),
);

let proj = projection.map(|p| {
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 is just a refactor. I found the map followed by and_then, try-operator and if-let quite hairy.

(
clean_projection(p.map_bound(|p| p.projection_ty), cx, None),
p.map_bound(|p| p.term),
)
});
if let Some(((_, trait_did, name), rhs)) = proj
.as_ref()
.and_then(|(lhs, rhs): &(Type, _)| Some((lhs.projection()?, rhs)))
if let Some(proj) = projection
&& let lhs = clean_projection(proj.map_bound(|p| p.projection_ty), cx, None)
&& let Some((_, trait_did, name)) = lhs.projection()
{
impl_trait_proj.entry(param_idx).or_default().push((
trait_did,
name,
*rhs,
p.get_bound_params()
proj.map_bound(|p| p.term),
pred.get_bound_params()
.into_iter()
.flatten()
.cloned()
Expand All @@ -863,13 +856,32 @@ fn clean_ty_generics<'tcx>(
return None;
}

Some(p)
Some(pred)
})
.collect::<Vec<_>>();

for (param, mut bounds) in impl_trait {
let mut has_sized = false;
bounds.retain(|b| {
if b.is_sized_bound(cx) {
has_sized = true;
Comment on lines +866 to +867
Copy link
Member Author

@fmease fmease Jul 26, 2023

Choose a reason for hiding this comment

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

Looking at this, I don't think it handles ~const Sized (TraitBoundModifier::MaybeConst) correctly.
I pretty sure it results in has_sized == false making us add + ?Sized to it.
Let me know if I should fix it here or in a follow-up (I guess there might be some more misuses of is_sized_bound).

Edit: impl ~const Sized is actually not allowed (at the moment?) since Sized is not a #[const_trait].

Copy link
Member

Choose a reason for hiding this comment

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

That answers the question then. ;)

false
} else {
true
}
});
if !has_sized {
bounds.push(GenericBound::maybe_sized(cx));
}

// Move trait bounds to the front.
bounds.sort_by_key(|b| !matches!(b, GenericBound::TraitBound(..)));
bounds.sort_by_key(|b| !b.is_trait_bound());

// Add back a `Sized` bound if there are no *trait* bounds remaining (incl. `?Sized`).
// Since all potential trait bounds are at the front we can just check the first bound.
if bounds.first().map_or(true, |b| !b.is_trait_bound()) {
bounds.insert(0, GenericBound::sized(cx));
}

let crate::core::ImplTraitParam::ParamIndex(idx) = param else { unreachable!() };
if let Some(proj) = impl_trait_proj.remove(&idx) {
Expand All @@ -891,7 +903,7 @@ fn clean_ty_generics<'tcx>(
// implicit `Sized` bound unless removed with `?Sized`.
// However, in the list of where-predicates below, `Sized` appears like a
// normal bound: It's either present (the type is sized) or
// absent (the type is unsized) but never *maybe* (i.e. `?Sized`).
// absent (the type might be unsized) but never *maybe* (i.e. `?Sized`).
//
// This is unsuitable for rendering.
// Thus, as a first step remove all `Sized` bounds that should be implicit.
Expand All @@ -902,8 +914,8 @@ fn clean_ty_generics<'tcx>(
let mut sized_params = FxHashSet::default();
where_predicates.retain(|pred| {
if let WherePredicate::BoundPredicate { ty: Generic(g), bounds, .. } = pred
&& *g != kw::SelfUpper
&& bounds.iter().any(|b| b.is_sized_bound(cx))
&& *g != kw::SelfUpper
&& bounds.iter().any(|b| b.is_sized_bound(cx))
{
sized_params.insert(*g);
false
Expand Down Expand Up @@ -2113,7 +2125,6 @@ fn clean_middle_opaque_bounds<'tcx>(
cx: &mut DocContext<'tcx>,
bounds: Vec<ty::Clause<'tcx>>,
) -> Type {
let mut regions = vec![];
let mut has_sized = false;
let mut bounds = bounds
.iter()
Expand All @@ -2122,10 +2133,7 @@ fn clean_middle_opaque_bounds<'tcx>(
let trait_ref = match bound_predicate.skip_binder() {
ty::ClauseKind::Trait(tr) => bound_predicate.rebind(tr.trait_ref),
ty::ClauseKind::TypeOutlives(ty::OutlivesPredicate(_ty, reg)) => {
if let Some(r) = clean_middle_region(reg) {
regions.push(GenericBound::Outlives(r));
}
return None;
return clean_middle_region(reg).map(GenericBound::Outlives);
}
_ => return None,
};
Expand Down Expand Up @@ -2161,10 +2169,20 @@ fn clean_middle_opaque_bounds<'tcx>(
Some(clean_poly_trait_ref_with_bindings(cx, trait_ref, bindings))
})
.collect::<Vec<_>>();
bounds.extend(regions);
if !has_sized && !bounds.is_empty() {
bounds.insert(0, GenericBound::maybe_sized(cx));

if !has_sized {
bounds.push(GenericBound::maybe_sized(cx));
}

// Move trait bounds to the front.
bounds.sort_by_key(|b| !b.is_trait_bound());

// Add back a `Sized` bound if there are no *trait* bounds remaining (incl. `?Sized`).
// Since all potential trait bounds are at the front we can just check the first bound.
if bounds.first().map_or(true, |b| !b.is_trait_bound()) {
bounds.insert(0, GenericBound::sized(cx));
}

ImplTrait(bounds)
}

Expand Down
17 changes: 13 additions & 4 deletions src/librustdoc/clean/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1219,15 +1219,24 @@ pub(crate) enum GenericBound {
}

impl GenericBound {
pub(crate) fn sized(cx: &mut DocContext<'_>) -> GenericBound {
Self::sized_with(cx, hir::TraitBoundModifier::None)
}

pub(crate) fn maybe_sized(cx: &mut DocContext<'_>) -> GenericBound {
Self::sized_with(cx, hir::TraitBoundModifier::Maybe)
}

fn sized_with(cx: &mut DocContext<'_>, modifier: hir::TraitBoundModifier) -> GenericBound {
let did = cx.tcx.require_lang_item(LangItem::Sized, None);
let empty = ty::Binder::dummy(ty::GenericArgs::empty());
let path = external_path(cx, did, false, ThinVec::new(), empty);
inline::record_extern_fqn(cx, did, ItemType::Trait);
GenericBound::TraitBound(
PolyTrait { trait_: path, generic_params: Vec::new() },
hir::TraitBoundModifier::Maybe,
)
GenericBound::TraitBound(PolyTrait { trait_: path, generic_params: Vec::new() }, modifier)
}

pub(crate) fn is_trait_bound(&self) -> bool {
matches!(self, Self::TraitBound(..))
}

pub(crate) fn is_sized_bound(&self, cx: &DocContext<'_>) -> bool {
Expand Down
35 changes: 24 additions & 11 deletions src/librustdoc/html/format.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1094,22 +1094,35 @@ fn fmt_type<'cx>(
};
let m = mutability.print_with_space();
let amp = if f.alternate() { "&" } else { "&amp;" };
match **ty {

if let clean::Generic(name) = **ty {
return primitive_link(
f,
PrimitiveType::Reference,
&format!("{amp}{lt}{m}{name}"),
cx,
);
}

write!(f, "{amp}{lt}{m}")?;

let needs_parens = match **ty {
clean::DynTrait(ref bounds, ref trait_lt)
if bounds.len() > 1 || trait_lt.is_some() =>
{
write!(f, "{}{}{}(", amp, lt, m)?;
fmt_type(ty, f, use_absolute, cx)?;
write!(f, ")")
}
clean::Generic(name) => {
primitive_link(f, PrimitiveType::Reference, &format!("{amp}{lt}{m}{name}"), cx)
}
_ => {
write!(f, "{}{}{}", amp, lt, m)?;
fmt_type(ty, f, use_absolute, cx)
true
}
clean::ImplTrait(ref bounds) if bounds.len() > 1 => true,
_ => false,
};
if needs_parens {
f.write_str("(")?;
}
fmt_type(ty, f, use_absolute, cx)?;
if needs_parens {
f.write_str(")")?;
}
Ok(())
}
clean::ImplTrait(ref bounds) => {
if f.alternate() {
Expand Down
2 changes: 1 addition & 1 deletion tests/rustdoc/extern-impl-trait.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,5 +7,5 @@ extern crate extern_impl_trait;
// @has 'foo/struct.X.html' '//h4[@class="code-header"]' "impl Foo<Associated = ()> + 'a"
pub use extern_impl_trait::X;

// @has 'foo/struct.Y.html' '//h4[@class="code-header"]' "impl ?Sized + Foo<Associated = ()> + 'a"
// @has 'foo/struct.Y.html' '//h4[@class="code-header"]' "impl Foo<Associated = ()> + ?Sized + 'a"
pub use extern_impl_trait::Y;
2 changes: 1 addition & 1 deletion tests/rustdoc/impl-everywhere.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,6 @@ pub fn foo_foo() -> impl Foo + Foo2 {
Bar
}

// @has foo/fn.foo_foo_foo.html '//section[@id="main-content"]//pre' "x: &'x impl Foo + Foo2"
// @has foo/fn.foo_foo_foo.html '//section[@id="main-content"]//pre' "x: &'x (impl Foo + Foo2)"
pub fn foo_foo_foo<'x>(_x: &'x (impl Foo + Foo2)) {
}
21 changes: 21 additions & 0 deletions tests/rustdoc/inline_cross/auxiliary/impl-sized.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
use std::fmt::Debug;

pub fn sized(x: impl Sized) -> impl Sized {
x
}

pub fn sized_outlives<'a>(x: impl Sized + 'a) -> impl Sized + 'a {
x
}

pub fn maybe_sized(x: &impl ?Sized) -> &impl ?Sized {
x
}

pub fn debug_maybe_sized(x: &(impl Debug + ?Sized)) -> &(impl Debug + ?Sized) {
x
}

pub fn maybe_sized_outlives<'t>(x: &(impl ?Sized + 't)) -> &(impl ?Sized + 't) {
x
}
27 changes: 27 additions & 0 deletions tests/rustdoc/inline_cross/impl-sized.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
#![crate_name = "user"]

// aux-crate:impl_sized=impl-sized.rs
// edition:2021

// @has user/fn.sized.html
// @has - '//pre[@class="rust item-decl"]' "sized(x: impl Sized) -> impl Sized"
pub use impl_sized::sized;

// @has user/fn.sized_outlives.html
// @has - '//pre[@class="rust item-decl"]' \
// "sized_outlives<'a>(x: impl Sized + 'a) -> impl Sized + 'a"
pub use impl_sized::sized_outlives;

// @has user/fn.maybe_sized.html
// @has - '//pre[@class="rust item-decl"]' "maybe_sized(x: &impl ?Sized) -> &impl ?Sized"
pub use impl_sized::maybe_sized;

// @has user/fn.debug_maybe_sized.html
// @has - '//pre[@class="rust item-decl"]' \
// "debug_maybe_sized(x: &(impl Debug + ?Sized)) -> &(impl Debug + ?Sized)"
pub use impl_sized::debug_maybe_sized;

// @has user/fn.maybe_sized_outlives.html
// @has - '//pre[@class="rust item-decl"]' \
// "maybe_sized_outlives<'t>(x: &(impl ?Sized + 't)) -> &(impl ?Sized + 't)"
pub use impl_sized::maybe_sized_outlives;