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
|
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.
src/librustdoc/clean/mod.rs
Outdated
| 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.
There was a problem hiding this comment.
I think it's based on my misunderstanding of the unstable TAIT feature, see below. I expect this will change meaningfully as I fix that up.
src/librustdoc/clean/mod.rs
Outdated
| 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?
There was a problem hiding this comment.
I think what happened here is that I failed to realize that #[define_opaque(Opaque)] is required, then misintepreted the compiler error I got back as a result.
In my defense, I did call out my unfamiliarity with unstable Rust features a risk factor in this PR originally 😅
I'll fix this.
There was a problem hiding this comment.
Wait, hang on, I just noticed trait Trait: Sized in your example. So I still think I was wrong, but not for the reason I thought above.
type Opaque = impl Trait + ?Sized; in your example already contains a Sized bound via Trait. We don't need to worry about the use site gaining implicit Sized without an explicit bound, because this isn't legal:
#![feature(type_alias_impl_trait)]
type Opaque = impl std::fmt::Debug + ?Sized;
#[define_opaque(Opaque)]
fn define() -> Opaque {}
// error[E0277]: the size for values of type `Opaque` cannot be known at compilation time
// --> src/lib.rs:6:16
// |
// 6 | fn define() -> Opaque {}
// | ^^^^^^ doesn't have a size known at compile-time
// |
// = help: the trait `Sized` is not implemented for `Opaque`
// = note: the return type of a function must have a statically known size
//
// For more information about this error, try `rustc --explain E0277`.Contrast this with:
trait MaybeUnsized<T: ?Sized> {
// T can be unsized here
fn by_ref(_: &T);
// implied `where T: Sized` here;
// we want to show that as an implied bound
fn by_value(_: T);
}where T in by_value has an implied Sized bound despite not having any bound. This is what my code meant by "use-site check." But this is wrong because ... 👇
As I wrote the above, I realized this is also legal:
#![feature(type_alias_impl_trait, trivial_bounds)]
trait Trait: Sized {}
impl Trait for () {}
type Opaque = impl std::fmt::Debug + ?Sized;
#[define_opaque(Opaque)]
fn define() -> Opaque where Opaque: Trait {}So the implied bounds on "the return value of define" are still not the same as the implied bounds on "Opaque the TAIT", and we still need what I had been calling a use-site check.
Sigh. I think I mentioned I'm not a type system expert when we first chatted about me opening a PR here, and I think that's why we're on the struggle bus here.
I'll fix this, and I will add even more test cases.
| 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 trait, 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.
| 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).
Edit: Please see #148379 (comment) first before trying to rewrite this. IMO, this fn shouldn't exist in the first place!
| || 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).
Edit: Please see #148379 (comment) first before trying to rewrite this. IMO, this fn shouldn't exist in the first place!
|
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
|
Thanks for taking a look @fmease! As you likely remember, this is my first PR that reaches this deep into rustc internals concerned with type-checking and such, so I appreciate the pointers to other places where I should look to reuse code from. For example, I missed the visitors, even though in retrospect it seems obvious such a system had to exist 😅 I'll do a cleanup pass aiming to fix up the issues you've flagged so far, and hopefully that'll make the PR as a whole easier to review! |
|
@rustbot label -S-waiting-on-review +S-waiting-on-author |
rustbot
added
S-waiting-on-author
There was a problem hiding this comment.
From #148379 (comment):
trait MaybeUnsized<T: ?Sized> { // T can be unsized here fn by_ref(_: &T); // implied `where T: Sized` here; // we want to show that as an implied bound fn by_value(_: T); }
Now I understand why you're recursing into HIR/middle types to find out whether they "require Sized".
This is kinda funny, I never thought of this as an "implied bound", and neither does rustc to be fair. What's actually going on in rustc / Rust is that we require all locals (incl. fn params) to be Sized (w/ exceptions like let _: [u8]; which is legal). However, in trait fns without body, rustc doesn't check that at the def-site, maybe call it an oversight.
My immediate reaction to recursing into Ty to find out whether it "requires Sized" in a trait fn w/o body, is let's not. This is a lot of complexity that possibly has false positives, needs maintenance and doesn't have much gain (I'll clarify later).
To be clear, I guess I'm fine with calling the lack of Sized checking in such cases "implied bounds". However, there's a much simpler way to express this: Just add $ty: Sized for all params of type $ty in trait fns w/o body. This is 100% correct: So for by_ref in the example above, add &T: Sized (trivially true but that's fine); for by_value, add T: Sized (if the arg ty is Box<T>, it's Box<T>: Sized, if it's str it's str: Sized or for<'_delay> str: Sized, etc.).
doesn't have much gain
I don't know if you really want these bounds to be "normalized" / as "simple" as possible (so [`&T: Sized`] → [], [`Adt<T>: Sized`] → [`T: Sized`] (depends on the def of Adt)) for C-S-C. I don't think that's valuable since C-S-C needs to (eventually?) know anyway that changing e.g. pub fn f<T: ?Sized>(_: &T) where &T: Sized {} to pub fn f<T: ?Sized>(_: &T) {} is not a breaking change.
Edit: If you only care about paramater types if they reference type parameters, you don't actually need to add $ParamTy: Sized for all param types $ParamTy. See #148379 (comment). I think you only need to do so for bare type parameters T since well-formedness checking takes care of the other cases.
There was a problem hiding this comment.
Thanks for the super detailed and very prompt feedback, I really appreciate it! Also thanks for bearing with me being a n00b :)
My plan is to (1) follow your "let's not" advice and (2) try to descope this PR so it's easier on both of us. Injecting non-normalized bounds like &T: Sized would be great, but they aren't crucial to have right now for c-s-c. We can easily start without them, and I think we should.
I have some travel coming up, so I'll push as many fixes as I can before I fly out but I might not manage to get everything. I'm happy to get more partial reviews if you have time, and otherwise I'll just pick up where I left off after I get home.
| ty: Ty<'tcx>, | ||
| target_param: ParamTy, | ||
| allow_unsized: bool, | ||
| ) -> bool { |
There was a problem hiding this comment.
Irrespective of the fact that I think this whole fn probably shouldn't exist, I have some questions re. its impl
(minor) For my understanding, the name of param_requires_sized_in_ty is slightly misleading, isn't it? It's more like param_implicitly_requires_sized_in_ty as it decides whether we should add an "implied" T: Sized?
I get why T → true and &T → false (1: T: Sized only if T: Sized, 2: &T: Sized trivially true). However, let's look at other type constructors.
To repeat myself, while it's true that Rust doesn't require the fn params of trait fn w/o body to be Sized, it still requires all types to be well-formed (ignoring a bunch known bugs and some other tiny exceptions). E.g., slice and array ty ctors always require their type argument to be Sized, so [T] and [T; N] are illegal if T doesn't impl Sized, that's also true in trait fn w/o body, meaning compilation would stop early and never reach this code. As you know, for ADTs type arguments aren't necessarily required to be Sized, so Adt<T> (we're inside a trait Trait<T: ?Sized>) is either illegal or it isn't depending on the definition of Adt.
However, you're actually recursing for Slice and Array (walking their argument) and recursing for structs (visiting their fields). Unless I'm blindingly missing something, that shouldn't be necessary at all. Given [T], this fn will never return true since the type checker would've already rejected this type if T needed Sized (it indeed isn't "implied"). Given Struct<T>, same deal. It should never return true.
(the same should apply to opaque tys btw)
tests/rustdoc-json/implied-bounds/bounded-generic-type-alias-impl-trait.rs
Show resolved
Hide resolved
obi1kenobi
force-pushed
the
pg/implied-bounds
branch
2 times, most recently
from
53cce9d to
1479c8e
Compare
obi1kenobi
force-pushed
the
pg/implied-bounds
branch
from
1479c8e to
e4e998c
Compare
There was a problem hiding this comment.
Cleaned up a bunch based on preliminary feedback from fmease, including:
- all test cases
cargo checkindependently, and have constrained generics - no more HIR walking to compute extra "implied" bounds on our own
- reusing more
cleancode
This cut quite a bit of code and complexity as a result.
Time permitting, I'd love another review pass! I'll be travelling for a little while and I look forward to coming back to this as soon as I'm back.
@rustbot label -S-waiting-on-author +S-waiting-on-review
rustbot
added
S-waiting-on-review
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