Respecting EnumMemberAttribute in AOT'd applications

[eerhardt]

I'd like to NativeAOT compile a library that respects the EnumMemberAttribute. This attribute can be applied to enum fields to rename what the field name is used when serializing values of an enum.

The issue is, I can't statically know all the enum types that will be passed into my code. So I need to write code like this:

https://github.com/microsoft/kiota-abstractions-dotnet/blob/02afdff08829667ad7411e87b9dfa797809243a0/src/RequestInformation.cs#L187-L210

        private static object ReplaceEnumValueByStringRepresentation(object source)
        {
            if(source is Enum enumValue && GetEnumName(enumValue) is string enumValueName)
            {
                return enumValueName;
            }

            return source;
        }
        private static string? GetEnumName<T>(T value) where T : Enum
        {
            var type = value.GetType();

            if(Enum.GetName(type, value) is not { } name)
                throw new ArgumentException($"Invalid Enum value {value} for enum of type {type}");

            if(type.GetField(name)?.GetCustomAttribute<EnumMemberAttribute>() is { } attribute)
                return attribute.Value;

            return name.ToFirstCharacterLowerCase();

But I am getting a warning on value.GetType() and using it to call type.GetField(name).

In talking with @MichalStrehovsky and @agocke, one recommendation is that since enum fields cannot be trimmed, we can suppress this warning.

We should document this so people can justify suppressing this warning.

Additionally, it would be great if the analysis tooling understood that since T : Enum, and we are calling GetField on a System.Type that is guaranteed to be an enum, we shouldn't warn.

[ghost]

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas See info in area-owners.md if you want to be subscribed.

Issue Details

---

I'd like to NativeAOT compile a library that respects the EnumMemberAttribute. This attribute can be applied to enum fields to rename what the field name is used when serializing values of an enum. The issue is, I can't statically know all the enum types that will be passed into my code. So I need to write code like this: https://github.com/microsoft/kiota-abstractions-dotnet/blob/02afdff08829667ad7411e87b9dfa797809243a0/src/RequestInformation.cs#L187-L210 ```C# private static object ReplaceEnumValueByStringRepresentation(object source) { if(source is Enum enumValue && GetEnumName(enumValue) is string enumValueName) { return enumValueName; } return source; } private static string? GetEnumName(T value) where T : Enum { var type = value.GetType(); if(Enum.GetName(type, value) is not { } name) throw new ArgumentException($"Invalid Enum value {value} for enum of type {type}"); if(type.GetField(name)?.GetCustomAttribute() is { } attribute) return attribute.Value; return name.ToFirstCharacterLowerCase(); ``` But I am getting a warning on `value.GetType()` and using it to call `type.GetField(name)`. In talking with @MichalStrehovsky and @agocke, one recommendation is that since enum fields cannot be trimmed, we can suppress this warning. We should document this so people can justify suppressing this warning. Additionally, it would be great if the analysis tooling understood that since `T : Enum`, and we are calling `GetField` on a System.Type that is guaranteed to be an enum, we shouldn't warn.

Author:	eerhardt
Assignees:	-
Labels:	`untriaged`, `area-NativeAOT-coreclr`
Milestone:	-

[teo-tsirpanis]

I think that instead of type.GetValue() it should be using typeof(T). There is also a generic overload of Enum.GetName in modern frameworks.

[eerhardt]

I think that instead of type.GetValue() it should be using typeof(T). There is also a generic overload of Enum.GetName in modern frameworks.

Initially I agreed, until I saw the code in the top method:

        private static object ReplaceEnumValueByStringRepresentation(object source)
        {
            if(source is Enum enumValue && GetEnumName(enumValue) is string enumValueName)

here is a call to GetEnumName<System.Enum>(enumValue). So when this call happens, typeof(T) == typeof(System.Enum), which doesn't have the field, nor will it work with Enum.GetName.

The issue is, I can't statically know all the enum types that will be passed into my code.

[MichalStrehovsky]

Reopening, we also need to handle the var type = value.GetType().GetField(...); case.

[MichalStrehovsky]

@sbomer, I reopened this because the original use case was actually calling out for a variation of what was fixed. The same variation was also hit in https://github.com/microsoft/OpenAPI.NET/pull/1717#discussion_r1681224383. I think we'd want to fix this, but the fix doesn't look exactly trivial on the ILLinker side.

The native AOT side is probably just:

diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/HandleCallAction.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/HandleCallAction.cs
index 74fe05d9bd8..fcb9695a485 100644
--- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/HandleCallAction.cs
+++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/HandleCallAction.cs
@@ -15,6 +15,7 @@
 using DependencyList = ILCompiler.DependencyAnalysisFramework.DependencyNodeCore<ILCompiler.DependencyAnalysis.NodeFactory>.DependencyList;
 using MultiValue = ILLink.Shared.DataFlow.ValueSet<ILLink.Shared.DataFlow.SingleValue>;
 using WellKnownType = ILLink.Shared.TypeSystemProxy.WellKnownType;
+using System.Diagnostics.CodeAnalysis;

 #nullable enable

@@ -392,8 +393,29 @@ private partial bool TryHandleIntrinsic (
                             TypeDesc? staticType = (valueNode as IValueWithStaticType)?.StaticType?.Type;
                             if (staticType is null || (!staticType.IsDefType && !staticType.IsArray))
                             {
-                                // We don't know anything about the type GetType was called on. Track this as a usual "result of a method call without any annotations"
-                                AddReturnValue(_reflectionMarker.Annotations.GetMethodReturnValue(calledMethod, _isNewObj));
+                                DynamicallyAccessedMemberTypes annotation = default;
+                                if (staticType is { IsSignatureVariable: true })
+                                {
+                                    var genericParam = (GenericParameterDesc)staticType.InstantiateSignature(_callingMethod.OwningType.Instantiation, _callingMethod.Instantiation);
+                                    foreach (TypeDesc constraint in genericParam.TypeConstraints)
+                                    {
+                                        if (constraint.IsWellKnownType(Internal.TypeSystem.WellKnownType.Enum))
+                                        {
+                                            annotation = DynamicallyAccessedMemberTypes.PublicFields;
+                                            break;
+                                        }
+                                    }
+                                }
+
+                                if (annotation != default)
+                                {
+                                    AddReturnValue(_reflectionMarker.Annotations.GetMethodReturnValue(calledMethod, _isNewObj, annotation));
+                                }
+                                else
+                                {
+                                    // We don't know anything about the type GetType was called on. Track this as a usual "result of a method call without any annotations"
+                                    AddReturnValue(_reflectionMarker.Annotations.GetMethodReturnValue(calledMethod, _isNewObj));
+                                }
                             }
                             else if (staticType.IsSealed() || staticType.IsTypeOf("System", "Delegate"))
                             {

However, on ILLinker side this is running into the following big comment:

https://github.com/dotnet/runtime/blob/66ae89812794186ee10da590a5d31d04125237ea/src/tools/illink/src/linker/Linker.Dataflow/HandleCallAction.cs#L108-L121

I.e. the StaticType is null and cannot be used (it is a TypeDefinition in Cecil, so cannot be a generic parameter). Do you have some idea of how we could proceed here?

[sbomer]

It looks like we could change StaticType to be a TypeReference instead - seems like that would better match what TypeDesc can represent in ILC. I'll give this a try.