Almost complete sync of logic for generating DAM/RUC reflection acces…

…s warnings between ILLink and NativeAOT (#84770)

This enables the last of the disabled `RequiresCapability` tests for NativeAOT.

The implementation basically copies the logic from ILLink on how to produce warnings when method or field is marked due to reflection access.

We decided that producing warnings due to reflection access to compiler generated methods is more trouble than benefit and thus we won't do that anymore. See reasoning in #85042.

Otherwise the behavior in NativeAOT fully copies the one in ILLink (small exceptions due to better method overload resolution in NativeAOT).

High-level behavior changes in NativeAOT:
* Finally allow the NativeAOT compiler to produce IL2110, IL2111, IL2113 and IL2115
* Avoid generating warnings for override methods if the warning is already generated on the base method (for DAM marking)
* Implement correct marking (and warnings) for "DAM on type" when the derived class has broader annotation than the base class/interface (this is mostly to avoid noise warnings)
* Rename a set of methods and classes in data flow scanner to "Token", because they're only used when a given entity is accessed through its token (so `ldtoken` and similar). This allows for more precise handling of the cases above.

Testing changes:
* Allow specifying tool exceptions for `SkipKepItemsValidationAttribute` - this allows to enable one of the reflection tests for its diagnostics coverage, avoiding the necessity to fix everything around Kept behavior yet.
* Couple of fixes in the test validator to correctly compare expected/actual diagnostics

This fixes final bits of #68786
Fixes #68688
This helps with #82447

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

@@ -476,7 +476,7 @@ protected virtual void Scan(MethodIL methodBody, ref InterproceduralState interp
                         {
                             if (methodBody.GetObject(reader.ReadILToken()) is MethodDesc methodOperand)
                             {
-                                HandleMethodReflectionAccess(methodBody, offset, methodOperand);
+                                HandleMethodTokenAccess(methodBody, offset, methodOperand);
                                 TrackNestedFunctionReference(methodOperand, ref interproceduralState);
                             }
 
@@ -537,7 +537,7 @@ protected virtual void Scan(MethodIL methodBody, ref InterproceduralState interp
                         {
                             if (methodBody.GetObject(reader.ReadILToken()) is MethodDesc methodOperand)
                             {
-                                HandleMethodReflectionAccess(methodBody, offset, methodOperand);
+                                HandleMethodTokenAccess(methodBody, offset, methodOperand);
                             }
 
                             PopUnknown(currentStack, 1, methodBody, offset);
@@ -963,22 +963,22 @@ private void ScanLdtoken(MethodIL methodBody, int offset, object operand, Stack<
                     }
                 }
 
-                HandleTypeReflectionAccess(methodBody, offset, type);
+                HandleTypeTokenAccess(methodBody, offset, type);
             }
             else if (operand is MethodDesc method)
             {
                 StackSlot slot = new StackSlot(new RuntimeMethodHandleValue(method));
                 currentStack.Push(slot);
 
-                HandleMethodReflectionAccess(methodBody, offset, method);
+                HandleMethodTokenAccess(methodBody, offset, method);
             }
             else
             {
                 Debug.Assert(operand is FieldDesc);
 
                 PushUnknown(currentStack);
 
-                HandleFieldReflectionAccess(methodBody, offset, (FieldDesc)operand);
+                HandleFieldTokenAccess(methodBody, offset, (FieldDesc)operand);
             }
         }
 
@@ -1248,19 +1248,19 @@ protected void AssignRefAndOutParameters(
         }
 
         /// <summary>
-        /// Called when type is accessed directly (basically only ldtoken)
+        /// Called when type is accessed directly by its token (so creating a RuntimeHandle)
         /// </summary>
-        protected abstract void HandleTypeReflectionAccess(MethodIL methodBody, int offset, TypeDesc accessedType);
+        protected abstract void HandleTypeTokenAccess(MethodIL methodBody, int offset, TypeDesc accessedType);
 
         /// <summary>
-        /// Called to handle reflection access to a method without any other specifics (ldtoken or ldftn for example)
+        /// Called to handle access to a method by its token (so creating a RuntimeHandle)
         /// </summary>
-        protected abstract void HandleMethodReflectionAccess(MethodIL methodBody, int offset, MethodDesc accessedMethod);
+        protected abstract void HandleMethodTokenAccess(MethodIL methodBody, int offset, MethodDesc accessedMethod);
 
         /// <summary>
-        /// Called to handle reflection access to a field without any other specifics (ldtoken for example)
+        /// Called to handle access to a field by its token (so creating a RuntimeHandle)
         /// </summary>
-        protected abstract void HandleFieldReflectionAccess(MethodIL methodBody, int offset, FieldDesc accessedField);
+        protected abstract void HandleFieldTokenAccess(MethodIL methodBody, int offset, FieldDesc accessedField);
 
         private void HandleCall(
             MethodIL callingMethodBody,

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

@@ -135,7 +135,42 @@ public static DependencyList ProcessTypeGetTypeDataflow(NodeFactory factory, Flo
             DynamicallyAccessedMemberTypes annotation = flowAnnotations.GetTypeAnnotation(type);
             Debug.Assert(annotation != DynamicallyAccessedMemberTypes.None);
             var reflectionMarker = new ReflectionMarker(logger, factory, flowAnnotations, typeHierarchyDataFlowOrigin: type, enabled: true);
-            reflectionMarker.MarkTypeForDynamicallyAccessedMembers(new MessageOrigin(type), type, annotation, type.GetDisplayName());
+
+            // We need to apply annotations to this type, and its base/interface types (recursively)
+            // But the annotations on base/interfaces may already be applied so we don't need to apply those
+            // again (and should avoid doing so as it would produce extra warnings).
+            MessageOrigin origin = new MessageOrigin(type);
+            if (type.HasBaseType)
+            {
+                var baseAnnotation = flowAnnotations.GetTypeAnnotation(type.BaseType);
+                var annotationToApplyToBase = Annotations.GetMissingMemberTypes(annotation, baseAnnotation);
+
+                // Apply any annotations that didn't exist on the base type to the base type.
+                // This may produce redundant warnings when the annotation is DAMT.All or DAMT.PublicConstructors and the base already has a
+                // subset of those annotations.
+                reflectionMarker.MarkTypeForDynamicallyAccessedMembers(origin, type.BaseType, annotationToApplyToBase, type.GetDisplayName(), declaredOnly: false);
+            }
+
+            // Most of the DynamicallyAccessedMemberTypes don't select members on interfaces. We only need to apply
+            // annotations to interfaces separately if dealing with DAMT.All or DAMT.Interfaces.
+            if (annotation.HasFlag(DynamicallyAccessedMemberTypes.Interfaces))
+            {
+                var annotationToApplyToInterfaces = annotation == DynamicallyAccessedMemberTypes.All ? annotation : DynamicallyAccessedMemberTypes.Interfaces;
+                foreach (var iface in type.RuntimeInterfaces)
+                {
+                    if (flowAnnotations.GetTypeAnnotation(iface).HasFlag(annotationToApplyToInterfaces))
+                        continue;
+
+                    // Apply All or Interfaces to the interface type.
+                    // DAMT.All may produce redundant warnings from implementing types, when the interface type already had some annotations.
+                    reflectionMarker.MarkTypeForDynamicallyAccessedMembers(origin, iface, annotationToApplyToInterfaces, type.GetDisplayName(), declaredOnly: false);
+                }
+            }
+
+            // The annotations this type inherited from its base types or interfaces should not produce
+            // warnings on the respective base/interface members, since those are already covered by applying
+            // the annotations to those types. So we only need to handle the members directly declared on this type.
+            reflectionMarker.MarkTypeForDynamicallyAccessedMembers(new MessageOrigin(type), type, annotation, type.GetDisplayName(), declaredOnly: true);
             return reflectionMarker.Dependencies;
         }
 
@@ -193,7 +228,7 @@ protected override void HandleStoreParameter(MethodIL methodBody, int offset, Me
         protected override void HandleStoreMethodReturnValue(MethodIL methodBody, int offset, MethodReturnValue returnValue, MultiValue valueToStore)
             => HandleStoreValueWithDynamicallyAccessedMembers(methodBody, offset, returnValue, valueToStore, returnValue.Method.GetDisplayName());
 
-        protected override void HandleTypeReflectionAccess(MethodIL methodBody, int offset, TypeDesc accessedType)
+        protected override void HandleTypeTokenAccess(MethodIL methodBody, int offset, TypeDesc accessedType)
         {
             // Note that ldtoken alone is technically a reflection access to the type
             // it doesn't lead to full reflection marking of the type
@@ -204,20 +239,20 @@ protected override void HandleTypeReflectionAccess(MethodIL methodBody, int offs
             ProcessGenericArgumentDataFlow(accessedType);
         }
 
-        protected override void HandleMethodReflectionAccess(MethodIL methodBody, int offset, MethodDesc accessedMethod)
+        protected override void HandleMethodTokenAccess(MethodIL methodBody, int offset, MethodDesc accessedMethod)
         {
             _origin = _origin.WithInstructionOffset(methodBody, offset);
 
-            TrimAnalysisPatterns.Add(new TrimAnalysisReflectionAccessPattern(accessedMethod, _origin));
+            TrimAnalysisPatterns.Add(new TrimAnalysisTokenAccessPattern(accessedMethod, _origin));
 
             ProcessGenericArgumentDataFlow(accessedMethod);
         }
 
-        protected override void HandleFieldReflectionAccess(MethodIL methodBody, int offset, FieldDesc accessedField)
+        protected override void HandleFieldTokenAccess(MethodIL methodBody, int offset, FieldDesc accessedField)
         {
             _origin = _origin.WithInstructionOffset(methodBody, offset);
 
-            TrimAnalysisPatterns.Add(new TrimAnalysisReflectionAccessPattern(accessedField, _origin));
+            TrimAnalysisPatterns.Add(new TrimAnalysisTokenAccessPattern(accessedField, _origin));
 
             ProcessGenericArgumentDataFlow(accessedField);
         }

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

@@ -15,7 +15,7 @@ public readonly struct TrimAnalysisPatternStore
     {
         private readonly Dictionary<(MessageOrigin, bool), TrimAnalysisAssignmentPattern> AssignmentPatterns;
         private readonly Dictionary<MessageOrigin, TrimAnalysisMethodCallPattern> MethodCallPatterns;
-        private readonly Dictionary<(MessageOrigin, TypeSystemEntity), TrimAnalysisReflectionAccessPattern> ReflectionAccessPatterns;
+        private readonly Dictionary<(MessageOrigin, TypeSystemEntity), TrimAnalysisTokenAccessPattern> TokenAccessPatterns;
         private readonly Dictionary<(MessageOrigin, TypeSystemEntity), TrimAnalysisGenericInstantiationAccessPattern> GenericInstantiations;
         private readonly Dictionary<(MessageOrigin, FieldDesc), TrimAnalysisFieldAccessPattern> FieldAccessPatterns;
         private readonly ValueSetLattice<SingleValue> Lattice;
@@ -25,7 +25,7 @@ public TrimAnalysisPatternStore(ValueSetLattice<SingleValue> lattice, Logger log
         {
             AssignmentPatterns = new Dictionary<(MessageOrigin, bool), TrimAnalysisAssignmentPattern>();
             MethodCallPatterns = new Dictionary<MessageOrigin, TrimAnalysisMethodCallPattern>();
-            ReflectionAccessPatterns = new Dictionary<(MessageOrigin, TypeSystemEntity), TrimAnalysisReflectionAccessPattern>();
+            TokenAccessPatterns = new Dictionary<(MessageOrigin, TypeSystemEntity), TrimAnalysisTokenAccessPattern>();
             GenericInstantiations = new Dictionary<(MessageOrigin, TypeSystemEntity), TrimAnalysisGenericInstantiationAccessPattern>();
             FieldAccessPatterns = new Dictionary<(MessageOrigin, FieldDesc), TrimAnalysisFieldAccessPattern>();
             Lattice = lattice;
@@ -60,9 +60,9 @@ public void Add(TrimAnalysisMethodCallPattern pattern)
             MethodCallPatterns[pattern.Origin] = pattern.Merge(Lattice, existingPattern);
         }
 
-        public void Add(TrimAnalysisReflectionAccessPattern pattern)
+        public void Add(TrimAnalysisTokenAccessPattern pattern)
         {
-            ReflectionAccessPatterns.TryAdd((pattern.Origin, pattern.Entity), pattern);
+            TokenAccessPatterns.TryAdd((pattern.Origin, pattern.Entity), pattern);
 
             // No Merge - there's nothing to merge since this pattern is uniquely identified by both the origin and the entity
             // and there's only one way to "access" a generic instantiation.
@@ -92,7 +92,7 @@ public void MarkAndProduceDiagnostics(ReflectionMarker reflectionMarker)
             foreach (var pattern in MethodCallPatterns.Values)
                 pattern.MarkAndProduceDiagnostics(reflectionMarker, _logger);
 
-            foreach (var pattern in ReflectionAccessPatterns.Values)
+            foreach (var pattern in TokenAccessPatterns.Values)
                 pattern.MarkAndProduceDiagnostics(reflectionMarker, _logger);
 
             foreach (var pattern in GenericInstantiations.Values)

src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/TrimAnalysisReflectionAccessPattern.cs → src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/TrimAnalysisTokenAccessPattern.cs

@@ -9,30 +9,30 @@
 
 namespace ILCompiler.Dataflow
 {
-    public readonly record struct TrimAnalysisReflectionAccessPattern
+    public readonly record struct TrimAnalysisTokenAccessPattern
     {
         public TypeSystemEntity Entity { init; get; }
         public MessageOrigin Origin { init; get; }
 
-        internal TrimAnalysisReflectionAccessPattern(TypeSystemEntity entity, MessageOrigin origin)
+        internal TrimAnalysisTokenAccessPattern(TypeSystemEntity entity, MessageOrigin origin)
         {
             Entity = entity;
             Origin = origin;
         }
 
         // No Merge - there's nothing to merge since this pattern is uniquely identified by both the origin and the entity
-        // and there's only one way to "reflection access" an entity.
+        // and there's only one way to access entity by its handle.
 
         public void MarkAndProduceDiagnostics(ReflectionMarker reflectionMarker, Logger logger)
         {
             switch (Entity)
             {
                 case MethodDesc method:
-                    reflectionMarker.CheckAndWarnOnReflectionAccess(Origin, method);
+                    reflectionMarker.CheckAndWarnOnReflectionAccess(Origin, method, ReflectionMarker.AccessKind.TokenAccess);
                     break;
 
                 case FieldDesc field:
-                    reflectionMarker.CheckAndWarnOnReflectionAccess(Origin, field);
+                    reflectionMarker.CheckAndWarnOnReflectionAccess(Origin, field, ReflectionMarker.AccessKind.TokenAccess);
                     break;
 
                 default:

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

@@ -149,11 +149,6 @@ public void LogError(TypeSystemEntity origin, DiagnosticId id, params string[] a
 
         internal bool IsWarningSuppressed(int code, MessageOrigin origin)
         {
-            // This is causing too much noise
-            // https://github.com/dotnet/runtime/issues/81156
-            if (code == 2110 || code == 2111 || code == 2113 || code == 2115)
-                return true;
-
             if (_suppressedWarnings.Contains(code))
                 return true;
 

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

@@ -383,7 +383,7 @@
     <Compile Include="Compiler\Dataflow\TrimAnalysisGenericInstantiationAccessPattern.cs" />
     <Compile Include="Compiler\Dataflow\TrimAnalysisMethodCallPattern.cs" />
     <Compile Include="Compiler\Dataflow\TrimAnalysisPatternStore.cs" />
-    <Compile Include="Compiler\Dataflow\TrimAnalysisReflectionAccessPattern.cs" />
+    <Compile Include="Compiler\Dataflow\TrimAnalysisTokenAccessPattern.cs" />
     <Compile Include="Compiler\Dataflow\TypeNameParser.Dataflow.cs" />
     <Compile Include="$(LibrariesProjectRoot)\Common\src\System\Reflection\TypeNameParser.cs">
       <Link>Compiler\Dataflow\TypeNameParser.cs</Link>

src/coreclr/tools/aot/Mono.Linker.Tests/TestCases/TestDatabase.cs

@@ -34,6 +34,11 @@ public static IEnumerable<object[]> LinkXml()
 			return TestNamesBySuiteName();
 		}
 
+		public static IEnumerable<object[]> Reflection ()
+		{
+			return TestNamesBySuiteName ();
+		}
+
 		public static IEnumerable<object[]> Repro ()
 		{
 			return TestNamesBySuiteName ();

src/coreclr/tools/aot/Mono.Linker.Tests/TestCases/TestSuites.cs

@@ -37,6 +37,20 @@ public void LinkXml (string t)
 			Run (t);
 		}
 
+		[Theory]
+		[MemberData (nameof (TestDatabase.Reflection), MemberType = typeof (TestDatabase))]
+		public void Reflection (string t)
+		{
+			switch (t) {
+			case "TypeHierarchyReflectionWarnings":
+				Run (t);
+				break;
+			default:
+				// Skip the rest for now
+				break;
+			}
+		}
+
 		[Theory]
 		[MemberData (nameof (TestDatabase.Repro), MemberType = typeof (TestDatabase))]
 		public void Repro (string t)

src/coreclr/tools/aot/Mono.Linker.Tests/TestCasesRunner/AssemblyQualifiedToken.cs

@@ -9,6 +9,7 @@
 using Internal.TypeSystem.Ecma;
 using Internal.TypeSystem;
 using Mono.Cecil;
+using MetadataType = Internal.TypeSystem.MetadataType;
 
 namespace Mono.Linker.Tests.TestCasesRunner
 {
@@ -27,6 +28,7 @@ public AssemblyQualifiedToken (TypeSystemEntity entity) =>
 				PropertyPseudoDesc property => (((EcmaType) property.OwningType).Module.Assembly.GetName ().Name, MetadataTokens.GetToken (property.Handle)),
 				EventPseudoDesc @event => (((EcmaType) @event.OwningType).Module.Assembly.GetName ().Name, MetadataTokens.GetToken (@event.Handle)),
 				ILStubMethod => (null, 0), // Ignore compiler generated methods
+				MetadataType mt when mt.GetType().Name == "BoxedValueType" => (null, 0),
 				_ => throw new NotSupportedException ($"The infra doesn't support getting a token for {entity} yet.")
 			};
 

src/coreclr/tools/aot/Mono.Linker.Tests/TestCasesRunner/ResultChecker.cs

@@ -80,7 +80,7 @@ public virtual void Check (ILCompilerTestCaseResult testResult)
 					PerformOutputAssemblyChecks (original, testResult);
 					PerformOutputSymbolChecks (original, testResult);
 
-					if (!HasAttribute (original.MainModule.GetType (testResult.TestCase.ReconstructedFullTypeName), nameof (SkipKeptItemsValidationAttribute))) {
+					if (!HasActiveSkipKeptItemsValidationAttribute (original.MainModule.GetType (testResult.TestCase.ReconstructedFullTypeName))) {
 						CreateAssemblyChecker (original, testResult).Verify ();
 					}
 				}
@@ -89,6 +89,16 @@ public virtual void Check (ILCompilerTestCaseResult testResult)
 			} finally {
 				_originalsResolver.Dispose ();
 			}
+
+			bool HasActiveSkipKeptItemsValidationAttribute(ICustomAttributeProvider provider)
+			{
+				if (TryGetCustomAttribute(provider, nameof(SkipKeptItemsValidationAttribute), out var attribute)) {
+					object? keptBy = attribute.GetPropertyValue (nameof (SkipKeptItemsValidationAttribute.By));
+					return keptBy is null ? true : ((Tool) keptBy).HasFlag (Tool.NativeAot);
+				}
+
+				return false;
+			}
 		}
 
 		protected virtual AssemblyChecker CreateAssemblyChecker (AssemblyDefinition original, ILCompilerTestCaseResult testResult)
@@ -316,8 +326,8 @@ private void VerifyLoggedMessages (AssemblyDefinition original, TestLogWriter lo
 											loggedMessages.Remove (loggedMessage);
 											break;
 										}
-										if (methodDesc.Name == ".ctor" &&
-											methodDesc.OwningType.ToString () == expectedMember.FullName) {
+										if (methodDesc.IsConstructor &&
+											new AssemblyQualifiedToken (methodDesc.OwningType).Equals(new AssemblyQualifiedToken (expectedMember))) {
 											expectedWarningFound = true;
 											loggedMessages.Remove (loggedMessage);
 											break;
@@ -400,10 +410,13 @@ static bool LogMessageHasSameOriginMember (MessageContainer mc, ICustomAttribute
 				Debug.Assert (origin != null);
 				if (origin?.MemberDefinition == null)
 					return false;
+				if (origin?.MemberDefinition is IAssemblyDesc asm)
+					return expectedOriginProvider is AssemblyDefinition expectedAsm && asm.GetName().Name == expectedAsm.Name.Name;
+
 				if (expectedOriginProvider is not IMemberDefinition expectedOriginMember)
 					return false;
 
-				var actualOriginToken = new AssemblyQualifiedToken (origin.Value.MemberDefinition);
+				var actualOriginToken = new AssemblyQualifiedToken (origin!.Value.MemberDefinition);
 				var expectedOriginToken = new AssemblyQualifiedToken (expectedOriginMember);
 				if (actualOriginToken.Equals (expectedOriginToken))
 					return true;

src/tools/illink/test/Mono.Linker.Tests.Cases.Expectations/Assertions/SkipKeptItemsValidationAttribute.cs

@@ -9,5 +9,7 @@ namespace Mono.Linker.Tests.Cases.Expectations.Assertions
 	public class SkipKeptItemsValidationAttribute : BaseExpectedLinkedBehaviorAttribute
 	{
 		public SkipKeptItemsValidationAttribute () { }
+
+		public Tool By { get; set; } = Tool.TrimmerAnalyzerAndNativeAot;
 	}
 }