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

[API Proposal]: Enum.GetCount #110914

Open
colejohnson66 opened this issue Dec 23, 2024 · 14 comments
Open

[API Proposal]: Enum.GetCount #110914

colejohnson66 opened this issue Dec 23, 2024 · 14 comments
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Runtime untriaged New issue has not been triaged by the area owner

Comments

@colejohnson66
Copy link

colejohnson66 commented Dec 23, 2024

Background and motivation

Quite a few times, it's desirable to get the number of enum members. The only runtime-provided solution involves an array allocation/fill, getting the count of that, then throwing away the array. For example, Enum.GetValues<T>().Length (or similar). Sometimes, people will add a _Count (or similarly named) element at the end of the member list, but this pollutes the member list, and is arguably wrong due to being typed as T not int.

On GitHub, I see ~900 uses of Enum.GetX<T>().Length and ~15k uses of Enum.GetX(typeof(T)).Length, many of which are even in @dotnet-provided (using typeof(T) and generics) or @microsoft-provided (using typeof(T) and generics) code.

API Proposal

namespace System.Collections.Generic;

public class Enum
{
    public static int GetCount(Type type);
    public static int GetCount<TEnum>() where TEnum : Enum;
    // alteratively, a constraint of only `TEnum:struct` would match the existing runtime methods
}

API Usage

int count = Enum.GetCount<MyEnum>();

Alternative Designs

No response

Risks

No response

@colejohnson66 colejohnson66 added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Dec 23, 2024
@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Dec 23, 2024
@dotnet-policy-service dotnet-policy-service bot added the untriaged New issue has not been triaged by the area owner label Dec 23, 2024
@CyrusNajmabadi
Copy link
Member

Quite a few times, it's desirable to get the number of enum elements.

Can you provide examples of this?

@vcsjones vcsjones added area-System.Runtime and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Dec 23, 2024
Copy link
Contributor

Tagging subscribers to this area: @dotnet/area-system-runtime
See info in area-owners.md if you want to be subscribed.

@colejohnson66
Copy link
Author

colejohnson66 commented Dec 23, 2024

A common situation I've encountered involves state machines for mapping an enum state to an execution delegate. These delegates are preallocated and stored into an array. Then, execution is simply _stateMap[(int)_state](...). The actual values of the enums don't matter in these cases, just that they're sequential.

While a dictionary would be more elegant in creation (and would avoid the cast at lookup), it's not as performant as a simple array access.

I don't have any code on hand to show, but the general idea looks like this (assuming we control States):

// the actual ordering of the array fill does not matter because ordering of state members is irrelevant for usage
Action<...>[] stateMap = new Action<...>[Enum.GetValues<States>().Length];
stateMap[(int)States.SomeState] = HandleSomeState;
stateMap[(int)States.SomeOtherState] = HandleSomeOtherState;
_stateMap = stateMap;

While there's indeed other, more performant, ways of encoding that length, none are as clean as the above. In the above case, one could consider the overhead of getting the values array negligible to the overall performance, but it's still present. In addition, one has to understand why it's written like that. A simple new Action<...>[Enum.GetCount<States>()] would be easier to grok, and more performant.

To clarify my original post: I've seen States.Count, but as I said, a "count" member is wrong because it's not a valid member, but would still be typed like it is one. This is only because .NET doesn't allow custom members inside enums; If we could define int Count = ..., that would also work, but I doubt that will ever come. I've also seen (int)States.SomeVeryOtherState+1, but that's confusing for readers who'd have to understand that SomeVeryOtherState is the last member. As mentioned above, the actual value and ordering of the members doesn't matter; Just their presence and that they're contiguous.

@EgorBo
Copy link
Member

EgorBo commented Dec 23, 2024

My understanding that the new API is proposed in order to eliminate a single gc allocation in e.g. Enum.GetValues<MethodImplOptions>().Length pattern, right?

It seems like it creates a short-living array allocation which is not escaped anywhere and then you only need its length. It looks like it should be perfectly handled by escape analysis feature @AndyAyersMS and @hez2010 are currently working on.

@hez2010
Copy link
Contributor

hez2010 commented Dec 23, 2024

It looks like it should be perfectly handled by escape analysis feature @AndyAyersMS and @hez2010 are currently working on.

It cannot be handled by escape analysis as it requires the length of the array to be known at compile time.
A way to make it work is to intrinsity Enum.GetValues<T> and Enum.GetNames<T>.

@CyrusNajmabadi
Copy link
Member

A common situation I've encountered involves state machines for mapping an enum state to an execution delegate

Count seems incorrect here, as it assumes that all the values would be under that Count value.

@CyrusNajmabadi
Copy link
Member

These delegates are preallocated and stored into an array.

Not getting why that can't be done with the existing apis exposed. :-)

@CyrusNajmabadi
Copy link
Member

This is only because .NET doesn't allow custom members inside enums; If we could define int Count = ..., that would also work, but I doubt that will ever come.

That is likely coming in the next version of C#. You will be able to do:

