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

Tracking Issue for core::mem::variant_count #73662

Open
1 of 3 tasks
doctorn opened this issue Jun 23, 2020 · 17 comments
Open
1 of 3 tasks

Tracking Issue for core::mem::variant_count #73662

doctorn opened this issue Jun 23, 2020 · 17 comments
Labels
B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. F-variant_count `#![feature(variant_count)]` S-tracking-design-concerns Status: There are blocking ❌ design concerns. T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@doctorn
Copy link
Contributor

doctorn commented Jun 23, 2020

The feature gate for the issue is #![feature(variant_count)].

Steps

Unresolved Questions

None

Implementation history

@doctorn doctorn added the C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. label Jun 23, 2020
@jonas-schievink jonas-schievink added B-unstable Blocker: Implemented in the nightly compiler and unstable. T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Jun 23, 2020
@doctorn doctorn changed the title Tracking Issue for core::mem::num_variants Tracking Issue for core::mem::variant_count Jun 24, 2020
@avl
Copy link

avl commented Jul 3, 2020

@RalfJung Responding to your response here instead, as per kennytm:s request.

The reason I asked about if it returned "largest discriminant + 1" is because such a behavior would be kind of reasonable with your motivation, about using the enum as an index into a Vec. There could be other situations where knowing the maximum discriminant value could actually be more important than knowing the number of elements. That said, maybe there should be two functions, one to get the maximum discriminant and one to get the number of elements. Or maybe the number of variants really is the only thing anyone ever cares about. Just wanted to bring up the question :-) .

The other point I was making was that perhaps an obligatory u64 is not necessary for the count. Other counts in rust use usize, and though I understand that you could in theory have 2^40 enum variants on a 32 bit machine, I don't think the compiler can really compile this even on a 64 bit machine right now. Also, I think having 2^40 enum variants on a 32-bit machine is sufficiently 'weird' that it seems reasonable for some friction to occur. I personally cannot imagine a scenario where an application would need more enum variants than the number of addressable bytes on the platform, though of course the fact I can't imagine it doesn't mean it doesn't exist :-) .

Really, there are some problems which are too big for some machines. As an example, a problem requiring a 100MB lookup-table is simply too big to process on a 16 bit machine. My argument is that a problem requiring a 2^40 variant enum is simply too big to process on a 32-bit machine. That said, a reasonable person could certainly disagree here!

Knowing the largest discriminant of an enum could be useful in these cases:

  • Choosing a representation during serialization. Knowing the largest discriminant means being able to tell how many bits will be needed to serialize it.
  • If an array is used as a sort of lookup-table with discriminants as keys, knowing the largest discriminant could be useful to be able to statically size the array correctly. Of course, such an approach only works well if the manually assigned discriminants are assigned in a mostly dense way.

@RalfJung
Copy link
Member

RalfJung commented Jul 3, 2020

I see, thanks.

I personally have no skin in this game, I doubt I will ever use that API.^^ I was just involved in reviewing. @doctorn would be the one to ask about motivation. :)

@Flying-Toast
Copy link
Contributor

We should probably add a deny lint to rustc that prevents calling variant_count::<T>() where T is not an enum.

