rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
98.6k stars 12.74k forks source link

Should `repr(transparent)` require *inhabited* 1-ZSTs? #96921

Open scottmcm opened 2 years ago

scottmcm commented 2 years ago

https://github.com/rust-lang/unsafe-code-guidelines/issues/337 pointed out that the following code compiles:

enum Never {}

#[repr(transparent)]
struct Foo(u32, Never);

https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=67b6fd93f899912b2aec43b4cf362f31

Does that make sense? Should we change the rule here, either retroactively or in a new edition?

RalfJung commented 2 years ago

It is certainly correct that every value of Foo can be transmute to u32 (because no such value can exist).

However, transmuting u32 to Foo is UB.

nox commented 2 years ago

IMO it makes sense to keep it for weird shenanigans like in the hyper issue that Ralf just linked here.

If you want to "disable" an enum variant (which you have control over, and has a specified repr), without changing the shape of the enum, nor its size, it's more or less the only way.

I understand that we want to make uninhabited types with inhabited fields be zero-sized, but maybe we should specifically avoid that for #[repr(transparent)] types?

RalfJung commented 2 years ago

If you want to "disable" an enum variant (which you have control over, and has a specified repr), without changing the shape of the enum, nor its size, it's more or less the only way.

Note that this issue is about structs. We don't do the layout optimization (of making an uninhabited struct have size 0) on structs because it is in conflict with partial initialization.

For enums, last time this came up, since enums cannot be partially initialized, most people seemed to be in favor of allowing more aggressive layout optimizations there. We might even already do those, I am not sure.

Also see https://github.com/rust-lang/rust/issues/93032.

nox commented 2 years ago

For enums, last time this came up, since enums cannot be partially initialized, most people seemed to be in favor of allowing more aggressive layout optimizations there. We might even already do those, I am not sure.

I thought the size needed to be preserved because of stuff like Foo::Bar(foo, panic!()) where the second field is of type !. Oh well.

RalfJung commented 2 years ago

That can be (and currently AFAIK is) compiled with temporaries. But anyway, that's a topic for https://github.com/rust-lang/rust/issues/93032.

I agree repr(transparent) should have the same size as its only non-ZST member. The question is whether we should allow uninhabited 1-ZST in repr(transparent). My inclination is no, because they already violate another principle of repr(transparent): that you can transmute between the non-ZST member and the wrapper type. (This might even be a soundness issue, if there are macros that rely on repr(transparent) as indication of a UB-free transmute.)

nox commented 2 years ago

My inclination is no, because they already violate another principle of repr(transparent): that you can transmute between the non-ZST member and the wrapper type. (This might even be a soundness issue, if there are macros that rely on repr(transparent) as indication of a UB-free transmute.)

I don't understand what you mean by this. Just because something is a #[repr(transparent)] doesn't mean you can transmute freely between the two. The transparent wrapper may come with additional invariants etc.

RalfJung commented 2 years ago

I don't understand what you mean by this. Just because something is a #[repr(transparent)] doesn't mean you can transmute freely between the two. The transparent wrapper may come with additional invariants etc.

You can't publicly safely transmute, no. But you should get the guarantee that the transmute itself does not cause immediate UB. In other words, the transmute may violate safety invariants, but it will never violate validity invariants. That is a major motivation for people to use repr(transparent).

nox commented 2 years ago

I see your point, and I don't mind either way, but people using repr(transparent) are the ones adding zero-sized fields or not to their transparent wrapper, so they would know what they are doing if they add an uninhabited zero-sized field. At least I would.

scottmcm commented 2 years ago

https://lib.rs/crates/ref-cast comes to mind, here.

But it looks like it's sound by essentially allowing only PhantomData as the extra fields in a repr(transparent): https://github.com/dtolnay/ref-cast/blob/master/src/trivial.rs

nikomatsakis commented 2 years ago

can someone clarify if there are any known cases of people using this? What is this hyper example? (cc @seanmonstar or @erickt -- is hyper using "uninhabitable" never types in combination with #[repr(transparent)]?)

My take is that this should be a warning for sure, but possibly an error -- it doesn't seem a priori invalid to me, but it's definitely suspicious. If you have an uninhabited type, then the function ought never to return, but in that case, why do you care about the type being transparent, which only affects functions that return? Could be that it arises because of generic or macro-created code, of course, where sometimes the fn returns and sometimes it doesn't.

seanmonstar commented 2 years ago

It seems it is used in hyper currently, to create a type that can satisfy some trait bounds, but that is otherwise never actually constructed. It was linked further up, here's a PR that discussed it in hyper: https://github.com/hyperium/hyper/pull/2831

joshtriplett commented 2 years ago

We talked about this in today's @rust-lang/lang meeting. We probably don't want to break this pattern since it's used, but it may make sense to add a lint.

nox commented 2 years ago

I don't think anyone but me thought of this stuff, and Hyper is a very alive project so if you feel like we should just do otherwise, that's possible too.