[rustdoc-json] Show whether ?Sized parameters are actually Sized

[obi1kenobi]

An attempt at fixing #143197 for rustdoc JSON only, in order to streamline the review process. Any HTML changes will happen separately.

• Add a new allow_unsized: bool field to GenericParamDefKind::Type.
• Expose that field as-is to rustdoc JSON, without tampering with ?Sized clauses there.

r? fmease

[rustbot]

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

[obi1kenobi]

cc @aDotInTheVoid

[fmease]

I know this is WIP but if I may let me collect initial perf data (I've only skimmed the changes and haven't reviewed anything yet).

[@]bors2 try [@]rust-timer queue

[rust-bors]

⌛ Trying commit bba78b8 with merge 55a9833…

To cancel the try build, run the command @bors2 try cancel.

[rust-bors]

☀️ Try build successful (CI)
Build commit: 55a9833 (55a98339ad460f9063d820eb23c41710d6f6e5b9, parent: ca98d4d4b3114116203699c2734805547df86f9a)

[rust-timer]

Finished benchmarking commit (55a9833): 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)	1.2%	[0.2%, 3.1%]	20
Regressions ❌ (secondary)	2.0%	[0.4%, 3.9%]	25
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.2%	[0.2%, 3.1%]	20

Max RSS (memory usage)

Results (primary 1.6%, secondary 2.4%)

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

	mean	range	count
Regressions ❌ (primary)	1.6%	[1.2%, 2.0%]	6
Regressions ❌ (secondary)	2.4%	[1.4%, 3.9%]	3
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.6%	[1.2%, 2.0%]	6

Cycles

Results (primary 2.4%, secondary 2.7%)

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

	mean	range	count
Regressions ❌ (primary)	2.4%	[2.0%, 2.9%]	6
Regressions ❌ (secondary)	3.1%	[2.4%, 3.7%]	15
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.8%	[-2.8%, -2.8%]	1
All ❌✅ (primary)	2.4%	[2.0%, 2.9%]	6

Binary size

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

Bootstrap: 463.852s -> 465.016s (0.25%)
Artifact size: 372.07 MiB -> 372.15 MiB (0.02%)

[aDotInTheVoid]

