dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.47k stars 4.76k forks source link

Enhancement: Consider the Enum.GetName() for Bitwise Flags Attribute #44244

Open mikependon opened 4 years ago

mikependon commented 4 years ago

Description

When we are dealing with the enumerations in a dynamic way, specifically if we are pre-compiling the operations of parsing and extraction of the enum values, the current API (GetName() method) seems to not fit enough in relation to Enum Bitwise (Flags) Attribute.

The rational behind, I am a maintainer of an ORM that is OSS and have to adjust the compiler to instead use the System.Convert.ToString(enumValue) over the Enum.GetName(enumValue) just to retrieve the proper name equivalent of bit-wised Enum.

Note: It seems to be in purpose not to get this supported? Therefore, I am confused whether this is a proposal or a bug, but please consider this issue whether it is a bug and/or API proposal. Definitely, it is an enhancement though.

Code Samples

Let us say you have this enumeration.

[Flags]
public enum StorageType
{
    File = 1,
    Folder = 2,
    Directory = 4,
    IsolatedStorage = 8,
    Registry = 16,
    Database = 32
}

GetName Single Value - WORKING:

static void TestGetNameSingleValue()
{
    var type = StorageType.Registry;
    var name = Enum.GetName(typeof(StorageType), type); // Returns 'Registry' correctly
    Console.WriteLine($"GetName (Single Value): {name}");
}

GetName Multiple Values - NOT WORKING:

static void TestGetNameMultipleValues()
{
    var type = StorageType.File | StorageType.Folder | StorageType.IsolatedStorage;
    var name = Enum.GetName(typeof(StorageType), type); // Returns 'NULL' wrongly, I expect it to return 'File | Folder | IsolatedStorage'
    Console.WriteLine($"GetName (Multiple Values): {name}");
}

Parse Method

As a partner to the GetName() method, this otherwise seems to be working as expected

Parse Single Value - WORKING

static void TestParseSingleValue()
{
    var name = StorageType.Registry.ToString();
    var value = Enum.Parse(typeof(StorageType), name); // Returns 'Registry' correctly
    Console.WriteLine($"Parse (Single Value): {value}");
}

Parse Multiple Values - WORKING

static void TestParseMultipleValues()
{
    var name = (StorageType.File | StorageType.Folder | StorageType.IsolatedStorage).ToString(); // I used the ToString() here to get the value of 'File, Folder, IsolatedStorage'
    var value = Enum.Parse(typeof(StorageType), name); // Returns 'File | Folder | IsolatedStorage' correctly
    Console.WriteLine($"Parse (Multiple Values): {value}");
}

Configuration

Regression?

No, it is also not working on the previous version of .NET (both Framework and Core).

Dotnet-GitSync-Bot commented 4 years ago

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

jkotas commented 4 years ago

It seems to be in purpose not to get this supported?

Yes, it is intentional that Enum.GetName handles single values only. Changing it would be a breaking change.

retrieve the proper name equivalent of bit-wised Enum

The existing Enum.ToString method does that. Why it does not work for you?

mikependon commented 4 years ago

Why does a support to this a breaking changes? It would only be an additional case on top of the existing single value support, in which it should not break any existing functionality and the code written by the .NET community (atleast IMHO).

ICYDK, the ToString() is an extension method to the object and that cannot be pre-compiled. Or, correct me here if I missed the process. Therefore, the alternative is to use the System.Convert.ToString() method instead. Though my solution is now working through this, TBH.

What puzzles me a little is the fact that Parse() can support it, and why not the GetName()? Though they are really a different API, but they are residing on the Enum Class in which IMO must have the same capability in parsing and extraction.

jkotas commented 4 years ago

Why does a support to this a breaking changes?

Somebody can have code like this:

string? simpleName = Enum.GetName(typeof(T), value);
if (simpleName != null)
   return simpleName;

... custom formatting logic for flags enums with multiple bits set...

If Enum.GetName started handling flags enums with multiple bits set, this code would break.

In general, changing the value that a method returns for given input is a treated as breaking change.

jkotas commented 4 years ago

Parse() can support it, and why not the GetName()

Parse() is meant to be paired with ToString()

mikependon commented 4 years ago

If Enum.GetName started handling flags enums with multiple bits set, this code would break.

In your sample, you are correct that the code for "custom formatting logic for flag enums with multiple bits" will not execute as the variable simpleName will not be null. But was not it the intention of the "method" is to return the value of the Enum Name of the multiple bit sets? Unless otherwise it is not a method that returns the correct Enum Name, but based on your sample, it is. So in this case, it is not breaking the behavior/intention of that method.

Parse() is meant to be paired with ToString()

Nah, was not it generalized for all string instead? I can do the code like this Enum.Parse(typeof(StorageType), "Folder, File, IsolatedStorage"). Philosophically, we should not just assume that it is always ToString() method we are going to use on this combination, though on my case I enforced to use the System.Convert.ToString() due to the fact that I can't pre-compile the ToString() method.


On the other hand, by focusing on the GetName() method and eliminating the Parse() method in the topic, it is a big question in the first place why do return null value in the GetName() method for the multiple bits.

In addition to this, will we really not improve the API if the fix or improvement has a breaking changes? We can always communicate that to the community through documentation right?

jkotas commented 4 years ago

will we really not improve the API if the fix or improvement has a breaking changes?

We care about compatibility a lot. We evaluate each breaking change carefully and proceed with it only if we have very strong justification for it. I do not see the strong justification here.

We can always communicate that to the community through documentation right?

It is not as simple as this. The breaking changes generate work. Somebody on the other side has to find places where the API is used, understand the code, and fix it to move to the new version. It is a lot of work over billions lines of .NET code out there.

use the System.Convert.ToString() due to the fact that I can't pre-compile the ToString() method.

This is limitation of crossgen tool. We would rather work on fixing this limitation instead of introducing redundant APIs to work around it.

mikependon commented 4 years ago

I will not gonna argue about the compatibility in which dotnet is popular to it.

Strong justification? TBH, there is an alternative solution to the problem that I brought here, and that alternative is not complex to implement in both native and compiler code. But, again, it is an apparent problem that null is being returned by the GetName() method for multiple-bits (flag-based) Enum type. We have a value there, but why return null? And beside, I also state that it is not affecting the use-case you brought on this thread, though I really agree with you about the billion lines-of-code written somewhere that might be related to this.

But having this implemented, imagine how many billion lines-of-code you are eliminating - making 3 lines of code to 1 liner - maybe/perhaps.

I hope you guys will evaluate this proposal again and hopeful a consideration in relation to this.