Warn when accessing DAM annotated method via reflection (#2145)

* Warn when accessing DAM annotated method via reflection

Any access to DAM annotated method must be validated by the trimmer. Normal calls and such are handled already, but direct reflection access or other indirect accesses can lead to trimmer allowing potential invocation of DAM annotated method without validating the annotations. This change will warn whenever such potential "leak" happens.

This applies to method parameter, method return value and field annotations.
Uses the same infra as RUC already just adds additional checks.

Tests for the new cases.

* Formatting

* Only warn on virtual methods with annotate return value

Annotated return value is mostly not problematic (the caller only "Reads" from it, which is always safe). The only case where it's problematic is if something would override the method at runtime (ref emit) in which case the trimmer can't validate the new code that it fulfills the return value requirements. But that can only happen if the method is virtual.

* PR feedback

* PR feedback

* Remove test code.

Author: vitek-karas

docs/error-codes.md

@@ -1689,7 +1689,7 @@ The only scopes supported on global unconditional suppressions are 'module', 'ty
 it is assumed that the suppression is put on the module. Global unconditional suppressions using invalid scopes are ignored.
 
 ```C#
-// Invalid scope 'method' used in 'UnconditionalSuppressMessageAttribute' on module 'Warning' with target 'MyTarget'.
+// IL2108: Invalid scope 'method' used in 'UnconditionalSuppressMessageAttribute' on module 'Warning' with target 'MyTarget'.
 [module: UnconditionalSuppressMessage ("Test suppression with invalid scope", "IL2026", Scope = "method", Target = "MyTarget")]
 
 class Warning
@@ -1722,6 +1722,37 @@ class Warning
   class Derived : UnsafeClass {}
   ```
 
+#### `IL2110`: Trim analysis: Field 'field' with 'DynamicallyAccessedMembersAttribute' is accessed via reflection. Trimmer can't guarantee availability of the requirements of the field.
+
+- Trimmer currently can't guarantee that all requirements of the `DynamicallyAccessedMembersAttribute` are fulfilled if the field is accessed via reflection.
+
+```C#
+[DynamicallyAccessedMembers(DynamicallyAccessedMemeberTypes.PublicMethods)]
+Type _field;
+
+void TestMethod()
+{
+    // IL2110: Field '_field' with 'DynamicallyAccessedMembersAttribute' is accessed via reflection. Trimmer can't guarantee availability of the requirements of the field.
+    typeof(Test).GetField("_field");
+}
+```
+
+#### `IL2111`: Trim analysis: Method 'method' with parameters or return value with `DynamicallyAccessedMembersAttribute` is accessed via reflection. Trimmer can't guarantee availability of the requirements of the field.
+
+- Trimmer currently can't guarantee that all requirements of the `DynamicallyAccessedMembersAttribute` are fulfilled if the method is accessed via reflection.
+
+```C#
+void MethodWithRequirements([DynamicallyAccessedMembers(DynamicallyAccessedMemeberTypes.PublicMethods)] Type type)
+{
+}
+
+void TestMethod()
+{
+    // IL2111: Method 'MethodWithRequirements' with parameters or return value with `DynamicallyAccessedMembersAttribute` is accessed via reflection. Trimmer can't guarantee availability of the requirements of the field.
+    typeof(Test).GetMethod("MethodWithRequirements");
+}
+```
+
 ## Single-File Warning Codes
 
 #### `IL3000`: 'member' always returns an empty string for assemblies embedded in a single-file app. If the path to the app directory is needed, consider calling 'System.AppContext.BaseDirectory'

src/linker/Linker.Dataflow/FlowAnnotations.cs

@@ -23,20 +23,14 @@ public FlowAnnotations (LinkContext context)
 			_hierarchyInfo = new TypeHierarchyCache (context);
 		}
 
-		public bool RequiresDataFlowAnalysis (MethodDefinition method)
-		{
-			return GetAnnotations (method.DeclaringType).TryGetAnnotation (method, out _);
-		}
+		public bool RequiresDataFlowAnalysis (MethodDefinition method) =>
+			GetAnnotations (method.DeclaringType).TryGetAnnotation (method, out _);
 
-		public bool RequiresDataFlowAnalysis (FieldDefinition field)
-		{
-			return GetAnnotations (field.DeclaringType).TryGetAnnotation (field, out _);
-		}
+		public bool RequiresDataFlowAnalysis (FieldDefinition field) =>
+			GetAnnotations (field.DeclaringType).TryGetAnnotation (field, out _);
 
-		public bool RequiresDataFlowAnalysis (GenericParameter genericParameter)
-		{
-			return GetGenericParameterAnnotation (genericParameter) != DynamicallyAccessedMemberTypes.None;
-		}
+		public bool RequiresDataFlowAnalysis (GenericParameter genericParameter) =>
+			GetGenericParameterAnnotation (genericParameter) != DynamicallyAccessedMemberTypes.None;
 
 		/// <summary>
 		/// Retrieves the annotations for the given parameter.
