From 3e39e6690693a0679d03e6177cc0055a615599b3 Mon Sep 17 00:00:00 2001 From: Sven Boemer Date: Thu, 25 Jul 2024 16:47:59 -0700 Subject: [PATCH] Support field access on variables of type Enum --- .../Compiler/Dataflow/HandleCallAction.cs | 8 ++++ .../TrimAnalysis/HandleCallAction.cs | 2 + .../Linker.Dataflow/HandleCallAction.cs | 4 ++ .../DataFlow/ObjectGetTypeDataflow.cs | 38 +++++++++++++------ 4 files changed, 41 insertions(+), 11 deletions(-) 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 ee69015714887..3e9b8a1fa7d58 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/HandleCallAction.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/HandleCallAction.cs @@ -390,6 +390,10 @@ private partial bool TryHandleIntrinsic ( // currently it won't do. TypeDesc? staticType = (valueNode as IValueWithStaticType)?.StaticType?.Type; + if (staticType?.IsByRef == true) + { + staticType = ((ByRefType)staticType).ParameterType; + } if (staticType is null || (!staticType.IsDefType && !staticType.IsArray)) { DynamicallyAccessedMemberTypes annotation = default; @@ -432,6 +436,10 @@ private partial bool TryHandleIntrinsic ( // we will also make it just work, even if the annotation doesn't match the usage. AddReturnValue(new SystemTypeValue(staticType)); } + else if (staticType.IsTypeOf("System", "Enum")) + { + AddReturnValue(_reflectionMarker.Annotations.GetMethodReturnValue(calledMethod, _isNewObj, DynamicallyAccessedMemberTypes.PublicFields)); + } else { Debug.Assert(staticType is MetadataType || staticType.IsArray); diff --git a/src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/HandleCallAction.cs b/src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/HandleCallAction.cs index 243c29db7002a..911e67f5584a6 100644 --- a/src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/HandleCallAction.cs +++ b/src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/HandleCallAction.cs @@ -113,6 +113,8 @@ private partial bool TryHandleIntrinsic ( // where a parameter is annotated and if something in the method sets a specific known type to it // we will also make it just work, even if the annotation doesn't match the usage. AddReturnValue (new SystemTypeValue (new (staticType))); + } else if (staticType.IsTypeOf ("System", "Enum")) { + AddReturnValue (FlowAnnotations.Instance.GetMethodReturnValue (calledMethod, _isNewObj, DynamicallyAccessedMemberTypes.PublicFields)); } else { var annotation = FlowAnnotations.GetTypeAnnotation (staticType); AddReturnValue (FlowAnnotations.Instance.GetMethodReturnValue (calledMethod, _isNewObj, annotation)); diff --git a/src/tools/illink/src/linker/Linker.Dataflow/HandleCallAction.cs b/src/tools/illink/src/linker/Linker.Dataflow/HandleCallAction.cs index 3d2ceb0fd599b..b2254f4b858a8 100644 --- a/src/tools/illink/src/linker/Linker.Dataflow/HandleCallAction.cs +++ b/src/tools/illink/src/linker/Linker.Dataflow/HandleCallAction.cs @@ -120,6 +120,8 @@ private partial bool TryHandleIntrinsic ( // currently it won't do. TypeReference? staticType = (valueNode as IValueWithStaticType)?.StaticType?.Type; + if (staticType?.IsByReference == true) + staticType = ((ByReferenceType) staticType).ElementType; TypeDefinition? staticTypeDef = staticType?.ResolveToTypeDefinition (_context); if (staticType is null || staticTypeDef is null) { DynamicallyAccessedMemberTypes annotation = default; @@ -153,6 +155,8 @@ private partial bool TryHandleIntrinsic ( // where a parameter is annotated and if something in the method sets a specific known type to it // we will also make it just work, even if the annotation doesn't match the usage. AddReturnValue (new SystemTypeValue (staticType)); + } else if (staticTypeDef.IsTypeOf ("System", "Enum")) { + AddReturnValue (_context.Annotations.FlowAnnotations.GetMethodReturnValue (calledMethod, _isNewObj, DynamicallyAccessedMemberTypes.PublicFields)); } else { // Make sure the type is marked (this will mark it as used via reflection, which is sort of true) // This should already be true for most cases (method params, fields, ...), but just in case diff --git a/src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/ObjectGetTypeDataflow.cs b/src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/ObjectGetTypeDataflow.cs index 99a8c2d15ae12..c88bd2bfdc920 100644 --- a/src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/ObjectGetTypeDataflow.cs +++ b/src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/ObjectGetTypeDataflow.cs @@ -97,7 +97,6 @@ public static void Test () class EnumTypeSatisfiesPublicFields { [Kept] - [ExpectedWarning ("IL2072")] static void ParameterType (Enum instance) { instance.GetType ().RequiresPublicFields (); @@ -113,7 +112,6 @@ class FieldType public FieldType (Enum instance) => field = instance; [Kept] - [ExpectedWarning ("IL2072")] public void Test () { field.GetType ().RequiresPublicFields (); @@ -134,18 +132,36 @@ static Enum ReturnType () } [Kept] - [ExpectedWarning ("IL2072")] static void TestReturnType () { ReturnType ().GetType ().RequiresPublicFields (); } + [Kept] + static void OutParameter (out Enum value) + { + value = EnumType.Value; + } + + [Kept] + // Analyzer doesn't assign a value to the out parameter after calling the OutParameter method, + // so when it looks up the value of the local 'value', it returns an empty value, and the + // GetType intrinsic handling can't see that the out param satisfies the public fields requirement. + // Similar for the other cases below. + [ExpectedWarning ("IL2072", Tool.Analyzer, "https://github.com/dotnet/runtime/issues/101734")] + static void TestOutParameter () + { + OutParameter (out var value); + value.GetType ().RequiresPublicFields (); + } + [Kept] public static void Test () { ParameterType (EnumType.Value); new FieldType (EnumType.Value).Test (); TestReturnType (); + TestOutParameter (); } } @@ -173,10 +189,10 @@ public static void TestAccessFromType () // Note: this doesn't warn for ILLink as a consequence of https://github.com/dotnet/runtime/issues/105345. // ILLink sees the field type as a generic parameter, whereas the other tools see it as System.Enum. - // The special handling that treats Enum as satisfying PublicFields only applies to generic parameter constraints, - // so ILLink doesn't warn here. Once this 105345 is fixed, ILLink should match the warning behavior of ILC - // here and in the similar cases below. - [ExpectedWarning ("IL2072", Tool.NativeAot | Tool.Analyzer, "https://github.com/dotnet/runtime/issues/105345")] + // The other tools don't warn because of the built-in handling for variables of type System.Enum, + // and ILLink doesn't warn because this goes through the case that handles generic parameters constrained to be Enum. + // When https://github.com/dotnet/runtime/issues/105345 is fixed this should go through the built-in handling for + // System.Enum, like it does for ILC and the analyzer. static void TestAccessTypeGenericParameterAsField () { TypeGenericParameterAsField.field.GetType ().RequiresPublicFields (); @@ -214,7 +230,7 @@ public static void TestAccessFromType () } } - [ExpectedWarning ("IL2072", Tool.NativeAot | Tool.Analyzer, "https://github.com/dotnet/runtime/issues/105345")] + // Note: this happens to work for ILLink due to https://github.com/dotnet/runtime/issues/105345. static void TestAccessTypeGenericParameterAsReturnType () { TypeGenericParameterAsReturnType.Method ().GetType ().RequiresPublicFields (); @@ -230,7 +246,7 @@ public static void Method (out T instance) } [Kept] - [ExpectedWarning ("IL2072")] + [ExpectedWarning ("IL2072", Tool.Analyzer, "https://github.com/dotnet/runtime/issues/101734")] public static void TestAccessFromType () { Method (out var instance); @@ -238,7 +254,7 @@ public static void TestAccessFromType () } } - [ExpectedWarning ("IL2072")] + [ExpectedWarning ("IL2072", Tool.Analyzer, "https://github.com/dotnet/runtime/issues/101734")] static void TestAccessTypeGenericParameterAsOutParam () { TypeGenericParameterAsOutParam.Method (out var instance); @@ -259,7 +275,7 @@ static T MethodGenericParameterAsReturnType () where T : Enum } [Kept] - [ExpectedWarning ("IL2072", Tool.NativeAot | Tool.Analyzer, "https://github.com/dotnet/runtime/issues/105345")] + // Note: this happens to work for ILLink due to https://github.com/dotnet/runtime/issues/105345. static void TestMethodGenericParameterAsReturnType () { MethodGenericParameterAsReturnType ().GetType ().RequiresPublicFields ();