Disable dead variant removal for #[repr(C)] enums.
#123043
Conversation
|
@oli-obk Hi! Could you maybe express your opinion on the test issue (and just look at the PR in general?) I could write a |
|
@rustbot labels -I-lang-nominated We discussed this in the lang call today and we agreed that this is a bug fix. This is OK to move forward. |
eea0d6a
to
d34e10c
Compare
|
I pushed shiny new I also added a line into |
This comment has been minimized.
This comment has been minimized.
|
Ok, apparently u64 has 4-byte align on some targets. I’ll make it less target-dependent. |
d34e10c
to
fa62263
Compare
This comment has been minimized.
This comment has been minimized.
|
That seems spurious. I don’t suppose I have enough rights to retry the pipeline? |
|
We're having some CI trouble currently, this may be related. So I suggest to wait for a few hours. You can retrigger the pipeline by closing and re-opening the PR. |
still needs an FCP though, right? Or is that bugfixy enough to just land? |
|
I understood this as saying that no FCP is needed.
|
|
The meeting notes explicitly say no FCP is needed:
(emph mine, ref: https://hackmd.io/@rust-lang-team/rkF1tAkzC#Disable-dead-variant-removal-for-reprC-enums-rust123043) |
|
@rustbot author |
Co-authored-by: Ralf Jung <post@ralfj.de>
d529329
to
8a3c07a
Compare
|
I removed the word “inhabited”. @rustbot ready |
|
@bors r+ rollup |
…iaskrgr Rollup of 9 pull requests Successful merges: - rust-lang#123043 (Disable dead variant removal for `#[repr(C)]` enums.) - rust-lang#126405 (Migrate some rustc_builtin_macros to SessionDiagnostic) - rust-lang#127037 (Remove some duplicated tests) - rust-lang#127283 (Reject SmartPointer constructions not serving the purpose) - rust-lang#127301 (Tweak some structured suggestions to be more verbose and accurate) - rust-lang#127307 (Allow to have different types for arguments of `Rustc::remap_path_prefix`) - rust-lang#127309 (jsondocck: add `$FILE` built-in variable) - rust-lang#127314 (Trivial update on tidy bless note) - rust-lang#127319 (Remove a use of `StructuredDiag`, which is incompatible with automatic error tainting and error translations) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#123043 - GoldsteinE:fix/repr-c-dead-branches, r=oli-obk Disable dead variant removal for `#[repr(C)]` enums. This prevents removing dead branches from a `#[repr(C)]` enum (they now get discriminants allocated as if they were inhabited). Implementation notes: ABI of something like ```rust #[repr(C)] enum Foo { Foo(!), } ``` is still `Uninhabited`, but its layout is now computed as if all the branches were inhabited. This seemed to me like a proper way to do it, especially given that ABI sanity check explicitly asserts that type-level uninhabitedness implies ABI uninhabitedness. This probably needs some sort of FCP (given that it changes `#[repr(C)]` layout, which is a stable guarantee), but I’m not sure how to call for one or which team is the most relevant. See rust-lang/unsafe-code-guidelines#500.
|
Nice, thanks! Should that be announced in some way? It’s kind of a rare case, but it’s also a subtle change in observable behaviour, so I’m not sure. |
This prevents removing dead branches from a
#[repr(C)]enum (they now get discriminants allocated as if they were inhabited).Implementation notes: ABI of something like
is still
Uninhabited, but its layout is now computed as if all the branches were inhabited.This seemed to me like a proper way to do it, especially given that ABI sanity check explicitly asserts that type-level uninhabitedness implies ABI uninhabitedness.
This probably needs some sort of FCP (given that it changes
#[repr(C)]layout, which is a stable guarantee), but I’m not sure how to call for one or which team is the most relevant.See rust-lang/unsafe-code-guidelines#500.