Disallow non-c-like but "fieldless" ADTs from being casted to integer if they use arbitrary enum discriminant

[nbdd0121]

Code like

#[repr(u8)]
enum Enum {
    Foo /* = 0 */,
    Bar(),
    Baz{}
}

let x = Enum::Bar() as u8;

seems to be unintentionally allowed so we couldn't disallow them now , but we could disallow them if arbitrary enum discriminant is used before 1.56 hits stable (stabilization was reverted).

Related: #88621

@rustbot label +T-lang

[rust-highfive]

r? @jackh726

(rust-highfive has picked a reviewer for you, use r? to override)

[jackh726]

cc @rust-lang/lang

[nikomatsakis]

We did discuss this general topic in a recent meeting but we haven't had much quorum to figure out how to go. I believe the conclusion in that meeting was that, until we have time to discuss this properly, we should roll back the stabilization.

[jackh726]

Uh...this seems to have been forgotten. And we just passed a release, so it's probably too late to roll back stabilization.
edit: stabilization was reverted, but still needs a decision

So, nominating for T-lang to make a decision on what to do going forward.

[nbdd0121]

It's reverted in #89884.

[jackh726]

Oh, okay. Then I will continue to keep this nominated (and update my previous comment) for lang team to discuss and figure out what to do.

[nikomatsakis]

@nbdd0121 is this issue specifically about an accidental stabilization? If so, I cannot compile any snippet like the above, butI can do this on nightly (both feature + repr are required):

#![feature(arbitrary_enum_discriminant)]

#[repr(u8)]
enum Enum {
    Foo = 1,
    Bar(),
    Baz{}
}

fn main() { }

If this issue is just about stabilization, then I think it can be closed. Is it also about deciding the right behavior on nightly?

[jackh726]

I kept the nomination is to decide the right behavior on nightly. I don't personally have the background here, but I see there's some discussion in #88621.

[nbdd0121]

@nbdd0121 is this issue specifically about an accidental stabilization? If so, I cannot compile any snippet like the above, butI can do this on nightly (both feature + repr are required):

I think I made a mistake in the description, it's now fixed.

#![feature(arbitrary_enum_discriminant)]

#[repr(u8)]
enum Enum {
    Foo = 1,
    Bar(),
    Baz{}
}

fn main() { }

If this issue is just about stabilization, then I think it can be closed. Is it also about deciding the right behavior on nightly?

This PR was about preventing the accidental stabilization of the example you mentioned. The stabilization is now reverted, but I think the changes made by this PR is still the desirable behaviour for nightly.

[nikomatsakis]

@nbdd0121 I edited the OP to indicate that there is no "time deadline" to fixing this, now that stabilization was reverted.

[joshtriplett]

We discussed this in today's @rust-lang/lang meeting, and we're comfortable going forward with this. This is forward-compatible; we could always change this and allow such casts in the future. But we felt in the meeting that it was reasonable to disallow this.

[jswrenn]

My understanding of #88621 is to reduce some of the complexity surrounding enums (i.e., go from "three styles" to "two styles), but I worry this PR actually adds documentation complexity and introduces a minor (but subtle) stability hazard to library authors.

Documentation Complexity

Without this PR:

The unit and struct-like variants of fieldless enums can be as-casted.

With this PR:

The unit and struct-like variants of fieldless enums without explicit discriminants can be as-casted. All values of C-like enums (i.e., enums where all variants are unit-like) with explicit discriminants can be as-casted.

AFAIK, this might be the only place in the language reference actually needing to invoke the concept of "C-like" enums. I'd prefer only having to explain two kinds of enums (fieldless and non-fieldless) rather than three (fieldless, C-like, and non-fieldless). I'd also like to ditch "C-like" for another reason: the term isn't self-explanatory; rather it leans on Rust programmers knowing a completely different language. (We wouldn't call non-fieldless enums "OCaml-like enums", would we!?)

Stability Hazard

Implicit discriminants mean that seemingly-innocuous acts like adding a variant to a non_exhaustive enum or reordering variants is potentially a breaking change. We can't easily walk back this design decision, but library authors can mitigate it by adding explicit discriminants wherever possible. Unfortunately PR would make adding explicit discriminants to some as-castable enums a breaking change:

// uncommenting the explicit discriminants...
#[repr(u8)]
enum Enum {
    Foo /* = 0 */,
    Bar() /* = 1 */,
    Baz{} /* = 2 */,
}
// ...will break downstream casts like this:
let x = Enum::Bar() as u8;

This is so niche that I (hope) breakage would be uncommon, but it's nonetheless a subtlety that would need to be documented.

[nbdd0121]

@jswrenn I think what we need in addition to this PR is a lint that warns against casting non-all-unit enum.

[joshtriplett]

Follow-up: we would very much appreciate if one of the folks working on either this or #88203 would be up for adding documentation on enum discriminant handling to the Rust reference.

[jackh726]

Given that lang-team is good to move forward with this and that appropriate tracking (in the tracking issue) is done for a decision on this prior to next stabilization, this LGTM.

@bors r+

[bors]

📌 Commit 996e5ed has been approved by jackh726

[ehuss]

@jackh726 Since the lang team is asking questions at #60553 (comment), it is not clear to me that a decision was made.

[jackh726]

@ehuss I'm going off of this comment: #89234 (comment)

I think the questions posted on the tracking issue are for PRs going forward, as well as for future stabilization.

[matthiaskrgr]

Failed in a rollup: #91414 (comment)
@bors r-

[jackh726]

@bors r+ rollup=iffy

[bors]

📌 Commit f7ef1c9 has been approved by jackh726