Hide #[repr(transparent)] where the field is non-public

[jhpratt]

Per @dtolnay in #72841:

Note that repr(transparent) is not the same as permission to inspect or manipulate the internal representation of a type. That permission only exists if the field is also exposed as pub.

As such, we should consider hiding #[repr(transparent)] when the field is not otherwise visible. This should not be the case when --document-private-items is passed for reasons that should be obvious.

This will affect some stdlib structs such as NonZeroU8

@rustbot label +A-docs +C-enhancement +T-rustdoc

[BGR360]

Is there consensus that this is a good thing to do? I understand that the thing that @jhpratt wanted to do with PathBuf was advised against, but for types that already have repr(transparent) like NonZeroU8, why shouldn't it be advertised? Is there no scenario where a user both relies on the fact that a type is repr(transparent) and doesn't do anything "impermissible" with it?

[jhpratt]

The reason I was advised against it is because the attribute is only meaningful if you are able to know what the inner type is. While something like NonZeroU8 is logically represented as u8, there's nothing that strictly days that's the case. This is the quote I included.

The #[repr(transparent)] doesn't give a user sufficient promises to do anything. So why show it at all?

[BGR360]

The #[repr(transparent)] doesn't give a user sufficient promises to do anything.

I mean, that's not true, is it? It makes some very explicit promises. Consider UnsafeCell. We have tons of code where I work that wraps up thread-safe C types in UnsafeCell so they can be accessed from multiple threads in Rust. When those types cross the FFI boundary, it seems critical for there to be a guarantee that the memory layout will be the same so we can transmute a value or reference to the C type into its Rust equivalent:

struct ffi_foo;

#[repr(transparent)]
pub struct Foo {
    inner: UnsafeCell<ffi_foo>,
}

impl Foo {
    pub fn go_kazoo(&self) {
        unsafe { ffi_foo_go_kazoo(self.inner.get()) }
    }
}

impl<'a> From<&'a ffi_foo> for &'a Foo {
    fn from(x: &'a ffi_foo) -> Self {
        // SAFETY: types have identical layout and ffi_foo is thread-safe
        unsafe { std::mem::transmute(x) }
    }
}

Maybe that example is misguided, but it still stands that repr(transparent) gives meaningful guarantees, right?

[jhpratt]

Ignoring the fact that the example you provided is unsound, I see no reference to the inner type of UnsafeCell in its documentation. As a result I'm deferring to the comment by @dtolnay who knows more about this than me. He's indicated that barring the field being public it cannot be relied upon.

[BGR360]

Which part is unsound?

Ignoring the details of my example, though, here's the Nomicon regarding repr(transparent):

The goal is to make it possible to transmute between the single field and the struct. An example of that is UnsafeCell, which can be transmuted into the type it wraps.

Also, passing the struct through FFI where the inner field type is expected on the other side is guaranteed to work. In particular, this is necessary for struct Foo(f32) to always have the same ABI as f32.

[dtolnay]

Yeah @jhpratt is correct. Just a repr(transparent) attribute does not give any public-facing guarantee when the field isn't pub.

Individual data structures may document a commitment that their layout may be publicly relied on, but that is simply not repr(transparent). Currently this kind of thing is just written in prose but this came up as well in rust-lang/unsafe-code-guidelines#302 (comment) about whether there should be some machine-readable way.

[BGR360]

Okay I see what you're saying. To put it in my own words, the set of all repr(transparent) types in the standard library can be split into two categories: "publicly transparent" (e.g., UnsafeCell, NonZero*) and "incidentally transparent" (e.g., PathBuf).

The unfortunate thing is that this split is currently not documented very well if at all. And as shown above, the Nomicon describes repr(transparent) as being equivalent to "publicly transparent." So at the very least, it seems that the Nomicon needs to be amended if this change is made.

But given that the Nomicon is written how it is, it seems possible that there are already crates that elide any additional prose and rely solely on the presence of repr(transparent) in their rustdocs to communicate that a type is "publicly transparent." So if this change is made without notification, perhaps existing documentation will lose information.

[dtolnay]

I disagree that the nomicon's description refers to publicly transparent. It's describing guarantees that the language makes to the module/crate that contains the type. Nothing in there is about what guarantees the module/crate makes to its downstream users.

[BGR360]

Hm, maybe not explicitly. But when I read it, it seems strongly implied.

The goal is to make it possible to transmute between the single field and the struct. An example of that is UnsafeCell, which can be transmuted into the type it wraps.

Nothing on that page, nor in the parent page, nor even in the Reference page on layouts, suggests to me that these rules only apply to the module/crate where the type is defined. My own intuition additionally tells me that the layout of a type is a property that is true for any crate in which the type is used.

