Add unstable core::mem::variant_count intrinsic
#73418
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
r? @kennytm (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
lcnr
reviewed
Jun 16, 2020
|
please also include a test case for empty enum. enum Void {}
assert_eq!(num_variants::<Void>(), 0); |
I had to do exactly this in PR #72121: In this case, I could have generated the appropriate const by wrapping the |
|
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 |
Aaron1011
reviewed
Jun 16, 2020
| @@ -1917,6 +1917,15 @@ extern "rust-intrinsic" { | |||
| #[rustc_const_unstable(feature = "const_discriminant", issue = "69821")] | |||
| pub fn discriminant_value<T>(v: &T) -> <T as DiscriminantKind>::Discriminant; | |||
|
|
|||
| /// Returns the number of variants of the type `T` cast to a `usize`; | |||
There was a problem hiding this comment.
Returning a usize is incompatible with #![feature(repr128)].
We don't currently expose a way for users to determine the discriminant type of an enum (see #70705). I think we have a few options here:
- Make this function return a
u128. Assuming we never addu256/i256to the language (both of which seem insane as an enum discriminant type), this would be compatible with enums with any number of variants. - Stabilize the
DiscriminantKindtrait, and have this function return a value of type<T as DiscriminantKind>::Discrimininant>. This does not solve the problem of converting the value to an integer, but it avoids returning au128for enums which don't need it. - Declare that enums with more than
usize::MAXvariants are unsupported (#[repr(u128)]enums with fewer variants are fine). We might want to consider changing this tou64, though I don't actually see any use-case for having an enum with more than 4 billion variants (let alone on a 32-bit platform).
There was a problem hiding this comment.
On second thought, Rust does have tier-3 support for a 16-bit platform (AVR). While having u16::MAX variants seems ridiculous, it seems slightly more questionable to forbid that compared to u32::MAX (though again, I have no idea why anyone would do such a thing). Perhaps a u32 is the way to go here.
There was a problem hiding this comment.
Seeing as the compiler uses an IdxVec to track declared variants we’re guaranteed to ICE if we have an enum with a number of variants exceeding usize::MAX anyway so I think they’re implicitly unsupported.
There was a problem hiding this comment.
Making it explicit is no bad thing though!
There was a problem hiding this comment.
Ah - I completely forgot about cross compilation... u64 seems like the best bet then
There was a problem hiding this comment.
Ok... Thinking about this some more, I think we stick with usize and add a note to say that if your enum has more than usize::MAX variants on the target platform, the return value is unspecified. The probability this ever happens is minuscule and we've covered our backs with the note.
There was a problem hiding this comment.
Seeing as the compiler uses an IdxVec to track declared variants we’re guaranteed to ICE if we have an enum with a number of variants exceeding usize::MAX anyway so I think they’re implicitly unsupported.
The compiler could be running on a 64bit system but generate code for a 16bit system. So you cannot really deduce anything from the type the compiler is using here.
There was a problem hiding this comment.
I’m in favour of <T as DiscriminantKind>::Discrimininant> or u128 or u64, in that specific order. Having safe and stable APIs that may return unspecified or incorrect results without a very very convincing reason is questionable.
uses an IdxVec to track declared variants we’re guaranteed to ICE if we have an enum with a number of variants exceeding usize::MAX anyway
This is an implementation detail that can be changed in the future to adapt to changing needs. The return type of a stable API will get set in stone for however long Rust exists.
There was a problem hiding this comment.
Unfortunately, you can’t use <T as DiscriminantKind>::Discriminant because if I have an enum with 256 variants the discriminant type will be u8 which can’t represent 256.
There was a problem hiding this comment.
Seeing as the compiler uses an
IdxVecto track declared variants we’re guaranteed to ICE if we have an enum with a number of variants exceedingusize::MAXanyway so I think they’re implicitly unsupported.
I should have struck this out because it doesn’t apply anyway in the case of cross comp
|
So moving this forward, do I need to open tracking issues for |
|
As there are no more immediate concerns, please file a single tracking issue and fill in the numbers (IMO we don't need a separate gate for r=me after that. |
Aaron1011
reviewed
Jun 22, 2020
| const TEST_BAR: usize = num_variants::<Bar>(); | ||
|
|
||
| fn main() { | ||
| assert_eq!(TEST_VOID, 0); |
There was a problem hiding this comment.
Could you add a test for a struct and some primitives (e.g bool, *const (), u8)?
There was a problem hiding this comment.
As in to test there is no ICE, or to test for a specific return value?
There was a problem hiding this comment.
I'm not sure what the proper behavior should be, but we should have a test that verifies whatever we decide the correct behavior is.
There was a problem hiding this comment.
It's left as unspecified as with core::mem::discriminant
There was a problem hiding this comment.
The underlying intrinsic always returns zero at the moment (apart from for structs where it will return one)
|
☔ The latest upstream changes (presumably #73643) made this pull request unmergeable. Please resolve the merge conflicts. |
3 tasks
|
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 |
|
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 |
|
bikeshed: I personally prefer |
|
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 |
lcnr
reviewed
Jun 23, 2020
RalfJung
reviewed
Jun 24, 2020
RalfJung
reviewed
Jun 24, 2020
doctorn
changed the title
core::mem::num_variants intrinsiccore::mem::variant_count intrinsic
RalfJung
reviewed
Jun 24, 2020
|
@bors r+ |
|
📌 Commit 4931996 has been approved by |
bors
added
S-waiting-on-bors
|
⌛ Testing commit 4931996 with merge e5c70de47ff0e2b2af1751bd6f084b085ccc1c08... |
|
@bors retry yield (will likely include this in a rollup after the clippy pr is merged) |
Manishearth
added a commit
to Manishearth/rust
that referenced
this pull request
Jun 26, 2020
Add unstable `core::mem::variant_count` intrinsic
Adds a new `const fn` intrinsic which can be used to determine the number of variants in an `enum`.
I've shown this to a couple of people and they invariably ask 'why on earth?', but there's actually a very neat use case:
At the moment, if you want to create an opaque array type that's indexed by an `enum` with one element for each variant, you either have to hard-code the number of variants, add a `LENGTH` variant or use a `Vec`, none of which are suitable in general (number of variants could change; pattern matching `LENGTH` becomes frustrating; might not have `alloc`). By including this intrinsic, it becomes possible to write the following:
```rust
#[derive(Copy, Clone)]
enum OpaqueIndex {
A = 0,
B,
C,
}
struct OpaqueVec<T>(Box<[T; std::mem::num_variants::<OpaqueIndex>()]>);
impl<T> std::ops::Index<OpaqueIndex> for OpaqueVec<T> {
type Output = T;
fn index(&self, idx: OpaqueIndex) -> &Self::Output {
&self.0[idx as usize]
}
}
```
(We even have a use cases for this in `rustc` and I plan to use it to re-implement the lang-items table.)
Manishearth
added a commit
to Manishearth/rust
that referenced
this pull request
Jun 26, 2020
Add unstable `core::mem::variant_count` intrinsic
Adds a new `const fn` intrinsic which can be used to determine the number of variants in an `enum`.
I've shown this to a couple of people and they invariably ask 'why on earth?', but there's actually a very neat use case:
At the moment, if you want to create an opaque array type that's indexed by an `enum` with one element for each variant, you either have to hard-code the number of variants, add a `LENGTH` variant or use a `Vec`, none of which are suitable in general (number of variants could change; pattern matching `LENGTH` becomes frustrating; might not have `alloc`). By including this intrinsic, it becomes possible to write the following:
```rust
#[derive(Copy, Clone)]
enum OpaqueIndex {
A = 0,
B,
C,
}
struct OpaqueVec<T>(Box<[T; std::mem::num_variants::<OpaqueIndex>()]>);
impl<T> std::ops::Index<OpaqueIndex> for OpaqueVec<T> {
type Output = T;
fn index(&self, idx: OpaqueIndex) -> &Self::Output {
&self.0[idx as usize]
}
}
```
(We even have a use cases for this in `rustc` and I plan to use it to re-implement the lang-items table.)
This was referenced Jun 26, 2020
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Jun 26, 2020
…arth Rollup of 14 pull requests Successful merges: - rust-lang#72617 (Add a fast path for `std::thread::panicking`.) - rust-lang#72738 (Self contained linking option) - rust-lang#72770 (Implement mixed script confusable lint.) - rust-lang#73418 (Add unstable `core::mem::variant_count` intrinsic) - rust-lang#73460 (Emit line info for generator variants) - rust-lang#73534 (Provide suggestions for some moved value errors) - rust-lang#73538 (make commented examples use valid syntax, and be more consistent ) - rust-lang#73581 (Create 0766 error code) - rust-lang#73619 (Document the mod keyword) - rust-lang#73621 (Document the mut keyword) - rust-lang#73648 (Document the return keyword) - rust-lang#73673 (Fix ptr doc warnings.) - rust-lang#73674 (Tweak binop errors) - rust-lang#73687 (Clean up E0701 explanation) Failed merges: - rust-lang#73708 (Explain move errors that occur due to method calls involving `self` (take two)) r? @ghost
|
What if some variants have explicitly given discriminants? Does this still return the number of variants, or 'largest discriminant + 1"? Also, regarding the number of variants, isn't usize::MAX a perfectly reasonable max size? It's the max size of arrays on the platform, and making it impossible to count the number of enum variants if it is larger than this seems reasonable. |
|
If we were to introduce "largest discriminant" instead of "variant_count", we could use the discriminant type. Like if you had 256 variants and repr(u8), the largest discriminant would be 256. I suppose both features could be useful. |
Nothing in the docs speaks about discriminants. It is all about the number of variants. I am curious why you think this function might have anything to do with discriminant values? EDIT: To be fair, the motivation requires assigning discriminant values contiguously starting from 0. I am not sure if "discriminant count" is ever useful for other enums.
You can totally have a EDIT: To be fair, the motivation only really works for things up to
How is "largest discriminant" ever useful? Imagine an enum with two valid discriminants, 0 and 100. If you transmute 50 so this enum, that is UB. Just knowing the largest discriminant doesn't give you any information at all about what the other discriminants are. |
|
Let's discuss these in the tracking issue #73662, thanks. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adds a new
const fnintrinsic which can be used to determine the number of variants in anenum.I've shown this to a couple of people and they invariably ask 'why on earth?', but there's actually a very neat use case:
At the moment, if you want to create an opaque array type that's indexed by an
enumwith one element for each variant, you either have to hard-code the number of variants, add aLENGTHvariant or use aVec, none of which are suitable in general (number of variants could change; pattern matchingLENGTHbecomes frustrating; might not havealloc). By including this intrinsic, it becomes possible to write the following:(We even have a use cases for this in
rustcand I plan to use it to re-implement the lang-items table.)