rustdoc: hide #[repr] if it isn't part of the public ABI

[fmease]

Follow-up to #115439.
Unblocks #116743, CC @dtolnay.
Fixes #128364.
Fixes #137440.

Only display the representation #[repr(REPR)] (where REPR is not Rust or transparent) of a given type if the type is not #[non_exhaustive], none of its variants (incl. the synthetic variants of structs) are #[doc(hidden)] or #[non_exhaustive] and all of its fields are public and not #[doc(hidden)] since it's likely not meant to be considered part of the public ABI otherwise.

--document-{private,hidden}-items works as expected in this context, too.

Moreover, we now also factor in the presence of #[doc(hidden)] and of #[non_exhaustive] when checking whether to show repr(transparent) or not.

[rustbot]

r? @notriddle

(rustbot has picked a reviewer for you, use r? to override)

[rustbot]

Some changes occurred in src/librustdoc/clean/types.rs

cc @camelid

[rustbot]

Some changes occurred in GUI tests.

cc @GuillaumeGomez

[GuillaumeGomez]

I think I'd go for any in both cases.

[fmease]

@RalfJung, does it sound good to you as well to consider the repr public if there exists at least one struct field that is public (there might be private and hidden ones) (if we have a struct) or if there exists at least one non-hidden enum variant (if we have an enum)? (With the extra rule that empty structs and enums also render the repr public).

Or should all fields (current version of this PR) and enum variants be public for the repr to be public?

[RalfJung]

Usually for structs, if there is at least one private field then we say you can't rely on the struct staying how it is. For instance if your repr(transparent) relies on another type being a ZST and that type has at least one private field, we warn about that (and we eventually want to make that an error).

So I'd say the same should go for the repr. If any field is private, then the repr is (by default) private.

[fmease]

Thanks, that makes sense! What about enum variants? Can users still make certain assumptions about the repr of an enum if some but not all of its variants are private or hidden?

[RalfJung]

There's no such things as private enum variants (unfortunately).

[dtolnay]

on a struct with a doc(hidden) field, should the repr be shown?

Here is an example of what goes wrong if you show repr on a struct with doc(hidden) field.

#[repr(C)]
pub struct Struct {
    pub first: u8,
    #[doc(hidden)]
    pub second: u16,
    pub third: u8,
}

Rustdoc purports a repr(C) struct in which the first byte is first, the second byte is third, and some other fields follow. Given the Rust-like syntax in which rustdoc shows #[repr(C)], this feels misleading. For a struct that is actually this:

#[repr(C)]
pub struct Struct {
    pub first: u8,
    pub third: u8,
    // ... other fields ...
}

one would expect they can cast &Struct to &[u8; 2] and read first and third from it. If they do that in this case though, they get UB from looking at a padding byte.

I think this would be a useful bar to keep in mind as a minimum desirable property; rustdoc should not show a repr in such a way that misleads reader about reality. That does not necessarily need to mean hiding such reprs, though that might be the most expedient path forward. Alternatively rustdoc could be more discerning about placing the /*private field*/ comment in between the correct fields when there is a repr.

[fmease]

Hmm, do we already track this in a GitHub issue? With the introduction of core::mem::offset_of, it feels like we should up the priority of this issue. If I remember correctly, it'd need quite a bit of rewiring inside rustdoc to render /* private field */ in the correct order for repr(C) structs since those fields are stripped early at the moment.

[fmease]

RalfJung, re taking doc(hidden) on variants into account when computing the visibility of a repr, I've included that in the heuristic to hide the repr(u8) on core::ffi::c_void which has consists of two doc(hidden) enum variants.

[RalfJung]

Sounds to me then like we'd want to hide the repr as soon as there is any hidden field (just like we hide it as soon as there is any private field) -- both for struct and enum (and union).

[GuillaumeGomez]

Seems like there is more to discuss. I'll add it to the next rustdoc team meeting agenda.

[camelid]

In discussing this as part of the rustdoc meeting, I realized we probably need to account for whether #[non_exhaustive] has been applied to the struct/enum/etc. If it is, then the API isn't stable.

[bors]

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

[dtolnay]

This appears to be stalled on everyone agreeing on a comprehensive set of rules.

Since I ran into this again today in #128364, I'll try to suggest a way to make progress again: Is it possible we can agree to err on the side of not showing repr? Let's make rustdoc render repr in only the most incontrovertible circumstances: everything is pub, nothing is hidden, there is at least 1 field, there is at least one variant, etc. Feel free to add as many other restrictions here as it takes until everyone agrees that we've reached an overestimation of what the actual rules should be.