Perhaps there's some detail that I'm missing when I read it, but really, if this is an important distinction, then it should be made more clearly. I'm more than happy to have my misconceptions ironed out through discussion, but I would wager that other noobs like me will walk away thinking the exact same things given the current text, and that's the real problem to me.

[jhpratt]

I would wager that other noobs like me will walk away thinking the exact same things given the current text, and that's the real problem to me.

To me that's precisely why this issue was created and should be resolved.

[riking]

I agree that nobody has any business attempting to transmute a PathBuf, but the non-pub nature of structures like NonZeroU8 is necessary to uphold the invariants - otherwise safe code could simply write a 0 to the field.

There will need to be some other attribute - possibly under rustdoc:: - to indicate whether the choice of inner type is stable.

[CAD97]

Incidentally related: #[repr(C)] has similar pitfalls in documentation, if not all fields are public, and can be actively misleading since it reorders // some fields omitted to the end, e.g. #66401

IOW: I think any #[repr] attribute should be hidden if any private fields are present. Library-stable layout is different from language-stable layout.

[Fryuni]

I'm in favor of this change. repr(transparent) guarantees that the struct has the same layout of its field, it makes no guarantees about the type of said field.

For example, seeing something like this in the docs:

#[repr(transparent)]
pub struct Foo<T> { /* private fields */ }

Does not guarantee that transmute<Foo<bool>, bool>(foo) is valid because the struct definition could be this:

#[repr(transparent)]
pub struct Foo<T> {
  content: Vec<T>,
}

repr(transparent) states that a struct has the same layout as something else. The docs should state what that something else is if that could be relied on. I could change Foo to hold a VecDeque and it would not be a breaking change if I never guaranteed that the layout would be that of a Vec.

A quick search through GH yields some examples of this happening:
https://github.com/Tamschi/ref-portals/blob/7baf0352b988364729b119af69475080013a23fa/src/rc.rs#L48-L51
https://github.com/eiz/eiz/blob/03a555fe4c0a251c5b1f8fa3d247ed99ad10af12/src/com.rs#L36-L37
https://github.com/aidangilmore/opaque-ke/blob/d1dfee9a6545d7dfddc6dd94f985f31b3221c54e/src/keypair.rs#L159-L160

I think any #[repr] attribute should be hidden if any private fields are present.

I think this might be too radical. repr(align(n)) doesn't have this problem, it is there to solve it actually. Without it, the structure has at least the largest alignment between the fields, but if the fields are private you cannot rely on what that value is. If the structure has repr(align(64)) the consumers can rely on it having at least 64-bit alignment regardless of things like generic parameters.
I'd also argue that people (not me, just speculating) working on really tight constraints might be relying on the niche-filling of enums, so repr(no_niche) should also be public.

[RalfJung]

Nothing on that page, nor in the parent page, nor even in the Reference page on layouts, suggests to me that these rules only apply to the module/crate where the type is defined. My own intuition additionally tells me that the layout of a type is a property that is true for any crate in which the type is used.

They apply globally, if you pin your dependency. But nothing in any of those documents says anything about how library authors are allowed to evolve their type definitions while being semver-compliant. It's perfectly semver-compatible to, for example, remove the repr(transparent) from PathBuf.

So yes, the layout of a type is a property that is true anywhere, but how that layout may change in the future looks very different from inside a module/crate and from the outside, and repr(transparent) says nothing about it.

[kpreid]

IOW: I think any #[repr] attribute should be hidden if any private fields are present.

Note that privacy is not sufficient to cover all cases where #[repr] would ideally be hidden. For example, suppose I have this enum (which, as any enum, has fully public contents):

#[repr(u8)]
pub enum Color {
    BLACK  = 0b00;
    RED    = 0b01;
    BLUE   = 0b10;
    PURPLE = 0b11;
}

I might want to make use of the bit-patterns of this enum for some algorithm within the crate, but not guarantee to users that the discriminant values are stable. (Though this is not solely about repr; there's also the detail that in that case I'd like to prohibit as-casting the enum to integer.)

And getting back to repr(transparent), here's a funky case where a repr(transparent) struct with all public fields, and all the same field types, can still change its representation, if ZSTness is changed:

mod v1_0 {
    pub struct Foo(());
    pub struct Bar(i32);
    
    #[repr(transparent)]
    pub struct Murky {
        pub foo: Foo,
        pub bar: Bar,
    }
}

mod v1_1 {
    pub struct Foo(i32);
    pub struct Bar(());
    
    #[repr(transparent)]
    pub struct Murky {
        pub foo: Foo,
        pub bar: Bar,
    }
}