@@ -45,35 +39,31 @@ public bool RequiresDataFlowAnalysis (GenericParameter genericParameter)
 		/// <returns></returns>
 		public DynamicallyAccessedMemberTypes GetParameterAnnotation (MethodDefinition method, int parameterIndex)
 		{
-			if (GetAnnotations (method.DeclaringType).TryGetAnnotation (method, out var annotation) && annotation.ParameterAnnotations != null) {
+			if (GetAnnotations (method.DeclaringType).TryGetAnnotation (method, out var annotation) &&
+				annotation.ParameterAnnotations != null)
 				return annotation.ParameterAnnotations[parameterIndex];
-			}
 
 			return DynamicallyAccessedMemberTypes.None;
 		}
 
 		public DynamicallyAccessedMemberTypes GetReturnParameterAnnotation (MethodDefinition method)
 		{
-			if (GetAnnotations (method.DeclaringType).TryGetAnnotation (method, out var annotation)) {
+			if (GetAnnotations (method.DeclaringType).TryGetAnnotation (method, out var annotation))
 				return annotation.ReturnParameterAnnotation;
-			}
 
 			return DynamicallyAccessedMemberTypes.None;
 		}
 
 		public DynamicallyAccessedMemberTypes GetFieldAnnotation (FieldDefinition field)
 		{
-			if (GetAnnotations (field.DeclaringType).TryGetAnnotation (field, out var annotation)) {
+			if (GetAnnotations (field.DeclaringType).TryGetAnnotation (field, out var annotation))
 				return annotation.Annotation;
-			}
 
 			return DynamicallyAccessedMemberTypes.None;
 		}
 
