Consider targets of delegates reflection-visible (#70198)

This is to make sure `Delegate.GetMethodInfo` API works in `IlcTrimMetadata=true` mode. In this mode, the presence of code doesn't automatically mean the method is visible to reflection (we only consider results of dataflow analysis or XML inputs, or `DynamicDependency`). Add delegate targets to this list so that `GetMethodInfo` API reliably works.

The compiler already tracks delegate creation sequence, so adding a callback to `MetadataManager` to inject the dependencies that make the method reflection-visible.

We also handle the situation when the delegate was created to a virtual method and the exact target isn't known until runtime. We do this by injecting conditional dependencies on virtual method implementations.

This change causes a 0.3% size regression on ASP.NET WebApi template with IlcTrimMetadata=true. I spot checked the diffs and they all look correct (there's a lot of delegates being created to support various captures and suddenly those things become reflectable - it's what we want).

Author: MichalStrehovsky

src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DelegateCreationInfo.cs

@@ -50,6 +50,16 @@ public MethodDesc TargetMethod
             }
         }
 
+        // The target method might be constrained if this was a "constrained ldftn" IL instruction.
+        // The real target can be computed after resolving the constraint.
+        public MethodDesc PossiblyUnresolvedTargetMethod
+        {
+            get
+            {
+                return _targetMethod;
+            }
+        }
+
         private bool TargetMethodIsUnboxingThunk
         {
             get

src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/EETypeNode.cs

@@ -344,6 +344,8 @@ public sealed override IEnumerable<CombinedDependencyListEntry> GetConditionalSt
                     {
                         factory.MetadataManager.NoteOverridingMethod(decl, impl);
                     }
+
+                    factory.MetadataManager.GetDependenciesForOverridingMethod(ref result, factory, decl, impl);
                 }
 
                 Debug.Assert(
@@ -389,6 +391,8 @@ public sealed override IEnumerable<CombinedDependencyListEntry> GetConditionalSt
                             }
 
                             factory.MetadataManager.NoteOverridingMethod(interfaceMethod, implMethod);
+
+                            factory.MetadataManager.GetDependenciesForOverridingMethod(ref result, factory, interfaceMethod, implMethod);
                         }
                         else
                         {
@@ -414,6 +418,8 @@ public sealed override IEnumerable<CombinedDependencyListEntry> GetConditionalSt
                                 result.Add(new CombinedDependencyListEntry(factory.MethodEntrypoint(defaultIntfMethod), factory.VirtualMethodUse(interfaceMethod), "Interface method"));
 
                                 factory.MetadataManager.NoteOverridingMethod(interfaceMethod, implMethod);
+
+                                factory.MetadataManager.GetDependenciesForOverridingMethod(ref result, factory, interfaceMethod, implMethod);
                             }
                         }
                     }

src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ReadyToRunGenericHelperNode.cs

@@ -215,6 +215,12 @@ protected override DependencyList ComputeNonRelocationBasedDependencies(NodeFact
                 dependencies.Add(new DependencyListEntry(dependency, "GenericLookupResultDependency"));
             }
 
+            if (_id == ReadyToRunHelperId.DelegateCtor)
+            {
+                MethodDesc targetMethod = ((DelegateCreationInfo)_target).PossiblyUnresolvedTargetMethod.GetCanonMethodTarget(CanonicalFormKind.Specific);
+                factory.MetadataManager.GetDependenciesDueToDelegateCreation(ref dependencies, factory, targetMethod);
+            }
+
             return dependencies;
         }
 

src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ReadyToRunHelperNode.cs

@@ -142,23 +142,27 @@ protected override DependencyList ComputeNonRelocationBasedDependencies(NodeFact
             }
             else if (_id == ReadyToRunHelperId.DelegateCtor)
             {
+                DependencyList dependencyList = null;
+
                 var info = (DelegateCreationInfo)_target;
                 if (info.NeedsVirtualMethodUseTracking)
                 {
                     MethodDesc targetMethod = info.TargetMethod;
 
-                    DependencyList dependencyList = new DependencyList();
 #if !SUPPORT_JIT
                     factory.MetadataManager.GetDependenciesDueToVirtualMethodReflectability(ref dependencyList, factory, targetMethod);
 
                     if (!factory.VTable(info.TargetMethod.OwningType).HasFixedSlots)
                     {
+                        dependencyList ??= new DependencyList();
                         dependencyList.Add(factory.VirtualMethodUse(info.TargetMethod), "ReadyToRun Delegate to virtual method");
                     }
 #endif
-
-                    return dependencyList;
                 }
+
+                factory.MetadataManager.GetDependenciesDueToDelegateCreation(ref dependencyList, factory, info.PossiblyUnresolvedTargetMethod);
+
+                return dependencyList;
             }
 
             return null;

