dotnet / csharplang

The official repo for the design of the C# programming language
11.05k stars 1.01k forks source link

Champion "Allow `System.Enum` as a constraint" (15.7) #104

Open gafter opened 7 years ago

gafter commented 7 years ago

Allows where T : System.Enum

See also https://github.com/dotnet/roslyn/issues/262

TylerBrinkley commented 7 years ago

I'm of the opinion that Enum was not allowed as a generic constraint initially in C# due to generic code-explosion as the current .NET runtimes don't share generic code for value types like they do for reference types. With the implementation of corefx#15453 this would be less of a problem as it should cover most use cases for an Enum generic type constraint and internally it would try to reduce this generic code-explosion by limiting usage of Enum as a generic type parameter as much as possible. I'm wondering if something could be done in the runtime to allow types and methods with generic enum type arguments to share code when the enum type shares the same underlying type.

Perhaps another reason Enum was not allowed as a generic constraint initially in C# was due to the potential expectation of users that the bitwise operators would be available on those generic types for flag operations. Implementing these bitwise operators were discussed in roslyn#262 and even before that on Roslyn's CodePlex site and the consensus was that there were issues when trying to implement them in IL due to differing underlying types. This expectation would be mitigated by the implementation of the flag enum operations provided in corefx#15453.

I may be mistaken but I believe removing the explicit filters in the compiler preventing Enum as a generic constraint would allow it to just work as expected as any other class type constraint.

I would love to see this implemented as soon as possible to enable the benefits corefx#15453 would provide. I'd gladly contribute in anyway needed.

temporaryfile commented 7 years ago

Generic constraints make sense with enum inheritance. Example from the future:

public enum WindowMessage
{
    WM_NULL = 0,
}

public enum ListViewMessage : WindowMessage
{
    LVM_FIRST = 0x1000,
    LVM_EDITLABELW = (LVM_FIRST + 118),
}

public static IntPtr SendMessage<TMessageType>(
    HWND hwnd,
    TMessageType Msg = WindowMessage.WM_NULL,
    UIntPtr wParam = default(UIntPtr),
    IntPtr lParam = default(IntPtr))
    where TMessageType : WindowMessage
{
    // [....]
}
TylerBrinkley commented 6 years ago

@gafter & @AnthonyDGreen is there anything myself or the community can do to make this a reality? I'd really like to move forward with corefx#15453 but this seems to be the blocker.

I believe the only code changes needed would be a few line removals in roslyn/Binder_Constraints.cs.

gafter commented 6 years ago

@TylerBrinkley I don't see what aspect of the proposed API would benefit from this.

TylerBrinkley commented 6 years ago

@gafter There are two issues with implementing corefx#15453 now without the generic Enum constraint.

  1. Adding the Enum constraint later to enable extension methods would be a breaking change and may not be acceptable. In the proposal I've added the constraint to the existing generic versions of Enum.Parse and Enum.TryParse but that would be a breaking change as noted in this comment. If we can't add the Enum constraint to those two methods it wouldn't be a big deal as all it may mean is an additional type check but for the other methods proposed it would be detrimental as they couldn't be made into extension methods.
  2. Even if adding the Enum constraint later is deemed an acceptable breaking change the FlagEnum methods cannot later be promoted to extension methods due to #665.
TylerBrinkley commented 6 years ago

@gafter or @AnthonyDGreen again I ask, is there anything myself or the community can do to make this a reality?

TylerBrinkley commented 6 years ago

Forgive me but I'm going to continue to spam this request until I get a response.

@gafter or @AnthonyDGreen is there anything myself or the community can do to make this a reality?

jnm2 commented 6 years ago

Ditto.

TylerBrinkley commented 6 years ago

@MadsTorgersen I'm sure you get endless GitHub mentions so I apologize upfront but I was hoping you could review this. We've been trying to get a response from @gafter or @AnthonyDGreen for awhile now on how to proceed with this request but without success.

I'm the author of the OSS library Enums.NET and have created a feature request in the corefx repo to include many of the enhancements my library provides directly to .NET Core. The one thing holding that request back is this missing C# language feature. Is there anything myself or the community can do to push this along? Thank you for your time.

gafter commented 6 years ago

I snuck this into the 7.x milestone to force the LDM to triage this next week.

TylerBrinkley commented 6 years ago

Thank you so much @gafter!

panost commented 6 years ago

@TylerBrinkley How about if you limit the enum scope by specifying the underlying type also ? for example

 where T : enum, int
or
 where T : enum(int)

and T will accept any Enum that has int as underlying type

That will enable to use addition or bit operations and also the code can be shared

That accompanied with ref extensions methods will enable us to write any Enum flag operation possible with few generic methods

