-
Notifications
You must be signed in to change notification settings - Fork 490
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
Update documentation for arbitrary_enum_discriminant feature #1055
Conversation
Stabilize `arbitrary_enum_discriminant` Closes rust-lang#60553. ---- ## Stabilization Report _copied from rust-lang#60553 (comment) ### Summary Enables a user to specify *explicit* discriminants on arbitrary enums. Previously, this was hard to achieve: ```rust #[repr(u8)] enum Foo { A(u8) = 0, B(i8) = 1, C(bool) = 42, } ``` Someone would need to add 41 hidden variants in between as a workaround with implicit discriminants. In conjunction with [RFC 2195](https://github.com/rust-lang/rfcs/blob/master/text/2195-really-tagged-unions.md), this feature would provide more flexibility for FFI and unsafe code involving enums. ### Test cases Most tests are in [`src/test/ui/enum-discriminant`](https://github.com/rust-lang/rust/tree/master/src/test/ui/enum-discriminant), there are two [historical](https://github.com/rust-lang/rust/blob/master/src/test/ui/parser/tag-variant-disr-non-nullary.rs) [tests](https://github.com/rust-lang/rust/blob/master/src/test/ui/parser/issue-17383.rs) that are now covered by the feature (removed by this pr due to them being obsolete). ### Edge cases The feature is well defined and does not have many edge cases. One [edge case](rust-lang#70509) was related to another unstable feature named `repr128` and is resolved. ### Previous PRs The [implementation PR](rust-lang#60732) added documentation to the Unstable Book, rust-lang/reference#1055 was opened as a continuation of rust-lang/reference#639. ### Resolution of unresolved questions The questions are resolved in rust-lang#60553 (comment). ---- (someone please add `needs-fcp`)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this looks great! 🎉
There's another link in const_eval.md
that references #custom-discriminant-values-for-fieldless-enumerations
that needs to be updated.
src/items/enumerations.md
Outdated
<ol> | ||
<li> | ||
|
||
if the enumeration is "C-like" (i.e., it has no tuple or struct variants); e.g.: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if the enumeration is "C-like" (i.e., it has no tuple or struct variants); e.g.: | |
if the enumeration is fieldless (i.e., it has no tuple or struct variants); e.g.: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is wrong because the compiler would tell me to add a repr for the enum.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand what this has to do with the representation. We have been moving the reference to use the term "fieldless enum" instead of "c-like"; that is the terminology we would prefer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, but only c-like enums are allowed to have a discriminant without a #[repr()]
attribute.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I'm still confused. The term "c-like enum" has historically been used for Rust enums that only has unit variants (that is, variants without fields). We have changed the terminology in the reference to use the phrase "fieldless enum" instead. We want to stop using the phrase "c-like" altogether. Indeed, this particular section is showing a fieldless enum. Why do you still want to use the term "c-like" here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, I can see how the literal interpretation of "fieldless" could be confused with those. The two examples with brackets define tuple and struct variants, they just have 0 fields. The unit variant has a distinctly different meaning which has different semantics. As defined here in the reference, a "fieldless enum" is one with only unit variants (your first example).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that is correct. The reference says
An enum where no constructors contain fields are called a
field-less enum.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, this PR is the one adding that definition, no? I guess my review could be more thorough here, but if you want to make that statement more precise, I would reword it to be that a fieldless enum is one that only has unit variants. I would also drop the hyphen. AFAIK, the language has always generally treated unit variants different from struct and tuple variants, even if the later two don't have fields.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all "fieldless" enums can be cast to their discriminant values. however only c-like enums may specify discriminants without a repr. @jswrenn has already said this on the original PR: #639 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Opened rust-lang/rust#88203. This should be made consistent and I see no problems for fieldless enums to have an unspecified repr.
fa1da0b
to
db8d5b0
Compare
db8d5b0
to
2e93a1f
Compare
@rustbot label -S-waiting-on-author |
Error: The feature Please let |
This comment was marked as resolved.
This comment was marked as resolved.
2e93a1f
to
ccce6d9
Compare
…ry_enum_discriminant, r=joshtriplett Stabilize arbitrary_enum_discriminant, take 2 Documentation has been updated in rust-lang/reference#1055. cc rust-lang#86860 for previous stabilization report. Not yet marks rust-lang#60553 as done: need documentation in the rust reference.
…ry_enum_discriminant, r=joshtriplett Stabilize arbitrary_enum_discriminant, take 2 Documentation has been updated in rust-lang/reference#1055. cc rust-lang#86860 for previous stabilization report. Not yet marks rust-lang#60553 as done: need documentation in the rust reference.
…ry_enum_discriminant, r=joshtriplett Stabilize arbitrary_enum_discriminant, take 2 Documentation has been updated in rust-lang/reference#1055. cc rust-lang#86860 for previous stabilization report. Not yet marks rust-lang#60553 as done: need documentation in the rust reference.
…ry_enum_discriminant, r=joshtriplett Stabilize arbitrary_enum_discriminant, take 2 Documentation has been updated in rust-lang/reference#1055. cc rust-lang#86860 for previous stabilization report. Not yet marks rust-lang#60553 as done: need documentation in the rust reference.
…ry_enum_discriminant, r=joshtriplett Stabilize arbitrary_enum_discriminant, take 2 Documentation has been updated in rust-lang/reference#1055. cc rust-lang#86860 for previous stabilization report. Not yet marks rust-lang#60553 as done: need documentation in the rust reference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Returning to this now that it has landed. I think the structure and content look great. I just want to nail down the details. Other than the comments I added, is this otherwise up to date with what eventually finalized?
src/items/enumerations.md
Outdated
following the variant name with `=` and a [constant expression]: | ||
|
||
|
||
1. if the enumeration is "C-like" (i.e., it has no tuple or struct variants); e.g.: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like we tried quite hard to scrub the presence of the "C-like" terminology, and this PR seems to bring it back. Would it be possible to come up with some other terminology here? Or, perhaps it doesn't need an explicit term, but could just say "if the enumeration only has Unit variants…"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, maybe we can use Unit-only
and link to a section when referring to it somewhere else. Will do so in a moment.
src/items/enumerations.md
Outdated
If an enumeration is C-like (with no tuple and struct variants), then its | ||
discriminant can be directly accessed with a [numeric cast]; e.g.: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to be missing the grandfathered support for casting enums with empty constructors, like:
enum Fieldless {
Tuple(),
Struct{},
Unit,
}
assert_eq!(Fieldless::Tuple() as u8, 0);
assert_eq!(Fieldless::Struct{} as u8, 1);
assert_eq!(Fieldless::Unit as u8, 2);
Was that excluded for a reason?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is that they cannot be casted to their discriminant if their discriminants are specified. This is summarized in rust-lang/rust#60553 (comment). This will likely need more discussion around the design, but none of the design concerns block the stabilization of arbitrary_enum_discriminant.
If you would like this to be included, then I can put that in.
src/items/enumerations.md
Outdated
An enum where no constructors contain fields are called a | ||
*<a id="field-less-enum">field-less enum</a>*. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the field-less term got a little confusing, and it's still not clear to me, does it also cover empty constructors, like:
enum Fieldless {
Tuple(),
Struct{},
Unit,
}
If so, perhaps that needs to be made explicit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I will add that as an example.
Co-Authored-By: Ryan Scheel <Ryan.havvy@gmail.com>
ccce6d9
to
5d9300c
Compare
|
||
impl Enum { | ||
fn discriminant(&self) -> u8 { | ||
unsafe { *(self as *const Self as *const u8) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't look right to me? Just because it's repr(u8)
I don't think that means that the discriminant is necessarily first.
If the enum
were repr(C)
or repr(C, u8)
then it'd be ok because the field order would be specified. But for repr(Rust)
I'm pretty sure we're allowed to put the discriminant after the payload, if we wanted to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, never mind, it looks like https://rust-lang.github.io/rfcs/2195-really-tagged-unions.html#guide-level-explanation means that repr(u8)
disables all field reordering too, not just sets the discriminant size :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found that odd, too. The RFC justifies this by saying the discriminant position needs to be deterministic, which seems possible to do without disabling other field reordering?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(At this point it seems too late to change, but an edition change could require repr(C)
for deterministic field ordering.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would need another flag to distinguish between the union-of-structs and struct-containing-union-of-structs layouts, though. Which might be nice, though!
I suppose the argument is that the size of the discriminant can't matter to safe code, so the only point of ever specifying its width is to get deterministic layout?
Explain how to get the discriminant out of a `#[repr(T)] enum` with payload example stolen from rust-lang/reference#1055 `@rustbot` label A-docs
Explain how to get the discriminant out of a `#[repr(T)] enum` with payload example stolen from rust-lang/reference#1055 ``@rustbot`` label A-docs
Explain how to get the discriminant out of a `#[repr(T)] enum` with payload example stolen from rust-lang/reference#1055 ```@rustbot``` label A-docs
Explain how to get the discriminant out of a `#[repr(T)] enum` with payload example stolen from rust-lang/reference#1055 ````@rustbot```` label A-docs
@rustbot ready |
@rustbot label -S-waiting-on-author +S-waiting-on-review |
The field-less casting was grandfathered in when arbitrary_enum_discriminant was stabilized. It probably shouldn't be allowed.
The reference only uses uppercase for the first letter.
Although both work, semantically an `<a>` without an `href` is intended to be a placeholder for a link.
This fixes some broken links.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went ahead and pushed some edits to fix some issues, but also to add the grandfathered behavior of allowing as
casting for fieldless enums. If anyone feels that it should be different, feel free to open a PR.
Thanks again fee1-dead, and sorry for the super long delay. |
Update books ## rust-lang/book 1 commits in 2bd5d42c9956369132228da6409f0e68da56c51a..2cd1b5593d26dc6a03c20f8619187ad4b2485552 2023-01-12 14:47:47 UTC to 2023-01-12 14:47:47 UTC - Typo (rust-lang/book#3457) ## rust-lang/nomicon 2 commits in 8ca261268068d80c0969260fff15199bad87b587..960d610e7f33889a2577f5f17c26f0d5c82b30df 2023-01-06 11:51:41 UTC to 2023-01-05 10:20:31 UTC - vec/raw: Simplify `RawVec::grow` (rust-lang/nomicon#392) - borrow-splitting: Use `take` instead of `replace` (rust-lang/nomicon#391) ## rust-lang/reference 4 commits in 3ae62681ff236d5528ef7c8c28ba7c6b2ecc6731..2cb0ed9ba56360949f492f9866afe8c293f9f9da 2023-01-13 03:16:35 UTC to 2023-01-07 00:08:06 UTC - Update field-expr.md (rust-lang/reference#1318) - Update documentation for arbitrary_enum_discriminant feature (rust-lang/reference#1055) - Add links to definitions of terminology ... (rust-lang/reference#1315) - Enable triagebot shortcuts (rust-lang/reference#1314) ## rust-lang/rust-by-example 2 commits in 8888f9428fe9a48f31de6bd2cef9b9bf80791edc..a9fb7d13eadfcc5f457962731f105b97f9a7474a 2023-01-14 10:25:39 UTC to 2023-01-11 18:25:42 UTC - get_or_insert example: print my_fruit as intended (rust-lang/rust-by-example#1664) - Update print.md (rust-lang/rust-by-example#1663) ## rust-lang/rustc-dev-guide 8 commits in b3e2a6e6c8a3aae5b5d950c63046f23bae07096d..7352353ae91c48b136d2ca7d03822e1448165e1e 2023-01-14 20:34:23 UTC to 2023-01-02 23:35:09 UTC - fix examples for rustc 1.68.0-nightly (935dc07 2022-12-19) (rust-lang#1556) (rust-lang/rustc-dev-guide#1557) - Update incremental-compilation-in-detail.md (rust-lang/rustc-dev-guide#1553) - Link to the youtube recording of my talk, not the summary (rust-lang/rustc-dev-guide#1554) - Change `src/test` to `tests` (rust-lang/rustc-dev-guide#1547) - add full name for ICE (rust-lang/rustc-dev-guide#1552) - Fix incorrect links (rust-lang/rustc-dev-guide#1549) - fix rebase link (rust-lang/rustc-dev-guide#1546) - Add a section for how to review code more easily (rust-lang/rustc-dev-guide#1538)
Explain how to get the discriminant out of a `#[repr(T)] enum` with payload example stolen from rust-lang/reference#1055 ````@rustbot```` label A-docs
…strieb Update links for custom discriminants. The discriminant documentation was updated in rust-lang/reference#1055 which changed the layout a bit. This updates the links to the updated locations.
…strieb Update links for custom discriminants. The discriminant documentation was updated in rust-lang/reference#1055 which changed the layout a bit. This updates the links to the updated locations.
…strieb Update links for custom discriminants. The discriminant documentation was updated in rust-lang/reference#1055 which changed the layout a bit. This updates the links to the updated locations.
Update links for custom discriminants. The discriminant documentation was updated in rust-lang/reference#1055 which changed the layout a bit. This updates the links to the updated locations.
Continuation of #639. This should only be merged after the feature is stabilized.