src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/MetadataManager.cs

@@ -430,6 +430,22 @@ public virtual void GetDependenciesDueToLdToken(ref DependencyList dependencies,
             // RuntimeFieldHandle data structure.
         }
 
+        /// <summary>
+        /// This method is an extension point that can provide additional metadata-based dependencies to delegate targets.
+        /// </summary>
+        public virtual void GetDependenciesDueToDelegateCreation(ref DependencyList dependencies, NodeFactory factory, MethodDesc target)
+        {
+            // MetadataManagers can override this to provide additional dependencies caused by the construction
+            // of a delegate to a method.
+        }
+
+        /// <summary>
+        /// This method is an extension point that can provide additional dependencies for overriden methods on constructed types.
+        /// </summary>
+        public virtual void GetDependenciesForOverridingMethod(ref CombinedDependencyList dependencies, NodeFactory factory, MethodDesc decl, MethodDesc impl)
+        {
+        }
+
         /// <summary>
         /// This method is an extension point that can provide additional metadata-based dependencies to generated method bodies.
         /// </summary>

src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/UsageBasedMetadataManager.cs

@@ -456,6 +456,46 @@ public override void GetDependenciesDueToLdToken(ref DependencyList dependencies
                 dependencies.Add(factory.ReflectableMethod(method), "LDTOKEN method");
         }
 
