Fix base type marking for DAM annotated classes (#110655)

Fixes #104740, also implements #110563 for ILC.

The annotation on DAM annotated classes gets triggered by calling GetType on a location typed as something DAM annotated (or deriving/implementing from it).

The marking is done in layers so that warning suppressions can be properly applied. The bug was that we didn't walk down the hierarchy and assumed someone else will do it. Nobody did. I'm adding a new node type to do the marking.

Previously, we only used one node for tracking. The node got dropped into the dependency graph when we saw GetType being called and it ensured the proper marking for that one type (not the bases). We also added conditional dependencies into MethodTables so that `Derived` can depend on this node of the `Base`. This ensured that if GetType was called on Base, we'd treat it same as GetType on Derived. This was resulting in marking too much and too little (we'd mark right away when we saw GetType call, irrespective of the MethodTable existence, and we wouldn't walk down to Base if the GetType was called on Derived.

The fix is to use two node types. One simply tracks "GetType was called on something". It doesn't bring any other dependencies with it. We only use it to condition other nodes. The other node represents "the dependencies from the annotations".

The way this works is:
* We see GetType called on Base, so we add ObjectGetTypeCalledNode for Base.
* We generate MethodTable for Derived, which adds a conditional dependency on ObjectGetTypeCalledNode of Derived if ObjectGetTypeCalledNode of Base is in the graph. (This ensures "walking up the hierarchy".)
* MethodTable of Derived also adds a conditional dependency on ObjectGetTypeFlowDependenciesNode of Derived if ObjectGetTypeCalledNode of Derived is part of the graph. This will do the actual marking but only if the MethodTable and GetType call was seen.
* ObjectGetTypeFlowDependenciesNode also "walks down the hierarchy" and makes sure ObjectGetTypeFlowDependenciesNode of all the bases and interfaces are present in the graph.

This happens to also address #110563. Because of that, I had to update tests since ILC started trimming more stuff without seeing the type as constructed.

src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/HandleCallAction.cs

@@ -455,7 +455,7 @@ private partial bool TryHandleIntrinsic (
 
                                 if (annotation != default)
                                 {
-                                    _reflectionMarker.Dependencies.Add(_reflectionMarker.Factory.ObjectGetTypeFlowDependencies(closestMetadataType), "GetType called on this type");
+                                    _reflectionMarker.Dependencies.Add(_reflectionMarker.Factory.ObjectGetTypeCalled(closestMetadataType), "GetType called on this type");
                                 }
 
                                 // Return a value which is "unknown type" with annotation. For now we'll use the return value node

src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/ReflectionMethodBodyScanner.cs

@@ -130,6 +130,25 @@ public static DependencyList ProcessTypeGetTypeDataflow(NodeFactory factory, Flo
         {
             DynamicallyAccessedMemberTypes annotation = flowAnnotations.GetTypeAnnotation(type);
             Debug.Assert(annotation != DynamicallyAccessedMemberTypes.None);
+
+            // We're on an interface and we're processing annotations for the purposes of a object.GetType() call.
+            // Most of the annotations don't apply to the members of interfaces - the result of object.GetType() is
+            // never the interface type, it's a concrete type that implements the interface. Limit this to the only
+            // annotations that are applicable in this situation.
+            if (type.IsInterface)
+            {
+                // .All applies to interface members same as to the type
+                if (annotation != DynamicallyAccessedMemberTypes.All)
+                {
+                    // Filter to the MemberTypes that apply to interfaces
+                    annotation &= DynamicallyAccessedMemberTypes.Interfaces;
+
+                    // If we're left with nothing, we're done
+                    if (annotation == DynamicallyAccessedMemberTypes.None)
+                        return new DependencyList();
+                }
+            }
+
             var reflectionMarker = new ReflectionMarker(logger, factory, flowAnnotations, typeHierarchyDataFlowOrigin: type, enabled: true);
 
             // We need to apply annotations to this type, and its base/interface types (recursively)

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

@@ -377,6 +377,11 @@ private void CreateNodeCaches()
                 return new GenericStaticBaseInfoNode(type);
             });
 
+            _objectGetTypeCalled = new NodeCache<MetadataType, ObjectGetTypeCalledNode>(type =>
+            {
+                return new ObjectGetTypeCalledNode(type);
+            });
+
             _objectGetTypeFlowDependencies = new NodeCache<MetadataType, ObjectGetTypeFlowDependenciesNode>(type =>
             {
                 return new ObjectGetTypeFlowDependenciesNode(type);
@@ -1147,6 +1152,12 @@ internal GenericStaticBaseInfoNode GenericStaticBaseInfo(MetadataType type)
             return _genericStaticBaseInfos.GetOrAdd(type);
         }
 
+        private NodeCache<MetadataType, ObjectGetTypeCalledNode> _objectGetTypeCalled;
+        internal ObjectGetTypeCalledNode ObjectGetTypeCalled(MetadataType type)
+        {
+            return _objectGetTypeCalled.GetOrAdd(type);
+        }
+
         private NodeCache<MetadataType, ObjectGetTypeFlowDependenciesNode> _objectGetTypeFlowDependencies;
         internal ObjectGetTypeFlowDependenciesNode ObjectGetTypeFlowDependencies(MetadataType type)
         {

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

@@ -0,0 +1,40 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+using System;
+using System.Collections.Generic;
+
+using ILCompiler.DependencyAnalysisFramework;
+
+using Internal.TypeSystem;
+
+namespace ILCompiler.DependencyAnalysis
+{
+    /// <summary>
+    /// Represents the fact that object.GetType was called on a location typed as this
+    /// type or one of the subtypes of it.
+    /// </summary>
+    internal sealed class ObjectGetTypeCalledNode : DependencyNodeCore<NodeFactory>
+    {
+        private readonly MetadataType _type;
+
+        public ObjectGetTypeCalledNode(MetadataType type)
+        {
+            _type = type;
+        }
+
+        protected override string GetName(NodeFactory factory)
+        {
+            return $"Object.GetType called on {_type}";
+        }
+
+        public override IEnumerable<DependencyListEntry> GetStaticDependencies(NodeFactory factory) => null;
+        public override bool InterestingForDynamicDependencyAnalysis => false;
+        public override bool HasDynamicDependencies => false;
+        public override bool HasConditionalStaticDependencies => false;
+        public override bool StaticDependenciesAreComputed => true;
+        public override IEnumerable<CombinedDependencyListEntry> GetConditionalStaticDependencies(NodeFactory factory) => null;
+        public override IEnumerable<CombinedDependencyListEntry> SearchDynamicDependencies(List<DependencyNodeCore<NodeFactory>> markedNodes, int firstNode, NodeFactory factory) => null;
+
+    }
+}

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

@@ -6,6 +6,8 @@
 
 using ILCompiler.DependencyAnalysisFramework;
 
+using ILLink.Shared.TrimAnalysis;
+
 using Internal.TypeSystem;
 
 namespace ILCompiler.DependencyAnalysis
@@ -25,21 +27,31 @@ public ObjectGetTypeFlowDependenciesNode(MetadataType type)
 
         protected override string GetName(NodeFactory factory)
         {
-            return $"Object.GetType dependencies called on {_type}";
+            return $"Object.GetType dependencies for {_type}";
         }
 
         public override IEnumerable<DependencyListEntry> GetStaticDependencies(NodeFactory factory)
         {
             var mdManager = (UsageBasedMetadataManager)factory.MetadataManager;
+            FlowAnnotations flowAnnotations = mdManager.FlowAnnotations;
+
+            DependencyList result = Dataflow.ReflectionMethodBodyScanner.ProcessTypeGetTypeDataflow(factory, mdManager.FlowAnnotations, mdManager.Logger, _type);
+
+            MetadataType baseType = _type.MetadataBaseType;
+            if (baseType != null && flowAnnotations.GetTypeAnnotation(baseType) != default)
+            {
+                result.Add(factory.ObjectGetTypeFlowDependencies(baseType), "Apply annotations to bases");
+            }
 
-            // We don't mark any members on interfaces - these nodes are only used as conditional dependencies
-            // of other nodes. Calling `object.GetType()` on something typed as an interface will return
-            // something that implements the interface, not the interface itself. We're not reflecting on the
-            // interface.
-            if (_type.IsInterface)
-                return Array.Empty<DependencyListEntry>();
+            foreach (DefType interfaceType in _type.RuntimeInterfaces)
+            {
+                if (flowAnnotations.GetTypeAnnotation(interfaceType) != default)
+                {
+                    result.Add(factory.ObjectGetTypeFlowDependencies((MetadataType)interfaceType), "Apply annotations to interfaces");
+                }
+            }
 
-            return Dataflow.ReflectionMethodBodyScanner.ProcessTypeGetTypeDataflow(factory, mdManager.FlowAnnotations, mdManager.Logger, _type);
+            return result;
         }
 
         public override bool InterestingForDynamicDependencyAnalysis => false;

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

@@ -408,9 +408,19 @@ public override void GetConditionalDependenciesDueToEETypePresence(ref CombinedD
         {
             // Check to see if we have any dataflow annotations on the type.
             // The check below also covers flow annotations inherited through base classes and implemented interfaces.
-            if (type.IsDefType
-                && !type.IsInterface /* "IFoo x; x.GetType();" -> this doesn't actually return an interface type */
-                && FlowAnnotations.GetTypeAnnotation(type) != default)
+            bool hasFlowAnnotations = type.IsDefType && FlowAnnotations.GetTypeAnnotation(type) != default;
+
+            if (hasFlowAnnotations)
+            {
+                dependencies ??= new CombinedDependencyList();
+                dependencies.Add(new DependencyNodeCore<NodeFactory>.CombinedDependencyListEntry(
+                    factory.ObjectGetTypeFlowDependencies((MetadataType)type),
+                    factory.ObjectGetTypeCalled((MetadataType)type),
+                    "Type exists and GetType called on it"));
+            }
+
+            if (hasFlowAnnotations
+                && !type.IsInterface /* "IFoo x; x.GetType();" -> this doesn't actually return an interface type */)
             {
                 // We have some flow annotations on this type.
                 //
@@ -428,8 +438,8 @@ public override void GetConditionalDependenciesDueToEETypePresence(ref CombinedD
                     // Ensure we have the flow dependencies.
                     dependencies ??= new CombinedDependencyList();
                     dependencies.Add(new DependencyNodeCore<NodeFactory>.CombinedDependencyListEntry(
-                        factory.ObjectGetTypeFlowDependencies((MetadataType)type),
-                        factory.ObjectGetTypeFlowDependencies((MetadataType)baseType),
+                        factory.ObjectGetTypeCalled((MetadataType)type),
+                        factory.ObjectGetTypeCalled((MetadataType)baseType),
                         "GetType called on the base type"));
 
                     // We don't have to follow all the bases since the base MethodTable will bubble this up
@@ -444,8 +454,8 @@ public override void GetConditionalDependenciesDueToEETypePresence(ref CombinedD
                         // Ensure we have the flow dependencies.
                         dependencies ??= new CombinedDependencyList();
                         dependencies.Add(new DependencyNodeCore<NodeFactory>.CombinedDependencyListEntry(
-                            factory.ObjectGetTypeFlowDependencies((MetadataType)type),
-                            factory.ObjectGetTypeFlowDependencies((MetadataType)interfaceType),
+                            factory.ObjectGetTypeCalled((MetadataType)type),
+                            factory.ObjectGetTypeCalled((MetadataType)interfaceType),
                             "GetType called on the interface"));
                     }
 

src/coreclr/tools/aot/ILCompiler.Compiler/ILCompiler.Compiler.csproj

@@ -428,6 +428,7 @@
     <Compile Include="Compiler\DependencyAnalysis\MethodExceptionHandlingInfoNode.cs" />
     <Compile Include="Compiler\DependencyAnalysis\ModuleInitializerListNode.cs" />
     <Compile Include="Compiler\DependencyAnalysis\NotReadOnlyFieldNode.cs" />
+    <Compile Include="Compiler\DependencyAnalysis\ObjectGetTypeCalledNode.cs" />
     <Compile Include="Compiler\DependencyAnalysis\ObjectGetTypeFlowDependenciesNode.cs" />
     <Compile Include="Compiler\DependencyAnalysis\PointerTypeMapNode.cs" />
     <Compile Include="Compiler\DependencyAnalysis\ReflectedDelegateNode.cs" />

src/tests/nativeaot/SmokeTests/TrimmingBehaviors/Dataflow.cs

@@ -565,10 +565,7 @@ public static void Run()
                 // so that we can tests this doesn't lead to keeping the type.
                 Assert.Equal(666, s_neverAllocatedTypeAskingForNonPublicMethods.GetType().CountMethods());
             }
-            // This is not great - the GetType() call above "wished" NeverAllocatedTypeAskingForNonPublicMethods
-            // into existence, but it shouldn't have. We could do better here if this is a problem.
-            // If we do that, change this .NotNull to .Null.
-            Assert.NotNull(typeof(TestObjectGetTypeDataflow).GetNestedTypeSecretly(nameof(NeverAllocatedTypeAskingForNonPublicMethods)));
+            Assert.Null(typeof(TestObjectGetTypeDataflow).GetNestedTypeSecretly(nameof(NeverAllocatedTypeAskingForNonPublicMethods)));
             // Sanity check
             Assert.NotNull(typeof(TestObjectGetTypeDataflow).GetNestedTypeSecretly(nameof(TypeWithNonPublicMethodsKept)));
 

src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/ObjectGetTypeDataflow.cs

@@ -18,6 +18,7 @@ public static void Main ()
 		{
 			SealedConstructorAsSource.Test ();
 			InstantiatedGenericAsSource.Test ();
+			InstantiatedGenericAsSourceInstantiated.Test ();
 			EnumTypeSatisfiesPublicFields.Test ();
 			EnumConstraintSatisfiesPublicFields.Test ();
 			InstantiatedTypeParameterAsSource.Test ();
@@ -50,15 +51,15 @@ class InstantiatedGenericAsSource
 		{
 			[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)]
 			class Generic<T> {
-				[ExpectedWarning ("IL2112")]
+				[ExpectedWarning ("IL2112", Tool.Trimmer | Tool.Analyzer, "https://github.com/dotnet/runtime/issues/110563")]
 				[RequiresUnreferencedCode (nameof (KeptForMethodParameter))]
 				public void KeptForMethodParameter () {}
 
-				[ExpectedWarning ("IL2112")]
+				[ExpectedWarning ("IL2112", Tool.Trimmer | Tool.Analyzer, "https://github.com/dotnet/runtime/issues/110563")]
 				[RequiresUnreferencedCode (nameof (KeptForField))]
 				public void KeptForField () {}
 
-				[ExpectedWarning ("IL2112")]
+				[ExpectedWarning ("IL2112", Tool.Trimmer | Tool.Analyzer, "https://github.com/dotnet/runtime/issues/110563")]
 				[RequiresUnreferencedCode (nameof (KeptJustBecause))]
 				public void KeptJustBecause () {}
 			}
@@ -82,6 +83,43 @@ public static void Test ()
 			}
 		}
 
+		class InstantiatedGenericAsSourceInstantiated
+		{
+			[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)]
+			class Generic<T>
+			{
+				[ExpectedWarning ("IL2112")]
+				[RequiresUnreferencedCode (nameof (KeptForMethodParameter))]
+				public void KeptForMethodParameter () { }
+
+				[ExpectedWarning ("IL2112")]
+				[RequiresUnreferencedCode (nameof (KeptForField))]
+				public void KeptForField () { }
+
+				[ExpectedWarning ("IL2112")]
+				[RequiresUnreferencedCode (nameof (KeptJustBecause))]
+				public void KeptJustBecause () { }
+			}
+
+			static void TestMethodParameter (Generic<int> instance)
+			{
+				instance.GetType ().GetMethod ("KeptForMethodParameter");
+			}
+
+			static Generic<int> field = new Generic<int> ();
+
+			static void TestField ()
+			{
+				field.GetType ().GetMethod ("KeptForField");
+			}
+
+			public static void Test ()
+			{
+				TestMethodParameter (null);
+				TestField ();
+			}
+		}
+
 		class EnumTypeSatisfiesPublicFields
 		{
 			static void ParameterType (Enum instance)

src/tools/illink/test/Mono.Linker.Tests.Cases/Generics/InstantiatedGenericEquality.cs

@@ -18,11 +18,11 @@ public static void Main ()
 
 		class GenericReturnType
 		{
-			[KeptAttributeAttribute (typeof (DynamicallyAccessedMembersAttribute))]
+			[KeptAttributeAttribute (typeof (DynamicallyAccessedMembersAttribute), By = Tool.Trimmer /* Type is needed due to IL metadata only */)]
 			[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)]
 			class ReturnType<T>
 			{
-				[Kept]
+				[Kept (By = Tool.Trimmer /* https://github.com/dotnet/runtime/issues/110563 */)]
 				public void Method () { }
 			}