-		public DynamicallyAccessedMemberTypes GetTypeAnnotation (TypeDefinition type)
-		{
-			return GetAnnotations (type).TypeAnnotation;
-		}
+		public DynamicallyAccessedMemberTypes GetTypeAnnotation (TypeDefinition type) =>
+			GetAnnotations (type).TypeAnnotation;
 
 		public DynamicallyAccessedMemberTypes GetGenericParameterAnnotation (GenericParameter genericParameter)
 		{
@@ -93,6 +83,17 @@ public DynamicallyAccessedMemberTypes GetGenericParameterAnnotation (GenericPara
 			return DynamicallyAccessedMemberTypes.None;
 		}
 
+		public bool ShouldWarnWhenAccessedForReflection (MethodDefinition method) =>
+			GetAnnotations (method.DeclaringType).TryGetAnnotation (method, out var annotation) &&
+			(annotation.ParameterAnnotations != null || annotation.ReturnParameterAnnotation != DynamicallyAccessedMemberTypes.None);
+
+		public bool MethodHasNoAnnotatedParameters (MethodDefinition method) =>
+			GetAnnotations (method.DeclaringType).TryGetAnnotation (method, out var annotation) &&
+			annotation.ParameterAnnotations == null;
+
+		public bool ShouldWarnWhenAccessedForReflection (FieldDefinition field) =>
+			GetAnnotations (field.DeclaringType).TryGetAnnotation (field, out _);
+
 		TypeAnnotations GetAnnotations (TypeDefinition type)
 		{
 			if (!_annotations.TryGetValue (type, out TypeAnnotations value)) {

src/linker/Linker.Steps/MarkStep.cs

@@ -1553,6 +1553,23 @@ void MarkField (FieldDefinition field, in DependencyInfo reason)
 				Annotations.Mark (field, reason);
 			}
 
+			switch (reason.Kind) {
+			case DependencyKind.AccessedViaReflection:
+			case DependencyKind.DynamicDependency:
+			case DependencyKind.DynamicallyAccessedMember:
+			case DependencyKind.InteropMethodDependency:
+				if (_context.Annotations.FlowAnnotations.ShouldWarnWhenAccessedForReflection (field) &&
+					!ShouldSuppressAnalysisWarningsForRequiresUnreferencedCode ())
+					_context.LogWarning (
+						$"Field '{field.GetDisplayName ()}' with 'DynamicallyAccessedMembersAttribute' is accessed via reflection. Trimmer can't guarantee availability of the requirements of the field.",
+						2110,
+						_scopeStack.CurrentScope.Origin,
+						MessageSubCategory.TrimAnalysis);
+
+				break;
+			}
+
+
 			if (CheckProcessed (field))
 				return;
 
@@ -2705,12 +2722,12 @@ protected virtual MethodDefinition MarkMethod (MethodReference reference, Depend
 
 			// Use the original reason as it's important to correctly generate warnings
 			// the updated reason is only useful for better tracking of dependencies.
-			ProcessRequiresUnreferencedCode (method, originalReasonKind);
+			ProcessAnalysisAnnotationsForMethod (method, originalReasonKind);
 
 			return method;
 		}
 
-		void ProcessRequiresUnreferencedCode (MethodDefinition method, DependencyKind dependencyKind)
+		void ProcessAnalysisAnnotationsForMethod (MethodDefinition method, DependencyKind dependencyKind)
 		{
 			switch (dependencyKind) {
 			// DirectCall, VirtualCall and NewObj are handled by ReflectionMethodBodyScanner
@@ -2767,24 +2784,85 @@ void ProcessRequiresUnreferencedCode (MethodDefinition method, DependencyKind de
 				return;
 
 			default:
-				// DirectCall, VirtualCall and NewObj are handled by ReflectionMethodBodyScanner
-				// This is necessary since the ReflectionMethodBodyScanner has intrinsic handling for some
-				// of the annotated methods annotated (for example Type.GetType)
-				// and it knows when it's OK and when it needs a warning. In this place we don't know
-				// and would have to warn every time
-
 				// All other cases have the potential of us missing a warning if we don't report it
 				// It is possible that in some cases we may report the same warning twice, but that's better than not reporting it.
 				break;
 			}
 
-			// All override methods should have the same annotations as their base methods (else we will produce warning IL2046.)
-			// When marking override methods with RequiresUnreferencedCode on a type annotated with DynamicallyAccessedMembers,
-			// we should only issue a warning for the base method.
-			if (dependencyKind != DependencyKind.DynamicallyAccessedMember ||
-				!method.IsVirtual ||
-				Annotations.GetBaseMethods (method) == null)
-				CheckAndReportRequiresUnreferencedCode (method);
+			// All override methods should have the same annotations as their base methods
+			// (else we will produce warning IL2046 or IL2092 or some other warning).
+			// When marking override methods via DynamicallyAccessedMembers, we should only issue a warning for the base method.
+			if (dependencyKind == DependencyKind.DynamicallyAccessedMember &&
+				method.IsVirtual &&
+				Annotations.GetBaseMethods (method) != null)
+				return;
+
+			CheckAndReportRequiresUnreferencedCode (method);
+
+			if (_context.Annotations.FlowAnnotations.ShouldWarnWhenAccessedForReflection (method)) {
+				// If the current scope has analysis warnings suppressed, don't generate any
+				if (ShouldSuppressAnalysisWarningsForRequiresUnreferencedCode ())
+					return;
+
+				// ReflectionMethodBodyScanner handles more cases for data flow annotations
+				// so don't warn for those.
+				switch (dependencyKind) {
+				case DependencyKind.AttributeConstructor:
+				case DependencyKind.AttributeProperty:
+					return;
+
+				default:
+					break;
+				}
+
+				// If the method only has annotation on the return value and it's not virtual avoid warning.
+				// Return value annotations are "consumed" by the caller of a method, and as such there is nothing
+				// wrong calling these dynamically. The only problem can happen if something overrides a virtual
+				// method with annotated return value at runtime - in this case the trimmer can't validate
+				// that the method will return only types which fulfill the annotation's requirements.
+				// For example:
+				//   class BaseWithAnnotation
+				//   {
+				//       [return: DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicFields)]
+				//       public abstract Type GetTypeWithFields();
+				//   }
+				//
+				//   class UsingTheBase
+				//   {
+				//       public void PrintFields(Base base)
+				//       {
+				//            // No warning here - GetTypeWithFields is correctly annotated to allow GetFields on the return value.
+				//            Console.WriteLine(string.Join(" ", base.GetTypeWithFields().GetFields().Select(f => f.Name)));
+				//       }
+				//   }
+				//
+				// If at runtime (through ref emit) something generates code like this:
+				//   class DerivedAtRuntimeFromBase
+				//   {
+				//       // No point in adding annotation on the return value - nothing will look at it anyway
+				//       // Linker will not see this code, so there are no checks
+				//       public override Type GetTypeWithFields() { return typeof(TestType); }
+				//   }
+				//
+				// If TestType from above is trimmed, it may note have all its fields, and there would be no warnings generated.
+				// But there has to be code like this somewhere in the app, in order to generate the override:
+				//   class RuntimeTypeGenerator
+				//   {
+				//       public MethodInfo GetBaseMethod()
+				//       {
+				//            // This must warn - that the GetTypeWithFields has annotation on the return value
+				//            return typeof(BaseWithAnnotation).GetMethod("GetTypeWithFields");
+				//       }
+				//   }
+				if (!method.IsVirtual && _context.Annotations.FlowAnnotations.MethodHasNoAnnotatedParameters (method))
+					return;
+
+				_context.LogWarning (
+					$"Method '{method.GetDisplayName ()}' with parameters or return value with `DynamicallyAccessedMembersAttribute` is accessed via reflection. Trimmer can't guarantee availability of the requirements of the field.",
+					2111,
+					_scopeStack.CurrentScope.Origin,
+					MessageSubCategory.TrimAnalysis);
+			}
 		}
 
 		internal bool ShouldSuppressAnalysisWarningsForRequiresUnreferencedCode ()