Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Dec 2, 2021

Conversation

nbdd0121
Copy link
Contributor

@nbdd0121 nbdd0121 commented Sep 24, 2021

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
Copy link
Collaborator

r? @jackh726

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 24, 2021
@rustbot rustbot added the T-lang Relevant to the language team, which will review and decide on the PR/issue. label Sep 24, 2021
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@jackh726
Copy link
Member

jackh726 commented Oct 5, 2021

cc @rust-lang/lang

@nikomatsakis
Copy link
Contributor

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
Copy link
Member

jackh726 commented Oct 21, 2021

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.

@jackh726 jackh726 added I-lang-nominated Nominated for discussion during a lang team meeting. S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 21, 2021
@nbdd0121
Copy link
Contributor Author

It's reverted in #89884.

@jackh726
Copy link
Member

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
Copy link
Contributor

@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
Copy link
Member

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
Copy link
Contributor Author

@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
Copy link
Contributor

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

@joshtriplett
Copy link
Member

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
Copy link
Member

jswrenn commented Nov 23, 2021

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
Copy link
Contributor Author

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

@joshtriplett joshtriplett removed the I-lang-nominated Nominated for discussion during a lang team meeting. label Nov 30, 2021
@joshtriplett
Copy link
Member

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
Copy link
Member

jackh726 commented Dec 1, 2021

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
Copy link
Contributor

bors commented Dec 1, 2021

📌 Commit 996e5ed has been approved by jackh726

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Dec 1, 2021
@ehuss
Copy link
Contributor

ehuss commented Dec 1, 2021

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

@jackh726
Copy link
Member

jackh726 commented Dec 1, 2021

@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 added a commit to matthiaskrgr/rust that referenced this pull request Dec 1, 2021
Disallow non-c-like but "fieldless" ADTs from being casted to integer if they use arbitrary enum discriminant

Code like

```rust
#[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: rust-lang#88621

`@rustbot` label +T-lang
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 1, 2021
Disallow non-c-like but "fieldless" ADTs from being casted to integer if they use arbitrary enum discriminant

Code like

```rust
#[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: rust-lang#88621

``@rustbot`` label +T-lang
@matthiaskrgr
Copy link
Member

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

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Dec 1, 2021
@rust-log-analyzer

This comment has been minimized.

... if they use arbitrary enum discriminant. Code like

```rust
enum Enum {
    Foo = 1,
    Bar(),
    Baz{}
}
```

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.
@jackh726
Copy link
Member

jackh726 commented Dec 1, 2021

@bors r+ rollup=iffy

@bors
Copy link
Contributor

bors commented Dec 1, 2021

📌 Commit f7ef1c9 has been approved by jackh726

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 1, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 2, 2021
…askrgr

Rollup of 4 iffy pull requests

Successful merges:

 - rust-lang#89234 (Disallow non-c-like but "fieldless" ADTs from being casted to integer if they use arbitrary enum discriminant)
 - rust-lang#91045 (Issue 90702 fix: Stop treating some crate loading failures as fatal errors)
 - rust-lang#91394 (Bump stage0 compiler)
 - rust-lang#91411 (Enable svh tests on msvc)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 0666a33 into rust-lang:master Dec 2, 2021
@rustbot rustbot added this to the 1.59.0 milestone Dec 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.