Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
scottmcm opened this issue May 10, 2022 · 13 comments
Open

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

scottmcm opened this issue May 10, 2022 · 13 comments
Labels
T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@scottmcm
Copy link
Member

rust-lang/unsafe-code-guidelines#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?

@scottmcm scottmcm added T-lang Relevant to the language team, which will review and decide on the PR/issue. I-lang-nominated Nominated for discussion during a lang team meeting. labels May 10, 2022
@RalfJung
Copy link
Member

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
Copy link
Contributor

nox commented May 11, 2022

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
Copy link
Member

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 #93032.

@nox
Copy link
Contributor

nox commented May 11, 2022

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
Copy link
Member

That can be (and currently AFAIK is) compiled with temporaries. But anyway, that's a topic for #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
Copy link
Contributor

nox commented May 11, 2022

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
Copy link
Member

RalfJung commented May 11, 2022

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
Copy link
Contributor

nox commented May 11, 2022

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
Copy link
Member Author

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
Copy link
Contributor

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
Copy link
Contributor

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: hyperium/hyper#2831

@joshtriplett
Copy link
Member

joshtriplett commented Jun 21, 2022

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.

@joshtriplett joshtriplett removed the I-lang-nominated Nominated for discussion during a lang team meeting. label Jun 21, 2022
@nox
Copy link
Contributor

nox commented Jun 22, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

6 participants