Allow arbitrary enums to have explicit discriminants

[nox]

Summary

This RFC gives users a way to control the discriminants of variants of all enumerations, not just the ones that are shaped like C-like enums (i.e. where all the variants have no fields).

Thanks

Thanks to Mazdak Farrokhzad (@Centril) and Simon Sapin (@SimonSapin) for the reviews, and my local bakery for their delicious baguettes. 🥖🥖

---

Rendered
Tracking issue

[oli-obk]

I think you have encountered the XY Problem. You are finding a solution to the problem that different enums don't give guarantees about their variant indices, when what you originally wanted was converting enums whose variants have fields into enums whose variants have no fields.

So here's my alternative:

Allow annotating enums with #[discriminant(OtherEnum)], where OtherEnum must have a variant for each variant in the annotated enum (with matching name). The annotated enum will then take all its discriminant values from OtherEnum.

as casts could then convert from annotated enums to their discriminant enum.

[eddyb]

That seems more convoluted, IMO. Definitely harder to implement, at least.

[oli-obk]

Sure, but it has a path forward for having this operation in safe code.

Alternatively a procedural macro could generate said conversions with this RFC. So the compiler magic might not be necessary.

[nox]

This would be great and nice, if only real world allowed me to do that.

I simplified my use case for the sake of the RFC: PropertyDeclaration actually has more variants than LonghandId, thanks to custom CSS properties (and some other stuff that I don't need to mention here):

#[repr(u16)]
enum PropertyDeclaration {
    Color(Color),
    Height(Length),
    InlineSize(Length),
    TransformOrigin(TransformOrigin),
    Custom(CustomDeclaration),
}

struct CustomDeclaration {
    name: Name,
    value: Value,
}

pub enum PropertyDeclarationId<'a> {
    Longhand(LonghandId),
    Custom(&'a Name),
}

impl PropertyDeclaration {
    pub fn id(&self) -> PropertyDeclarationId {
        if let PropertyDeclaration::Custom(ref declaration) =  *self {
            return PropertyDeclarationId::Custom(&declaration.name);
        }
        let id = unsafe { *(self as *const _ as *const LonghandId) };
        PropertyDeclarationId::Longhand(id)
    }
}

In general, I think this alternative is too specific to the exact use case described in the RFC, and cannot fulfil more intricate ones like the actual stuff I require in Servo, or use cases I could imagine for this feature with FFI, @gankro may have an opinion on that regard here.

Edit: I'll edit this RFC to include ite nonetheless.

[Diggsey]

You mention in the RFC that rust is generating a 4KB jump table for the required match expression - this seems excessive, even if the discriminants don't match exactly. How many variants does this enum have? Maybe part of the solution should be improving the code that rust generates in this case.

[nox]

@Diggsey There are >320 variants, so far. I welcome any rustc improvement but they won't be able to remove all jump tables if the discriminants of the common variants to AnimationValue and PropertyDeclaration don't coincide.

[nox]

@Diggsey Also if you are curious, here is the PR where PropertyDeclaration::id was slimmed down from 4KB to mere 96 bytes, and there is the PR where I simplified AnimationValue::id.

[burdges]

It'd be cool if this worked with ATCs to permit defining both discriminant and enum together:

trail Fields { type Id<T>; }
struct YesFields {}
impl Fields for YesFields { type It<T> = T; }
struct NoFields {}
impl Fields for NoFields { type It<T> = (); }

enum MyEnumInner<F: Fields> {
    Variant1(F::Id::<u64>) = MyDiscriminant::Variant1(()),
    Variant2(F::Id::<String>) = MyDiscriminant::Variant2(()),
    ...
}

pub type MyEnum = MyEnumInner<YesFields>;
pub type MyDiscriminant = MyEnumInner<NoFields>;

We've a circular definition above, but rustc could presumably break this cycle.

[nox]

@burdges That seems unrelated to the RFC described in this pull request.

[mark-i-m]

I can totally sympathize with this RFC. I have often wanted to go from an enum with fields to a value of another type.

My biggest two gripes against this RFC are

1. The casting here is annoying: LonghandId::Color as u16,
2. This really shouldn't require any unsafe code at all: unsafe { *(self as *const Self as *const LonghandId) }

In that light I propose the following variant/alternative:

// same as before
#[derive(Clone, Copy)]
#[repr(u16)]
enum LonghandId {
    Color,
    Height,
    InlineSize,
    TransformOrigin,
}

// explicitly mention that the given type is the descriminant (it can also be a struct!), 
// but the compiler needs to be able to prove at compile time that two values of the
// descriminant type are actually different. Note: different means different bit patterns, 
// not different in the `Eq` sense.
#[repr(LonghandId)]
enum AnimationValue {
    Color(Color) = LonghandId::Color, // no cast
    Height(Length) = LonghandId::Height, // also each descriminant assignment must be unique!
    InlineSize(Void) = LonghandId::InlineSize,
    TransformOrigin(TransformOrigin) = LonghandId::TransformOrigin,
}

impl AnimationValue {
    fn id(&self) -> LonghandId {
        (*self) as LonghandId // compiler can just return the descriminant! Completely safe!
    }
}

[burdges]

We could support discriminants with their own fields too, making this these enums into extensions. I suppose #[extends(LonghandId)] would be an alternative to #[repr(LonghandId)], but certainly #[repr(..)] makes sense.

[nox]

Please, I have already addressed that this doesn't fit my use case in #2363 (comment) and the RFC already includes such an encoding as an alternative, except with #[discriminant(Foo)] instead of
#[repr(Foo)].

Removing the need for unsafe code for the cast is a nice thought, but again it is unrelated to this RFC.

[mark-i-m]

@nox Sorry if this is a stupid question, but I still don't understand how the two proposals are not functionally equivalent. It seems like everything you can do in one proposal, you can do in the other. What am I missing?

[RalfJung]

I assume this is restricted to enums with a non-Rust repr? Otherwise, doesn't the part that says "if no set explicitly, start with 0 and count up" clash with the niche-filling enum optimizations?

[SimonSapin]

@RalfJung I had the same reaction. nox and eddyb explained that this RFC controls the discriminant of each variant, but this is a separate concept from the tag that may or may not be part of the memory representation. #[repr] forces a tag to be present, containing the discriminant.

But yes, since mem::Discriminant is opaque this new syntax is only useful when also used with #[repr].

[nox]

@mark-i-m #2363 (comment) One of the enums with fields has more variants than LonghandId. Your (and @oli-obk's) alternative is a strict subset of what is allowed by this RFC.

[RalfJung]

@SimonSapin I am not sure I understand, but if the feature is not useful for repr(Rust), maybe it'd be better to not allow it for now?

[nox]

@RalfJung I listed it in the unresolved questions. I could imagine rustc managing to use niche-filling optimisations for 2 different variants with an inner enum, if it could see that they use disjoint sets of discriminants and that there is an easy way to compute the discriminant from the niche value. This is quite theoretical though so I don't mind mandating a #[repr].

[Evrey]

I realy like the Variant(...) = ID, syntax. It keeps enums compact and prevents error-prone double typing.

With #[discriminant(X)] you'd have a bunch of problems and questions:

• Is X truely just a numeric C-style enum?
• Is X an enum at all?
• Does X have the same #[repr({integer})]?
• Does X have the very same number of variants?
• Do Xs variants have the exact same names?
• Do Xs variants have the exact same order?
• Aaand having an additional X more than doubles your lines of code by adding redundant bloat.

Also, #[discriminant = {expr}] doubles your lines of code, or triples with readability line breaks, while having a more "magic"-ish syntax than just using the well-known Variant = {expr},.

---

There are two problems left for my taste:

1. How does one name those explicit discriminants?
2. How does one get those discriminants?

Now, about the first point... I especially like the Variant(...) = {expr}, proposal, because it looks just like C-style enums with Variant = {expr},. We do not want to match on magic numbers, especially not for huge enums, which is why Servo has those MyEnum and MyEnumId redundancies. It is much more understandable to match on MyEnumId::X than writing 0x1023. But wait, MyEnumId::X is how you get the discriminant of a C-style enum! So, why not allow this?

#[repr(u8)]
enum X {
    A(u32) = 0,
    B(f32) = 1,
}

assert!(X::A(42).id() == X::A);
assert!(0_u8 == X::A);

This could, however, mean trouble if type A = X::A is a thing. (It isn't.) Whatever. This way you do not need a second Id enum at all, while also not having to learn new syntax.

So... how do we get that discriminant? Writing a pointer-converting boiler plate function called fn id() all the time is no fun, especially for new magic syntax that is supposed to reduce the amount of boiler plate code. We already already have core::mem::discriminant, but this one is of not much help here. You can only use this for equality checks, it takes tremendous 64 bits, always, no matter the #[repr({integer})], writing mem::discriminant(X::A) == mem::discriminant(x) is painfully verbose, and you cannot really get the integer-representation out of it for e.g. FFI or RPC.

I'm not really up to date on the #[repr({integer})]-on-enums thing, but what about making this attribute implementing a const fn id_{integer}(&self) ->{integer} on the enum? Or implement an EnumRepr<{integer}> trait on it or whatever. Okay, the name may be different. fn id() alone might clash too much with common function names, fn discriminant() is so verbose that you might as well just impl Into<{integer}> on core::mem::Discriminant.

An alternative would be to treat enums logically as a (Discriminant, UnionOfVariants), or C-style enums as just (Discriminant). Then, instead of having to decide on a magic function name or trait names, we write this: X::A(42).0 == X::A. This has quite some downsides, however, like what does X::A(42).1 yield, do we want that, is it safe, what about non-#[repr({integer})]-enums, especially for those with highly optimised layouts like Option<bool>, etc.

---

Edit: Just noticed this:

• under the default representation, the specified discriminants are interpreted as isize;

Under the default representation, there might be heavy layout optimisations at work merging discriminants of nested enums into a completely different numeric set. In addition to that core::mem::Discriminant chose u64 in its opaque implementation. I.e., applying this syntax to default representation enums is either nonsensical or must block layout optimisations. The exact numeric discriminant type is debatable.

[nox]

@Evrey I'll add a commit listing all your arguments against #[discriminant], thanks!

---

How does one name those explicit discriminants?

We can't interpret X::A as some sort of magical ID enum, because X::A is already a function of type u32 -> X in your sample. Note that in Servo we also store the LonghandId enum in various places, without any PropertyDeclaration or AnimationValue anywhere. Please also note that as I mentioned multiple times already, PropertyDeclaration has more variants than LonghandId, so we can't directly tie the two in any way. So even if there was some magical way to say Enum::Discriminant to refer to that enum's discriminant type, we would still have a LonghandId enum in Servo.

How does one get those discriminants?

I agree that a way to safely get the u8 discriminant of an #[repr(u8)] enum value would be useful, but that's orthogonal to that RFC: after all #[repr(u8)] and #[repr(C, u8)] on enums landed without such an API.

[mark-i-m]

I see... but I still have the same two annoyances against the current form:

• The casting here is annoying: LonghandId::Color as u16
• This really shouldn't require any unsafe code at all: unsafe { *(self as *const Self as *const LonghandId) }

Especially the second one seems like an essential part of any reasonable solution IMHO...

[SimonSapin]

@mark-i-m I think that these two points are valid independently of this RFC. (Re unsafe code, see RFC 2195 in particular.) Improvements there can be proposed separately without necessarily blocking this RFC.

[nox]

The casting here is annoying: LonghandId::Color as u16

This really shouldn't require any unsafe code at all: unsafe { *(self as *const Self as *const LonghandId) }

Given what I said about PropertyDeclaration having more variants than LonghandId, the casting cannot be avoided in my particular use case. Do you see why?

[main--]

This is unfortunately quite painful to use, given now all methods matching against AnimationValue need to have dummy arms for all of these variants:

Note that this is #1872. Feels like a quite elegant workaround to me - the unsafe cast asserts that the ! variants are dead and then you can just match without thinking about them.

[nox]

@main-- I know about this RFC, but it has been postponed and I don't think I should have to rely on that instead of just not having ! variants.

[main--]

@nox I mean, you're essentially trying to come up with a way to define Rust enums with gaps in their discriminator space - this RFC proposes to explicitly specify the remaining ones whereas I suggested to list the missing ones. It's fundamentally the same thing, depending on the usecase either one may be more convenient. Yet I favor the ! solution as it doesn't add new syntax and complexity to enums but instead simply relies on a fix to match behavior that should have happened a long time ago (in my opinion). It just feels like a much more generic approach to me that can help with this problem and several others too.

[nox]

Having to specifically define variants that are uninhabited for no other reason than to make 2 discriminants in 2 different enums coincide is a damn workaround that I will not qualify as "fundamentally the same thing" as being able to omit said variants. Why would I pollute my mind and my type definition with variants that I won't use?

[main--]

I guess if most of your variants are dead it makes considerably less sense than, say, one of thirty. The thing is - comparing a language feature tailored specifically to this problem to a general mechanism that the language already offers (minus a papercut) just isn't fair, hence my reluctance to call it a workaround.

My point is that the alternative is not simply omitting the dead variants. Instead, you have to manually specify discriminants. When I say it's the same thing what I mean is that we're talking about two different ways to specify enum discriminants (same outcome). Your proposal introduces a new language feature which is clearly an improvement over the (mostly) existing workaround, I merely doubt that it's enough of an improvement to justify the complexity.

[nox]

This is a very small addition to the language, semantics-wise and implementation-wise. As far as I remember discussions with @eddyb, this will be easily implementable.

[SimonSapin]

I don’t believe added complexity is significant here, the same syntax already exists on field-less variants.

[mark-i-m]

I went back and re-read the RFC and I think my objections are more specific about the example. The actual proposal is just about allowing Variant(..) = Disc, which I think is quite reasonable. I don't think y it's the best solution to the give example use case, but I'm not opposed to the rfc on its own merit...

[SimonSapin]

(Aside: there’s a bunch of things in Stylo that are objectionable ;) (I say this as the author of many of these things.) A lot of it can be improved but the devil is in the corner cases, and better solutions are often not as easy as they first seem.)

[clarfonthey]

I may be not reading correctly, but does this work for repr(Rust) enums too? And would:

enum A {
   B = 0,
}

have size zero or one? Also, would:

enum B {
    B = 1,
    C(!) = 256,
} 

have size 0 or 2?

(These are super esoteric examples but it seems like a valid thing to ask considering how imho these don't make sense, but would be accepted.)

[eddyb]

@pnkfelix I'd say I prefer 0 => Bread(BreadKind), but I agree with your conclusion.

[joshtriplett]

On Fri, Apr 12, 2019 at 01:04:29AM -0700, Eduard-Mihai Burtescu wrote: @pnkfelix I'd say I prefer `0 => Bread(BreadKind),` but agree with your conclusion.

I like that syntax and would happily support it in place of the proposed syntax.

[nox]

I don't like this syntax at all.

It's completely at odds with C-like enums, which means that if you currently have a C-like enum and then add a field to one of its variants, you must rewrite the entire type definition.

It also puts more emphasis on the discriminant rather than the damn variant, even though the discriminants most probably aren't the most important part of the enum. It's certainly not the most important part of my use case.

That syntax also makes the example from the RFC horrible:

#[repr(u16)]
enum AnimationValue {
    LonghandId::Color as u16 => Color(Color),
    LonghandId::Height as u16 => Height(Length),
    LonghandId::TransformOrigin as u16 => TransformOrigin(TransformOrigin),
}

It may also be a problem to macro authors, who may want to be passed an enum definition and emit explicit discriminants for them, and now they must first check whether any of the variants have fields.

Edit: But I do like a lot that @pnkfelix named his example ParisianSandwichIngredient.

[joshtriplett]

On Fri, Apr 12, 2019 at 09:37:31AM -0700, Anthony Ramine wrote: I don't like this syntax at all. It's completely at odds with C-like enums, which means that if you currently have a C-like enum and then add a field to one of its variants, you must rewrite the entire type definition.

[...]

It may also be a problem to macro authors, who may want to be passed an enum definition and emit explicit discriminants for them, and now they must first check whether any of the variants have fields.

I'd taken it as implicit that the same syntax would be permitted for C-like enums. (That said, I also don't mind the `= value` synax, if people prefer that.)

[nox]

I'd taken it as implicit that the same syntax would be permitted for C-like enums.

Oh I see. That part is fine then. I still don't think it makes for a readable type definition in presence of complicated discriminant expressions.

[joshtriplett]

On Fri, Apr 12, 2019 at 10:08:03AM -0700, Anthony Ramine wrote: > I'd taken it as implicit that the same syntax would be permitted for C-like enums. Oh I see. That part is fine then. I still don't think it makes for a readable type definition in presence of complicated discriminant expressions.

That's fair. I'm not sure if it's a good or bad idea to allow an "alternative" syntax, such that both syntaxes work; on the one hand that'd allow flexibility in writing, and on the other hand having only one syntax makes for more consistency. I'm happy to check off on either or both.

[burdges]

It's easier to make the other form work with { } although maybe this just adds confusion.

enum ParisianSandwichIngredient {
    Bread = 0  { bread_kind: BreadKind },
    Ham = 1 { ham_kind: HamKind },
    Butter(ButterKind) = 2,
}

[withoutboats]

I'd prefer not to invent a new syntax for this feature and just keep with the obvious extension to what we have, even if its not perfect.

[eddyb]

Yeah, I was only replying to @pnkfelix, but I'm not proposing/in favor of a syntax different than the one in this RFC.

[rfcbot]

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

[rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

The RFC will be merged soon.

[Centril]

Huzzah! This RFC has been merged!

Tracking issue: rust-lang/rust#60553

[ghost]

Just a few thoughs. The RFC focuses to solve some issue with the Servo design. I like the basic idea, but not the part where discriminate is a different type. If they have enums for anim, color, etc features why arent they different enums? And have a separate enum that combines all of them. I think its a design flaw in servo that shall not be fixed by an RFC.
Once it is "fixed" there is no need to have a separate type for discriminant, and hence turning enum into id can be something built in.

[nox]

And have a separate enum that combines all of them. I think its a design flaw in servo that shall not be fixed by an RFC.

It is not.