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: Add a Generic version of GetValues to Enum (probably GetName/GetNames) #2364

Closed
andreise opened this issue Feb 25, 2018 · 30 comments · Fixed by #33589
Closed
Labels
api-approved API was approved in API review, it can be implemented area-System.Reflection
Milestone

Comments

@andreise
Copy link

I suggest to implement Generic version of GetValues method in the Enum class.

In present, if we going to get an typed array of some enum values we have to write following code:
SomeEnum[] values = (SomeEnum[])Enum.GetValues(typeof(SomeEnum));

It seems as unconvenient way.
In my opinion, the following Generics-based syntax should seems shorter, more convenient and safely (by reason of type casting necessity):
SomeEnum[] values = Enum.GetValues<SomeEnum>();

The possible way to implement this syntax is:

public static TEnum[] GetValues<TEnum>() where TEnum : struct
{
    Type enumType = typeof(TEnum);
    return (TEnum[])enumType.GetEnumValues();
}

This proposal is inspired by the great Generic-based version of Enum.TryParse method laying besides with legacy non-Generic version:

public static bool TryParse(Type enumType, String value, out Object result)
public static bool TryParse<TEnum>(String value, out TEnum result) where TEnum : struct

If proposal to implement Generic version of GetValues will be accepted I guess following Generics-based implementations of GetName GetNames should be implemented for symmetry reason:

public static String GetName<TEnum>(Object value) where TEnum : struct
{
    Type enumType = typeof(TEnum);
    return enumType.GetEnumName(value);
}

public static String[] GetNames<TEnum>() where TEnum : struct
{
    Type enumType = typeof(TEnum);
    return enumType.GetEnumNames();
}

I introduce possible implementation of this proposal in dotnet/coreclr/pull#16557

This is a small point improvement suggestion.
It can be implemented separately of as a part of total Enum improvements discussing in dotnet/corefx/#15453

@andreise andreise changed the title Add a Generic version of GetValues to Enum (probably GetName/GetNames) API Proposal: Add a Generic version of GetValues to Enum (probably GetName/GetNames) Feb 26, 2018
@terrajobst
Copy link
Member

terrajobst commented Oct 10, 2018

While @joperezr owns this are I believe this issue is read for an API review. @joperezr, feel free to unmark if you think otherwise 😄

@TylerBrinkley
Copy link
Contributor

TylerBrinkley commented Dec 13, 2018

Split off from dotnet/corefx#15453

Enums are essential commonly used types but have several areas in need of improvement. For each non-generic Enum method there should be an equivalent generic version.

Rationale and Usage

  1. Nearly all of Enum's static methods are non-generic leading to the following issues.
    • Requires boxing for methods with enum input parameters losing type-safety, eg. IsDefined and GetName.
    • Requires casting/unboxing for methods with an enum return value, eg. ToObject and GetValues.
    • Requires the enum type to be explicitly specified as an argument.
    • Requires invocation using static method syntax.

With this proposal implemented what used to be this to validate a standard enum value

MyEnum value = ???;
bool isValid = Enum.IsDefined(typeof(MyEnum), value);

now becomes this

MyEnum value = ???;
bool isValid = value.IsDefined();

With this implemented it will address dotnet/corefx#10692.

Proposed API

namespace System {
    public abstract class Enum : ValueType, IComparable, IConvertible, IFormattable {
        public static string Format<TEnum>(TEnum value, string format) where TEnum : struct, Enum;
        public static string GetName<TEnum>(this TEnum value) where TEnum : struct, Enum;
        public static IReadOnlyList<string> GetNames<TEnum>() where TEnum : struct, Enum;
        public static IReadOnlyList<TEnum> GetValues<TEnum>() where TEnum : struct, Enum;
        public static bool IsDefined<TEnum>(this TEnum value) where TEnum : struct, Enum;
        public static TEnum ToObject<TEnum>(object value) where TEnum : struct, Enum;
        public static TEnum ToObject<TEnum>(sbyte value) where TEnum : struct, Enum;
        public static TEnum ToObject<TEnum>(byte value) where TEnum : struct, Enum;
        public static TEnum ToObject<TEnum>(short value) where TEnum : struct, Enum;
        public static TEnum ToObject<TEnum>(ushort value) where TEnum : struct, Enum;
        public static TEnum ToObject<TEnum>(int value) where TEnum : struct, Enum;
        public static TEnum ToObject<TEnum>(uint value) where TEnum : struct, Enum;
        public static TEnum ToObject<TEnum>(long value) where TEnum : struct, Enum;
        public static TEnum ToObject<TEnum>(ulong value) where TEnum : struct, Enum;
    }
}

API Details

This proposal makes use of a C# language feature that needs to be added in order for this proposal to make the most impact.

This proposal specifies extension methods within System.Enum and as such requires C# to allow extension methods within non-static classes as is proposed in csharplang#301. Promoting these to extension methods later would be a breaking change due to csharplang#665 but I feel this is acceptable.

Alternatively, the extension methods could be defined in a separate static EnumExtensions class. This is uglier but would avoid this issue and the extension methods would be available immediately instead of needing to wait for a later C# version to support this.

Open Questions

  • Is promoting the System.Enum extension methods later an acceptable breaking change due to csharplang#665? If not should we wait for a later C# version that supports extension methods in System.Enum or should we introduce a separate EnumExtensions class?

Updates

  • Changed GetNames and GetValues to return an IReadOnlyList instead of IEnumerable.
  • Added Format.

@terrajobst
Copy link
Member

terrajobst commented Aug 27, 2019

@TylerBrinkley

This looks like a separate proposal. Let's keep this focused on just this API:

namespace System
{
    public partial abstract class Enum
    {
       public static TEnum[] GetValues<TEnum>() where TEnum : Enum;
    }
}

This looks reasonable and we agree that it makes calling code nicer. @tannergooding brought up runtime concerns around polluting generic instantiations. @jkotas what are your thoughts on this?

@tannergooding
Copy link
Member

Right, just want to confirm that there isn't any concern around adding a generic API to a non-generic type. IIRC, there was concerns raised around that in the past.

@TylerBrinkley
Copy link
Contributor

TylerBrinkley commented Aug 27, 2019

This looks like a separate proposal.

I've moved those proposed API's back to dotnet/corefx#15453.

I'm especially opposed to returning an array for GetValues as an array cannot be reused and must be allocated on each call. It should instead return an IReadOnlyList<TEnum> as that could then be shared even if the current implementation doesn't do so.

@jkotas
Copy link
Member

jkotas commented Aug 27, 2019

@jkotas what are your thoughts on this?

We do have a number of non-generic methods in the surface. How are we deciding when it is worth creating duplicate generic wrapper to save a bit of typing, e.g. why not have generic versions of Enum.GetName, Enum.IsDefined or Attribute.GetCustomAttributes too?

If Enum.GetValues is different enough from the other APIs to deserve the generic wrapper, I do not have a problem with adding it.

returning an array for GetValues as an array cannot be reused

Does it really matter? Enum.GetValues is not the kind of API that is likely to be on a hot-path, and returning interface instead of array would add a different kind of overhead.

@TylerBrinkley
Copy link
Contributor

Does it really matter? Enum.GetValues is not the kind of API that is likely to be on a hot-path, and returning interface instead of array would add a different kind of overhead.

It's common to use enums to represent options in a dropdown and if GetValues returns an array then each time a page is requested with that dropdown it will add non-insignificant allocations, especially when there are many options.

@tannergooding
Copy link
Member

it will add non-insignificant allocations,

It will be a single allocation (the array) that is relatively small overall (even for the full country list, you are looking at under 2kb for the array, if you assume ~200 countries and 8 bytes per entry).

@tannergooding
Copy link
Member

The cost of the allocation could also be amortized by caching on the UX side or be utilizing an array pool.

@TylerBrinkley
Copy link
Contributor

The cost of the allocation could also be amortized by caching on the UX side or be utilizing an array pool.

That's true. If a server were to do the caching that would reduce the allocation cost.

I'd still prefer it to return an IReadOnlyList<TEnum> but acknowledge returning an array would be acceptable.

@svick
Copy link
Contributor

svick commented Aug 28, 2019

@tannergooding How do you use an array pool here, when the implementation of GetValues creates the array, but only the caller knows when it's no longer necessary?

@TylerBrinkley
Copy link
Contributor

TylerBrinkley commented Aug 28, 2019

FWIW, from my benchmarking in dotnet/coreclr#22161 the current implementation of the non-generic version of this method allocates much more than would be expected, about 1kb for an enum with 36 members.

@GSPP
Copy link

GSPP commented Sep 2, 2019

The lack of convenient enum reflection APIs has been long-standing. At a minimum, there should be a maximally convenient API for all common enum reflection scenarios.

If such a convenient API does not have good performance it might make sense to add a high-performance API in some form.

But callers can already ensure optimal performance by doing caching work themselves.

IMO, the most important use case, that is not currently served, is a convenient API.