After that, we can incrementally agree to additional deliberate cases where repr should be made part of the documented API of a type. Each of these can be FCP'd with the team if needed.

The current state of erring on the side of rendering repr in too many cases that have not been agreed to is unfortunate.

[GuillaumeGomez]

I think it's good like this. Like @dtolnay mentioned, we can always add new rules later on. Should we start the FCP?

[fmease]

FCP

Not quite yet, I still need to update the approach, PR description and PR itself :) I'll do so in a moment. I've only rebased.

[fmease]

Thinking back to past discussions, one reason for being liberal in showing / conservative in hiding #[repr(...)] was the fact that there's currently no way to override this heuristic, i.e., to force rustdoc to show the repr. Ideas about a #[doc(repr(...))] were floating around.

Without it, users would need to declare this information in prose instead.

Thinking aloud, I guess it does make sense as a first step even if it's not the greatest (e.g., repr packed and C can be meaningful (as part of the public ABI) even if e.g. later fields are private/hidden as they don't necessarily influence the alignment and thus the offset of earlier public fields (for e.g., core::mem::offset_of)).

[bors]

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

[GuillaumeGomez]

@rfcbot resolve positive-bugs in cargo-semver-check

[fmease]

We now also consider #[non_exhaustive]. See the updated PR/FCP description.

@rustbot review

[fmease]

Checking if there's noticeable query invocation overhead.
@bors2 try @rust-timer queue

[rust-bors]

⌛ Trying commit 94911f1 with merge 5e6424b…

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

[rust-bors]

☀️ Try build successful (CI)
Build commit: 5e6424b (5e6424bb0d0c85ce18fab29ccab5952ddb21b06b, parent: 3bc767e1a215c4bf8f099b32e84edb85780591b1)

[obi1kenobi]

Just checking: will this affect the JSON output as well, or only HTML?

Also, I wasn't able to find any discussion of why e.g. repr(i32) on a non_exhaustive enum would be considered non-public. I did see the c_void being shown as repr(u8) issue, which is certainly unfortunate. But perhaps blanket-banning everything #[non_exhaustive] might be an over-correction?

[fmease]

I'm not married to the non-exhaustive logic, it was suggested here: #116882 (review)

[fmease]

Just checking: will this affect the JSON output as well, or only HTML?

As previously discussed, we don't apply these heuristics if the output format is JSON.

[obi1kenobi]

As previously discussed, we don't apply these heuristics if the output format is JSON.

Thanks! It didn't look to me like it was affected either, but I wanted to err on the not-unsafe side :)

I'm not married to the non-exhaustive logic, it was suggested here: #116882 (review)

This is totally not my area of responsibility, so feel free to decide what you think is best. But here's my counter-argument.

Say I'm building a state machine that represents some computational work being performed on a cluster, something like GitHub Actions for example. There are different states jobs could be part of:

#[repr(i8)]
pub enum JobState {
    NotStarted,
    Scheduling,
    Running,
    Completed,
    Errored,
}

I want it to be public API that this enum is repr(i8). But no system is set in stone forever — I may wish to add other states like Cancelled, or maybe HardwareNotAvailable if GPUs are required but none are free at the moment. So I'd also like the enum to be #[non_exhaustive].

What I'd like to commit to in the public API is this:

• there will not be more than 256 states
• the currently-defined states turn into numbers well-defined in both value and width
• more states may be added in the future, subject to the previous two constraints.

If #[repr(i8)] and #[non_exhaustive] in rustdoc are mutually-exclusive, I'm forced to pick one. I don't like that! It makes both of those attributes less useful. In particular, if we end up with people avoiding #[non_exhaustive] over this, that will in turn cause more SemVer breakage and more (avoidable) major bumps!

Between this case and the repr(transparent) one, I really think we need a "is this public API" modifier for repr. I'd love to see that RFC'd. In the meantime, if I had to pick, I'd instead rely on "users are expected to read the textual description of a type when looking at docs.rs, not just look at the signature".

[rust-timer]

Finished benchmarking commit (5e6424b): comparison URL.

Overall result: no relevant changes - no action needed

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.

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

Instruction count

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

Max RSS (memory usage)

Results (primary 5.1%, 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)	5.1%	[5.1%, 5.1%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.7%	[-3.0%, -2.3%]	2
All ❌✅ (primary)	5.1%	[5.1%, 5.1%]	1

Cycles

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

Binary size

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

Bootstrap: 757.216s -> 756.086s (-0.15%)
Artifact size: 372.11 MiB -> 372.12 MiB (0.00%)