public static class EnumExtensions
{
    extension(States)
    {
        public static int Count => ... whatever you want ...;
    }
}

You'll then be able to do States.Count as a distinct static property, independent of the enum member values.

@colejohnson66
Copy link
Author

colejohnson66 commented Dec 23, 2024

A common situation I've encountered involves state machines for mapping an enum state to an execution delegate

Count seems incorrect here, as it assumes that all the values would be under that Count value.

Debatable. Enum members are not an ordered collection, so the meaning of "count" there has no impact on the other meanings of count. This API would get you the count of enum members. The meaning of that count is up to the user. You're arguing that the only proper usage is for an order list. Even then, the original purpose of this proposal is exactly for the situations where the members are known to be in order and zero-based.

These delegates are preallocated and stored into an array.

Not getting why that can't be done with the existing apis exposed. :-)

Please explain. How can the existing APIs help here? Are you suggesting that the code I showed works? Of course! I've done it multiple times. That doesn't mean the proposal is wrong — it's for a different issue. The example I showed was just showing my motivation for making this proposal.

This is only because .NET doesn't allow custom members inside enums; If we could define int Count = ..., that would also work, but I doubt that will ever come.

That is likely coming in the next version of C#. You will be able to do:

public static class EnumExtensions
{
    extension(States)
    {
        public static int Count => ... whatever you want ...;
    }
}

You'll then be able to do States.Count as a distinct static property, independent of the enum member values.

Key word: likely. We've been waiting on extension evolution for literal years. Not to discount it; I want C# to get it right, but saying "don't add an API because we might do something else" is an irrelevant discard. What happens if .NET 10 and C# 14 come and extensions are delayed again? A simple API that gives you the length of an array inside Enum.EnumInfo<TStorage> is a much smaller ask.

@colejohnson66
Copy link
Author

colejohnson66 commented Dec 23, 2024

My understanding that the new API is proposed in order to eliminate a single gc allocation in e.g. Enum.GetValues<MethodImplOptions>().Length pattern, right?

It seems like it creates a short-living array allocation which is not escaped anywhere and then you only need its length. It looks like it should be perfectly handled by escape analysis feature @AndyAyersMS and @hez2010 are currently working on.

It is not just about the allocation, but the mental logic required to understand what that line means. Alternatively, the runtime could intrinsicify Enum.GetX<T>().Length and Enum.GetX(typeof(X)).Length, but that's a bigger challenge (pattern-matching and whatnot) than a simple API. In addition, as @hez2010 said, escape analysis will likely solve nothing because the values aren't known.

@hez2010
Copy link
Contributor

hez2010 commented Dec 23, 2024

Intrinsify Enum.GetValues<T>() should be enough.
Especially, it would turn
Enum.GetValues<T>() into

var values = new UnderlyingType[<CNS_INT>];
values[0] = ...; values[1] = ...; ...
return values;

as we know the T exactly.
Then with escape analysis, the array values can be allocated on the stack.
Because we are only taking the Length of the array later, the whole thing would be folded, and the entire array would just gone.

This will be much easier than introducing a new API.

As for GetNames<T>, the escape analysis doesn't support stack allocating an array of gcref types today.

@CyrusNajmabadi
Copy link
Member

The meaning of that count is up to the user.

But you're saying you need it to initialize an array. Which only makes sense if the values are all less than that Count.

Even then, the original purpose of this proposal is exactly for the situations where the members are known to be in order and zero-based.

This seems too niche then. Why not just write an extension that gets the elements, and asks for .Length then? I'd only want an API for this if it's useful for all the cases where people use enums.

where the members are known to be in order and zero-based.

This doesn't seem sufficient. Take a simple example like:

enum X
{
    A,
    B,
    C,
    Legacy_C = C,
    D,
    E,
}

What does .Count return here? If the purpose here is to make an array with an item at each position for each state machine item, then you would want .Count - 1 here. Or you'd want a Count of distinct values.

@CyrusNajmabadi
Copy link
Member

The example I showed was just showing my motivation for making this proposal.

Ok. But im' saying that the motivation seems lacking to me. It only makes sense for narrow use cases, and is trivially possible to provide as an extension method (or an extension property in an upcoming C# version). To me, i would prefer it not be in the BCL unless super sensible and relevant to a wide set of cases.

@CyrusNajmabadi
Copy link
Member

Key word: likely. We've been waiting on extension evolution for literal years. Not to discount it; I want C# to get it right, but saying "don't add an API because we might do something else" is an irrelevant discard.

I disagree. We should not add marginal APIs that start not making much sense initially, and become even less sensible soon afterwards.

What happens if .NET 10 and C# 14 come and extensions are delayed again?

Then you use an extension method.

A simple API that gives you the length of an array inside Enum.EnumInfo is a much smaller ask.

It's not a simple API in my mind. I still don't understand waht the semantics are. And i only have seen one use case, which seems suspect to me. I'd need either a lot more use cases presented or some super high value cases shown to motivate this for me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Runtime untriaged New issue has not been triaged by the area owner
Projects
None yet
Development

No branches or pull requests

5 participants