(I've not looked at the impl).

I agree that we should probably remove these misleading bounds in HTML.

But I wounder if we should do the same in JSON? Rather than having a separate bool field?

[aDotInTheVoid]

This should test that we don't remove the ?Sized bound from the bounds list, even though it ends up not mattering.

[obi1kenobi]

But I wonder if we should do the same in JSON? Rather than having a separate bool field?

It's a good question. I think there are two broadly-speaking reasonable choices, and one option we should avoid.

We should avoid stripping ?Sized without including a way to say we did so. If we did that, cargo-semver-checks will get flooded with incorrect "false-positive" bug reports of people saying "look ?Sized is right here but c-s-c says I removed it." We'll probably end up having to try to parse source code to figure out if ?Sized is there so we can customize the diagnostic message to explain why the ?Sized bound doesn't matter. This isn't great and I'd love to avoid it if we can. I bet cargo-public-api would have the same issue, so I don't think it's unique to us.

Then the two broadly-acceptable options are both adding a bool, and differ over what the bool says:

• leave ?Sized in but say if unsized types are disallowed despite it (this PR)
• strip ?Sized but say if it was originally present

I'm ambivalent between the two options, and could easily be talked into going for the second one if you have a strong preference that way.

[obi1kenobi]

Taking a look at the benchmarks, I'm surprised that e.g. a simple "hello world" program takes 3% more instructions to document when there are no generics in it. This seems suspect — I wouldn't expect my code changes to get hit basically at all, let alone to the tune of a 3% slowdown.

[aDotInTheVoid]

Then the two broadly-acceptable options are both adding a bool, and differ over what the bool says:

• leave ?Sized in but say if unsized types are disallowed despite it (this PR)
• strip ?Sized but say if it was originally present

I'm strongly in favour if the first option. That way, consumers arn't required to look at that field if they don't need to.

I agree that we should probably remove these misleading bounds in HTML.

EDIT: Stuff in here was incorrect, see fmeases comment below

I'm less sure about this now. If I have

pub fn foo<T: othercrate::Trait + ?Sized>(_t: T)

And othercrate goes from:

pub trait Trait {}

to

pub trait Trait: ?Sized {}

than that changes the meaning of my code, and the ?Sized bound on foo now matters.

At the very least, the HTML change will want to go through a T-rustdoc-frontend fcp/poll. It's worth spinning of to a seperate PR, to unblock the JSON change.

[fmease]

pub trait Trait: ?Sized {} isn't allowed, traits are ?Sized by default and for a trait to go from trait Trait {} to trait Trait: Sized {} would be a breaking change on its own.

[obi1kenobi]

Updated to only include rustdoc JSON changes as suggested.

If required, I plan to open a PR with the HTML changes separately after we have a final outcome on this PR. I've saved the old contents of the branch (including HTML changes and tests) under a different branch name.

I believe this PR should now pass the rustdoc JSON test suite, and leave the HTML tests unchanged.

I'm not sure what to do about the perf change, though it's possible removing the HTML changes has had some impact. It may be prudent to re-measure before we decide.

@rustbot label -T-rustdoc-frontend

[rustbot]

rustdoc-json-types is a public (although nightly-only) API. If possible, consider changing src/librustdoc/json/conversions.rs; otherwise, make sure you bump the FORMAT_VERSION constant.

cc @CraftSpider, @aDotInTheVoid, @Enselic, @obi1kenobi

These commits modify tests/rustdoc-json.
rustdoc-json is a public (but unstable) interface.

Please ensure that if you've changed the output:

• It's intentional.
• The FORMAT_VERSION in src/librustdoc-json-types is bumped if necessary.

cc @aDotInTheVoid, @obi1kenobi

[aDotInTheVoid]

@bors2 try @rust-timer queue

I’m happy this is the correct approach/behaviour/public api, but haven’t reviewed the implementation (yet). I’ll either do that in a week when I’m back from camping, or you could ask fmease (or someone else on T-Rustdoc) to do it beforehand.

[rust-bors]

☀️ Try build successful (CI)
Build commit: c1707dd (c1707dd4e3487b87d643b25dd6de03155310caab, parent: e8a792daf500b5ff8097896ddb6cc037abe92487)

[rust-timer]

Finished benchmarking commit (c1707dd): 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.9%	[0.3%, 2.0%]	16
Regressions ❌ (secondary)	1.3%	[0.3%, 2.8%]	26
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.9%	[0.3%, 2.0%]	16

Max RSS (memory usage)

Results (primary 1.8%)

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

	mean	range	count
Regressions ❌ (primary)	1.8%	[1.6%, 2.1%]	4
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.8%	[1.6%, 2.1%]	4

Cycles

Results (primary 2.8%, secondary 2.0%)

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

	mean	range	count
Regressions ❌ (primary)	2.8%	[2.1%, 3.6%]	3
Regressions ❌ (secondary)	2.8%	[2.2%, 3.7%]	13
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.6%	[-4.0%, -3.2%]	2
All ❌✅ (primary)	2.8%	[2.1%, 3.6%]	3

Binary size

Results (secondary 0.0%)

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.0%	[0.0%, 0.0%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Bootstrap: 471.66s -> 469.982s (-0.36%)
Artifact size: 378.20 MiB -> 378.25 MiB (0.01%)

[bors]

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

[aDotInTheVoid]

This seems suspect — I wouldn't expect my code changes to get hit basically at all, let alone to the tune of a 3% slowdown.

Perf effects seem real. Possibly due to increasing the size of clean:: data structures. I think it's worth trying the proposed optimization in the comments, but it might not help if the slowdown isn't from running more code, but less compact data structures.

[aDotInTheVoid]

@fmease would you mind reviewing this bit? I'm not familiar enough with how rustc/rustdoc represent generic bounds to be confident saying this is correct.

[fmease]

Thanks for taking a stab at implementing your proposal!

In my opinion, this PR is not ready to be merged yet because adding a flag to type parameters is not the correct approach as I've outlined in my review comments.

Moreover, it would be great if we could slightly improve the perf (I haven't looked at the impl yet, will do so later or some other time). I can say that these regressions are definitely not spurious. From experience I know that adding any query calls to hot cleaning procedures will almost certainly regress perf due to query call overhead (if green) or actual computation (if red) (here: of is_sized_raw and evaluate_obligation most likely, looking at the report)!

This might be a first, but ideally we'd gate that extra computation behind "backend==HTML" inside clean if this PR remains perf-heavy.

View changes since this review

[fmease]

This flag doesn't scale to associated types. Relaxed bounds are not just permitted on type parameters but also in the so-called item bounds of associated types. How would you represent allow unsized for

pub trait Trait { type Type: Bound + ?Sized; }
//                                   ^^^^^^ effective or ineffective?
pub trait Bound/*: Sized*/ {}

[fmease]

Side note: This flag doesn't scale to (A) Sized Hierarchy (disclaimer: the RFC is still unapproved; impl unstable+incomplete) which gets rid the imprecise notion of "unsized" or (B) to the addition of other default traits apart from Sized (disclaimer: only proposed so far in various venues; e.g., RFC 3783; impl internal+incomplete) and consequently also relaxed bounds (e.g., ?Move, ?Leak) which may likely appear in more places (e.g., in trait object types: dyn Trait + ?AutoBound)

[fmease]

This flag doesn't scale to existential impl-Trait like RPIT(IT)s. How would you represent allow unsized for

pub fn make() -> Box<impl Bound + ?Sized> {
//                                ^^^^^^ effective or ineffective?
    Box::new([]) as Box<[_]> // (1)
    //Box::new([]) // (2)
}

pub trait Bound {} // (1)
impl Bound for [u8] {} // (1)

//pub trait Bound: Sized {} // (2)
//impl<const N: usize> Bound for [u8; N] {} // (2)

[fmease]

Given these shortcomings, I would strongly advise against taking this approach / representation, even for an MVP.

Moreover, I know that your goal is to make rustdoc identify arbitrary redundant bounds (e.g., the Clone in Copy + Clone), so any scheme should be extensible enough to be able to represent this eventually I would say, wouldn't you?

---

The obvious alternative approach would be to add a redundant: bool (redundant?: boolean) to GenericBound::Trait (well, and to GenericBound::Outlives) which should be general enough to represent all aforementioned cases IINM.

[obi1kenobi]

Moreover, I know that your goal is to make rustdoc identify arbitrary redundant bounds (e.g., the Clone in Copy + Clone)

I'd say that's "nice to have" at best. With our current approach for finding type-related breakage, this would likely be a nice optimization but not more than that. Happy to discuss this in more detail if useful!

so any scheme should be extensible enough to be able to represent this eventually I would say, wouldn't you?

In light of the above, I think this is nice to have but not required.

I'm open to reworking the scheme as the language and our needs evolve, for example if the sized hierarchy proposal goes through.

The obvious alternative approach would be to add a redundant: bool (redundant?: boolean) to GenericBound::Trait (well, and to GenericBound::Outlives) which should be general enough to represent all aforementioned cases IINM.

I'm not sure I quite understand this proposal.

In the case of T: Copy + Clone, T: Clone is redundant.

In the case of T: ?Sized + ImplySized, T: ?Sized is not redundant — it's inapplicable. Would the additional boolean field be true in this case too?

Assuming so, I'm afraid I don't know how to have rustc determine whether a bound is redundant in the general case. This PR is already at the limit of my understanding of the rustc internals 😅 If you're open to driving that approach forward, I'd be thrilled to use it. But if you don't have cycles for it, I'd prefer something more limited that unblocks this work — even if we know we need to extend or tweak it in the future. Having rustdoc JSON be unstable gives us that freedom, and I'd like us to use it.

[fmease]

Respectfully, don't overindex on the "Copy+Clone" case¹ or the word "redundant". In the examples I used the word "ineffective" instead but that also doesn't quite generalize. Just pick "bikeshed_property" in your head.

What's far more important is the assoc ty bounds case and the existential impl-Trait (RPIT(IT), TAIT, ATPIT) case which demonstrates quite clearly that a Boolean flag on ty param defs simply doesn't model the domain sufficiently / correctly. I'm sorry to say, but I simply won't accept that. It's a property of an individual bound, that's what I was trying to get at.

Footnotes

1. I.e., checking if the explicitly user-written bounds contain bounds already implied by any supertrait bounds. ↩

[obi1kenobi]

Apologies — I think I did a poor job explaining. Let me try again.

I believe that the case of "explicitly user-written bounds contain bounds already implied by any supertrait bounds" is merely an optimization and not important for cargo-semver-checks. This encompasses any T: A + B for traits A, B such that A: B, but does not cover ?Bound cases. The plan for semver-checking T: A + B cases is to use a witness program to test whether the old and new bounds are compatible by asking rustc to compile the witness and observing success or failure.

Implied bound relaxations like ?Sized can't be handled in the same way — I don't know of a way to generate a witness here. Adding bounds is compatible with our plan for generating witnesses, but subtracting them like ?Bound is not. This is why we need to know whether ?Bound actually supports non-Bound cases or not.

What's far more important is the assoc ty bounds case and the existential impl-Trait (RPIT(IT), TAIT, ATPIT) case which demonstrates quite clearly that a Boolean flag on ty param defs simply doesn't model the domain sufficiently / correctly.

I agree with this. I failed to mention it last time, but I'm happy to extend the approach to cover assoc ty and existential cases. Those just weren't on my radar when I opened this PR, so thanks for bringing them up!

[fmease]

I'd strongly prefer if we didn't compute this for the HTML backend since it isn't used there (yet, if ever) and since it's still too perf-heavy for my liking.

[fmease]

(unused trait impl as far as I can tell)