public static void SetFlag(ref this T baseVal, T flag, bool value) where T : enum(int)
public static void SetByteFlag(ref this T baseVal, T flag, bool value) where T : enum(byte)

Edit: DOH!! It appears that my suggestion was first proposed by HaloFour two and a half years ago and not only that, it appears that F# already has enum generic constraints with underlying type. I am sorry about that..

tarrowood-imangi commented 6 years ago

Glad to see that after about a decade of complaints and hacked workarounds, there's finally some semblance of movement on this. I've lost count of how many times I've needed this, it's an extreme limitation when trying to do any kind of generic GUI programming. Thanks, @TylerBrinkley, for being the squeaky wheel here.

NVentimiglia commented 6 years ago

When using generics, we have to use DynamicInvoke, boxing, and EnumParse when converting with enums. This is especially true of framework code that aims to be 'user friend;y' and let consumers define their own types. (See MVVM / RX type programming)

This activity causes performance issue, especially on mobile devices.

If I could implicitly cast Enums to/from int without the boxing, dynamic invocation, and conversion I would be happy.

// Replace This
void Convert<TValue>(TValue val)
{
   int b = Convert.ToInt32(val);
}

// With This
void Convert<TValue>(TValue val) where TValue : enum, int
{
     int a = val;
}

// Replace This
void Convert(int val)
{
     var a = (TValue)Enum.Parse(typeof(TValue), val);
}

// With This
void Convert<TValue>(int val) where TValue : enum, int
{
     var a = (TValue)val;
}

Updated examples, my bad.

HaloFour commented 6 years ago

@NVentimiglia

I don't understand what you're asking. Explicitly casting to/from an enum and it's underlying integral type (which is not necessarily int or compatible with int) is a non-op. Adding helper methods into the mix would only add overhead.

NVentimiglia commented 6 years ago

@HaloFour What helper method am I suggesting ?

HaloFour commented 6 years ago

@NVentimiglia

That Convert<T> method you described. Why would that be better than just explicitly casting MyEnum to int? And you don't need Enum.Parse to convert from int to MyEnum. Just cast.

NVentimiglia commented 6 years ago

@HaloFour

HaloFour commented 6 years ago

@NVentimiglia

Expliciting casting an enum to an int (or back) does not involve boxing.

Yes, an enum constraint will help when writing generic helper methods. Note that there are no generic constraints to allow you to constrain the underlying type of the enum (e.g. int) so any generic method is still going to have to deal with the fact that T could be any size and you can't do direct arithmetic operations on it. I don't see why these generic methods required doing allocations today, though.

NVentimiglia commented 6 years ago

@HaloFour

are these box casts ?

void Convert(int value)
{
    MyEnum a = (MyEnum) Enum.ToObject(typeof(MyEnum), value)
}

void Convert<TValue>(TValue value)
{
   // I believe this boxes internally https://msdn.microsoft.com/en-us/library/sf1aw27b(v=vs.110).aspx
    int a = Convert.ToInt32(value);
}

Im a little lost, if this is not an issue, why do libraries such as ENUMS.NET exist ?

HaloFour commented 6 years ago

@NVentimiglia

You could make them boxing casts by simply casting them to object first. The question is why would you? Generic constraints aren't going to help you there.

jnm2 commented 6 years ago

@NVentimiglia Why again aren't you doing MyEnum a = (MyEnum)value and int a = (int)value?

NVentimiglia commented 6 years ago

@jnm2

Can not convert TValue to int; in generic code

Joe4evr commented 6 years ago

My understand that would be a box cast and a heap allocation.

Well, HaloFour told you that that isn't the case, so you can stop believing this now.

HaloFour commented 6 years ago

@NVentimiglia

are these box casts ?

No, converting between an integral type to an enum does not require boxing. If the integral type is the same as the underlying integral type of the enum then it's a simple assignment, otherwise the correct IL would need to be emitted to convert between integral types.

Can not convert TValue to int; in generic code

Indeed you can't, and an enum generic constraint can't help you there because an enum could be any integral type and different IL might be required to perform the conversion. You'd likely need some other flavor of generics added to the CLR to be able to further constrain the underlying generic type, as suggested by @panost.

NVentimiglia commented 6 years ago

@HaloFour

I think I may have expressed my problem incorrectly.

The problem I am facing is specifically in the context of generics, so I am forced to use the static enum methods which use object and box casting internally. If this is incorrect, then I am deeply confused as I am getting mixed messages.

Yes, I think I will need another flavor of generics as suggested by panost

https://github.com/dotnet/corefx/issues/15453#issuecomment-350055728

panost commented 6 years ago

I wonder if actually another flavor of generics is needed at CLR level. The generic method

