Formally define repr(u32, i8, etc...) and repr(C) on enums with payloads

[Gankro]

Formally define the enum #[repr(u32, i8, etc..)] and #[repr(C)] attributes to force a non-C-like enum to have defined layouts. This serves two purposes: allowing low-level Rust code to independently initialize the tag and payload, and allowing C(++) to safely manipulate these types.

Rendered

[joshtriplett]

I really like this, and I think it'll significantly improve FFI.

A few bits of feedback:

• I'd prefer the alternative where "The tag shouldn't be opaque to outer enums". I don't think that would break any reasonable assumptions made in C code. C can't make any assumptions about the layout of Option<MyEnum> (other than Option on pointer types mapping NULL to None), since Option doesn't use repr (and should not use repr since it would invalidate enum layout optimizations).
• Please explicitly state that the documentation for repr(X) should explicitly state that native Rust code should not use that attribute unless it needs to manipulate an enum at the raw memory level or pass it to C code non-opaquely, as doing so inhibits useful optimizations.
• Regarding "A field/method for the tag/payload", the former would be the discriminant_value function (still not stabilized). I don't know that we need a field for the payload, since it won't necessarily have a corresponding type.
• Nice branch name. 👍

[jld]

There's another representation that's slightly different from the one proposed: a union of structs where each struct's first field is the discriminant. (For FFI there could be an extra union case with only the discriminant, but I believe it's valid C to assume a struct's first field is at offset 0 and access it by pointer casting.) The difference from the struct-of-tag-and-payload representation shows up in cases like this:

#[repr(u8)]
enum MyEnum {
  Word(u64),
  Bytes([u8; 15]),
}

If the whole payload is a union, it gets 8-aligned and the enum takes 24 bytes; if each variant is laid out separately, it takes 16 bytes. Yet this isn't an “enum optimization” in any of the ways that cause the problems identified in this RFC: the C/C++ interface is well-defined and fairly straightforward, and the discriminant's meaning is unaffected by the bits stored in the data fields.

This is, incidentally, how rustc currently handles enums when no optimizations or special cases apply; note this comment, currently in rustc::ty::layout, which dates back to rust-lang/rust@a301db7:

    /// General-case enums: for each case there is a struct, and they
    /// all start with a field for the discriminant.

[joshtriplett]

@jld That's an interesting distinction.

Such a representation would always have an advantage on size, but might potentially prove more annoying to define and access from non-C languages. On balance, though, since Rust already uses that layout, using the same layout for the repr case seems reasonable to me.

Can I suggest switching the RFC to that representation, observing that Rust uses that representation by default, and mentioning the "two-element struct" representation as an alternative?

[pythonesque]

I agree with @jld here, though it may turn out that C++ compatibility concerns are the deciding factor.

[arielb1]

This means that the contents of a #[repr(X)] enum cannot be inspected by outer enums for storage optimizations -- Option<MyEnum> must be larger than MyEnum.

When is that needed? This breaks the rule that by-value enums must always be "valid", and I don't see you using it (e..g. if dest is initialized when the function ends, then an Option<MyEnum> would still work).

[Gankro]

Yes, having reflected on this more, I believe you're correct. I'd just like @bluss to verify that their original use-case only requires the payload to be opaque. And it would be fine for Option<Flag> to use the the Flag's tag to represent None.

[Gankro]

Having reflected on this even more, here is an interesting case where this matters:

I am recursively doing this in-place deserialization process, and at some point I need to deserialize Option<MyEnum> in place. To do so, I need to do:

*result = Some(mem::uninitialized());
if let Some(ref mut my_enum) = result {
  deserialize_my_enum(my_enum);
}

That is, if we make MyEnum "super opaque" it means Option<MyEnum> effectively becomes a pseudo-repr(X) type, and we can defer initialization of the payload... except that this runs afoul of my "must be valid when you match" rule. It's possible we could create more fine-grained rules to allow it, though.

Alternatively we can declare this to be unsupported, and force people who want this to make a #[repr(u8)] OpaqueOption<T> { Some(T), None }.

