Fix ToString handling of [Flags] enums with negative values (dotnet#8…

…3616)

* Fix ToString handling of [Flags] enums with negative values

Our Enum rewrite earlier in .NET 8 broke the handling of ToString for [Flags] enums with negative values.  Prior to that rewrite, all of the enum values were stored as a sorted array of ulongs, regardless of the enum's underlying type. Now, they're stored as a sorted array of the underlying type.  However, that means that for signed underlying types, the position of negative values moved from the end to the beginning.  We knew this, and accordingly updated tests that reflected that order (which as an implementation detail is observable via APIs that get the underlying values).  But what we didn't notice because we didn't have tests covering it is that the logic for formatting [Flags] enums actually depends on those negative values being after the non-negative ones.  That's for two reasons.  First, there's logic that special-cases 0 and assumes that an enum value of 0 must be in the first slot of the values array if it exists.  Second, the logic for deciding which enum values should be included starts at the largest unsigned value and walks down subtracting out matching bit patterns as they're found; if the values are sorted differently, the resulting strings are different.  Not only might different names be included, but a number might be rendered if the order of evaluation means that no perfect subsetting is found even if there would have been had a different order been used.

This fixes the issues by restoring the ordering based on the values being unsigned.  When we sort, we sort based on the unsigned version of the underlying primitive, even if it's signed.

* Collapse GetEnumInfo for integer types to only be unsigned

Rather than 13 possible `EnumInfo<T>`, we consolidate down to just 8, such that for all of the underlying types that are signed integers, we instead always use their unsigned counterparts.  This then ensures that the data for the values is always sorted according to the unsigned representation and all logic in Enum that performs searches based on value is always doing so consistently.

src/coreclr/System.Private.CoreLib/src/System/Enum.CoreCLR.cs

@@ -82,17 +82,22 @@ internal static unsafe RuntimeType InternalGetUnderlyingType(RuntimeType enumTyp
         }
 
         [MethodImpl(MethodImplOptions.AggressiveInlining)]