Regarding the API itself though, IMO it feels a bit janky:

  • Calling variant_count where T isn't an enum is unspecified behavior (a lint would help with this)
  • Getting the variant count of an enum with > usize::MAX variants is unspecified (although practically impossible, it seems strange to forbid something that's technically allowed by the language)
  • Returns a usize, even if the repr is something else

What would be the drawbacks to doing this with a trait?

trait Enum {
    type Repr;
    /// Returns the maximum possible index into an array containing every variant of this enum.
    /// Returns `None` if the enum has no variants.
    const fn max_variant() -> Option<Repr>;
}

The compiler could automatically implement this trait for all enums, and it would be used like:

enum Foo { /* ... */ }

<Foo as core::intrinsics::Enum>::max_variant()

@Flying-Toast
Copy link
Contributor

Flying-Toast commented Apr 5, 2021

RE: @doctorn's comment in 73418:

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.

We could also use a similar API like my previous comment, but without a trait:

/// Returns the maximum possible index into an array containing every variant of the enum `T`.
/// Returns `None` if the enum has no variants.
fn max_variant<T>() -> Option<<T as DiscriminantKind>::Discriminant>

Combined with a rustc lint for non-enum Ts, I think that would get rid of all unspecified behavior. Thoughts?

@doctorn
Copy link
Contributor Author

doctorn commented Apr 5, 2021

Honestly the Enum trait sounded better to me. I much prefer having the number of variants returned rather than the maximum repr.

@Flying-Toast
Copy link
Contributor

Flying-Toast commented Apr 6, 2021

Honestly the Enum trait sounded better to me. I much prefer having the number of variants returned rather than the maximum repr.

Ok.

One more thing - the discriminant_value intrinsic seems to use u64s for discriminants; maybe variant_count should return a u64 to be consistent with that?

@lcnr
Copy link
Contributor

lcnr commented Apr 6, 2021

the discriminant_value intrinsic seems to use u64s for discriminants;

https://doc.rust-lang.org/nightly/std/intrinsics/fn.discriminant_value.html

it does not, the docs are outdated on stable though '^^

bors added a commit to rust-lang-ci/rust that referenced this issue Oct 11, 2021
Add enum_intrinsics_non_enums lint

There is a clippy lint to prevent calling [`mem::discriminant`](https://doc.rust-lang.org/std/mem/fn.discriminant.html) with a non-enum type. I think the lint is worthy of being included in rustc, given that `discriminant::<T>()` where `T` is a non-enum has an unspecified return value, and there are no valid use cases where you'd actually want this.

I've also made the lint check [variant_count](https://doc.rust-lang.org/core/mem/fn.variant_count.html) (rust-lang#73662).

closes rust-lang#83899
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jan 5, 2022
…ant-count, r=the8472

Add note about non_exhaustive to variant_count

Since `variant_count` isn't returning something opaque, I thought it makes sense to explicitly call out that its return value may change for some enums.

cc rust-lang#73662
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jan 5, 2022
…ant-count, r=the8472

Add note about non_exhaustive to variant_count

Since `variant_count` isn't returning something opaque, I thought it makes sense to explicitly call out that its return value may change for some enums.

cc rust-lang#73662
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jan 6, 2022
…ant-count, r=the8472

Add note about non_exhaustive to variant_count

Since `variant_count` isn't returning something opaque, I thought it makes sense to explicitly call out that its return value may change for some enums.

cc rust-lang#73662
ehuss added a commit to ehuss/rust that referenced this issue Jan 8, 2022
…ant-count, r=the8472

Add note about non_exhaustive to variant_count

Since `variant_count` isn't returning something opaque, I thought it makes sense to explicitly call out that its return value may change for some enums.

cc rust-lang#73662
@BusyJay
Copy link

BusyJay commented Jan 13, 2022

Should this function be a const function?

@johnkjellberg
Copy link

Should this function be a const function?

The way I want to use this require it to be const. And I don't really see why it shouldn't?

@doctorn
Copy link
Contributor Author

doctorn commented Jan 25, 2022

@BusyJay I believe this function is const, but the docs incorrectly omit this

@RalfJung
Copy link
Member

The docs mention constness in the top-right corner: const: [unstable](https://github.com/rust-lang/rust/issues/73662).

const fn is only printed for functions whose constness is stable.

@joshtriplett joshtriplett added the S-tracking-design-concerns Status: There are blocking ❌ design concerns. label Jul 13, 2022
@joshtriplett
Copy link
Member

We discussed this in today's @rust-lang/lang meeting.

This seems like it fits in with a broader story of enums, including things like AsRepr.

In the absence of those, we're not sure if we want to have this function in its current state.

@EriKWDev
Copy link

EriKWDev commented Sep 7, 2023

I use this all the time.

I love having enums represent all the variants I want and then storing data for them in arrays which requires me to know the number of them in compile time.

#[repr(usize)]
enum MyKind {
    Kind1,
    Kind2,
}

// This constant gets outdated if the number of variants change...
pub const NUM_MY_KIND: usize = 2;

I make game engines and games and this is a demonstration of a bit of data-oriented-ness.

struct KindData1 {/* ... */}
struct KindData2 {/* ... */}
struct KindData3 {/* ... */}

struct LargeData1 {/* ... */}
struct LargeData2 {/* ... */}
struct LargeData3 {/* ... */}

struct Data {
    //...
    base_1: [KindData1; NUM_MY_KIND],
    base_2: [KindData2; NUM_MY_KIND],
    base_3: [KindData3; NUM_MY_KIND],

    large_data_1: Vec<LargeData1>,
    large_data_2: Vec<LargeData2>,
    large_data_3: Vec<LargeData3>,

    // These are the same length
    kinds: Vec<MyKind>,
    flags_1: Vec<[Option<usize>; NUM_MY_KIND]>,
    flags_2: Vec<[Option<usize>; NUM_MY_KIND]>,
    flags_3: Vec<[Option<usize>; NUM_MY_KIND]>,
}

fn func(data: &Data) {
    // Operate on data common to all kinds
    for &kind in &data.kinds {
        let i = kind as usize;
        let (d1, d2) = (&mut data.base_1[i], &mut data.base_2[i]);
        // ..
    }
    for &kind in &data.kinds {
        let i = kind as usize;
        let (d2, d3) = (&mut data.base_2[i], &mut data.base_3[i]);
        // ..
    }

    assert!(data.kinds.len() == data.flags_1.len());
    assert!(data.flags_2.len() == data.flags_3.len());
    for (index, &kind) in data.kinds.iter().enumerate() {
        let i = kind as usize;

        // Operate on permutations of having and not having data
        if let (Some(id1), Some(id2)) = (data.flags_1[index][i], data.flags_2[index][i]) {
            let base1 = &data.base_1[i];
            let base2 = &data.base_2[i];
            let (d1, d2) = (&mut data.large_data_1[id1], &mut data.large_data_2[id2]);
            // ...
        }
    }

    // Operate on all data. I want this to be packed in memory
    for data_1 in &mut data.large_data_1 {
        // ..
    }
    for data_2 in &mut data.large_data_2 {
        // ..
    }

    // ..
}

I have data that is dependent on the kind but not unique per instance of the kind. This way I don't have to do a match at each location and getting the permutations of per-kind data is a simple lookup in arrays.

The other usage I have in some places is where I maybe have unique (small, usually just an id) data per instance of a kind, and I like representing that as a Vec<[Option<usize>; NUM_KIND]>. This allows me to store the data for all kinds packed in a Vec and then use the sparse Vec<[Option<usize>; NUM_KINDS]> to point back into the packed Vecs. Kind of like an ECS


The constant that keeps track of the number of kinds would be very nice to be able to get without setting manually :)

(I have tried to add a 'Length' member to the enum (const NUM: usize = MyEnum::Length as usize), but that gets very annoying at places where you do a match since it has to be ignored all the time..)

@dtolnay dtolnay added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Nov 2, 2023
@joshtriplett joshtriplett added the I-lang-nominated The issue / PR has been nominated for discussion during a lang team meeting. label Feb 14, 2024
@joshtriplett
Copy link
Member

Nominating this for lang, because empirically we've stalled out on a potentially useful library addition on the basis of proposed lang features, which themselves seem to have stalled out.

I'd love to see AsRepr if someone wants to pick that back up and get it over the finish line. But also, this might be a reasonable interim step, and it doesn't seem like it does any harm to have it even if we add better solutions in the future.

@MarinPostma
Copy link

MarinPostma commented Mar 1, 2024

@joshtriplett I'd like to pick this up, if someone can point me in the right direction

@scottmcm
Copy link
Member

scottmcm commented Mar 6, 2024

I volunteered to explore something here. Initial idea thread: https://rust-lang.zulipchat.com/#narrow/stream/213817-t-lang/topic/.60enum.23.60.20magic/near/425164499

@traviscross
Copy link
Contributor

@rustbot labels -I-lang-nominated

We discussed this in the meeting today, and as above, scottmcm volunteered to make a proposal here and we decided to wait for that. So let's unnominate this for now, but please nominate the proposal, wherever it is made, for us.

@rustbot rustbot removed the I-lang-nominated The issue / PR has been nominated for discussion during a lang team meeting. label Mar 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. F-variant_count `#![feature(variant_count)]` S-tracking-design-concerns Status: There are blocking ❌ design concerns. T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests