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

Enum Improvements #20008

Closed
TylerBrinkley opened this issue Jan 25, 2017 · 67 comments
Closed

Enum Improvements #20008

TylerBrinkley opened this issue Jan 25, 2017 · 67 comments
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation area-System.Runtime design-discussion Ongoing discussion about design without consensus
Milestone

Comments

@TylerBrinkley
Copy link
Contributor

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 #18063.

Proposed API

namespace System {
    public abstract class Enum : ValueType, IComparable, IConvertible, IFormattable {
        public static TEnum Parse<TEnum>(ReadOnlySpan<char> value) where TEnum : struct, Enum;
        public static TEnum Parse<TEnum>(ReadOnlySpan<char> value, bool ignoreCase) where TEnum : struct, Enum;
        public static object Parse(Type enumType, ReadOnlySpan<char> value);
        public static object Parse(Type enumType, ReadOnlySpan<char> value, bool ignoreCase);
        public static bool TryParse<TEnum>(ReadOnlySpan<char> value, out TEnum result) where TEnum : struct, Enum;
        public static bool TryParse<TEnum>(ReadOnlySpan<char> value, bool ignoreCase, out TEnum result) where TEnum : struct, Enum;
        public static bool TryParse(Type enumType, ReadOnlySpan<char> value, out object result);
        public static bool TryParse(Type enumType, ReadOnlySpan<char> value, bool ignoreCase, out object result);

        // Generic versions of existing methods
        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;
        public static int CompareTo<TEnum>(TEnum value, TEnum other) where TEnum : struct, Enum;
        public static bool Equals<TEnum>(TEnum value, TEnum other) where TEnum : struct, Enum;
        public static byte ToByte<TEnum>(TEnum value) where TEnum : struct, Enum;
        public static short ToInt16<TEnum>(TEnum value) where TEnum : struct, Enum;
        public static int ToInt32<TEnum>(TEnum value) where TEnum : struct, Enum;
        public static long ToInt64<TEnum>(TEnum value) where TEnum : struct, Enum;
        public static sbyte ToSByte<TEnum>(TEnum value) where TEnum : struct, Enum;
        public static string ToString<TEnum>(TEnum value) where TEnum : struct, Enum;
        public static string ToString<TEnum>(TEnum value, string format) where TEnum : struct, Enum;
        public static ushort ToUInt16<TEnum>(TEnum value) where TEnum : struct, Enum;
        public static uint ToUInt32<TEnum>(TEnum value) where TEnum : struct, Enum;
        public static ulong ToUInt64<TEnum>(TEnum 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?

Implementation Details

This proposal stems from my work on the open source library Enums.NET which addresses each of these issues and will be the basis for an implementation. For efficiency this is how Enums.NET is implemented.

First, using Enum as a generic type argument causes generic code-explosion in today's runtimes so it's essential that it's included sparingly. For this reason most logic is contained in an EnumCache<TInt, TIntProvider> object where TInt is the underlying type and TIntProvider is the underlying type's operations provider which implements the interface INumericProvider<TInt> which provides the bitwise operations and other needed operations. Including the TIntProvider as a type parameter allows the runtime to inline calls to its methods so there's no interface method dispatch, a technique described in this generic calculations article.

Now how can one access this EnumCache object when the caller only has the enum type as a generic type argument? This is solved by the bridge-like object EnumInfo<TEnum, TInt, TIntProvider> which implements the interfaces IEnumInfo<TEnum> for generic methods and IEnumInfo for non-generic methods. It simply acts as a bridge to delegate calls from the interfaces to EnumCache<TInt, TIntProvider>. In order to do that it needs to be able to efficiently convert values between TEnum and TInt. This is performed by the conversion methods ToInt and ToEnum which are declared as extern methods for efficiency so that they're simply defined as casts from one type to the other. These method's functionality can be achieved with the Unsafe.As method. This EnumInfo<TEnum, TInt, TIntProvider> is created via reflection upon it's first call and is stored in the static field Enums<TEnum>.Info as an IEnumInfo<TEnum> which is then called by Enums's generic methods.

For non-generic methods the RuntimeType.GenericCache property will be set to the IEnumInfo to use.

Updates

  • Added more implementation details.
  • Added more API details.
  • Added enumerating members usage example.
  • Added generic versions of instance methods for performance reasons.
  • Removed the System.Enums namespace and moved its members into the System namespace.
  • Moved PrimaryAttribute into the System.ComponentModel namespace and switched to using the existing System.ComponentModel.AttributeCollection.
  • Removed IsValid method as it's meaning is rather subjective and promoted IsValidFlagCombination as an extension method.
  • Renamed GetAllFlags to AllFlags as it more closely resembles a property but can't be because it is generic. Similar naming to Array.Empty.
  • Removed EnumComparer as one can just use Comparer<T>.Default and EqualityComparer<T>.Default instead.
  • Split the proposed API to a base API addition and an EnumMember support API addition.
  • Added ReadOnlySpan<char> parse overloads.
  • Moved FlagEnum.IsFlagEnum into System.Enum.
  • No longer proposes adding the Enum constraint to existing methods.
  • Split EnumMember API addition into the separate issue dotnet/corefx#34077.
  • Split generic API additions that don't necessitate the performance improvements of this implementation to separate issue API Proposal: Add a Generic version of GetValues to Enum (probably GetName/GetNames) #2364.
  • Split Flag API additions to separate issue dotnet/corefx#34079.
  • Split PrimaryAttribute addition to separate issue dotnet/corefx#34080.
  • Moved Format to API Proposal: Add a Generic version of GetValues to Enum (probably GetName/GetNames) #2364 as well.
  • Moved proposed methods from API Proposal: Add a Generic version of GetValues to Enum (probably GetName/GetNames) #2364 back to this issue.
  • Removed Format as it's no different from ToString(string format) except it throws an ArgumentNullException for a null format parameter.
@TylerBrinkley
Copy link
Contributor Author

Updated the proposal to include some more implementation details.

@TylerBrinkley
Copy link
Contributor Author

@danmosemsft I've updated the proposal to include more details about the new API defined.

@svick
Copy link
Contributor

svick commented Jan 28, 2017

The second one is that this proposal specifies extension methods within System.Enum and as such requires C# to allow extension methods within non-static classes, a proposal of which needs to be worked up.

There already is a proposal for that too: dotnet/roslyn#16271.

@TylerBrinkley
Copy link
Contributor Author

@svick Thanks, I've updated the proposal accordingly.

@TylerBrinkley
Copy link
Contributor Author

I've updated the proposal to include generic versions of instance methods for performance reasons.

@TylerBrinkley
Copy link
Contributor Author

@danmosemsft are you the CoreFX owner of this proposal. If so, could you let me know what needs to be done to get this API reviewed?

@danmoseley
Copy link
Member

@joperezr @AlexGhiondea are the owners of this area so one of them will either give feedback on it or flip to "api-ready-for-review".

@TylerBrinkley
Copy link
Contributor Author

Thanks @danmosemsft.

@TylerBrinkley
Copy link
Contributor Author

TylerBrinkley commented Feb 1, 2017

I've updated the proposal by removing the System.Enums namespace and moved its members into the System namespace. Does anyone think ReadOnlyAttributeCollection and PrimaryAttribute should be included in another namespace, maybe System.ComponentModel?

@TylerBrinkley
Copy link
Contributor Author

I see that there is already an AttributeCollection in the System.ComponentModel namespace which would suit my needs with some additions but it's included in the System.ComponentModel.TypeConverter assembly. Are binding redirects possible in .NET Core to enable moving this AttributeCollection class into mscorlib?

@danmoseley
Copy link
Member

If it's an interesting type that we could use as is (or modify without breaking existing users) then it's possible for us to move it down so this code could depend on i.

@TylerBrinkley
Copy link
Contributor Author

Excellent, I'll update my proposal accordingly.

@TylerBrinkley
Copy link
Contributor Author

@joperezr @AlexGhiondea I believe I'm done with my proposal updates. When you have some time could you please look over this proposal?

@magol
Copy link

magol commented Feb 1, 2017

I like the proposal a lot.
One api that I'm missing is a method that remove not valid flags.

public static TEnum RemoveNotValidFlags(this TEnum value) where TEnum : struct, Enum;

See #18190

@TylerBrinkley
Copy link
Contributor Author

TylerBrinkley commented Feb 1, 2017

For that case I'd suggest doing this.

myFlagEnumValue = myFlagEnumValue.CommonFlags(FlagEnum.AllFlags<MyFlagEnum>());

I know it's less convenient but it seems too much of a specialized case to warrant a new extension method.

@TylerBrinkley
Copy link
Contributor Author

Maybe as just a static method we could have the following.

public static TEnum GetValidFlags<TEnum>(TEnum value) where TEnum : struct, Enum;

and in use would be

myFlagEnumValue = FlagEnum.GetValidFlags(myFlagEnumValue);

Thoughts, @magol?

@magol
Copy link

magol commented Feb 1, 2017

You right, maybe it is not a good method for a General api arter all. ☺️

@AlexGhiondea
Copy link
Contributor

@TylerBrinkley thanks for putting this together. It is a very interesting proposal that I am trying to better understand.

I have a few questions

  • This requires 2 language features to enable if I read it correctly. Does that mean it won't be possible to even implement this until those features are shipped.
  • I am trying to understand how I would use the GetMember methods -- that looks like an object model on top of an enum value -- and I understanding it wrong?
  • Why are there both generic and non-generic versions of some APIs?
  • What are your thoughts on implementing this? Have you looked at the implementation in CoreCLR for the Enum type? I believe quite a few tricks and calls into runtime and JitHelpers are used to make the enum work and have reasonable performance.

/cc @jkotas

@TylerBrinkley
Copy link
Contributor Author

@AlexGhiondea thanks for looking at the proposal.

  • This proposal makes use of 2 language features that haven't been implemented yet but they are not required in order to implement this proposal. Essentially, the proposed extension methods would be implemented as static methods initially and so users would need to use static method invocation syntax to use them. When these 2 language features are shipped those static methods would then be updated to become extension methods.
  • No you're absolutely right. EnumMember is an object model on top of an enum member's name, value, and attributes and enables retrieving them at the same time such as in the second use case example above using GetMembers. It is not an object model on top of any enum value though, only those values that are defined hence the inclusion of the name and attributes.
  • Non-generic API's are still very useful for implementing things like serializers and code generators. The non-generic TryParse was added for similar reasons in Add a non-generic version of Enum.TryParse #14083.
  • The implementation would be based on Enums.NET's implementation which has great performance as can be seen here and would not require all those tricks and calls into the runtime and JitHelpers. A brief explanation of its implementation details can be seen in the Implementation Details section on the proposal. The only part I'm not sure how to implement would be the ToInt and ToEnum extern methods which are simply defined as casts from one type to the other. In Enums.NET I use Fody to implement these methods directly using IL. How does CoreCLR implement extern methods?

@jkotas
Copy link
Member

jkotas commented Feb 2, 2017

Enums.NET's implementation which has great performance

It may have good enough performance for some uses, but it does not have what I would consider great performance. Great performance would mean parity when you do the same with bitmask operations.

@jkotas
Copy link
Member

jkotas commented Feb 2, 2017

How does CoreCLR implement extern methods

The JitHelpers are other tricks that @AlexGhiondea is talking about... .

@svick
Copy link
Contributor

svick commented Feb 2, 2017

@TylerBrinkley

This proposal makes use of 2 language features that haven't been implemented yet but they are not required in order to implement this proposal. Essentially, the proposed extension methods would be implemented as static methods initially and so users would need to use static method invocation syntax to use them. When these 2 language features are shipped those static methods would then be updated to become extension methods.

That's true for the "extension methods in non-static classes" feature, but not for "enum as generic constraint". I think that adding that constraint later (the proposal even adds it to some existing methods) would be a breaking change. And I think the break is not just theoretical, but that it would break real code that has a generic method that calls one of the existing generic methods, something like:

T MyMethod<T>() where T : struct => Enum.Parse<T>(GetEnumNameSomehow());

I can see two ways out of this:

  1. Don't add the constraint now and keep it that way forever.

  2. Add the constraint to the new methods now (without C# support, that would probably require implementation in IL or some kind of post-compile IL rewriting). Keep existing methods without the constraint.

    This assumes that consuming the constraint from C#, VB and F# already works fine (which I have not checked).

Neither option sounds great, but 1 is probably more practical, assuming it does not make sense to wait until enum constraints arrive in C# (which may never actually happen).

@TylerBrinkley
Copy link
Contributor Author

@jkotas

Great performance would mean parity when you do the same with bitmask operations.

From my performance testing of Enums.NET's HasAllFlags method's performance comes very close to doing the same with bitmask operations, much more compared to the already implemented HasFlag method. I just ran the performance on the .NET Framework on my computer with this testing method and HasFlag took 959 ms, HasAllFlags took 139 ms, and manually using bitmask operations took 83 ms. I think the only performance overhead in using HasAllFlags over manually using bitmask operations is due to the interface dispatch call internally to IEnumInfo<TEnum>.

I've added a little more details to the implementation details of the proposal regarding the use of the generic type parameter TIntProvider and how it's operations including the bitwise operations are being inlined.

@jkotas
Copy link
Member

jkotas commented Feb 2, 2017

Runtime throughput is not the only performance metric we are concerned about. The startup and code bloat is another one - using generic helper methods for bitmasks checks is many times (100x?) worse on this performance metric.

@TylerBrinkley
Copy link
Contributor Author

TylerBrinkley commented Feb 2, 2017

@svick

That's a great point. You're right, it would be a breaking change if another generic method with a T : struct constraint currently calls Enum.Parse or Enum.TryParse. If the constraint on Enum.Parse and Enum.TryParse aren't changed it'll just mean there'll be a runtime type check which wouldn't be a big deal as parsing should take orders of magnitude more time to evaluate. While I think breaking changes should be avoided at all costs I think this might be a case for an exception as its impact should be very limited given the existing T : struct constraint. My guess would be most users who may be affected by this want their generic parameter constrained to T : struct, Enum anyway which will be available to them at the same time, at least in the C# world.

In regards to the implementation of these new methods now, option 1 is not really an option as that would mean no extension methods and would mean runtime type checks which would greatly decrease performance for the bitmask operation methods which should be near native in performance. If the breaking change is not acceptable I'm in favor of option 2 if possible or else wait for C#'s Enum type constraint.

This assumes that consuming the constraint from C#, VB and F# already works fine (which I have not checked).

Consuming the Enum constraint from C#, VB, and F# does already work.

@magol
Copy link

magol commented Feb 3, 2017

Is it possible to do any optimization by roslyn som that the calls to this method is replace with native bit operations at compile time?

@TylerBrinkley
Copy link
Contributor Author

@magol that should be possible similar to what's proposed for ToString

@TylerBrinkley
Copy link
Contributor Author

TylerBrinkley commented Feb 28, 2017

As a proof of concept I've started migrating my code into mscorlib at TylerBrinkley/coreclr.

@AlexGhiondea
Copy link
Contributor

@TylerBrinkley thanks for the update! I would be interested to learn some numbers once you have your proof of concept operational.

@TylerBrinkley
Copy link
Contributor Author

TylerBrinkley commented Mar 8, 2017

@AlexGhiondea Currently, Enum.ToObject will accept overflow values. Am I correct in assuming that that behavior must be preserved? In Enums.NET I throw an OverflowException in those cases. It seems more like a bug that it allows overflowing values.

@TylerBrinkley
Copy link
Contributor Author

@terrajobst and @danmosemsft would it be better for me to break this proposal up into smaller chunks so that it could be api reviewed?

@TylerBrinkley
Copy link
Contributor Author

I've moved the EnumMember API addition to dotnet/corefx#34077.

I've moved the generic API additions that don't necessitate the performance improvements of this implementation to #2364.

I've moved the flag API additions to dotnet/corefx#34079.

I've moved the PrimaryAttribute addition to dotnet/corefx#34080.

This ticket will remain for a few performance API's and as the basis for the improved performance implementation.

@TylerBrinkley TylerBrinkley changed the title Enum Improvements Enum Performance Improvements Dec 13, 2018
@TylerBrinkley
Copy link
Contributor Author

I've implemented the generic implementation proposed here without any API changes in coreclr#22161.

@ericwj
Copy link

ericwj commented Apr 4, 2019

Would it be doable to have TryParse overloads that take an argument that makes it return false if the string or span is integral?

I can think of two variants, 1) one where any integral value would be invalid, because it is meaningless. 2) The other is to shoot down invalid values, such as TryParse<ConsoleColor>("16", ...)

Ad 1) in my opinion it is fairly common to define some valid input that should be constrained to known strings as an enum. All that is probably needed is to check whether the first character (if any) is a letter or underscore. Especially for technical apps it is very common to have invariant strings that need to be parsed and to define those as enums. E.g. suppose a console application that takes a PlatformID as an argument should be able to use one of these overloads to shoot down 2 although it would be Win32NT.

For variant 2, having to parse first and then check with something like IsDefined is in fact for the most part double work.

@TylerBrinkley
Copy link
Contributor Author

Would it be doable to have TryParse overloads that take an argument that makes it return false if the string or span is integral?

That would be outside the scope of this issue so perhaps you may want to open a new one?

You can achieve this today with Enums.NET by specifying only EnumFormat.Name.

@ericwj
Copy link

ericwj commented Apr 4, 2019

Kudos for this for starters - I just bumped into having to do ReadOnlySpan<char>.ToString() to get an enum parsed ;)

@TylerBrinkley TylerBrinkley changed the title Enum Performance Improvements Enum Improvements Aug 27, 2019
@huoyaoyuan
Copy link
Member

What's about implementing IEquatable<T> and IComparable<T> on enums? It looks not so simple, because enums do have metadata, unlike arrays being pure runtime-magic.
Or skip it for 1 year and jump to type classes, which is planned for C# 10 and I assume to come with .NET 6?

@mikernet
Copy link
Contributor

mikernet commented Nov 22, 2019

@TylerBrinkley I'm not sure which approach you've taken to implement this but I just posted this comment which details a way to implement these operations with full bitwise operator speed. I thought I would mention it here as it turns out this appears to be the main discussion area for these.

When the methods are written like my example, they get inlined directly to bitwise operations and the branches are eliminated by the JIT so it produces very efficient code.

@LegoCylon
Copy link

I'm not sure if you're interested in additional related features but I'd love to see support added to be able to enumerate values or names in the order they are defined (likely with an optional parameter on the GetNames and GetValues methods) since the current methods only enumerate in ascending value.

@koszeggy
Copy link

Ha! It's funny to see that almost everyone implemented their well-performing enum library when it used to be so slow in the .NET Framework era. So did I, of course. :)

I'm really happy to see the improvements made in .NET Core. I would've never created my alternative version if System.Enum was always that fast. In fact, I needed to do a massive refactoring in my libraries, just to justify its today's existence and make it faster again than the Core version. :) But to be frank, maybe its last really useful feature that it supports ReadOnlySpan<char>, which is still not implemented in the System version.

Anyhow, if it helps anything, feel free to re-use any solutions from it. Maybe it hasn't as many features as Tyler's version and my approach was also much simpler in some aspects, but who knows... maybe you can find some useful parts in it.

@joperezr joperezr removed the untriaged New issue has not been triaged by the area owner label Jul 6, 2020
@joperezr joperezr modified the milestones: 5.0.0, Future Jul 6, 2020
@xaviergxf
Copy link

How about this api? Humanizr could use this...

public static ReadOnlySpan<char> GetName<TEnum>(this TEnum value) where TEnum : struct, Enum;

@huoyaoyuan
Copy link
Member

public static ReadOnlySpan<char> GetName<TEnum>(this TEnum value) where TEnum : struct, Enum;

Returning a span is never an option because of lifetime issue. Any api with result in span would require caller to allocate the span.

@tannergooding
Copy link
Member

As with #14084 this isn't a direction we'd like to move forward on.

The idiomatic bit operations are guaranteed to be efficient where-as these named methods aren't and potentially push users towards a pit of failure.

@tannergooding tannergooding closed this as not planned Won't fix, can't repro, duplicate, stale Sep 9, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Oct 10, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation area-System.Runtime design-discussion Ongoing discussion about design without consensus
Projects
None yet
Development

No branches or pull requests