public static void SetFlag<T>(ref this T baseVal, T flag, bool value) where T : enum(int)

can be emitted to the non-generic method

[Some Attribute to identify it as Enum generic method]
public static void SetFlag(ref int baseVal, int flag, bool value) 

since at CLR level an Enum and it's underlying type are interchangeable. Also that makes the definition of the byte overload with different name unnecessary, since the method

public static void SetFlag<T>(ref this T baseVal, T flag, bool value) where T : enum(byte)

will be emitted as

public static void SetFlag(ref byte baseVal, byte flag, bool value) 

which has already different signature

jrmoreno1 commented 6 years ago

@TylerBrinkley: Why do you belive this:

I'm of the opinion that Enum was not allowed as a generic constraint initially in C# due to generic code-explosion as the current .NET runtimes don't share generic code for value types like they do for reference types.

Not disagreeing as I don't know enough to say, but even if enums aren't optimized by underlying type (say System_CannonEnumInt), the amount of "code bloat" would be limited to the number of enums used -- efffectively trading what seems to me to be a small amount of memory for type safety, that would seem worthwhile to me. Why would it be more worrisome for enums than for the other types, given that it would only happen if they were used?

jnm2 commented 6 years ago

(Canon and cannon have different meanings.)

jrmoreno1 commented 6 years ago

@jnm2 : sorry, typing on a iPad, I really meant System.__CanonEnumInt

TylerBrinkley commented 6 years ago

@jrmoreno1 most types are reference types so using them in generics does not tend to add much code bloat but typically there are only so many value types implemented that you'd consider using generically as they're sealed types and so usually each represent separate concepts. Enums however are a special case in that they're easy to create and make a lot of sense using them generically as they represent one common concept.

From my developing of Enums.NET, one of my stress tests was to retrieve all enums available in the current AppDomain and this is where I was starting to see the code bloat. I don't quite recall the initial numbers but it was using maybe around 50 MB of memory for around 1400 enums. Part of that is due to the large functionality provided by the library and thus more methods but I was able to minimize it somewhat with various techniques but it is still an issue I need to consider when making changes. The saving grace however is as you said, that the code bloat is limited to the number of enums you use but also the number of methods implemented in any generic types.

agocke commented 6 years ago

Proposal to add to this proposal:

New explicit conversions between variables of generic type constrained to System.Enum to all of the legal enum underlying integral types. An exception would be thrown if the enum value could not be represented with the given integral type.

The reason being that the most common use I can find would be the following method:

void FastHasFlag<T>(T t, int flag) where T : System.Enum, struct
    => (((int)t) & flag) != 0;

Without the explicit conversions, you'll be forced to convert through object, producing boxing, and removing the motivation for the method, which is to eliminate bit manipulation of flag enums for checking flag existence.

panost commented 6 years ago

@agocke This generic method, is not actually type-safe. The T maybe an Int64 and the C# can't complain since (int) is an explicit cast

Anyway, you have to use this generic method as