[bluss]

Yes it seems entirely right, it's just the payload.

It's about crate nodrop and arrayvec's use of it to store an uninitialized and/or partly uninitialized payload.

Yes as I understand it's ok if the enclosing Option uses the repr(u8) enum's tag. @eddyb has in fact already implemented that in the big enum layout PR(!), and it works fine with nodrop, even if it broke a test I wrote. Because my test was buggy.

I was envisioning a ManuallyDrop with more guarantees or a MaybeUninitialized or stable fully generic unions (without Copy bound) would the way to bring the arrayvec and nodrop use case to sound ground. (I've been discussing ideas with a few others the past weeks (breadcrumby link).)

If the uninitialized-in-repr-enum use case gets blessed by way of this rfc I think it's great if it can do so on other merits. But crate nodrop provides a working proof of concept.

[arielb1]

Also, the representation should be more of the form:

type Tag = uX;

#[repr(C)]
union MyEnum {
    tag: Tag,
    variant_1: Variant1,
    variant_2: Variant2
}

#[repr(C)]
struct Variant1 {
    tag: Tag,
    field1: Field1,
    ...
}

#[repr(C)]
struct Variant2 {
    tag: Tag,
    field1: Field1,
    ...
}

Because e.g.

#[repr(u8)]
pub enum X {
    Y { a: u8, b: u32 },
    Z,
}

fn main() {
    assert_eq!(std::mem::size_of::<X>(), 8);
}

where the enum is represented as

   0     1     2     3     4     5     6     7
+-----+-----+-----+-----+-----+-----+-----+-----+
| tag |  a  | <padding> |  -------- b --------- |
+-----+-----+-----------+-----------------------+

Where the first field of the enum is allowed to not have the same alignment as the enum.

[eddyb]

@jld @arielb1 @pythonesque That (union of variants each containing the tag and their payload) is indeed what Rust enums use, as an optimization, but it's not, AFAIK, typically used in C or C++, and harder to work with in those languages than "tag followed by union of payloads".

I think that minimizing the padding through this method, just like field reordering, shouldn't apply to C-compatible types as I do not see the optimization benefits justifying the complexity.

[glaebhoerl]

Would it be reasonable to allow syntax like #[repr(C, u8)] to specify both "must be C-compatible" and the size of the tag, instead of just assuming the former whenever the latter is done? Then whether to do the alignment optimization, as well as whether to do #[repr(C)] on the member structs as mentioned in the RFC, could both depend on whether or not C representation was requested (without taking away the ability specify the tag size at the same time). If not it's probably simplest to just always do the FFI-appropriate thing.

[eddyb]

@glaebhoerl I'm pretty sure that's how it already works (except for the union-of-(tag, payload) vs (tag, union-of-payloads)).

[mystor]

I think that these should probably be implemented separate from this RFC. I'd be inclined to instead allow the ecosystem to generate these declarations using a Custom Derive like #[derive(EnumRepr)] which would define an my_enum_repr (or similar) module with Tag, Payload, and structs for each payload.

Right now I don't think we have the tools to expose these in a nice way, especially if we consider a theoretical enum with a variant called Tag or Payload (this change would be breaking for those enums).

[Gankro]

Yes, I was willing to punt on these since serde and cbindgen could both generate these automatically if need be.

[eddyb]

This sounds partly like #1450.

[mystor]

I'm worried that this has limited usefulness without the ability to define untagged unions with non-Copy variants, which IIRC has its stability blocked on #1897 or similar.
I suppose it can still be used for copy variants and for C/C++ FFI right now.

[est31]

Saying "it has been decided already" is neither a good attitude towards the RFC process, nor is it a substitute for a drawback section!

[Gankro]

Would people like me to decouple the proposal so repr(u32) gets the Rust union(tag) layout, and repr(C, u32) gets the tag, union layout?

[joshtriplett]

@Gankro That sounds reasonable, though it needs very careful documentation.

And I don't think either one should inhibit optimization of Option<MyEnum>.

[Gankro]

Note that the Rust union(tag) layout is explicitly declared valid to manipulate in C++ >= 11 (not sure about earlier versions):

And I believe C doesn't actually care about accessing unions in weird ways, so it's also valid there. So the only reason to care about the (tag, union)-style is because it's a bit more ergonomic to manipulate in low-level code, and might make it easier to work with existing C(++) codebases.

[RReverser]

because it's a bit more ergonomic to manipulate in low-level code

Yeah, ergonomically having separate tag and actual value structs is more convenient. Otherwise you will have an extra wrapper struct for each union variant which already has a nested named struct type, and that tag in each of variants will be ignored anyway when it's not part of the union.

[Gankro]

I have significantly updated the contents of the RFC based on feedback:

• repr(u32, i8, etc...) now uses the current Rust union(tag) layout for compatibility and performance reasons
• repr(C) now forces the alternative (tag, union) layout that was originally proposed.

In addition, the Drawbacks section has been properly filled out, and two Real "unresolved questions" have been added:

• Should Option<MyEnum> be forced to be larger than MyEnum? (should the tag be opaque?)
• Should every payload in a repr(C) enum's union be a struct?

[RReverser]

Just to reconfirm: this doesn't impose similar Copy restrictions as union do?

On one hand, such restrictions would help with "it's very bad" note in the parsing example, but on another, it might be not a sufficient reason to limit the usefulness of tagged enums (and it's a breaking change).

[Gankro]

This RFC only defines the layout. So you won't be able to write out a MyEnumRepr type for non-Copy payloads in Rust today, but when/if unions remove that restriction you will be able to. (and you can do whatever in C(++))

[RReverser]

@Gankro Right, what I wanted to reconfirm is that this RFC doesn't impose additional restriction on variant contents. And since it doesn't, that "it would be very bad if we panicked past this point" comment seems a bit excessive, since for Copy types (as currently limited by union) it's not scary at all.

[Gankro]

This specific enum isn't the best example of it, but e.g. Option<u32>, bool, and char are all Copyable but have Undefined values that you could get by failing to complete the parse, effectively transmuting one variant into the other. This itself would only be a problem if the caller had a catch_unwind, though.

[RReverser]

This itself would only be a problem if the caller had a catch_unwind, though.

Ah yes, indeed, but then the problem is much more generic than this proposal.

[upsuper]

If it is possible to get the tag value of enum, it would also help Servo to shrink some lookup functions which are currently compiled to a jump table or lookup table. See Gecko bug 1428285.

[nikomatsakis]

@rfcbot fcp merge

We discussed this today in the @rust-lang/lang meeting. This RFC is basically documenting the existing behavior of the compiler, if I understand correctly. Clearly, it's of use for projects that are attempting to do low-level optimizations that operate directly on enums and other Rust values. Having a defined layout here thus enables a lot of use cases and doesn't seem to have a lot of downside (at least none were brought up here that I saw in skimming the thread), and we've clearly iterated a bit on the precise iteration and wound up at a pretty flexible point. (Further, there is always room to add more repr flags and things in the future as needed, so the commitment here is low.)

[rfcbot]

Team member @nikomatsakis has proposed to merge this. The next step is review by the rest of the tagged teams:

• @aturon
• @cramertj
• @eddyb
• @nikomatsakis
• @nrc
• @pnkfelix
• @withoutboats

No concerns currently listed.

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[Gankro]

Some data points:

• webrender (and therefore servo and firefox-nightly-with-a-flag) is already relying on this RFC to optimize deserialization of the draw commands sent over IPC. This is done through a minor fork of serde_derive that I maintain.

• cbindgen now supports generating C and C++ headers for both kinds of enum layout, which we intend to use to make the rust<->C++ FFI boundary in firefox more rust-idiomatic and clean.

[rfcbot]

The final comment period is now complete.

[aturon]

This RFC has been merged! Thanks @Gankro!

[Gankro]

👏🎊🎉

[golddranks]

The rendered link is broken.

[Havvy]

@golddranks https://rust-lang.github.io/rfcs/2195-really-tagged-unions.html