Make `Enum.GetValues` AOT-safe #72236

MichalStrehovsky · 2022-07-15T01:06:04Z

...at the cost of a small compat break. We return int[] instead of SomeInt32Enum[].

We can also delete intrinsic handling of Enum.GetValues in dataflow analysis but I don't want to conflict with Vitek's #71485 right now.

Cc @dotnet/ilc-contrib

...at the cost of a small compat break. We return `int[]` instead of `SomeInt32Enum[]`. Fixes dotnet#72140. We can also delete intrinsic handling of `Enum.GetValues` in dataflow analysis but I don't want to conflict with Vitek's dotnet#71485.

dotnet-issue-labeler · 2022-07-15T01:06:09Z

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

agocke · 2022-07-15T02:57:34Z

Who owns the System.Enum surface area? I presume this needs to go through a back-compat review

MichalStrehovsky · 2022-07-15T02:58:41Z

Who owns the System.Enum surface area? I presume this needs to go through a back-compat review

Only changing it for NativeAOT.

agocke · 2022-07-15T02:59:20Z

Woops, missed that this was the nativeaot runtime

vitek-karas · 2022-07-15T08:03:08Z

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

-            return result;
+            // Compat: we should be returning SomeInt32Enum[]. Instead we return Int32[].
+            // This is a tradeoff since SomeInt32Enum[] type might not have been pregenerated.
+            return (Array)Enum.GetEnumInfo(this).ValuesAsUnderlyingType.Clone();


What guarantees that int[] will exist though? Or for that matter other enum backing types, like byte[], short[] and so on.

The comment here assumes that they are available, without explanation. I think it would be good to add a short explanation.

What guarantees that int[] will exist though?

Good old new keyword:

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

Lines 53 to 64 in 6f13196

ValuesAsUnderlyingType = Type.GetTypeCode(UnderlyingType) switch

{

TypeCode.Byte => new byte[numValues],

TypeCode.SByte => new sbyte[numValues],

TypeCode.UInt16 => new ushort[numValues],

TypeCode.Int16 => new short[numValues],

TypeCode.UInt32 => new uint[numValues],

TypeCode.Int32 => new int[numValues],

TypeCode.UInt64 => new ulong[numValues],

TypeCode.Int64 => new long[numValues],

_ => throw new NotSupportedException(),

};

I'll add a comment.

Sorry - I was too lazy to lookup what ValuesAsUnderlyingType does :-(

MichalStrehovsky · 2022-07-15T08:58:29Z

/azp run runtime-extra-platforms

azure-pipelines · 2022-07-15T08:58:50Z

Azure Pipelines successfully started running 1 pipeline(s).

LakshanF

LGTM but there seems to be some related test failures

System.Reflection.Tests
System.Runtime.Tests
System.IO.FileSystem.DisabledFileLocking.Tests, not clear if the failure is related to the enum change
System.Collections.NonGeneric.Tests
System.Numerics.Vectors.Tests

MichalStrehovsky · 2022-07-18T21:44:14Z

LGTM but there seems to be some related test failures

The System.Numerics.Vectors.Tests is #72149. I think that failure is non-deterministic. I'm not sure if it really should be closed unfortunately.

I'm looking into the appcompat implications of the other failures (whether we just fixup/disable/add new tests and be done with it, or whether it's blockers).

…untime into getenumvalues

MichalStrehovsky · 2022-07-19T08:09:29Z

I looked at the failing tests. The main implication is that if one does Array Enum.GetValues(Type) and then uses the non-generic IEnumerable to iterate over the elements of the arrays, it will return boxed underlying type instead of the boxed enum.

So the compat concern is basically that. Would someone call GetValues and then do a foreach, calling ToString on each element? We would see different values. Or would they call object.Equals on these? They would not be equal to boxed enums.

It's basically what the tests do:

Assert.Equal() Failure
           ↓ (pos 0)
Expected: [Enum1, Enum2, Enum10, Enum18, Enum45]
Actual:   [1, 2, 10, 18, 45]
           ↑ (pos 0)

Couple other alternative:

Introduce a new annotations (e.g. DynamicallyCreatedArrayAttribute?). This would be similar to DynamicallyAccessedMembers but would imply the presence of an array of that type. We can make Enum.GetValues, and Array.CreateInstance AOT safe with this. Might be getting late for that.
Shared code over enums. It's getting late for that too.
Keep it as AOT-unsafe

LakshanF · 2022-07-19T13:02:40Z

This is unfortunate. I would expect in most code, the expectations on post operations after the call to this API, would be to see the Enum type and not the underlying type. I think we need to give an indicator to the developer of the behavioral change in native AOT but can see as you indicated. the options you listed above might be getting too late now.

I think keeping it unsafe with the recommended message to use the generic API might be the best option :-(

stephentoub · 2022-07-19T13:26:27Z

Would someone call GetValues and then do a foreach, calling ToString on each element?

I looked around usage in grep.app. While by far the most common usage is just foreach'ing, there are a non-trivial number of uses that do databinding, e.g.
https://github.com/HearthSim/Hearthstone-Deck-Tracker/blob/4272f593cdb2282e65dcd73312cb4500ab1cb5da/Hearthstone%20Deck%20Tracker/Controls/Stats/ConstructedFilters.xaml.cs#L36
or that use ToString, either directly, e.g.:
https://github.com/Jimmey-Jiang/Common.Utility/blob/103fb680d2818144fe635f957cb262f7f9b810b6/src/Common.Utility_EN/RenumDropList/RenumDropList.cs#L17-L34
or via interpolation, e.g.:
https://github.com/AlanMorel/MapleServer2/blob/38e9e05f63eaf1f7c78de2c141d659537a4ab9a5/MapleServer2/Commands/Game/GMCommands.cs#L179-L182
or that do type tests, e.g.:
https://github.com/ShareX/ShareX/blob/6dec8a9a10a379fe3ae557813592bbfc252d97b8/ShareX.HelpersLib/Helpers/Helpers.cs#L187-L190
or that use the non-generic Array.IndexOf, e.g.:
https://github.com/ShareX/ShareX/blob/6dec8a9a10a379fe3ae557813592bbfc252d97b8/ShareX.HelpersLib/Extensions/EnumExtensions.cs#L86-L90
Seems this change has the potential to break all such usage (with AOT).

MichalPetryka · 2022-07-19T13:27:25Z

The biggest issue is that the generic overload wasn't there in .Net Standard 2.0, so a lot of libraries targetting that use the type one. As such, having it work properly would be really useful. Personally I've always copied the data to properly typed arrays, but I've seen samples that just cast the arrays to the enum array type.

jkotas · 2022-07-19T14:06:13Z

If we cannot fix the existing API, can we introduce a new one? I am thinking GetEnumValuesAsUnderlyingType.

Libraries that target NS2.0 would be able to use this API using reflection light-up and still be AOT compatible. It is not the case with the generic overload. Calling the generic overload via reflection requires MakeGenericMethod that comes with the same AOT problem.

jkotas · 2022-07-19T14:13:06Z

GetEnumValuesAsUnderlyingType would be also good for non-runtime reflection. System.Reflection.MetadataLoadContext does not support GetEnumValues today since it cannot create the enum array.

jkotas · 2022-07-19T14:18:27Z

Another option that does not require new APIs and that is compatible with NS2.0: Enumerate the enum members as field info. It comes with performance penalty.

using System.Reflection;

foreach (var f in typeof(DayOfWeek).GetFields(BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Static))
{
    Console.WriteLine(f.Name);
    Console.WriteLine(f.GetRawConstantValue()); // This returns the field value as boxed underlying type
}

teo-tsirpanis · 2022-07-19T14:57:49Z

Can we make the NativeAOT compiler recognize Enum.GetValues(typeof(T)) and transform it into Enum.GetValues<T>()? It would still raise a warning to suggest users to migrate to the generic overload, but will do what is expected to do at runtime. All but the last examples @stephentoub provided would work, and the last can be easily changed.

jkotas · 2022-07-19T15:38:10Z

Can we make the NativeAOT compiler recognize Enum.GetValues(typeof(T)) and transform it into Enum.GetValues()?

The AOT compiler does that today:

runtime/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/ReflectionMethodBodyScanner.cs

Lines 549 to 571 in 5fc5c68

    
           case IntrinsicId.Enum_GetValues: 
        
               { 
        
                   // Enum.GetValues returns System.Array, but it's the array of the enum type under the hood 
        
                   // and people depend on this undocumented detail (could have returned enum of the underlying 
        
                   // type instead). 
        
                   // 
        
                   // At least until we have shared enum code, this needs extra handling to get it right. 
        
                   foreach (var value in methodParams[0]) 
        
                   { 
        
                       if (value is SystemTypeValue systemTypeValue 
        
                           && !systemTypeValue.RepresentedType.Type.IsGenericDefinition 
        
                           && !systemTypeValue.RepresentedType.Type.ContainsSignatureVariables(treatGenericParameterLikeSignatureVariable: true)) 
        
                       { 
        
                           if (systemTypeValue.RepresentedType.Type.IsEnum) 
        
                           { 
        
                               _reflectionMarker.Dependencies.Add(_factory.ConstructedTypeSymbol(systemTypeValue.RepresentedType.Type.MakeArrayType()), "Enum.GetValues"); 
        
                           } 
        
                       } 
        
                       else 
        
                           CheckAndReportRequires(calledMethod, new MessageOrigin(callingMethodBody, offset), RequiresDynamicCodeAttribute); 
        
                   } 
        
               } 
        
               break;

The problem is what to do with Enum.GetValues where the type is not statically known. For example,

runtime/src/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/EnumConverter.cs

Line 174 in 215b39a

Array objValues = Enum.GetValues(EnumType);

MichalStrehovsky · 2022-07-19T23:59:41Z

Thanks for the data Stephen! The data binding scenario is very likely the reason why .NET Native backtracked on this too (nobody could remember why we backtracked there, only that we backtracked).

I've submitted an API proposal for the API Jan proposed here: #72498. I marked it blocking. If we can get it approved soon, I think it would be reasonable to land before RC 1 snaps off.

jkotas · 2022-07-20T19:23:22Z

The next step is to get #72498 API approved. This PR can be closed.

Make Enum.GetValues AOT-safe

031eef9

...at the cost of a small compat break. We return `int[]` instead of `SomeInt32Enum[]`. Fixes dotnet#72140. We can also delete intrinsic handling of `Enum.GetValues` in dataflow analysis but I don't want to conflict with Vitek's dotnet#71485.

MichalStrehovsky requested a review from LakshanF July 15, 2022 01:06

dotnet-issue-labeler bot added area-NativeAOT-coreclr new-api-needs-documentation labels Jul 15, 2022

ghost assigned MichalStrehovsky Jul 15, 2022

Of course we had a smoke test for it

ea26dd1

agocke approved these changes Jul 15, 2022

View reviewed changes

vitek-karas reviewed Jul 15, 2022

View reviewed changes

This was referenced Jul 15, 2022

System.Net.Http.Function.Tests and System.Net.Websockets.Client.Tests work items failing on runtime-extra-platforms Mono_Release runs #72209

Closed

matrix4x4createfromaxisangletest failing on NativeAOT #72149

Closed

LakshanF reviewed Jul 18, 2022

View reviewed changes

MichalStrehovsky added 2 commits July 19, 2022 16:58

Fix the tests

06b5c36

Merge branch 'getenumvalues' of https://github.com/MichalStrehovsky/r…

0ea7bef

…untime into getenumvalues

runfoapp bot mentioned this pull request Jul 19, 2022

system.net.quic.functional.tests failing with stack buffer overflow #72429

Closed

runfoapp bot mentioned this pull request Jul 19, 2022

System.Net.Http.Functional.Tests.SyncHttpHandler_DiagnosticsTest.SendAsync_SuppressedGlobalStaticPropagationEnvVar failing in CI #72428

Closed

MichalStrehovsky mentioned this pull request Jul 19, 2022

[API Proposal]: Type.GetEnumValuesAsUnderlyingType #72498

Closed

MichalStrehovsky mentioned this pull request Jul 20, 2022

Update llvm-libunwind from v9.0.0 to v14.0.6 #72442

Merged

jkotas closed this Jul 20, 2022

LakshanF mentioned this pull request Jul 29, 2022

Implement new API GetEnumValuesAsUnderlyingType #73057

Merged

ghost locked as resolved and limited conversation to collaborators Aug 20, 2022

MichalStrehovsky deleted the getenumvalues branch April 26, 2023 01:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make `Enum.GetValues` AOT-safe #72236

Make `Enum.GetValues` AOT-safe #72236

MichalStrehovsky commented Jul 15, 2022

dotnet-issue-labeler bot commented Jul 15, 2022

agocke commented Jul 15, 2022

MichalStrehovsky commented Jul 15, 2022

agocke commented Jul 15, 2022

vitek-karas Jul 15, 2022

MichalStrehovsky Jul 15, 2022

vitek-karas Jul 15, 2022

MichalStrehovsky commented Jul 15, 2022

azure-pipelines bot commented Jul 15, 2022

LakshanF left a comment

MichalStrehovsky commented Jul 18, 2022

MichalStrehovsky commented Jul 19, 2022

LakshanF commented Jul 19, 2022

stephentoub commented Jul 19, 2022 •

edited

Loading

MichalPetryka commented Jul 19, 2022 •

edited

Loading

jkotas commented Jul 19, 2022 •

edited

Loading

jkotas commented Jul 19, 2022 •

edited

Loading

jkotas commented Jul 19, 2022

teo-tsirpanis commented Jul 19, 2022

jkotas commented Jul 19, 2022

MichalStrehovsky commented Jul 19, 2022

jkotas commented Jul 20, 2022

	ValuesAsUnderlyingType = Type.GetTypeCode(UnderlyingType) switch
	{
	TypeCode.Byte => new byte[numValues],
	TypeCode.SByte => new sbyte[numValues],
	TypeCode.UInt16 => new ushort[numValues],
	TypeCode.Int16 => new short[numValues],
	TypeCode.UInt32 => new uint[numValues],
	TypeCode.Int32 => new int[numValues],
	TypeCode.UInt64 => new ulong[numValues],
	TypeCode.Int64 => new long[numValues],
	_ => throw new NotSupportedException(),
	};

Make Enum.GetValues AOT-safe #72236

Make Enum.GetValues AOT-safe #72236

Conversation

MichalStrehovsky commented Jul 15, 2022

dotnet-issue-labeler bot commented Jul 15, 2022

agocke commented Jul 15, 2022

MichalStrehovsky commented Jul 15, 2022

agocke commented Jul 15, 2022

vitek-karas Jul 15, 2022

Choose a reason for hiding this comment

MichalStrehovsky Jul 15, 2022

Choose a reason for hiding this comment

vitek-karas Jul 15, 2022

Choose a reason for hiding this comment

MichalStrehovsky commented Jul 15, 2022

azure-pipelines bot commented Jul 15, 2022

LakshanF left a comment

Choose a reason for hiding this comment

MichalStrehovsky commented Jul 18, 2022

MichalStrehovsky commented Jul 19, 2022

LakshanF commented Jul 19, 2022

stephentoub commented Jul 19, 2022 • edited Loading

MichalPetryka commented Jul 19, 2022 • edited Loading

jkotas commented Jul 19, 2022 • edited Loading

jkotas commented Jul 19, 2022 • edited Loading

jkotas commented Jul 19, 2022

teo-tsirpanis commented Jul 19, 2022

jkotas commented Jul 19, 2022

MichalStrehovsky commented Jul 19, 2022

jkotas commented Jul 20, 2022

Make `Enum.GetValues` AOT-safe #72236

Make `Enum.GetValues` AOT-safe #72236

stephentoub commented Jul 19, 2022 •

edited

Loading

MichalPetryka commented Jul 19, 2022 •

edited

Loading

jkotas commented Jul 19, 2022 •

edited

Loading

jkotas commented Jul 19, 2022 •

edited

Loading