Determine how to handle casting of arbitrary discriminants with different enum styles

[ehuss]

This is an issue to track making a decision on how to handle certain styles of enums with respect to the new arbitrary_enum_discriminant which has been stabilized in 1.56 (#86860) which will be released October 21st 2021. I think it would be good to make a decision before it reaches stable.

I have a concern that this introduces three styles of enums, and I think it would be best to only have two styles. In summary, as of 1.56, they will be:

Kind 1: Unit-only variants:

// Does not require repr
enum Kind1 {
    Foo = 1,
    Bar = 2,
    Baz = 3,
}

// Can be cast to its discriminant:
let x = Kind1::Foo as u8;

Kind 2: Tuple or struct variants without fields:

#[repr(u8)]   // Required repr
enum Kind2 {
    Foo() = 1,
    Bar{} = 2,
    Baz = 3,
}

// Can be cast to its discriminant:
let x = Kind2::Foo() as u8;

Kind 3: Tuple or struct variants with fields:

#[repr(u8)]   // Required repr
enum Kind3 {
    Foo(i32) = 1,
    Bar{f: i32} = 2,
    Bar = 3,
}

// ERROR: Does not allow casting.
let x = Kind3::Foo(1) as u8;

The question mainly centers on how Kind2 should be treated. #88203 is a proposal to make it treated the same as Kind1 (that is, does not require repr). Another proposal is to make it treated the same as Kind3 (does not allow casting), PR up at #89234. Another option it to leave it as-is, and keep three styles.

Only Kind1 is allowed in 1.55 and older versions.

(As an aside, this also highlights that our current terminology of "Fieldless enums" is confusing, since it has historically meant only Kind1 style, but it can be confused with Kind2 since those do not have fields, either.)

More discussion at https://rust-lang.zulipchat.com/#narrow/stream/213817-t-lang/topic/arbitrary.20enum.20discriminants/near/251899236

[ehuss]

@rust-lang/lang Nominating to verify what we should do. It was observed that the following has been allowed forever:

// Works with or without repr.
enum Fieldless {
    Foo(),
    Bar{},
    Baz,
}

fn main() {
    assert_eq!(Fieldless::Foo() as u8, 0);
    assert_eq!(Fieldless::Bar{} as u8, 1);
    assert_eq!(Fieldless::Baz as u8, 2);
}

Given that this has always been allowed, I'm thinking it would be awkward to reject the same code if it has an explicit discriminant. I'm not even certain it is possible to do that (at least not easily). I don't see a way to check whether or not a variant has a discriminant expression where the castability is checked here.

So, I think some options are:

1. Proceed with Make specifying repr optional for fieldless enums #88203, and make Kind2 behave the same as Kind1. I believe this should be safe to do at any time, so there is no time pressure for this option.
2. Find some way to reject casting only with explicit discriminants (and make Kind2 behave as Kind3). This is under time pressure, because it would be a breaking change (although unlikely anyone would write this code).
3. Leave it as-is and have 3 styles of enums.
4. Revert stabilization to make more time to make a decision.

I don't know which would be the best option (or if there are other options?). I guess I slightly lean towards 1 or maybe 3.

[nbdd0121]

I think we should take option 3 for now and add a future-compat lint to prevent Fieldless::Foo() as u8 style casting.

[illicitonion]

2. Find some way to reject casting only with explicit discriminants (and make Kind2 behave as Kind3). This is under time pressure, because it would be a breaking change (although unlikely anyone would write this code).

I believe adding something like this to CastTy::from_ty should do the job, and the caller can error appropriately? d.variants.iter().all(|v| if let VariantDiscr::Explicit(_) = v.discr { false } else { true })

---

Personally, I lean strongly towards "as casts for enums were a mistake", so adding a future-compat lint to avoid them, and leaning into alternative APIs such as implemented in #81642 sound good to me. In the short term, rejecting them (so as to not open up more things to potentially be linting in the future) would be my leaning, even if it's slightly inconsistent for now.

[ehuss]

I believe adding something like this to CastTy::from_ty should do the job,

I don't think that would handle relative variants like:

#[repr(u8)]
enum E {
    V1 = 1,
    V2(),
    V3{}
}

let x = E::V2() as u8;

[nbdd0121]

d.variants.iter().any(|v| matches!(v.discr, VariantDiscr::Explicit(_)) && v.ctor_kind != CtorKind::Const)

should do the job. We can add this to is_payloadfree.

[nbdd0121]

PR for option 2: #89234

[Mark-Simulacrum]

Discussed today during T-lang meeting -- people in the meeting (not quorum, FWIW) were surprised that new as casts were permitted by the stabilization here (#86860), so felt that given the time pressure it makes sense to revert stabilization for the time being. We can re-stabilize with more discussion, potentially, but don't have sufficient quorum to make a decision right this moment.

[Mark-Simulacrum]

(Marking as a regression for tracking the revert)

[Mark-Simulacrum]

Revert handled on 1.57 with #89884 and 1.56 with #89854, detagging.

[ehuss]

Closing as this is essentially resolved due to the stabilization in 1.66. See #60553 (comment) for some final thoughts.

[nbdd0121]

Well, this is "resolved" in a sense that we have unblocked arbitrary_enum_discriminant (by banning certain constructs if arbitrary enum discriminant is used), but we still have a messy state where there are a lot of inconsistencies. Should we still have the issue tracking to see if we could make the rule simpler?