Explain how to get the discriminant out of a #[repr(T)] enum with payload
#104892
Conversation
|
r? @scottmcm (rustbot has picked a reviewer for you, use r? to override) |
rustbot
added
S-waiting-on-review
|
Hey! It looks like you've submitted a new PR for the library teams! If this PR contains changes to any Examples of
|
rustbot
added
the
A-docs
lukas-code
changed the title
#[repr(T)] enum#[repr(T)] enum with payload
library/core/src/mem/mod.rs
Outdated
| /// | ||
| /// impl Enum { | ||
| /// fn discriminant(&self) -> u8 { | ||
| /// unsafe { *(self as *const Self as *const u8) } |
There was a problem hiding this comment.
We're trying to avoid as on pointers now, where possible, in favour of methods. So please change this to something using cast & friends, maybe
| /// unsafe { *(self as *const Self as *const u8) } | |
| /// unsafe { <*const _>::from(self).cast::<u8>().read() } |
Or you could consider something like
pub fn discriminant(&self) -> &u8 {
unsafe { NonNull::from(self).cast::<u8>().as_ref() }
}There was a problem hiding this comment.
Oh, and it would be good to have a comment on the unsafe block repeating why it's sound. Maybe
// SAFETY: because `Self` is marked `repr(u8)`,
// its layout is a `repr(C)` `union` between `repr(C)` structs,
// each of which has the `u8` discriminant as its first field,
// so we can read the discriminant without offsetting the pointer.
There was a problem hiding this comment.
TIL that <*const _>::from(reference) works. Since there is no impl From<&T> for *const T, it probably selects From<*const T> for *const T and then coerces the parameter?
We should probably document the "canonical" way to convert a reference to a raw pointer when coercion isn't an option somewhere. So far I've seen reference as *const _, ptr::addr_of!(*reference) and now <*const _>::from(reference).
There was a problem hiding this comment.
I've removed the as cast on pointers now, but kept *ptr over ptr.read() since u8 is Copy and that seems "safer" to me.
library/core/src/mem/mod.rs
Outdated
| @@ -1129,6 +1129,33 @@ impl<T> fmt::Debug for Discriminant<T> { | |||
| /// assert_eq!(mem::discriminant(&Foo::B(1)), mem::discriminant(&Foo::B(2))); | |||
| /// assert_ne!(mem::discriminant(&Foo::B(3)), mem::discriminant(&Foo::C(3))); | |||
| /// ``` | |||
| /// | |||
| /// Note that it is *undefined behavior* to [`transmute`] from [`Discriminant`] to a primitive. | |||
| /// To access the value of a discriminant with a primitive representation, use pointer casts instead: | |||
There was a problem hiding this comment.
Note that the test below currently "passes" even without the repr, though it's UB without the repr -- and MIRI doesn't complain about it either.
So thus I think it's important to put extra emphasis on how this only applies for non-default reprs, and it'd be nice to link to some documentation about those other reprs.
Here's a first draft of an idea, please improve it:
Note that it is undefined behavior to [
transmute] from [Discriminant] to a primitive.If an enum has opted-in to having a defined layout, then it's possible to use pointers to read the memory location storing the discriminant. That cannot be done for anything using the default
enumlayout, however, as it's undefined where the discriminant is stored -- it might not even be stored at all!
rustbot
added
S-waiting-on-author
library/core/src/mem/mod.rs
Outdated
| /// of some variant will not change between compilations with the same compiler. See the [Reference] | ||
| /// for more information. | ||
| /// | ||
| /// [Reference]: ../../reference/items/enumerations.html#discriminants |
There was a problem hiding this comment.
This section doesn't exist until rust-lang/reference#1055 is merged, but the linked page still contains some relevant information.
There was a problem hiding this comment.
The CI didn't like this, so we need to revert 946d51e later.
lukas-code
force-pushed
the
discriminant
branch
from
0a39168 to
e06b61c
Compare
|
I've updated it now to include some links to the Reference on enum reprs. I'd rather not link to the RFC, since RFCs tend to get outdated over time and that has led to confusion before. I intentionally left out I also added the easy way to get the discriminant of a c-like enum now, to avoid having someone use unsafe code when it isn't necessary. Docs preview available here: https://lukas-code.github.io/rust-docs/core/mem/fn.discriminant.html |
|
@rustbot ready |
rustbot
added
S-waiting-on-review
|
Thanks for the updates! Having the safe way there first is a great addition. @bors r+ rollup |
bors
added
S-waiting-on-bors
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
…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#95836 (Use `rust_out{exe_suffix}` for doctests) - rust-lang#104882 (notify lcnr on changes to `ObligationCtxt`) - rust-lang#104892 (Explain how to get the discriminant out of a `#[repr(T)] enum` with payload) - rust-lang#104917 (Allow non-org members to label `requires-debug-assertions`) - rust-lang#104931 (Pretty-print generators with their `generator_kind`) - rust-lang#104934 (Remove redundant `all` in cfg) - rust-lang#104944 (Support unit tests for jsondoclint) - rust-lang#104946 (rustdoc: improve popover focus handling JS) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
example stolen from rust-lang/reference#1055
@rustbot label A-docs