I have seen many enum tickets on this issue tracker. I think it would be best if all of them were looked at together and a holistic solution was implemented.

@terrajobst
Copy link
Member

terrajobst commented Sep 24, 2019

Video

I stand corrected, we believe we'd prefer this API shape:

namespace System
{
    public partial abstract class Enum
    {
        public static TEnum[] GetValues<TEnum>() where TEnum : struct, Enum;
        public static string GetName<TEnum>(TEnum value) where TEnum : struct, Enum;
        public static string[] GetNames<TEnum>() where TEnum : struct, Enum;     
        public static bool IsDefined<TEnum>(TEnum value) where TEnum : struct, Enum;       
        public static bool HasFlag<TEnum>(TEnum value, TEnum test) where TEnum : struct, Enum;
    }
}

@TylerBrinkley
Copy link
Contributor

Please consider adding HasAnyFlags and HasAllFlags as opposed to HasFlag which is confusing whether it checks if the value has all of the flags defined in the other parameter or any of the flags in the other parameter.

@terrajobst
Copy link
Member

terrajobst commented Sep 25, 2019

Hmm, not sure it's worth it. HasFlag today means HasAllFlags (runtime implemented). When would you ever want the semantics of HasAnyFlags()?

@TylerBrinkley
Copy link
Contributor

Granted, most users will only be checking one enum flag value and as such the semantics between HasAllFlags and HasAnyFlags are identical.

However, consider a DaysOfWeek enum specified as such.

[Flags]
public enum DaysOfWeek
{
    None = 0,
    Sunday = 1,
    Monday = 2,
    Tuesday = 4,
    Wednesday = 8,
    Thursday = 16,
    Friday = 32,
    Weekdays = Monday | Tuesday | Wednesday | Thursday | Friday,
    Saturday = 64,
    Weekend = Sunday | Saturday
}

If you wanted to determine if a given DaysOfWeek value contains a weekday your code would look like this

(value.HasFlag(DaysOfWeek.Monday) || value.HasFlag(DaysOfWeek.Tuesday) || value.HasFlag(DaysOfWeek.Wednesday) || value.HasFlag(DaysOfWeek.Thursday) || value.HasFlag(DaysOfWeek.Friday))

or this without HasAnyFlags.

(value & DaysOfWeek.WeekDays) != 0

I don't think this example is too far-fetched and can definitely see this being used on something like AttributeTargets, BindingFlags, or a flag enum representing permissions such as Admin, Manager, User, Developer, etc.

@jkotas
Copy link
Member

jkotas commented Sep 25, 2019

(value & DaysOfWeek.WeekDays) != 0

IMHO, this is the best way to check for presence of any flag: There is no ambiguity about what it does, and it is more concise and faster than Enum.HasFlags. Is there really enough usability value in wrapping this in a helper method? If you are working with flags, you have to be familiar with how bit masks work and checking the bits using the built-in C# operators should come pretty natural.

@TylerBrinkley
Copy link
Contributor

You could use that same argument for not adding HasFlag. However, I suppose the most likely reason it was added was for the common case of checking for a single flag as the name implies. My vote, FWIW, is to have both less ambiguously named methods.

Could we consider any other flag enum operations as proposed in dotnet/corefx#34079? The one I consider to have the most value in that proposal is the GetFlags method which returns an IEnumerable<T> of the individual flags that compose a flag enum value which could be very useful but is non-trivial to write even for a specific enum type.

Also, if we ever want to make GetName, IsDefined, or HasFlag extension methods in the future if/when C# supports declaring extension methods in a non-static class such as Enum we would break user code if they use a using static directive due to dotnet/csharplang#665. Consider this code which would break.

using System;
using static System.Enum;

namespace ConsoleApplication1
{
    public static class Program
    {
        public static void Main()
        {
            // IsDefined can not be found if it's promoted to an extension method as using
            // static requires extension methods to be invoked with extension method syntax
            Console.WriteLine(IsDefined(DayOfWeek.Wednesday)); 
        }
    }
}

I think using static directive usage is pretty low and thus would support that breaking change.

@jkotas
Copy link
Member

jkotas commented Sep 25, 2019

To make progress, it may be best to scope this issue to just GetValues, GetName, GetNames and leave the discussion about helper methods for flags operations to dotnet/corefx#34079.

@TylerBrinkley
Copy link
Contributor

Fair enough. My other point about the extension method issue should be discussed however.

@terrajobst
Copy link
Member

Fair enough. My other point about the extension method issue should be discussed however.