if ( FastHasFlag(fileSysInfo.Attributes, (int)(FileAttributes.Directory|FileAttributes.ReadOnly) ) ....

and it will return true, if the fileSysInfo is either ReadOnly or Directory. Perhaps not what the user expects, but that's not my point here. My point is that the non-generic version of the above will be used as

static bool FastHasFlag(int t, int flag) => (t & flag) != 0;
....
if ( FastHasFlag( (int)fileSysInfo.Attributes, (int)(FileAttributes.Directory|FileAttributes.ReadOnly) ) ....

I see very little benefit from using the generic method...

yaakov-h commented 6 years ago

Perhaps I'm missing something, but why not just:

void FastHasFlag<T>(T t, T flag) where T : System.Enum, struct
    => (t & flag) != default(T);

??

agocke commented 6 years ago

@yaakov-h Same problem. There is no predefined & operator for System.Enum.

jnm2 commented 6 years ago

@agocke The T flag parameter is very appealing. What would it take to consider bitwise operators to exist on generic types constrained to System.Enum, struct and emit bitwise opcodes for T? CLR support?

Joe4evr commented 6 years ago

I don't think it would really require CLR support, just a bit of cleverness in the JIT. All types that are legal to back an enum (at this time) support the bitwise operators, if I'm not mistaken. So as long as we can figure out what the compiler would emit for that kind of code, the JIT will be able to rewrite it into the appropriate op when it compiles the method for each backing type.

agocke commented 6 years ago

I'm not sure what the compiler could emit in this case. We would need to know the enum underlying type to emit an & operation, so we're back to my proposal.

HaloFour commented 6 years ago

Toying around on https://sharplab.io/ it seems that the IL emitted for & between different enum types is the same, regardless of the enum's underlying type. The following C#:

public static A Foo(A a, A flag) {
   return (a & flag);
}

would always produce the following IL (in release mode):

ldarg.0
ldarg.1
and
ret

but the following C#:

public static bool FastHasFlag(A a, A flag) {
    return (a & flag) != default;
}

would produce either:

ldarg.0
ldarg.2
and
ldc.i4.0
cgt.un
ret

or, if the underlying type is 64-bit:

ldarg.0
ldarg.2
and
ldc.i4.0
conv.i8
cgt.un
ret

I'd think that there would be alternate IL that the compiler could emit in this case which could enable the method bodies to be identical regardless of the underlying type of the enum. Is the conv.i8 even really necessary in this case?

agocke commented 6 years ago

@HaloFour Perhaps I'm reading the spec wrong, but I think the operative section is this:

For all other purposes, including verification and execution of code, an unboxed enum freely interconverts with its underlying type

Note that that doesn't say that type parameters constrained to System.Enum freely interconvert with their underlying type. Thus, the and instruction doesn't appear like valid IL, to me.

HaloFour commented 6 years ago

@agocke

I won't be able to play around with ilasm until later tonight. It's quite possible that the verifier will balk at the and instruction applied to T that is constrained to System.Enum and struct. But if it did I could see that as something that could be relaxed in that specific case.

agocke commented 6 years ago

I should also add:

There is basically a 0% probability of the CLR taking any changes for this feature. It's in Roslyn or nothing.

HaloFour commented 6 years ago

@agocke

You're right. It compiles (on full .NET Framework 4.7) and it actually works, but it doesn't verify:

.method public hidebysig static bool  FastHasFlag<valuetype .ctor ([mscorlib]System.ValueType, [mscorlib]System.Enum) E>(!!E 'value', !!E flag) cil managed
{
    .maxstack  2
    ldarg.0
    ldarg.1
    and
    ret
}
[found (unboxed) 'E'] Expected I, I4, or I8 on the stack.

Shame.

panost commented 6 years ago

Sorry for my ignorance, I am sure if I search here and at Roslyn forums, I probably find answer for the following: Since F# already has this (generic constraints on enum with a given underlying type) then

  1. How exactly F# has implemented this ? What is the IL + attributes of a generic method with enum constraint ?
  2. How exactly C# compiler interprets such a method (referencing an assembly build with F#) ?
  3. Why Roslyn team didn't just copy the implementation from F# and close this issue 2 years ago ?
HaloFour commented 6 years ago

@panost

F# has their own method of maintaining type metadata which is totally opaque to the CLR or any other language. They don't actually apply proper CLR generic type constraints to many of their generic constraints, they rely on their own compiler to enforce it at compile time.

TylerBrinkley commented 6 years ago

The only way I could see allowing the bitwise operators to be used in generic enum code without a CLR change is after corefx#15453 is implemented, assuming it is, for Roslyn to transform these bitwise operators to use the newly introduced equivalent flag manipulation API's.

Then for better performance the Jitter could be updated to replace calls to these methods with the IL the bitwise operators would output in non-generic code.

The big caveat here though is that it would only be supported where the new API's are available which is most likely unacceptable as a language feature.

svick commented 6 years ago

@HaloFour Maybe those verification rules could be changed? If, as you say, that IL actually works, then making this work doesn't actually require CLR changes.

HaloFour commented 6 years ago

@svick

Maybe those verification rules could be changed? If, as you say, that IL actually works, then making this work doesn't actually require CLR changes.

That would still require work from the CLR team to change verification, no? I take it from @agocke 's comments that if this doesn't work "out of the box" that they aren't going to make that change, at least not yet. There's also the concern that while this works on the full framework CLR that another implementation might fail since it's not supported by the spec.

panost commented 6 years ago

@HaloFour Ok thanks

That's very close with what we want in C# also. Given that the generic C# declaration

    public static void SetFlag<T>(ref this T baseVal, T flag, bool value) where T : enum(int) {
        if ( value ) {
           baseVal |= flag;
        } else {
           baseVal &= ~flag;
        }
    }

can be actually be emitted as

    [Extension]
    [EnumGenericMethod("T")]
    public static void SetFlag([EnumGeneric("T")]ref int baseVal, [EnumGeneric("T")]int flag, bool value) {
        if ( value ) {
           baseVal |= flag;
        } else {
           baseVal &= ~flag;
        }
    }

and that doesn't require any CLR change. It's just a Roslyn change, to treat the above non-generic method decorated with some special attributes, as an enum generic method

HaloFour commented 6 years ago

@panost

That would be a pretty radical departure from how generics work in C# today. I personally don't like it as it is a very easy mechanism to defeat and doesn't work at all across languages.