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

        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.

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 microsoft/OpenAPI.NET#1717 (comment). 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:

runtime/src/tools/illink/src/linker/Linker.Dataflow/HandleCallAction.cs

Lines 108 to 121 in 66ae898

	// Note that valueNode can be statically typed in IL as some generic argument type.
	// For example:
	// void Method<T>(T instance) { instance.GetType().... }
	// Currently this case will end up with null StaticType - since there's no typedef for the generic argument type.
	// But it could be that T is annotated with for example PublicMethods:
	// void Method<[DAM(PublicMethods)] T>(T instance) { instance.GetType().GetMethod("Test"); }
	// In this case it's in theory possible to handle it, by treating the T basically as a base class
	// for the actual type of "instance". But the analysis for this would be pretty complicated (as the marking
	// has to happen on the callsite, which doesn't know that GetType() will be used...).
	// For now we're intentionally ignoring this case - it will produce a warning.
	// The counter example is:
	// Method<Base>(new Derived);
	// In this case to get correct results, trimmer would have to mark all public methods on Derived. Which
	// currently it won't do.

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.