C# doesn't allow extension methods inside of non-static classes. This would require a new type (like System.EnumExtensions). Given that, I don't believe this minor syntactic sugar is worth it.

@terrajobst
Copy link
Member

terrajobst commented Oct 10, 2019

To make progress, it may be best to scope this issue to just GetValues, GetName, GetNames and leave the discussion about helper methods for flags operations to dotnet/corefx#34079.

We should include HasFlag for internal consistency. For better or worse, Enum included this API and adding the generic alternative is easier to explain than leaving it off. It also means developers don't have to repeat themselves:

var x = Enum.HasFlag(dayOfWeek, DaysOfWeek.Monday);
var y = (dayOfWeek & DaysOfWeek.Monday) == DaysOfWeek.Monday;

However, I really don't think HasAnyFlag is worth it. For starters, it's much more rare and second the C# syntax is even shorter:

var x = Enum.HasAnyFlag(dayOfWeek, DaysOfWeek.Workdays);
var y = (dayOfWeek & DaysOfWeek.Workdays) != 0;

@jkotas
Copy link
Member

jkotas commented Oct 11, 2019

We should include HasFlag for internal consistency.

@terrajobst The generic HasFlag does not provide internal consistency. The generic HasFlag is static method, but the non-generic method is a instance method that is more convenient to use.

var x = Enum.HasFlag(dayOfWeek, DaysOfWeek.Monday);

You can write this today as more concise var x = dayOfWeek.HasFlag(DaysOfWeek.Monday) and it works just fine. We run complex pattern matching in the JIT to convert it back to the efficient code that you would get from (dayOfWeek & DaysOfWeek.Monday) == DaysOfWeek.Monday;

The syntax sugar provided by the generic API is longer. I do not see who would ever want to use this longer version in ordinary code. The only difference is going to be that the pattern matching in the JIT for this is going to need to be 50% more complex so that the generic API is also converted back to the efficient code.

Enum.HasFlag would only make sense if we had a complete set of generic operations for flags algebra, but that should be better discussed separately.

I really don't think HasAnyFlag is worth it. For starters, it's much more rare and second the C# syntax is even shorter:

Checking for all flags in the set is rare. The much more frequent flag operations are to check for exact one flag to be set or to check for any flag in a set to be set. As you have said the C# syntax for that is shorter. Everybody should better be using that in the first place to save on the busy work required to make the Enum.HasFlag work efficiently.

@terrajobst
Copy link
Member

@terrajobst The generic HasFlag does not provide internal consistency. The generic HasFlag is static method, but the non-generic method is a instance method that is more convenient to use.

I see; HasFlag() is the only instance method in this set. In that case I agree that leaving it off makes more sense, which brings us back to:

namespace System
{
    public partial abstract class Enum
    {
        public static TEnum[] GetValues<TEnum>() where TEnum : struct, Enum;
        public static string GetName<TEnum>(TEnum value) where TEnum : struct, Enum;
        public static string[] GetNames<TEnum>() where TEnum : struct, Enum;     
        public static bool IsDefined<TEnum>(TEnum value) where TEnum : struct, Enum;       
    }
}

@TylerBrinkley
Copy link
Contributor

I'd love to submit the PR for these additions but would like to wait until the repo's are merged before doing so if no one else objects.

@TylerBrinkley
Copy link
Contributor

Also, with nullability now in C# 8, GetName should return string? as null is returned when the value is not defined.

@terrajobst
Copy link
Member

I'd love to submit the PR for these additions but would like to wait until the repo's are merged before doing so if no one else objects.

No objections; this is going to be a 5.0 feature anyway.

@danmoseley
Copy link
Member

@TylerBrinkley still interested?

@danmoseley danmoseley transferred this issue from dotnet/corefx Jan 29, 2020
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added area-System.Reflection untriaged New issue has not been triaged by the area owner labels Jan 29, 2020
@danmoseley danmoseley added the api-approved API was approved in API review, it can be implemented label Jan 29, 2020
@danmoseley
Copy link
Member

re-adding approved label

@danmoseley danmoseley removed the untriaged New issue has not been triaged by the area owner label Jan 29, 2020
@maryamariyan maryamariyan added the untriaged New issue has not been triaged by the area owner label Feb 23, 2020
@maryamariyan maryamariyan removed the untriaged New issue has not been triaged by the area owner label Mar 3, 2020
@steveharter steveharter added this to the 5.0.0 milestone Jul 6, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 18, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Reflection
Projects
None yet
Development

Successfully merging a pull request may close this issue.