rustdoc JSON: Add implied bounds to generic types, impl Trait, and assoc types.

[obi1kenobi]

View all comments

Include implied and elaborated (henceforth, implied) bounds in the rustdoc JSON representations of associated types, impl Trait types, and generic type parameters. Implemented as a new implied_bounds: Vec<GenericBound> field, alongside the existing bounds: 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 where clauses (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 clean and elsewhere. This is intended to mitigate possible performance impact on rustdoc as a whole, while providing this data to JSON-based workloads like cargo-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:

• Get a sense of the overall API:
  • src/rustdoc-json-types/lib.rs
  • src/librustdoc/clean/types.rs
  • src/librustdoc/clean/mod.rs
• Review how implied bounds are computed
  • src/librustdoc/json/implied_bounds.rs
  • src/librustdoc/json/conversions.rs
• Review remainder of implementation, which is predominantly plumbing
• Test suite

Work deferred to future PRs

• Implied outlives bounds. Just like T: 'a can be an implied bound, 'a: 'b can 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.
• Implied bounds for RTN. I'm open to implementing this too, but again I wanted to limit scope for this PR. RTN isn't stable yet anyway, so I felt rustdoc JSON support for its implied bounds could wait.
• Precise implied bounds analysis for extern opaques. Currently, we don't seem to have sufficient metadata about opaques coming from other crates to determine e.g. whether the opaque is implied to be Sized at 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

• DocContext use outside of clean. 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 the clean code — but that requires a DocContext which is not otherwise available in the JSON component.
  • Currently, we construct a DocContext when needed — which may be often, and may not be a trivial cost. I'm not sure!
  • Instead, we could duplicate the code we need from clean into the JSON component, adjusting it to not need DocContext. This isn't great either.
  • Instead, we could put a DocContext inside JsonRenderer somehow. This isn't great either, because JsonRenderer is passed around by immutable reference, while clean uses &mut DocContext. We may need either RefCell or Mutex or lots of plumbing to replace &JsonRenderer with &mut JsonRenderer.
  • Possibly other options? I'd love reviewer feedback on this!
• List of stable lang-item traits. I wasn't able to find a way to get a list of stable lang-item traits that we should consider for implied bounds analysis. We currently have a hard-coded list of them, which may be brittle as new lang-item traits are stabilized. If there's a better way to do this, I'm all ears.

Included tests

• Generic type params, associated types, TAITs, RPIT (both regular and in async), ATPIT/GAT cases, and assorted DST/sizedness edge cases.
• Implied bounds both on and originating from trait associated item constraints: both equality and constraints.
• Fn/FnMut/FnOnce and AsyncFn/AsyncFnMut/AsyncFnOnce implied bounds preserve full parenthesized signatures.
• Implied Sized bounds as a result of Rust language rules:
  • Sized default when ?Sized is not present, such as in generic parameters, associated types, etc.
  • Sized requirement for function parameters, return values, and contents of slices and arrays.
  • Transitive Sized requirement when a possibly-DST struct or tuple (e.g. struct S<T: ?Sized>(T);) is used in a position that requires it be Sized, like a function parameter or return value.
  • No implied Sized if such a possibly-DST type is used behind indirection, like &/&mut/*const/*mut.
• Checks that bounds appear either in bounds (explicit, syntactically-included bounds) or implied_bounds (implied or elaborated bounds), never both.

r? fmease

cc @aDotInTheVoid

[rustbot]

fmease is currently at their maximum review capacity.
They may take a while to respond.

[obi1kenobi]

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 implied_outlives to generic lifetime parameters in PR vs in a future one.

[aDotInTheVoid]

i’ll take a look at the code later, but for now:

@bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: 972828a (972828af4644b91a03dc856d943eb07c56de72a0, parent: bd3ac0330018c23b111bbee176f32c377be7b319)

[rust-timer]

Finished benchmarking commit (972828a): comparison URL.

Overall result: ❌✅ regressions and improvements - please read the text below

Benchmarking 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 @rustbot label: +perf-regression-triaged. If not, please fix the regressions and do another perf run. If its results are neutral or positive, the label will be automatically removed.

@bors rollup=never
@rustbot label: -S-waiting-on-perf +perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	0.4%	[0.1%, 3.0%]	14
Regressions ❌ (secondary)	0.3%	[0.1%, 0.4%]	20
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.1%	[-0.2%, -0.1%]	2
All ❌✅ (primary)	0.4%	[0.1%, 3.0%]	14

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.

	mean	range	count
Regressions ❌ (primary)	2.0%	[1.2%, 3.1%]	12
Regressions ❌ (secondary)	1.6%	[1.2%, 2.7%]	9
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.0%	[1.2%, 3.1%]	12

Cycles

Results (primary 2.6%, secondary -4.2%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	2.6%	[2.6%, 2.6%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-4.2%	[-4.6%, -3.9%]	2
All ❌✅ (primary)	2.6%	[2.6%, 2.6%]	1

Binary size

Results (primary 1.1%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	1.1%	[1.1%, 1.1%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.1%	[1.1%, 1.1%]	1

Bootstrap: 474.434s -> 474.73s (0.06%)
Artifact size: 390.87 MiB -> 390.95 MiB (0.02%)

[aDotInTheVoid]

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?

View changes since this review

[aDotInTheVoid]

N.B. This isn't tested, as rustdoc json doesn't inline. I'm pretty certain it's correct, but worth flagging.

[obi1kenobi]

Could you move all the implied-bounds-*.rs tests into a new implied-bounds/ folder?

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.

[bors]

☔ The latest upstream changes (presumably #139558) made this pull request unmergeable. Please resolve the merge conflicts.

[rust-timer]

Finished benchmarking commit (0e2f486): comparison URL.

Overall result: ❌ regressions - please read the text below

Benchmarking 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 @rustbot label: +perf-regression-triaged. If not, please fix the regressions and do another perf run. If its results are neutral or positive, the label will be automatically removed.

@bors rollup=never
@rustbot label: -S-waiting-on-perf +perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	0.2%	[0.1%, 0.3%]	5
Regressions ❌ (secondary)	0.2%	[0.1%, 0.4%]	15
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.2%	[0.1%, 0.3%]	5

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.

	mean	range	count
Regressions ❌ (primary)	1.1%	[0.8%, 1.4%]	3
Regressions ❌ (secondary)	1.2%	[1.2%, 1.2%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.0%	[-2.7%, -1.3%]	6
All ❌✅ (primary)	1.1%	[0.8%, 1.4%]	3

Cycles

Results (primary -3.4%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-3.4%	[-3.4%, -3.4%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-3.4%	[-3.4%, -3.4%]	1

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 473.061s -> 470.796s (-0.48%)
Artifact size: 383.58 MiB -> 383.68 MiB (0.03%)

[obi1kenobi]

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!

[obi1kenobi]

@rustbot label -S-waiting-on-review +S-waiting-on-author

[fmease]

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.

[obi1kenobi]

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.

[fmease]

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)

[obi1kenobi]

Cleaned up a bunch based on preliminary feedback from fmease, including:

• all test cases cargo check independently, and have constrained generics
• no more HIR walking to compute extra "implied" bounds on our own
• reusing more clean code

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

View changes since this review

[obi1kenobi]

I'm back from my trip and ready to pick this back up, pending reviewer availability ofc 👍

[fmease]

I try to get to this before the end of the week & I'll try my hardest to get this PR to ship sooner than later 🤞 Don't hesitate to ping me.

[obi1kenobi]

Thank you, I appreciate it! I'll try to keep it free of merge conflicts in the meantime.

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.