Implement arbitrary_enum_discriminant #60732
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @pnkfelix (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
rust-highfive
added
the
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Would be nice to get a test case that has some CTFE code involving the new discriminants, just to make sure that works, too. Cc @oli-obk do you know if this needs adjustments in Miri (specifically in |
This comment has been minimized.
This comment has been minimized.
No, this requires no changes for anything beyond the AST. Everything else is already setup to handle this. Some const eval tests would be nice though. |
|
So... there's this fun comment in the layouting source: rust/src/librustc/ty/layout.rs Lines 879 to 883 in a9ec99f I'm guessing you can observe this as follows: enum Foo {
A(&'static i32),
B,
}
enum Bar {
A(&'static i32) = 0,
B = 1,
}
assert_eq!(std::mem::size_of::<Foo>() == std::mem::size_of::<usize>());
assert_eq!(std::mem::size_of::<Bar>() > std::mem::size_of::<usize>()); |
Would something like this fit the bill? #![feature(arbitrary_enum_discriminant,const_raw_ptr_deref)]
#[allow(dead_code)]
#[repr(u8)]
enum Enum {
Unit = 3,
Tuple(u16) = 2,
Struct {
a: u8,
b: u16,
} = 1,
}
impl Enum {
const unsafe fn tag(&self) -> u8 {
*(self as *const Self as *const u8)
}
}
const UNIT_TAG: u8 = unsafe { Enum::Unit.tag() };
const TUPLE_TAG: u8 = unsafe { Enum::Tuple(5).tag() };
const STRUCT_TAG: u8 = unsafe { Enum::Struct{a: 7, b: 11}.tag() };
fn main() {
assert_eq!(3, UNIT_TAG);
assert_eq!(2, TUPLE_TAG);
assert_eq!(1, STRUCT_TAG);
} |
|
@oli-obk To be sure I understand correctly: is the expected behavior that both |
|
No, it is perfectly fine for them to have different sizes for now, it's just an implementation detail that we can fix in the future. Maybe add my code as a canary test? One thing that I'm wondering: the RFC states that the only legal way to get the discriminant back is to start out with a One thing that I think should also have a test, too: code matching on an enum with both fields and explicit discriminants. |
Looks great! |
I think you're referring to the stipulation that:
As it stands, the functionality enabled by this PR forbids you from #![feature(arbitrary_enum_discriminant)]
#[repr(u8)]
#[allow(dead_code)]
enum Enum {
Unit = 3,
Tuple(()) = 2,
Struct {
a: (),
b: (),
} = 1,
}
fn main() {
println!("{:?}", Enum::Unit as u8);
}and produces this error: (Click to Expand)But this is permitted: #![feature(arbitrary_enum_discriminant)]
#[allow(dead_code)]
enum Enum {
Unit = 3,
Tuple() = 2,
Struct {} = 1,
}
fn main() {
assert_eq!(3, Enum::Unit as u8);
assert_eq!(2, Enum::Tuple() as u8);
assert_eq!(1, Enum::Struct{} as u8);
}...and I think it's reasonable and consistent to permit that, since rust already permits writing: #[allow(dead_code)]
enum Enum {
Unit,
Tuple(),
Struct{},
}
fn main() {
assert_eq!(0, Enum::Unit as u8);
assert_eq!(1, Enum::Tuple() as u8);
assert_eq!(2, Enum::Struct{} as u8);
}I can't think of a compelling reason to add an explicit However, although the RFC explicitly does not enable this functionality, it seems to me like |
|
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
I see your point. I think this is a good idea, and I'll take a stab at it in the next few days! Implementation wise, I think |
|
(hmm, i'm sorry: I meant to post my change to your doc ( 6c09c00a66dd76107fbdf2ef5b62427095844f5c ) as a suggestion, not as a commit on its own. Feel free to revise that as you like. I'll try to ensure my future suggestions are deployed as intended: i.e., as suggestions, not commits.) |
|
I have one minor question about the diagnostic code; once that is addressed, r=me. |
pnkfelix
added
S-waiting-on-author
|
@jswrenn wrote:
i don't mind landing the bulk of the code as is and landing the change suggested here in a later PR. I'll let @jswrenn decide which approach they would prefer to take. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@Centril, github still lists you as 'Requesting Changes', but the unresolved suggestion isn't reachable (because of a rebase), so I cannot mark it as resolved. To the best of my knowledge, I've resolved all issues raised so far. |
|
☔ The latest upstream changes (presumably #61347) made this pull request unmergeable. Please resolve the merge conflicts. |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
(I think this may be waiting for another review round after updates, despite what the label says) |
oli-obk
added
S-waiting-on-review
|
@bors r+ |
|
📌 Commit ac98342 has been approved by |
bors
added
S-waiting-on-bors
Implement arbitrary_enum_discriminant Implements RFC rust-lang/rfcs#2363 (tracking issue #60553).
|
☀️ Test successful - checks-travis, status-appveyor |
4056: INT: Fix creation of new module if no corresponding directory exists r=mchernyavsky a=mchernyavsky Fixes #3995. 4069: Support for arbitrary enum discriminants r=mchernyavsky a=mibac138 Reference: rust-lang/rust#60732 Co-authored-by: mchernyavsky <[email protected]> Co-authored-by: mibac138 <[email protected]>
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`)
Implements RFC rust-lang/rfcs#2363 (tracking issue #60553).