From 558778602272bd8d557007de16fe140a47dc48d4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Strehovsk=C3=BD?= Date: Mon, 25 Nov 2024 22:44:50 +0100 Subject: [PATCH 1/3] Enable more ILLink test cases with native AOT When I was working on the `DAMT.All*`, I noticed we don't run many Reflection tests, this is progress towards that. Also fixes rare `CreateInstance` overloads - these were never worth the effort to implement honestly, but more code sharing made implementing them a breeze. Contributes to #82447. --- .../Compiler/Dataflow/HandleCallAction.cs | 30 ++++++++++++++----- .../Compiler/Dataflow/ReflectionMarker.cs | 27 +++++++++++++++++ .../TestCases/TestSuites.cs | 18 +++++------ .../Reflection/ActivatorCreateInstance.cs | 7 +++-- .../ExpressionPropertyMethodInfo.cs | 4 +-- .../Reflection/MethodsUsedViaReflection.cs | 2 +- 6 files changed, 65 insertions(+), 23 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 6f9988295b6605..9ca8b98e9e1353 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/HandleCallAction.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/HandleCallAction.cs @@ -638,17 +638,31 @@ private partial bool TryGetBaseType(TypeProxy type, out TypeProxy? baseType) return false; } -#pragma warning disable IDE0060 private partial bool TryResolveTypeNameForCreateInstanceAndMark(in MethodProxy calledMethod, string assemblyName, string typeName, out TypeProxy resolvedType) { - // TODO: niche APIs that we probably shouldn't even have added - // We have to issue a warning, otherwise we could break the app without a warning. - // This is not the ideal warning, but it's good enough for now. - _diagnosticContext.AddDiagnostic(DiagnosticId.UnrecognizedParameterInMethodCreateInstance, calledMethod.GetParameter((ParameterIndex)(1 + (calledMethod.HasImplicitThis() ? 1 : 0))).GetDisplayName(), calledMethod.GetDisplayName()); - resolvedType = default; - return false; + if (!System.Reflection.Metadata.AssemblyNameInfo.TryParse(assemblyName, out var an) + || _callingMethod.Context.ResolveAssembly(an) is not ModuleDesc resolvedAssembly) + { + _diagnosticContext.AddDiagnostic(DiagnosticId.UnresolvedAssemblyInCreateInstance, + assemblyName, + calledMethod.GetDisplayName()); + resolvedType = default; + return false; + } + + if (!_reflectionMarker.TryResolveTypeNameAndMark(resolvedAssembly, typeName, _diagnosticContext, "Reflection", out TypeDesc? foundType)) + { + // It's not wrong to have a reference to non-existing type - the code may well expect to get an exception in this case + // Note that we did find the assembly, so it's not a ILLink config problem, it's either intentional, or wrong versions of assemblies + // but ILLink can't know that. In case a user tries to create an array using System.Activator we should simply ignore it, the user + // might expect an exception to be thrown. + resolvedType = default; + return false; + } + + resolvedType = new TypeProxy(foundType); + return true; } -#pragma warning restore IDE0060 private partial void MarkStaticConstructor(TypeProxy type) => _reflectionMarker.MarkStaticConstructor(_diagnosticContext.Origin, type.Type, _reason); diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/ReflectionMarker.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/ReflectionMarker.cs index 83b592554c6c50..056032d025bfe8 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/ReflectionMarker.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/ReflectionMarker.cs @@ -116,6 +116,33 @@ internal bool TryResolveTypeNameAndMark(string typeName, in DiagnosticContext di return true; } + internal bool TryResolveTypeNameAndMark(ModuleDesc assembly, string typeName, in DiagnosticContext diagnosticContext, string reason, [NotNullWhen(true)] out TypeDesc? type) + { + List referencedModules = new(); + TypeDesc foundType = CustomAttributeTypeNameParser.GetTypeByCustomAttributeTypeNameForDataFlow(typeName, assembly, assembly.Context, + referencedModules, needsAssemblyName: false, out _); + if (foundType == null) + { + type = default; + return false; + } + + if (_enabled) + { + foreach (ModuleDesc referencedModule in referencedModules) + { + // Also add module metadata in case this reference was through a type forward + if (Factory.MetadataManager.CanGenerateMetadata(referencedModule.GetGlobalModuleType())) + _dependencies.Add(Factory.ModuleMetadata(referencedModule), reason); + } + + MarkType(diagnosticContext.Origin, foundType, reason); + } + + type = foundType; + return true; + } + internal void MarkType(in MessageOrigin origin, TypeDesc type, string reason, AccessKind accessKind = AccessKind.Unspecified) { if (!_enabled) diff --git a/src/coreclr/tools/aot/ILCompiler.Trimming.Tests/TestCases/TestSuites.cs b/src/coreclr/tools/aot/ILCompiler.Trimming.Tests/TestCases/TestSuites.cs index 0ac9ba0eb1134d..e9ac526fb714b5 100644 --- a/src/coreclr/tools/aot/ILCompiler.Trimming.Tests/TestCases/TestSuites.cs +++ b/src/coreclr/tools/aot/ILCompiler.Trimming.Tests/TestCases/TestSuites.cs @@ -69,17 +69,17 @@ public void LinkXml (string t) [MemberData (nameof (TestDatabase.Reflection), MemberType = typeof (TestDatabase))] public void Reflection (string t) { - switch (t) { - case "TypeHierarchyReflectionWarnings": - case "ParametersUsedViaReflection": - case "UnsafeAccessor": - case "TypeUsedViaReflection": - case "RunClassConstructor": - case "NestedTypeUsedViaReflection": - Run (t); + switch (t) + { + case "ObjectGetType": + // Skip for now + break; + case "ObjectGetTypeLibraryMode": + case "TypeHierarchyLibraryModeSuppressions": + // No Library mode break; default: - // Skip the rest for now + Run (t); break; } } diff --git a/src/tools/illink/test/Mono.Linker.Tests.Cases/Reflection/ActivatorCreateInstance.cs b/src/tools/illink/test/Mono.Linker.Tests.Cases/Reflection/ActivatorCreateInstance.cs index 70289934fe5367..5dfe65e6a134ad 100644 --- a/src/tools/illink/test/Mono.Linker.Tests.Cases/Reflection/ActivatorCreateInstance.cs +++ b/src/tools/illink/test/Mono.Linker.Tests.Cases/Reflection/ActivatorCreateInstance.cs @@ -171,7 +171,7 @@ public FromParameterOnStaticMethodTypeB (int arg) { } } // Small formatting difference - [ExpectedWarning ("IL2067", nameof (Activator) + "." + nameof (Activator.CreateInstance) + "(Type, Object[])", Tool.Trimmer, "")] + [ExpectedWarning ("IL2067", nameof (Activator) + "." + nameof (Activator.CreateInstance) + "(Type, Object[])", Tool.Trimmer | Tool.NativeAot, "")] [ExpectedWarning ("IL2067", nameof (Activator) + "." + nameof (Activator.CreateInstance) + "(Type, params Object[])", Tool.Analyzer, "")] [ExpectedWarning ("IL2067", nameof (Activator) + "." + nameof (Activator.CreateInstance), nameof (CultureInfo))] [Kept] @@ -197,7 +197,7 @@ public FromParameterOnInstanceMethodType (int arg, int arg2) { } [ExpectedWarning ("IL2067", nameof (Activator) + "." + nameof (Activator.CreateInstance) + "(Type)")] // Small formatting difference - [ExpectedWarning ("IL2067", nameof (Activator) + "." + nameof (Activator.CreateInstance) + "(Type, Object[])", Tool.Trimmer, "")] + [ExpectedWarning ("IL2067", nameof (Activator) + "." + nameof (Activator.CreateInstance) + "(Type, Object[])", Tool.Trimmer | Tool.NativeAot, "")] [ExpectedWarning ("IL2067", nameof (Activator) + "." + nameof (Activator.CreateInstance) + "(Type, params Object[])", Tool.Analyzer, "")] [Kept] private static void FromParameterWithNonPublicConstructors ( @@ -552,6 +552,7 @@ private static void TestCreateInstanceOfTWithNewConstraint< [Kept] class TestCreateInstanceOfTWithNoConstraintType { + [Kept(By = Tool.NativeAot /* native AOT intrinsically expands CreateInstance and would keep this method, albeit not reflection-visible */)] public TestCreateInstanceOfTWithNoConstraintType () { } @@ -680,7 +681,7 @@ public void TestCreateInstance () [Kept] [KeptBaseType (typeof (AnnotatedBase))] - [KeptMember (".ctor()")] + [KeptMember (".ctor()", By = Tool.Trimmer /* This type is never allocated so there's no reason to keep ctor due to a GetType call */)] class Derived : AnnotatedBase { [Kept] diff --git a/src/tools/illink/test/Mono.Linker.Tests.Cases/Reflection/ExpressionPropertyMethodInfo.cs b/src/tools/illink/test/Mono.Linker.Tests.Cases/Reflection/ExpressionPropertyMethodInfo.cs index 27f3422c76d945..258f687bb0a021 100644 --- a/src/tools/illink/test/Mono.Linker.Tests.Cases/Reflection/ExpressionPropertyMethodInfo.cs +++ b/src/tools/illink/test/Mono.Linker.Tests.Cases/Reflection/ExpressionPropertyMethodInfo.cs @@ -83,10 +83,10 @@ public int InstancePropertyViaReflection { } [Kept] - [ExpectedWarning ("IL2026", nameof (StaticPropertyExpressionAccess), Tool.Trimmer, "https://github.com/dotnet/linker/issues/2669")] + [ExpectedWarning ("IL2026", nameof (StaticPropertyExpressionAccess), Tool.Trimmer | Tool.NativeAot, "https://github.com/dotnet/linker/issues/2669")] [ExpectedWarning ("IL2026", nameof (StaticPropertyViaReflection))] [ExpectedWarning ("IL2026", nameof (StaticPropertyViaRuntimeMethod))] - [ExpectedWarning ("IL2026", nameof (InstancePropertyExpressionAccess), Tool.Trimmer, "https://github.com/dotnet/linker/issues/2669")] + [ExpectedWarning ("IL2026", nameof (InstancePropertyExpressionAccess), Tool.Trimmer | Tool.NativeAot, "https://github.com/dotnet/linker/issues/2669")] [ExpectedWarning ("IL2026", nameof (InstancePropertyViaReflection))] public static void Test () { diff --git a/src/tools/illink/test/Mono.Linker.Tests.Cases/Reflection/MethodsUsedViaReflection.cs b/src/tools/illink/test/Mono.Linker.Tests.Cases/Reflection/MethodsUsedViaReflection.cs index c6cb02a55017ed..7a750652ef8d0f 100644 --- a/src/tools/illink/test/Mono.Linker.Tests.Cases/Reflection/MethodsUsedViaReflection.cs +++ b/src/tools/illink/test/Mono.Linker.Tests.Cases/Reflection/MethodsUsedViaReflection.cs @@ -337,7 +337,7 @@ private void PrivateMethodWithRUC () { } } [Kept] - [ExpectedWarning ("IL2026", Tool.Trimmer, "https://github.com/dotnet/linker/issues/2638")] + [ExpectedWarning ("IL2026", Tool.Trimmer | Tool.NativeAot, "https://github.com/dotnet/linker/issues/2638")] public static void Test () { BindingFlags left = BindingFlags.Instance | BindingFlags.Static; From 3b95e0b3c6a403567c6baf3d42f301db170735c5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Strehovsk=C3=BD?= Date: Mon, 25 Nov 2024 23:39:34 +0100 Subject: [PATCH 2/3] Broke an unrelated test --- .../Mono.Linker.Tests.Cases/DataFlow/ComplexTypeHandling.cs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/ComplexTypeHandling.cs b/src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/ComplexTypeHandling.cs index 8d72fc68f36485..94d57025d0f3f8 100644 --- a/src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/ComplexTypeHandling.cs +++ b/src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/ComplexTypeHandling.cs @@ -142,7 +142,7 @@ static void TestArrayTypeGetType () // doesn't work on arrays, but the current implementation will preserve it anyway due to how it processes // string -> Type resolution. This will only impact code which would have failed at runtime, so very unlikely to // actually occur in real apps (and even if it does happen, it just increases size, doesn't break behavior). - [Kept (By = Tool.Trimmer)] // NativeAOT doesn't preserve array element types just due to the usage of the array type + [Kept (By = Tool.Trimmer | Tool.NativeAot)] class ArrayCreateInstanceByNameElement { public ArrayCreateInstanceByNameElement () @@ -151,7 +151,6 @@ public ArrayCreateInstanceByNameElement () } [Kept] - [ExpectedWarning ("IL2032", Tool.NativeAot, "https://github.com/dotnet/runtime/issues/82447")] static void TestArrayCreateInstanceByName () { Activator.CreateInstance ("test", "Mono.Linker.Tests.Cases.DataFlow.ComplexTypeHandling+ArrayCreateInstanceByNameElement[]"); From a53c0c3eaffc835cced2c3b2fdf3422755dba69a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Strehovsk=C3=BD?= Date: Mon, 2 Dec 2024 21:22:35 -0800 Subject: [PATCH 3/3] Update src/coreclr/tools/aot/ILCompiler.Trimming.Tests/TestCases/TestSuites.cs Co-authored-by: kasperk81 <83082615+kasperk81@users.noreply.github.com> --- .../tools/aot/ILCompiler.Trimming.Tests/TestCases/TestSuites.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/tools/aot/ILCompiler.Trimming.Tests/TestCases/TestSuites.cs b/src/coreclr/tools/aot/ILCompiler.Trimming.Tests/TestCases/TestSuites.cs index e9ac526fb714b5..a3493db1786a8d 100644 --- a/src/coreclr/tools/aot/ILCompiler.Trimming.Tests/TestCases/TestSuites.cs +++ b/src/coreclr/tools/aot/ILCompiler.Trimming.Tests/TestCases/TestSuites.cs @@ -75,7 +75,7 @@ public void Reflection (string t) // Skip for now break; case "ObjectGetTypeLibraryMode": - case "TypeHierarchyLibraryModeSuppressions": + case "TypeHierarchyLibraryModeSuppressions": // No Library mode break; default: