Add implied bounds to generic types, impl Trait, and assoc types. #148379
Conversation
|
|
rustbot
added
A-rustdoc-json
|
I'm still not particularly familiar with rustdoc or rustc internals, and I definitely don't have very good intuition for how things work yet. This PR is approximately 80h of me doing my best to figure things out on my own. I don't expect I got everything right—there are probably things that could be improved. But I did write lots of tests with all the edge cases I could think of, and I tried hard not to write anything egregiously wrong :) Feedback is very welcome, as is advice on resolving the remaining TODOs. In particular, let me know if you have a preference between adding |
This comment has been minimized.
This comment has been minimized.
|
i’ll take a look at the code later, but for now: @bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Add implied bounds to generic types, impl Trait, and assoc types.
rustbot
added
the
S-waiting-on-perf
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (972828a): comparison URL. Overall result: ❌✅ regressions and improvements - please read the text belowBenchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please do so in sufficient writing along with @bors rollup=never Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary 2.0%, secondary 1.6%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary 2.6%, secondary -4.2%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary 1.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 474.434s -> 474.73s (0.06%) |
rustbot
added
perf-regression
There was a problem hiding this comment.
I've only looked at the rustdoc-json side, but it seems fine. I'm not comfortable reviewing the ty/clean side of this, so I'll leave that to fmease.
I've also not yet looked an the tests in detail. But they seem quite comprehensive, thanks.
Could you move all the implied-bounds-*.rs tests into a new implied-bounds/ folder?
src/librustdoc/clean/inline.rs
Outdated
| .map(|item| clean_impl_item(item, cx)) | ||
| .collect::<Vec<_>>(), | ||
| clean_generics(impl_.generics, cx), | ||
| clean_generics(impl_.generics, did, cx), |
There was a problem hiding this comment.
N.B. This isn't tested, as rustdoc json doesn't inline. I'm pretty certain it's correct, but worth flagging.
Yes, I was going to ask if you're okay with that actually. Happy to do it prior to merging; will leave as-is for now for continuity of review in case fmease has already started reviewing. |
|
☔ The latest upstream changes (presumably #139558) made this pull request unmergeable. Please resolve the merge conflicts. |
obi1kenobi
force-pushed
the
pg/implied-bounds
branch
from
b35832e to
43cc2c3
Compare
This comment has been minimized.
This comment has been minimized.
fmease
added
S-waiting-on-review
obi1kenobi
force-pushed
the
pg/implied-bounds
branch
from
43cc2c3 to
85d31c2
Compare
This comment has been minimized.
This comment has been minimized.
obi1kenobi
force-pushed
the
pg/implied-bounds
branch
from
85d31c2 to
e411e2e
Compare
|
I made significant changes based on the preliminary feedback, and I've updated the PR description to match. I also significantly expanded the test coverage, and fixed several bugs and gaps in the implementation as a result. This took a while — thanks for bearing with me! |
|
These commits modify Please ensure that if you've changed the output:
rustdoc-json-types is a public (although nightly-only) API. If possible, consider changing |
obi1kenobi
force-pushed
the
pg/implied-bounds
branch
from
e411e2e to
658536c
Compare
| StructItem(s) => ItemEnum::Struct(from_clean_struct(s, owner_def_id, renderer)), | ||
| UnionItem(u) => ItemEnum::Union(from_clean_union(u, owner_def_id, renderer)), | ||
| StructFieldItem(f) => ItemEnum::StructField(f.into_json(renderer)), | ||
| EnumItem(e) => ItemEnum::Enum(e.into_json(renderer)), | ||
| EnumItem(e) => ItemEnum::Enum(from_clean_enum(e, owner_def_id, renderer)), |
There was a problem hiding this comment.
Processing here required the owner_def_id, so we moved away from .into_json() to separate functions, mirroring other cases like the existing from_clean_static().
The changes to function items et al below are analogous and also because of owner_def_id.
| impl FromClean<clean::Struct> for Struct { | ||
| fn from_clean(struct_: &clean::Struct, renderer: &JsonRenderer<'_>) -> Self { | ||
| let has_stripped_fields = struct_.has_stripped_entries(); | ||
| let clean::Struct { ctor_kind, generics, fields } = struct_; | ||
|
|
||
| let kind = match ctor_kind { | ||
| Some(CtorKind::Fn) => StructKind::Tuple(renderer.ids_keeping_stripped(fields)), | ||
| Some(CtorKind::Const) => { | ||
| assert!(fields.is_empty()); | ||
| StructKind::Unit | ||
| } | ||
| None => StructKind::Plain { fields: renderer.ids(fields), has_stripped_fields }, | ||
| }; | ||
|
|
||
| Struct { | ||
| kind, | ||
| generics: generics.into_json(renderer), | ||
| impls: Vec::new(), // Added in JsonRenderer::item | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl FromClean<clean::Union> for Union { | ||
| fn from_clean(union_: &clean::Union, renderer: &JsonRenderer<'_>) -> Self { | ||
| let has_stripped_fields = union_.has_stripped_entries(); | ||
| let clean::Union { generics, fields } = union_; | ||
| Union { | ||
| generics: generics.into_json(renderer), | ||
| has_stripped_fields, | ||
| fields: renderer.ids(fields), | ||
| impls: Vec::new(), // Added in JsonRenderer::item | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
These aren't deletions per se — we just moved these to dedicated from_clean_struct() / from_clean_union() functions due to the change in function signature.
| // This path is only hit when we aren't going through `from_clean_generics()`, | ||
| // i.e. we're processing cases like HRTB binders or function-pointer generic params. | ||
| // | ||
| // Non-lifetime binders don't currently allow bounds on type parameters, and | ||
| // late-bound type params are rejected on function pointers. That leaves no | ||
| // place for implied bounds to come from, as of today's version of Rust. | ||
| implied_bounds: Vec::new(), |
There was a problem hiding this comment.
I believe the argument in this comment to be true, but I'd love it if reviewers tried to poke holes at it. Perhaps I've missed an edge case.
| let explicit_maybe_sized = | ||
| explicit_bounds.iter().any(|bound| is_maybe_sized_bound(bound, renderer)); | ||
| let implicitly_sized = if *needs_sized_check && explicit_maybe_sized { |
There was a problem hiding this comment.
I wrote this code this way for readability. In principle, we could use one more layer of nesting and put the explicit_maybe_sized computation after if *needs_sized_check, at the cost of some readability. It's possible the compiler is already doing such a reordering for us, if it can prove that is_maybe_sized_bound() is a pure function.
Happy to tweak this based on reviewer feedback.
| /// Returns `true` if a projection predicate on `proj_trait_ref` should be attached to `trait_ref`. | ||
| /// | ||
| /// Most projections target the exact trait that defines the associated item, which is the | ||
| /// `proj_trait_ref == trait_ref` fast path. A small set of lang-item traits define their | ||
| /// associated items on a base trait while the surface traits are its supertraits (today: `FnOnce` | ||
| /// and `AsyncFnOnce` vs `Fn`/`FnMut` and `AsyncFn`/`AsyncFnMut`). We remap those projections to the | ||
| /// supertrait so implied bounds carry the full signature; this list reflects the current language | ||
| /// surface and should be extended if new lang-item traits gain this shape. | ||
| fn projection_applies_to_trait_ref<'tcx>( | ||
| tcx: TyCtxt<'tcx>, | ||
| proj_trait_ref: ty::TraitRef<'tcx>, | ||
| trait_ref: ty::Binder<'tcx, ty::TraitRef<'tcx>>, | ||
| ) -> bool { | ||
| let trait_ref = trait_ref.skip_binder(); | ||
| if proj_trait_ref == trait_ref { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
I'm not 100% sure whether an analogous case is possible outside of lang items. To be honest, this was at the end of chasing an edge case within an edge case, and I reached the complexity limit of how many things I could keep in mind at once.
Flagging for discussion with folks smarter than me.
There was a problem hiding this comment.
(preliminary review)
Disclaimer: I haven't yet checked in which cases you use this function but I can guess you use to it resugar/reconstruct/clean implied bounds just like "normal" clean does with user-written ones (duplicating efforts)?
I'm not 100% sure whether an analogous case is possible outside of lang items. To be honest, this was at the end of chasing an edge case within an edge case, and I reached the complexity limit of how many things I could keep in mind at once.
I'm not sure if I understand your question. Are you wondering whether handling {Async,}Fn* is sufficient for this function or if there are classes of cases where it yields false negatives?
Because, yes there are. trait Sub: Deref {} → Sub<Target = ()> is legal. Similarly for all other traits (needn't be lang items) with assoc types.
| let Some(local_parent) = parent.as_local() else { | ||
| // Cross-crate TAITs don't carry the parent WF info in metadata, | ||
| // so we can't infer outlives bounds here. | ||
| return Vec::new(); | ||
| }; |
There was a problem hiding this comment.
The inside of this might be unreachable? We're inside the let Some(local_def_id) = opaque_def_id.as_local() arm so I'd expect the parent is also local.
I'm not positive about this, so I wrote this defensively. If reviewers agree it's unreachable, I'm happy to replace it with an assertion instead.
|
Let's see if I have permissions for this or not... @bors try @rust-timer queue EDIT: womp womp 😢 |
This comment has been minimized.
This comment has been minimized.
|
@obi1kenobi: 🔑 Insufficient privileges: not in try users |
This comment has been minimized.
This comment has been minimized.
|
Got you. ;) @bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Add implied bounds to generic types, impl Trait, and assoc types.
rustbot
added
the
S-waiting-on-perf
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
(preliminary review)
I'm far from done reading through the added code. I have a bunch questions but they might become obvious once I've finished reading.
E.g., I'm a bit surprised that there's a whole bunch of manual code for figuring out if a HIR or a middle type requires Sized. It seems a bit unfortunate that we're not relying on rustc and its trait solver for this. I'm sure there are reasons.
Lastly, I'm very worried about duplicating parts of clean. I've spent a lot of time ripping out reimplementations of clean inside rustdoc (e.g., for auto trait impls) and I'm not done yet. Every new impl needs maintenance and inevitably comes along with its own bugs and limitations (then, sporadic contributors might come along and improve only 1 of n impls since they're not aware of all the disconnected impls that exist (HIR vs. middle is already a nightmare ^^')).
I'm sorry for my tone. I'm sure several things will become clearer to me the more I read and we should hopefully be able to remedy some of the things I've alluded to before merge.
| bounds, | ||
| origin: ImplTraitOrigin::Opaque { | ||
| def_id: impl_trait_def_id, | ||
| needs_sized_check: if cx.tcx.opt_rpitit_info(impl_trait_def_id).is_some() { |
There was a problem hiding this comment.
(preliminary review)
This might benefit from being computed lazily to avoid calling these queries in rustdoc-html (given that this is only depends on tcx and impl_trait_def_id; I get that the local HIR case depends on hir::OpaqueTyOrigin (but you should be able to call opaque_ty_origin, too, might be too heavy tho for rustdoc-json)). Tho I didn't get to a point in the patch yet where I can grasp the purpose of needs_sized_check.
| UnsafeBinderTy { generic_params, ty } | ||
| } | ||
|
|
||
| fn opaque_needs_sized_check<D>(origin: &OpaqueTyOrigin<D>) -> bool { |
There was a problem hiding this comment.
(preliminary review)
Disclaimer: I'm probably missing something since I haven't got that far yet.
I don't understand the following:
TAITs don't need a use-site sizedness check.
?Sizedin their definition always opts them out of being sized.
Are you suggesting that (TAIT) impl ?Sized + … is never bounded by Sized because that's not true:
#![feature(type_alias_impl_trait)]
type Opaque = impl Trait + ?Sized;
trait Trait: Sized {}
impl Trait for () {}
#[define_opaque(Opaque)]
fn define() -> Opaque {}
fn scope() {
std::mem::drop::<Opaque>; // OK. `Opaque` is sized.
}Or does this perhaps have something to do with issue RUST-149438?
| let fn_once_trait = tcx.lang_items().fn_once_trait(); | ||
| let async_fn_once_trait = tcx.lang_items().async_fn_once_trait(); | ||
|
|
||
| match Some(proj_trait_ref.def_id) { |
There was a problem hiding this comment.
(preliminary review)
This can be fully generalized by just walking the supertraits of the given crate, no hard-coding needed. That's what we also do during "normal" clean.
| /// Returns `true` if a projection predicate on `proj_trait_ref` should be attached to `trait_ref`. | ||
| /// | ||
| /// Most projections target the exact trait that defines the associated item, which is the | ||
| /// `proj_trait_ref == trait_ref` fast path. A small set of lang-item traits define their | ||
| /// associated items on a base trait while the surface traits are its supertraits (today: `FnOnce` | ||
| /// and `AsyncFnOnce` vs `Fn`/`FnMut` and `AsyncFn`/`AsyncFnMut`). We remap those projections to the | ||
| /// supertrait so implied bounds carry the full signature; this list reflects the current language | ||
| /// surface and should be extended if new lang-item traits gain this shape. | ||
| fn projection_applies_to_trait_ref<'tcx>( | ||
| tcx: TyCtxt<'tcx>, | ||
| proj_trait_ref: ty::TraitRef<'tcx>, | ||
| trait_ref: ty::Binder<'tcx, ty::TraitRef<'tcx>>, | ||
| ) -> bool { | ||
| let trait_ref = trait_ref.skip_binder(); | ||
| if proj_trait_ref == trait_ref { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
(preliminary review)
Disclaimer: I haven't yet checked in which cases you use this function but I can guess you use to it resugar/reconstruct/clean implied bounds just like "normal" clean does with user-written ones (duplicating efforts)?
I'm not 100% sure whether an analogous case is possible outside of lang items. To be honest, this was at the end of chasing an edge case within an edge case, and I reached the complexity limit of how many things I could keep in mind at once.
I'm not sure if I understand your question. Are you wondering whether handling {Async,}Fn* is sufficient for this function or if there are classes of cases where it yields false negatives?
Because, yes there are. trait Sub: Deref {} → Sub<Target = ()> is legal. Similarly for all other traits (needn't be lang items) with assoc types.
| let param_def = generics.param_at(index as usize, tcx); | ||
| match param_def.kind { | ||
| ty::GenericParamDefKind::Type { .. } => Some(ParamTy::for_def(param_def)), | ||
| _ => None, | ||
| } |
There was a problem hiding this comment.
(preliminary review)
| let param_def = generics.param_at(index as usize, tcx); | |
| match param_def.kind { | |
| ty::GenericParamDefKind::Type { .. } => Some(ParamTy::for_def(param_def)), | |
| _ => None, | |
| } | |
| Ty::new_param(tcx, index, tcx.item_name(param_def_id)) |
and let's assume param_def_id points to a type param; if sb. passes sth. else to param_ty_for_param, that's a bug in my eyes. Disclaimer: I haven't read the rest of the code. Even if it was possible now, rewriting it to make it impossible / a bug would preferable I think.
| BehindPointer, | ||
| } | ||
|
|
||
| fn opaque_use_in_ty<'hir, Unambig>( |
There was a problem hiding this comment.
(preliminary review)
Note that we have a great suite of visitors in rustc; you might be able to simplify this fn by using HIR intravisit (but maybe you already decided against it).
| || param_requires_sized_in_ty(tcx, sig.output(), target_param, false) | ||
| } | ||
|
|
||
| fn param_requires_sized_in_ty<'tcx>( |
There was a problem hiding this comment.
(preliminary review)
Note that we have a great suite of visitors in rustc; you might be able to simplify this fn by using middle type visitors (but maybe you already decided against it).
|
Finished benchmarking commit (0e2f486): comparison URL. Overall result: ❌ regressions - please read the text belowBenchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please do so in sufficient writing along with @bors rollup=never Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary 1.1%, secondary -1.2%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary -3.4%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 473.061s -> 470.796s (-0.48%) |
rustbot
removed
the
S-waiting-on-perf
8 participants
Include implied and elaborated (henceforth, implied) bounds in the rustdoc JSON representations of associated types,
impl Traittypes, and generic type parameters. Implemented as a newimplied_bounds: Vec<GenericBound>field, alongside the existingbounds: Vec<GenericBound>field that stores explicit (syntactic) bounds. An attempt at implementing #143197, based on the feedback and discussion in #143559 (comment).Rustdoc JSON distingushes between explicit bounds specified together with the generic parameter definition versus ones given via
whereclauses (which we do not change). The design of this PR considers implied bounds an inherent property of the generic type parameter, and does not distinguish the source of the implied bound between the two sets of explicit bounds. I believe this simplifies the design and implementation without hurting any use existing or realistic future use case.Per T-rustdoc feedback, the implementation is JSON-only and aims to minimize changes to
cleanand elsewhere. This is intended to mitigate possible performance impact on rustdoc as a whole, while providing this data to JSON-based workloads likecargo-semver-checks.Please see below for what this PR does, what is deferred to future PRs, and what open questions remain.
Please also note that half the line count is the test suite, and a bunch more is just "adjust dozens of pattern match sites to use a slightly different shape," so this PR isn't as large as it seems 😅
Recommended review order:
src/rustdoc-json-types/lib.rssrc/librustdoc/clean/types.rssrc/librustdoc/clean/mod.rssrc/librustdoc/json/implied_bounds.rssrc/librustdoc/json/conversions.rsWork deferred to future PRs
T: 'acan be an implied bound,'a: 'bcan be an implied bound too. The former is implemented in this PR. To limit scope, I intend to open a separate PR for the latter after this one is merged.Sizedat its use site. Fixing this seemed like a big lift for a somewhat uncommon case, so I felt it's best tackled later — perfect being the enemy of good enough and all.Open questions for this PR
DocContextuse outside ofclean. Per T-rustdoc recommendations, this implementation moves all implied bounds computation into the JSON component. In turn that means the JSON-side code needs to do some cleaning of the clauses that represent those implied bounds. To avoid duplication, the current implementation reuses thecleancode — but that requires aDocContextwhich is not otherwise available in the JSON component.DocContextwhen needed — which may be often, and may not be a trivial cost. I'm not sure!cleaninto the JSON component, adjusting it to not needDocContext. This isn't great either.DocContextinsideJsonRenderersomehow. This isn't great either, becauseJsonRendereris passed around by immutable reference, whilecleanuses&mut DocContext. We may need eitherRefCellorMutexor lots of plumbing to replace&JsonRendererwith&mut JsonRenderer.Included tests
Fn/FnMut/FnOnceandAsyncFn/AsyncFnMut/AsyncFnOnceimplied bounds preserve full parenthesized signatures.Sizedbounds as a result of Rust language rules:Sizeddefault when?Sizedis not present, such as in generic parameters, associated types, etc.Sizedrequirement for function parameters, return values, and contents of slices and arrays.Sizedrequirement when a possibly-DST struct or tuple (e.g.struct S<T: ?Sized>(T);) is used in a position that requires it beSized, like a function parameter or return value.Sizedif such a possibly-DST type is used behind indirection, like&/&mut/*const/*mut.bounds(explicit, syntactically-included bounds) orimplied_bounds(implied or elaborated bounds), never both.r? fmease
cc @aDotInTheVoid