-        private static EnumInfo<TUnderlyingValue> GetEnumInfo<TUnderlyingValue>(RuntimeType enumType, bool getNames = true)
-            where TUnderlyingValue : struct, INumber<TUnderlyingValue>
+        private static EnumInfo<TStorage> GetEnumInfo<TStorage>(RuntimeType enumType, bool getNames = true)
+            where TStorage : struct, INumber<TStorage>
         {
-            return enumType.GenericCache is EnumInfo<TUnderlyingValue> info && (!getNames || info.Names is not null) ?
+            Debug.Assert(
+                typeof(TStorage) == typeof(byte) || typeof(TStorage) == typeof(ushort) || typeof(TStorage) == typeof(uint) || typeof(TStorage) == typeof(ulong) ||
+                typeof(TStorage) == typeof(nuint) || typeof(TStorage) == typeof(float) || typeof(TStorage) == typeof(double) || typeof(TStorage) == typeof(char),
+                $"Unexpected {nameof(TStorage)} == {typeof(TStorage)}");
+
+            return enumType.GenericCache is EnumInfo<TStorage> info && (!getNames || info.Names is not null) ?
                 info :
                 InitializeEnumInfo(enumType, getNames);
 
             [MethodImpl(MethodImplOptions.NoInlining)]
-            static EnumInfo<TUnderlyingValue> InitializeEnumInfo(RuntimeType enumType, bool getNames)
+            static EnumInfo<TStorage> InitializeEnumInfo(RuntimeType enumType, bool getNames)
             {
-                TUnderlyingValue[]? values = null;
+                TStorage[]? values = null;
                 string[]? names = null;
 
                 GetEnumValuesAndNames(
@@ -101,12 +106,12 @@ static EnumInfo<TUnderlyingValue> InitializeEnumInfo(RuntimeType enumType, bool
                     ObjectHandleOnStack.Create(ref names),
                     getNames ? Interop.BOOL.TRUE : Interop.BOOL.FALSE);
 
-                Debug.Assert(values!.GetType() == typeof(TUnderlyingValue[]));
+                Debug.Assert(values!.GetType() == typeof(TStorage[]));
                 Debug.Assert(!getNames || names!.GetType() == typeof(string[]));
 
                 bool hasFlagsAttribute = enumType.IsDefined(typeof(FlagsAttribute), inherit: false);
 
-                var entry = new EnumInfo<TUnderlyingValue>(hasFlagsAttribute, values, names!);
+                var entry = new EnumInfo<TStorage>(hasFlagsAttribute, values, names!);
                 enumType.GenericCache = entry;
                 return entry;
             }

src/coreclr/nativeaot/System.Private.CoreLib/src/System/Enum.NativeAot.cs

@@ -7,6 +7,7 @@
 using System.Reflection;
 using System.Runtime;
 using System.Runtime.CompilerServices;
+using System.Runtime.InteropServices;
 using Internal.Runtime.Augments;
 using Internal.Runtime.CompilerServices;
 using Internal.Reflection.Augments;
@@ -28,33 +29,36 @@ internal static EnumInfo GetEnumInfo(Type enumType, bool getNames = true)
             RuntimeType rt = (RuntimeType)enumType;
             return Type.GetTypeCode(RuntimeAugments.GetEnumUnderlyingType(rt.TypeHandle)) switch
             {
-                TypeCode.SByte => GetEnumInfo<sbyte>(rt),
-                TypeCode.Byte => GetEnumInfo<byte>(rt),
-                TypeCode.Int16 => GetEnumInfo<short>(rt),
-                TypeCode.UInt16 => GetEnumInfo<ushort>(rt),
-                TypeCode.Int32 => GetEnumInfo<int>(rt),
-                TypeCode.UInt32 => GetEnumInfo<uint>(rt),
-                TypeCode.Int64 => GetEnumInfo<long>(rt),
-                TypeCode.UInt64 => GetEnumInfo<ulong>(rt),
+                TypeCode.SByte or TypeCode.Byte => GetEnumInfo<byte>(rt),
+                TypeCode.Int16 or TypeCode.UInt16 => GetEnumInfo<ushort>(rt),
+                TypeCode.Int32 or TypeCode.UInt32 => GetEnumInfo<uint>(rt),
+                TypeCode.Int64 or TypeCode.UInt64 => GetEnumInfo<ulong>(rt),
                 _ => throw new NotSupportedException(),
             };
         }
 
-        internal static EnumInfo<TUnderlyingValue> GetEnumInfo<TUnderlyingValue>(Type enumType, bool getNames = true)
-            where TUnderlyingValue : struct, INumber<TUnderlyingValue>
+        internal static EnumInfo<TStorage> GetEnumInfo<TStorage>(Type enumType, bool getNames = true)
+            where TStorage : struct, INumber<TStorage>
         {
             Debug.Assert(enumType != null);
             Debug.Assert(enumType is RuntimeType);
             Debug.Assert(enumType.IsEnum);
+            Debug.Assert(
+                typeof(TStorage) == typeof(byte) ||
+                typeof(TStorage) == typeof(ushort) ||
+                typeof(TStorage) == typeof(uint) ||
+                typeof(TStorage) == typeof(ulong));
 
-            return (EnumInfo<TUnderlyingValue>)ReflectionAugments.ReflectionCoreCallbacks.GetEnumInfo(enumType,
+            return (EnumInfo<TStorage>)ReflectionAugments.ReflectionCoreCallbacks.GetEnumInfo(enumType,
                 static (underlyingType, names, valuesAsObject, isFlags) =>
                 {
                     // Only after we've sorted, create the underlying array.
-                    var values = new TUnderlyingValue[valuesAsObject.Length];
+                    var values = new TStorage[valuesAsObject.Length];
                     for (int i = 0; i < valuesAsObject.Length; i++)
-                        values[i] = (TUnderlyingValue)valuesAsObject[i];
-                    return new EnumInfo<TUnderlyingValue>(underlyingType, values, names, isFlags);
+                    {
+                        values[i] = (TStorage)valuesAsObject[i];
+                    }
+                    return new EnumInfo<TStorage>(underlyingType, values, names, isFlags);
             });
         }
 #pragma warning restore

src/coreclr/nativeaot/System.Private.CoreLib/src/System/Reflection/EnumInfo.cs

@@ -7,6 +7,9 @@
 using System.Numerics;
 using System.Runtime;
 using System.Runtime.CompilerServices;
+using System.Runtime.InteropServices;
+
+#pragma warning disable CS8500 // This takes the address of, gets the size of, or declares a pointer to a managed type
 
 namespace System.Reflection
 {
@@ -26,10 +29,10 @@ private protected EnumInfo(Type underlyingType, string[] names, bool isFlags)
     }
 
     [ReflectionBlocked]
-    public sealed class EnumInfo<TUnderlyingValue> : EnumInfo
-        where TUnderlyingValue : struct, INumber<TUnderlyingValue>
+    public sealed class EnumInfo<TStorage> : EnumInfo
+        where TStorage : struct, INumber<TStorage>
     {
-        public EnumInfo(Type underlyingType, TUnderlyingValue[] values, string[] names, bool isFlags) :
+        public EnumInfo(Type underlyingType, TStorage[] values, string[] names, bool isFlags) :
             base(underlyingType, names, isFlags)
         {
             Debug.Assert(values.Length == names.Length);
@@ -39,10 +42,14 @@ public EnumInfo(Type underlyingType, TUnderlyingValue[] values, string[] names,
             ValuesAreSequentialFromZero = Enum.AreSequentialFromZero(values);
         }
 
-        internal TUnderlyingValue[] Values { get; }
+        internal TStorage[] Values { get; }
         internal bool ValuesAreSequentialFromZero { get; }
 
-        public TUnderlyingValue[] CloneValues() =>
-            new ReadOnlySpan<TUnderlyingValue>(Values).ToArray();
+        /// <summary>Create a copy of <see cref="Values"/>.</summary>
+        public unsafe TResult[] CloneValues<TResult>() where TResult : struct
+        {
+            Debug.Assert(sizeof(TStorage) == sizeof(TResult));
+            return MemoryMarshal.Cast<TStorage, TResult>(Values).ToArray();
+        }
     }
 }

src/coreclr/nativeaot/System.Private.CoreLib/src/System/Reflection/Runtime/General/ReflectionCoreCallbacksImplementation.cs

@@ -413,22 +413,21 @@ public sealed override EnumInfo GetEnumInfo(Type type, Func<Type, string[], obje
             return info;
         }
 
-        private class EnumUnderlyingTypeComparer : IComparer<object>
+        private sealed class EnumUnderlyingTypeComparer : IComparer<object>
         {
             public static readonly EnumUnderlyingTypeComparer Instance = new EnumUnderlyingTypeComparer();
 
             public int Compare(object? x, object? y)
-                => x switch
+            {
+                Debug.Assert(x is byte or ushort or uint or ulong);
+                return x switch
                 {
-                    int i => i.CompareTo((int)y!),
-                    uint ui => ui.CompareTo((uint)y!),
                     byte b => b.CompareTo((byte)y!),
                     ushort us => us.CompareTo((ushort)y!),
-                    short s => s.CompareTo((short)y!),
-                    sbyte sb => sb.CompareTo((sbyte)y!),
-                    long l => l.CompareTo((long)y!),
+                    uint ui => ui.CompareTo((uint)y!),
                     _ => ((ulong)x!).CompareTo((ulong)y!),
                 };
+            }
         }
 
         public sealed override DynamicInvokeInfo GetDelegateDynamicInvokeInfo(Type type)

src/coreclr/nativeaot/System.Private.CoreLib/src/System/RuntimeType.cs

@@ -115,7 +115,7 @@ public sealed override Array GetEnumValuesAsUnderlyingType()
             if (!IsActualEnum)
                 throw new ArgumentException(SR.Arg_MustBeEnum, "enumType");
 
-            return (Array)Enum.GetValuesAsUnderlyingTypeNoCopy(this).Clone();
+            return Enum.GetValuesAsUnderlyingType(this);
         }
 
         internal bool IsActualEnum

src/coreclr/nativeaot/System.Private.Reflection.Execution/src/Internal/Reflection/Execution/NativeFormatEnumInfo.cs

@@ -40,7 +40,19 @@ public static void GetEnumValuesAndNames(MetadataReader reader, TypeDefinitionHa
                 if (0 != (field.Flags & FieldAttributes.Static))
                 {
                     unsortedNames[i] = field.Name.GetString(reader);
-                    unsortedBoxedValues[i] = field.DefaultValue.ParseConstantNumericValue(reader);
+                    var handle = field.DefaultValue;
+                    unsortedBoxedValues[i] = handle.HandleType switch
+                    {
+                        HandleType.ConstantSByteValue => (byte)handle.ToConstantSByteValueHandle(reader).GetConstantSByteValue(reader).Value,
+                        HandleType.ConstantByteValue => handle.ToConstantByteValueHandle(reader).GetConstantByteValue(reader).Value,
+                        HandleType.ConstantInt16Value => (ushort)handle.ToConstantInt16ValueHandle(reader).GetConstantInt16Value(reader).Value,
+                        HandleType.ConstantUInt16Value => handle.ToConstantUInt16ValueHandle(reader).GetConstantUInt16Value(reader).Value,
+                        HandleType.ConstantInt32Value => (uint)handle.ToConstantInt32ValueHandle(reader).GetConstantInt32Value(reader).Value,
+                        HandleType.ConstantUInt32Value => handle.ToConstantUInt32ValueHandle(reader).GetConstantUInt32Value(reader).Value,
+                        HandleType.ConstantInt64Value => (ulong)handle.ToConstantInt64ValueHandle(reader).GetConstantInt64Value(reader).Value,
+                        HandleType.ConstantUInt64Value => handle.ToConstantUInt64ValueHandle(reader).GetConstantUInt64Value(reader).Value,
+                        _ => handle.ParseConstantNumericValue(reader), // fallback for unhandled cases
+                    };
                     i++;
                 }
             }
@@ -49,7 +61,10 @@ public static void GetEnumValuesAndNames(MetadataReader reader, TypeDefinitionHa
             foreach (CustomAttributeHandle cah in typeDef.CustomAttributes)
             {
                 if (cah.IsCustomAttributeOfType(reader, "System", "FlagsAttribute"))
+                {
                     isFlags = true;
+                    break;
+                }
             }
         }
     }

src/coreclr/vm/reflectioninvocation.cpp

@@ -1939,6 +1939,19 @@ extern "C" void QCALLTYPE Enum_GetValuesAndNames(QCall::TypeHandle pEnumType, QC
 
     CorElementType type = pMT->GetClass()->GetInternalCorElementType();
 
+    // For underlying types that are signed integers, replace them with their
+    // unsigned counterparts, as expected by the managed Enum implementation.
+    // See the comment in Enum.cs for an explanation.
+    switch (type)
+    {
+        case ELEMENT_TYPE_I1: type = ELEMENT_TYPE_U1; break;
+        case ELEMENT_TYPE_I2: type = ELEMENT_TYPE_U2; break;
+        case ELEMENT_TYPE_I4: type = ELEMENT_TYPE_U4; break;
+        case ELEMENT_TYPE_I8: type = ELEMENT_TYPE_U8; break;
+        case ELEMENT_TYPE_I:  type = ELEMENT_TYPE_U;  break;
+        default: break;
+    }
+
     mdFieldDef field;
     while (pImport->EnumNext(&fieldEnum, &field))
     {