v1_0::Murky and v1_1::Murky are semver-compatible (if they are from different versions of the same crate) but the change will break code that assumes transparency allows converting a Murky to a Foo, or a Bar to a Murky.

None of this says that rustdoc shouldn't hide #[repr(transparent)] with private fields; just that it is not sufficient to make that the only way to hide reprs.

[the8472]

Imo one should have to explicitly opt into making any repr part of the documented API.

[Manishearth]

Anyway I think there is not much harm fixing the issue as stated, and we can figure out a principled approach without breaking stuff in the future.

I think we should only do this for transparent and not C, since it is useful knowing if an opaque struct is repr(C)

[CAD97]

It's pretty clear-cut with #[repr(transparent)] that the transparent-inner type being hidden makes #[repr(transparent)] useless to downstream.

For #[repr(C)], there's a separate (and older) issue also tracked: #66401

[RalfJung]

The current situation is that some people consider then always public but others do not. See e.g. #90435 (comment) So no you cannot just assume they are public when you see them in a random library and the type has private fields. That's already the case today. rustdoc just does not reflect that.

[BurntSushi]

It's pretty clear-cut with #[repr(transparent)] that the transparent-inner type being hidden makes #[repr(transparent)] useless to downstream.

Can you elaborate a bit more on this? I can interpret this sentence in a couple different ways and I'm not sure what the right parse is. :)

[Manishearth]

The current situation is that some people consider then always public but others do not

Yes, agreed, but I think informally enough of the ecosystem relies on this for repr(C) that it's more or less the case that removing repr(C) is a breaking change. repr(transparent) is indeed murkier. Personally I would say that it ought to be something you can rely on for layout purposes but that does not necessarily imply transmutability.

[fmease]

It makes sense for rustdoc to make an educated guess by default, as long as the criteria is simple enough to describe in one sentence in the rustdoc book, and offers a way to explicitly turn it on or off if the guess is wrong.

Anyway I think there is not much harm fixing the issue as stated, and we can figure out a principled approach without breaking stuff in the future.

Okay, so what if we – for the time being – show #[repr(transparent)] iff the non-ZST field is public or if at least one 1-ZST field is public in case all fields are 1-ZST fields (one sentence for the book)? The user can use #[cfg_attr(not(doc), repr(transparent))] to completely opt out of repr(transparent). Since we aren't gonna introduce a new doc attribute for the time being, there won't be a way to explicitly opt into repr(transparent) in order to “overwrite” this rule. I argue that this is completely fine since this situation only ever arises if the user wants to consider a type with a private non-ZST field or one where all fields are 1-ZSTs & private to be repr(transparent) which doesn't make sense to me.

In pseudo code:

fields.find(is_non_zst)
    .map_or_else(|| fields.any(is_pub), is_pub)

I can spin up a PR relatively quickly if you like this approach.

If you consider this rule too complicated, I can also just go for repr(transparent) iff all fields are public but since there's currently no way to explicitly opt into repr(transparent), there will be many more false positives which the user can't do much about.

[Manishearth]

This seems fine.

#[cfg_attr(doc, repr(transparent))] to completely opt out of repr(transparent)

I assume you mean not(doc) here

[RalfJung]

#[cfg_attr(not(doc), repr(transparent))] turns out to have a problem -- when rendering re-exported items from dependent crates, the attribute does show up. (See e.g. std::ffi::CStr.)

[Fryuni]

#[cfg_attr(not(doc), repr(transparent))] turns out to have a problem -- when rendering re-exported items from dependent crates, the attribute does show up. (See e.g. std::ffi::CStr.)

That sounds like a bug to me... I mean, this would also break the documented intended use case of the doc cfg. Would it not?
Re-exporting a platform-specific type and using the any(platform, doc) trick would break my docs since the upstream type would not exist without the cfg being passed down.

Is there an issue for this, or, if this is the intended behavior, is there any discussion/explanation on why and an issue to add it to the docs?

Totally caught me by surprise.

[notriddle]

Yeah, it's bad. It falls out of the way cross-crate inlining and compiling works.

Fixing it is actually going to be pretty complicated. I couldn't find a bug report for it (not very many people use cfg(doc) and not very many people use cross-crate inlining, so I suppose they don't hit it much).

#114952

[RalfJung]

@fmease wrote

when working on a fix for #90435 (which is now sadly blocked on #114952 I guess), ...

I don't think this is blocked on #114952. Just the easy hack of using #[cfg_attr(not(doc), repr(transparent))] sadly doesn't entirely work, so a proper implementation in rustdoc might be needed.

[fmease]

Seems like I misinterpreted your comment. I'm gonna pick up the work on it again then.

[fmease]

I've now opened #115439.