Proposal: Enum.IsDefined()

[jamesqo]

I am aware of the issues https://github.com/dotnet/corefx/issues/10692 and https://github.com/dotnet/corefx/issues/15453. However, https://github.com/dotnet/corefx/issues/10692 isn't a formal proposal, and https://github.com/dotnet/corefx/issues/15453 will take forever to get through because it has so much other stuff. I think this API is valuable enough that I am opening a new issue to expedite things.

Proposal

namespace System {
    public abstract class Enum : ValueType, IComparable, IConvertible, IFormattable {
        public bool IsDefined();
    }
}

Performance

The new API doesn't need to have great performance. (Yes, it will still box.) 99% of the time I use this in Debug.Assert, so it has no effect on release perf. Also, as @benaadams mentions in the first issue, this is faster than the existing IsDefined method because

Enum.IsDefined(typeof(MyEnum), argument) is also quite slow as it needs to do lots of conversion checks etc; whereas if you already have the enum these don't need to be done.

It's faster and terser than the existing method and it's used very often, so it seems reasonable to add this, no?

edit: If for whatever reason the boxing becomes a performance problem, we can consider making the method an intrinsic to avoid it (just like https://github.com/dotnet/coreclr/pull/7895 avoided boxing for Enum.GetHashCode).

[jamesqo]

/cc @benaadams, @joperezr, @AlexGhiondea, @terrajobst

This is just one method and will be quick to review, so maybe this can be labeled ready-for-review?

[joperezr]

@jamesqo do you know roughly how much faster would this be compared to the static method?

[jamesqo]

@benaadams You were the one who originally made that comment, would you know?

[stephentoub]

I don't understand the rationale stated for adding this. On the one hand, the proposal states "The new API doesn't need to have great performance." On the other hand, it states "It's faster and terser than the existing method." So is the motivation here really just the terseness, comparing Enum.IsDefined(typeof(Color), value) against value.IsDefined()? Or if performance is a motivating factor, why would we add a method that will box?

[jnm2]

+1 for a non-boxing instance method.

[TylerBrinkley]

I'd really hate for this to get added when dotnet/runtime#20008 only needs C# to allow the enum generic constraint in csharplang#104. dotnet/runtime#20008 doesn't box, uses the same terse syntax but as an extension method, and will perform much better. In Enums.NET, which dotnet/runtime#20008 is based on, the IsDefined extension method runs about 5400% faster than Enum.IsDefined. Instead I think we should put more effort into getting csharplang#104 done.

[benaadams]

@jamesqo as far as I can remember the current method takes a meandering path checking if both types are Enums and the same type of enum etc

https://github.com/dotnet/coreclr/blob/master/src/mscorlib/src/System/RtType.cs#L3597-L3642

public override bool IsEnumDefined(object value)
{
    if (value == null)
        throw new ArgumentNullException(nameof(value));
    Contract.EndContractBlock();

    // Check if both of them are of the same type
    RuntimeType valueType = (RuntimeType)value.GetType();

    // If the value is an Enum then we need to extract the underlying value from it
    if (valueType.IsEnum)
    {
        if (!valueType.IsEquivalentTo(this))
            throw new ArgumentException(SR.Format(SR.Arg_EnumAndObjectMustBeSameType, valueType.ToString(), this.ToString()));

        valueType = (RuntimeType)valueType.GetEnumUnderlyingType();
    }

    // If a string is passed in
    if (valueType == RuntimeType.StringType)
    {
        // Get all of the Fields, calling GetHashEntry directly to avoid copying
        string[] names = Enum.InternalGetNames(this);
        if (Array.IndexOf(names, value) >= 0)
            return true;
        else
            return false;
    }

    // If an enum or integer value is passed in
    if (Type.IsIntegerType(valueType))
    {
        RuntimeType underlyingType = Enum.InternalGetUnderlyingType(this);
        if (underlyingType != valueType)
            throw new ArgumentException(SR.Format(SR.Arg_EnumUnderlyingTypeAndObjectMustBeSameType, valueType.ToString(), underlyingType.ToString()));

        ulong[] ulValues = Enum.InternalGetValues(this);
        ulong ulValue = Enum.ToUInt64(value);

        return (Array.BinarySearch(ulValues, ulValue) >= 0);
    }
    else
    {
        throw new InvalidOperationException(SR.InvalidOperation_UnknownEnumType);
    }
}

Whereas a instance method would be a compile time assurance and cut down the lookups on type. However it would still box.

[jamesqo]

@stephentoub

So is the motivation here really just the terseness, comparing Enum.IsDefined(typeof(Color), value) against value.IsDefined()?

Yes, that's the reason why I opened this issue. I offered performance as a secondary reason.

Or if performance is a motivating factor, why would we add a method that will box?

@stephentoub @jnm2 @TylerBrinkley, I put this in the description but didn't emphasize it enough there: the new method doesn't have to box. https://github.com/dotnet/coreclr/pull/7895 made some changes to the JIT to avoid boxing for Enum.GetHashCode(), so the same thing can be done for Enum.IsDefined() if it warrants optimization. (/cc @MichalStrehovsky to confirm)

---

@TylerBrinkley

when dotnet/runtime#20008 only needs C# to allow the enum generic constraint in csharplang#104.

IIRC, a generic enum constraint proposal has been around for years (since Roslyn was on CodePlex) and they're not getting anywhere with it. I don't think it's going to happen anytime soon.

Yep, I was right. Here's the long discussion from CodePlex on enum constraints: http://roslyn.codeplex.com/discussions/572464. Here's the GitHub discussion: https://github.com/dotnet/roslyn/issues/262. As you can see, they've talked about it for quite a while by now and they're not getting anywhere.

[stephentoub]

Yes, that's the reason why I opened this issue.

Then, FWIW, I don't see this providing enough value to be worthwhile.

[JonHanna]

IIRC, a generic enum constraint proposal has been around for years (since Roslyn was on CodePlex) and they're not getting anywhere with it. I don't think it's going to happen anytime soon.

It hasn't, but personally I think the difference between a non-boxing generic method that used it if it did exist and the proposal here are on opposite sides of the threshold of being worthwhile.

[benaadams]

made some changes to the JIT to avoid boxing for Enum.GetHashCode()

I had a look at this for .ToString; the Jit typeforwards GetHashCode to the underlying type (e.g. byte, int, etc); and they have it defined so it doesn't box.

Alas it isn't as straightforward for ToString and and IsDefined as the value type doesn't have the methods directly defined

[TylerBrinkley]

IIRC, a generic enum constraint proposal has been around for years (since Roslyn was on CodePlex) and they're not getting anywhere with it. I don't think it's going to happen anytime soon.

While true, I think what's held them back is a lack of a compelling use case for the BCL which dotnet/runtime#20008 provides. I'm trying to resurrect dialog on csharplang#104 so hopefully we might see some traction.

[MichalStrehovsky]

https://github.com/dotnet/coreclr/pull/7895 made some changes to the JIT to avoid boxing for Enum.GetHashCode(), so the same thing can be done for Enum.IsDefined() if it warrants optimization. (/cc @MichalStrehovsky to confirm)

As @benaadams points out, the trick done for GetHashCode isn't applicable (that wasn't even a codegen feature - I did the change purely on the vm side). This really is a case of having a value type and expecting to call an instance method on a reference type. This would need a bunch of understanding on the codegen side. Codegen isn't even aware of enums right now (can't really tell a difference between an enum based on Int32 and Int32 itself).

From an implementation perspective, this:

namespace System {
    public abstract class Enum : ValueType, IComparable, IConvertible, IFormattable {
        public static bool IsDefined<T>(T value);
    }
}

Would work better. When generating fragile/non-versionable code, we could even generate specialized method bodies per each T and optimize the hell out of it (e.g. for a dense enum, the method body for IsDefined would only check if the value is in the range with two cmp instructions and return). The question really is whether anyone needs this to be so fast and whether it wouldn't be better to spend the time elsewhere.

[joperezr]

Given the above discussion, it doesn't seem there is enough value to add this, so I'm closing this for now. Please do feel free to re-open if there are new scenarios that would make this API more valuable.