+        public override void GetDependenciesDueToDelegateCreation(ref DependencyList dependencies, NodeFactory factory, MethodDesc target)
+        {
+            if (!IsReflectionBlocked(target))
+            {
+                dependencies = dependencies ?? new DependencyList();
+                dependencies.Add(factory.ReflectableMethod(target), "Target of a delegate");
+            }
+        }
+
+        public override void GetDependenciesForOverridingMethod(ref CombinedDependencyList dependencies, NodeFactory factory, MethodDesc decl, MethodDesc impl)
+        {
+            Debug.Assert(decl.IsVirtual && MetadataVirtualMethodAlgorithm.FindSlotDefiningMethodForVirtualMethod(decl) == decl);
+
+            // If a virtual method slot is reflection visible, all implementations become reflection visible.
+            //
+            // We could technically come up with a weaker position on this because the code below just needs to
+            // to ensure that delegates to virtual methods can have their GetMethodInfo() called.
+            // Delegate construction introduces a ReflectableMethod for the slot defining method; it doesn't need to.
+            // We could have a specialized node type to track that specific thing and introduce a conditional dependency
+            // on that.
+            //
+            // class Base { abstract Boo(); }
+            // class Derived1 : Base { override Boo() { } }
+            // class Derived2 : Base { override Boo() { } }
+            //
+            // typeof(Derived2).GetMethods(...)
+            //
+            // In the above case, we don't really need Derived1.Boo to become reflection visible
+            // but the below code will do that because ReflectableMethodNode tracks all reflectable methods,
+            // without keeping information about subtleities like "reflectable delegate".
+            if (!IsReflectionBlocked(decl) && !IsReflectionBlocked(impl))
+            {
+                dependencies ??= new CombinedDependencyList();
+                dependencies.Add(new DependencyNodeCore<NodeFactory>.CombinedDependencyListEntry(
+                    factory.ReflectableMethod(impl.GetCanonMethodTarget(CanonicalFormKind.Specific)),
+                    factory.ReflectableMethod(decl.GetCanonMethodTarget(CanonicalFormKind.Specific)),
+                    "Virtual method declaration is reflectable"));
+            }
+        }
+
         protected override void GetDependenciesDueToMethodCodePresenceInternal(ref DependencyList dependencies, NodeFactory factory, MethodDesc method, MethodIL methodIL)
         {
             bool scanReflection = (_generationOptions & UsageBasedMetadataGenerationOptions.ReflectionILScanning) != 0;

src/tests/nativeaot/SmokeTests/Reflection/Reflection.cs

@@ -16,7 +16,7 @@
 [assembly: TestAssembly]
 [module: TestModule]
 
-internal class ReflectionTest
+internal static class ReflectionTest
 {
     private static int Main()
     {
@@ -27,6 +27,8 @@ private static int Main()
         //
         // Tests for dependency graph in the compiler
         //
+        TestSimpleDelegateTargets.Run();
+        TestVirtualDelegateTargets.Run();
         TestRunClassConstructor.Run();
 #if !OPTIMIZED_MODE_WITHOUT_SCANNER
         TestContainment.Run();
@@ -1656,6 +1658,104 @@ public static void Run()
         }
     }
 
+    class TestSimpleDelegateTargets
+    {
+        class TestClass
+        {
+            public static void StaticMethod() { }
+            public void InstanceMethod() { }
+            public static void SimplyCalledMethod() { }
+        }
+
+        class TestClass<T>
+        {
+            public static void StaticMethod() { }
+        }
+
+        static void CheckGeneric<T>()
+        {
+            Action staticMethod = TestClass<T>.StaticMethod;
+            if (staticMethod.GetMethodInfo().Name != nameof(TestClass<T>.StaticMethod))
+                throw new Exception();
+        }
+
+        public static void Run()
+        {
+            Console.WriteLine("Testing delegate targets are reflectable...");
+
+            Action staticMethod = TestClass.StaticMethod;
+            if (staticMethod.GetMethodInfo().Name != nameof(TestClass.StaticMethod))
+                throw new Exception();
+
+            Action instanceMethod = new TestClass().InstanceMethod;
+            if (instanceMethod.GetMethodInfo().Name != nameof(TestClass.InstanceMethod))
+                throw new Exception();
+
+            TestClass.SimplyCalledMethod();
+
+            Assert.Equal(
+#if REFLECTION_FROM_USAGE
+                3,
+#else
+                2,
+#endif
+                typeof(TestClass).CountMethods());
+
+            CheckGeneric<object>();
+        }
+    }
+
+    class TestVirtualDelegateTargets
+    {
+        abstract class Base
+        {
+            public virtual void VirtualMethod() { }
+            public abstract void AbstractMethod();
+        }
+
+        class Derived : Base, IBar
+        {
+            public override void AbstractMethod() { }
+            public override void VirtualMethod() { }
+            void IFoo.InterfaceMethod() { }
+        }
+
+        interface IFoo
+        {
+            void InterfaceMethod();
+            void DefaultInterfaceMethod() { }
+        }
+
+        interface IBar : IFoo
+        {
+            void IFoo.DefaultInterfaceMethod() { }
+        }
+
+        static Base s_baseInstance = new Derived();
+        static IFoo s_ifooInstance = new Derived();
+
+        public static void Run()
+        {
+            Console.WriteLine("Testing virtual delegate targets are reflectable...");
+
+            Action abstractMethod = s_baseInstance.AbstractMethod;
+            if (abstractMethod.GetMethodInfo().Name != nameof(Derived.AbstractMethod))
+                throw new Exception();
+
+            Action virtualMethod = s_baseInstance.VirtualMethod;
+            if (virtualMethod.GetMethodInfo().Name != nameof(Derived.VirtualMethod))
+                throw new Exception();
+
+            Action interfaceMethod = s_ifooInstance.InterfaceMethod;
+            if (!interfaceMethod.GetMethodInfo().Name.EndsWith("IFoo.InterfaceMethod"))
+                throw new Exception();
+
+            Action defaultMethod = s_ifooInstance.DefaultInterfaceMethod;
+            if (!defaultMethod.GetMethodInfo().Name.EndsWith("IFoo.DefaultInterfaceMethod"))
+                throw new Exception();
+        }
+    }
+
     class TestRunClassConstructor
     {
         static class TypeWithNoStaticFieldsButACCtor
@@ -1709,6 +1809,11 @@ private static bool HasTypeHandle(Type type)
         return true;
     }
 
+    [UnconditionalSuppressMessage("ReflectionAnalysis", "IL2070:UnrecognizedReflectionPattern",
+        Justification = "That's the point")]
+    public static int CountMethods(this Type t)
+        => t.GetMethods(BindingFlags.Instance | BindingFlags.Static | BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.DeclaredOnly).Length;
+
     class Assert
     {
         public static void Equal<T>(T expected, T actual)