repr(transparent) should not consider non_exhaustive types as 1-ZSTs outside their crate

[scottmcm]

In crate A:

#[non_exhaustive]
pub struct UnitType {}

In crate B:

#[repr(transparent)]
pub struct TransparentThing(i32, UnitType);

I expected that to fail in crate B, because the reason crate A put #[non_exhaustive] on the type was to allow it to get additional fields in the future, which would no longer allow it to work in repr(transparent).

But this actually compiles today, creating what I would consider an accidental semver hazard.

https://rust-lang.zulipchat.com/#narrow/stream/213817-t-lang/topic/Stability.20of.201-ZSTness/near/215120408

[Mark-Simulacrum]

I'm going to nominate for lang, because I think (perhaps with a future compat lint) we should not look at foreign types' size unless that type is repr(transparent) for the purposes of checking here.

[scottmcm]

Note that this also applies to the way this was done before non_exhaustive:

pub struct UnitType(());

As that private field also keeps others from constructing or exhaustively matching it, but it would still be accepted as 1-ZST by repr(transparent).

We discussed this in the lang team meeting today. The consensus was that this is an accidental semver hazard that we'd like to do something about.

@rustbot modify labels: +E-help-wanted +E-medium

It'd be great to get a PR we could use to determine the impact of this. Ideally this isn't happening in the wild and can just be an error (maybe for one case or the other, if one is common and the other isn't) but it'll probably need at least a future imcompatibility lint.

[jsomedon]

Hello folks, can I take this issue? I am kinda new to the community but I want to start contributing. :-)

[scottmcm]

@jsomedon Absolutely! Unfortunately I don't really know the code here, so don't have suggestions for where to start, but you could always try tracing through the code around non_exhaustive and dropping by zulip if you run into any trouble.

[jsomedon]

Should a non-exhaustive struct be fine to stay in repr(transparent) in its own crate? (My understanding is that it's okay? As you can do whatever u want with the non-exhaustive in its own defining crate?)

[scottmcm]

@jsomedon Yes, I think that's ok, like how one can exhaustively match it inside its own defining crate.

[basil-cow]

@jsomedon do you plan to continue working on this?

[jsomedon]

No, probably too overwhelming to my rust level. I would rather see someone else fixing this and read their code instead.

[basil-cow]

@rustbot claim

[TheWastl]

If we should not look at foreign types' sizes, should it be allowed to use an opaque zero-sized type as the single non-zero-sized field in a repr(transparent) struct?

Crate A:

#[non_exhaustive]
struct Unit;

Crate B:

#[repr(transparent)]
struct Transparent(Unit, ());

This is currently rejected, but I think the zero-sizedness of opaque types (i.e. types with non_exhaustive or private fields) should be considered an implementation detail of crate A.

Edit: Note that this is also a potential semver hazard: If a crate contains a struct with only private fields, removing all fields should not be an API-breaking change.

[TheWastl]

@Areredify Are you still working on this?

[RalfJung]

Btw, I just noticed we have a somewhat similar restricting in repr(packed), which shows an error if it transitively contains a repr(align). This seems related in terms of being a semver hazard, doesn't it?

[TheWastl]

@RalfJung

If adding repr(align) is not a breaking API change, then we run into another issue: repr(transparent) requires that the contained ZSTs have an alignment of 1. But if libraries can make their ZSTs repr(align(2)) (for example), then external types may not be used as ZSTs in repr(transparent) types at all! Unfortunately, PhantomData is such an external type ...

Granted, adding repr(align) to ZSTs is probably not done very often. But I think this should at least be documented somewhere.

[basil-cow]

@TheWastl I can't work on this, but I was close to completing it if you want to pick it up. The issue was that I was traversing type contents incorrectly, iirc. It's at transparent_nonexhaustive branch.

[mati865]

@rustbot release-assignment

[fee1-dead]

This is probably not complete; we need to track it becoming a hard error in the future.

[mystise]

Is there a way for types to opt-in to a semver guarantee that they will remain ZSTs?

As a basic example:

// Crate A
#[repr(transparent)]
pub struct Token(());

impl Token {
	pub unsafe fn new() -> Self {
		Self(())
	}
}

OR

// Crate A
#[repr(transparent)]
pub struct Token(std::marker::PhantomData<()>);

impl Token {
	pub unsafe fn new() -> Self {
		Self(std::marker::PhantomData)
	}
}

// Crate B
#[repr(transparent)]
pub struct NeedsToken(u32, crate_a::Token);

Currently this hits this warning, but if Token is expected to only be handed out in certain ways by the library, say for generative lifetimes or ghost cells, then it being unsafe to construct could be relied upon in unsafe code, which means that making its elements public would present a soundness issue.

Is there a way to mark Token as "always zero sized barring a semver change"?

[fee1-dead]

I think we could make it accept repr(transparent) types that have private fields? But I'm not sure if repr(transparent) over a ZST makes it a guarantee that its layout cannot be changed.

[Jules-Bertholet]

The below struct definitions trip this lint. I see no reason why they must be illegal, and the first one could even be useful:

#[repr(transparent)]
pub struct Foo(usize, core::mem::ManuallyDrop<()>);

#[repr(transparent)]
pub struct Bar(usize, std::cell::UnsafeCell<()>);

#[repr(transparent)]
pub struct Baz(usize, core::mem::MaybeUninit<()>);

I think we could make it accept repr(transparent) types that have private fields?

I think that is the right choice.