Move parameter annotation checks to shared code for analyzer (#2707)

* Move parameter annotation checks to shared code for analyzer

Add GetMethodThisParameterValue
Make GetParameterAnnotations return a nonnull
DynamicallyAccessedMemberTypes

* Use the shared code for parameter annotation checks in the linker

* Remove redundant handling from analyzer

* Use ReturnsVoid() extension method

* Fall back to annotations for unimplemented intrinsics in analyzer

10 files changed, 111 insertions, 56 deletions

diff --git a/src/ILLink.RoslynAnalyzer/TrimAnalysis/HandleCallAction.cs b/src/ILLink.RoslynAnalyzer/TrimAnalysis/HandleCallAction.cs
index 0a4a9b62a..d46481268 100644
--- a/src/ILLink.RoslynAnalyzer/TrimAnalysis/HandleCallAction.cs
+++ b/src/ILLink.RoslynAnalyzer/TrimAnalysis/HandleCallAction.cs
@@ -47,6 +47,15 @@ namespace ILLink.Shared.TrimAnalysis
 		private partial MethodThisParameterValue GetMethodThisParameterValue (MethodProxy method, DynamicallyAccessedMemberTypes dynamicallyAccessedMemberTypes)
 			=> new (method.Method, dynamicallyAccessedMemberTypes);
 
+		private partial MethodThisParameterValue GetMethodThisParameterValue (MethodProxy method)
+			=> GetMethodThisParameterValue (method, method.Method.GetDynamicallyAccessedMemberTypes ());
+
+		private partial DynamicallyAccessedMemberTypes GetMethodParameterAnnotation (MethodProxy method, int parameterIndex)
+		{
+			Debug.Assert (method.Method.Parameters.Length > parameterIndex);
+			return FlowAnnotations.GetMethodParameterAnnotation (method.Method.Parameters[parameterIndex]);
+		}
+
 		private partial MethodParameterValue GetMethodParameterValue (MethodProxy method, int parameterIndex, DynamicallyAccessedMemberTypes dynamicallyAccessedMemberTypes)
 			=> new (method.Method.Parameters[parameterIndex], dynamicallyAccessedMemberTypes);
 
@@ -62,6 +71,19 @@ namespace ILLink.Shared.TrimAnalysis
 				yield return new SystemTypeValue (new TypeProxy (nestedType));
 		}
 
+		private partial bool MethodIsTypeConstructor (MethodProxy method)
+		{
+			if (!method.Method.IsConstructor ())
+				return false;
+			var type = method.Method.ContainingType;
+			while (type is not null) {
+				if (type.IsTypeOf (WellKnownType.System_Type))
+					return true;
+				type = type.BaseType;
+			}
+			return false;
+		}
+
 		private partial bool TryGetBaseType (TypeProxy type, out TypeProxy? baseType)
 		{
 			if (type.Type.BaseType is not null) {
diff --git a/src/ILLink.RoslynAnalyzer/TrimAnalysis/TrimAnalysisVisitor.cs b/src/ILLink.RoslynAnalyzer/TrimAnalysis/TrimAnalysisVisitor.cs
index 4e3cda618..13fc5ca6c 100644
--- a/src/ILLink.RoslynAnalyzer/TrimAnalysis/TrimAnalysisVisitor.cs
+++ b/src/ILLink.RoslynAnalyzer/TrimAnalysis/TrimAnalysisVisitor.cs
@@ -241,6 +241,7 @@ namespace ILLink.RoslynAnalyzer.TrimAnalysis
 
 			var diagnosticContext = DiagnosticContext.CreateDisabled ();
 			var handleCallAction = new HandleCallAction (diagnosticContext, Context.OwningSymbol, operation);
+
 			if (!handleCallAction.Invoke (new MethodProxy (calledMethod), instance, arguments, out MultiValue methodReturnValue, out var intrinsicId)) {
 				switch (intrinsicId) {
 				case IntrinsicId.Array_Empty:
diff --git a/src/ILLink.Shared/TrimAnalysis/HandleCallAction.cs b/src/ILLink.Shared/TrimAnalysis/HandleCallAction.cs
index 28e79650d..a7f422292 100644
--- a/src/ILLink.Shared/TrimAnalysis/HandleCallAction.cs
+++ b/src/ILLink.Shared/TrimAnalysis/HandleCallAction.cs
@@ -787,8 +787,16 @@ namespace ILLink.Shared.TrimAnalysis
 				break;
 
 			case IntrinsicId.None:
-				methodReturnValue = MultiValueLattice.Top;
-				return false;
+				// Verify the argument values match the annotations on the parameter definition
+				if (requiresDataFlowAnalysis) {
+					if (!calledMethod.IsStatic ()) {
+						_requireDynamicallyAccessedMembersAction.Invoke (instanceValue, GetMethodThisParameterValue (calledMethod));
+					}
+					for (int argumentIndex = 0; argumentIndex < argumentValues.Count; argumentIndex++) {
+						_requireDynamicallyAccessedMembersAction.Invoke (argumentValues[argumentIndex], GetMethodParameterValue (calledMethod, argumentIndex));
+					}
+				}
+				break;
 
 			// Disable warnings for all unimplemented intrinsics. Some intrinsic methods have annotations, but analyzing them
 			// would produce unnecessary warnings even for cases that are intrinsically handled. So we disable handling these calls
@@ -802,6 +810,9 @@ namespace ILLink.Shared.TrimAnalysis
 			// Note that this will be DynamicallyAccessedMembers.None for the intrinsics which don't return types.
 			returnValue ??= calledMethod.ReturnsVoid () ? MultiValueLattice.Top : GetMethodReturnValue (calledMethod, returnValueDynamicallyAccessedMemberTypes);
 
+			if (MethodIsTypeConstructor (calledMethod))
+				returnValue = UnknownValue.Instance;
+
 			// Validate that the return value has the correct annotations as per the method return value annotations
 			if (returnValueDynamicallyAccessedMemberTypes != 0) {
 				foreach (var uniqueValue in returnValue.Value) {
@@ -918,6 +929,11 @@ namespace ILLink.Shared.TrimAnalysis
 
 		private partial bool MethodRequiresDataFlowAnalysis (MethodProxy method);
 
+		/// <Summary>
+		/// Returns true if the method is a .ctor for System.Type or a type that derives from System.Type (i.e. fields and params of this type can have DynamicallyAccessedMembers annotations)
+		/// </Summary>
+		private partial bool MethodIsTypeConstructor (MethodProxy method);
+
 		private partial DynamicallyAccessedMemberTypes GetReturnValueAnnotation (MethodProxy method);
 
 		private partial MethodReturnValue GetMethodReturnValue (MethodProxy method, DynamicallyAccessedMemberTypes dynamicallyAccessedMemberTypes);
@@ -926,8 +942,15 @@ namespace ILLink.Shared.TrimAnalysis
 
 		private partial MethodThisParameterValue GetMethodThisParameterValue (MethodProxy method, DynamicallyAccessedMemberTypes dynamicallyAccessedMemberTypes);
 
+		private partial MethodThisParameterValue GetMethodThisParameterValue (MethodProxy method);
+
+		private partial DynamicallyAccessedMemberTypes GetMethodParameterAnnotation (MethodProxy method, int parameterIndex);
+
 		private partial MethodParameterValue GetMethodParameterValue (MethodProxy method, int parameterIndex, DynamicallyAccessedMemberTypes dynamicallyAccessedMemberTypes);
 
+		private MethodParameterValue GetMethodParameterValue (MethodProxy method, int parameterIndex)
+			=> GetMethodParameterValue (method, parameterIndex, GetMethodParameterAnnotation (method, parameterIndex)!);
+
 		private partial IEnumerable<SystemReflectionMethodBaseValue> GetMethodsOnTypeHierarchy (TypeProxy type, string name, BindingFlags? bindingFlags);
 
 		private partial IEnumerable<SystemTypeValue> GetNestedTypesOnType (TypeProxy type, string name, BindingFlags? bindingFlags);
diff --git a/src/linker/Linker.Dataflow/HandleCallAction.cs b/src/linker/Linker.Dataflow/HandleCallAction.cs
index f662cdf0b..ba3f56dc8 100644
--- a/src/linker/Linker.Dataflow/HandleCallAction.cs
+++ b/src/linker/Linker.Dataflow/HandleCallAction.cs
@@ -2,6 +2,7 @@
 // Licensed under the MIT license. See LICENSE file in the project root for full license information.
 
 using System.Collections.Generic;
+using System.Diagnostics;
 using System.Diagnostics.CodeAnalysis;
 using System.Reflection;
 using ILLink.Shared.TypeSystemProxy;
@@ -37,6 +38,19 @@ namespace ILLink.Shared.TrimAnalysis
 		private partial bool MethodRequiresDataFlowAnalysis (MethodProxy method)
 			=> _context.Annotations.FlowAnnotations.RequiresDataFlowAnalysis (method.Method);
 
+		private partial bool MethodIsTypeConstructor (MethodProxy method)
+		{
+			if (!method.Method.IsConstructor)
+				return false;
+			TypeDefinition? type = method.Method.DeclaringType;
+			while (type is not null) {
+				if (type.IsTypeOf (WellKnownType.System_Type))
+					return true;
+				type = _context.Resolve (type.BaseType);
+			}
+			return false;
+		}
+
 		private partial DynamicallyAccessedMemberTypes GetReturnValueAnnotation (MethodProxy method)
 			=> _context.Annotations.FlowAnnotations.GetReturnParameterAnnotation (method.Method);
 
@@ -46,9 +60,18 @@ namespace ILLink.Shared.TrimAnalysis
 		private partial GenericParameterValue GetGenericParameterValue (GenericParameterProxy genericParameter)
 			=> new (genericParameter.GenericParameter, _context.Annotations.FlowAnnotations.GetGenericParameterAnnotation (genericParameter.GenericParameter));
 
+		private partial MethodThisParameterValue GetMethodThisParameterValue (MethodProxy method)
+			=> GetMethodThisParameterValue (method, _context.Annotations.FlowAnnotations.GetParameterAnnotation (method.Method, 0));
+
 		private partial MethodThisParameterValue GetMethodThisParameterValue (MethodProxy method, DynamicallyAccessedMemberTypes dynamicallyAccessedMemberTypes)
 			=> new (method.Method, dynamicallyAccessedMemberTypes);
 
+		private partial DynamicallyAccessedMemberTypes GetMethodParameterAnnotation (MethodProxy method, int parameterIndex)
+		{
+			Debug.Assert (method.Method.Parameters.Count > parameterIndex);
+			return _context.Annotations.FlowAnnotations.GetParameterAnnotation (method.Method, parameterIndex + (method.IsStatic () ? 0 : 1));
+		}
+
 		private partial MethodParameterValue GetMethodParameterValue (MethodProxy method, int parameterIndex, DynamicallyAccessedMemberTypes dynamicallyAccessedMemberTypes)
 			=> new (
 				ReflectionMethodBodyScanner.ResolveToTypeDefinition (_context, method.Method.Parameters[parameterIndex].ParameterType),
diff --git a/src/linker/Linker.Dataflow/MethodBodyScanner.cs b/src/linker/Linker.Dataflow/MethodBodyScanner.cs
index aab50423a..4a9e5e2f5 100644
--- a/src/linker/Linker.Dataflow/MethodBodyScanner.cs
+++ b/src/linker/Linker.Dataflow/MethodBodyScanner.cs
@@ -537,7 +537,7 @@ namespace Mono.Linker.Dataflow
 						// Pop function pointer
 						PopUnknown (currentStack, 1, methodBody, operation.Offset);
 
-						if (GetReturnTypeWithoutModifiers (signature.ReturnType).MetadataType != MetadataType.Void)
+						if (!signature.ReturnsVoid ())
 							PushUnknown (currentStack);
 					}
 					break;
@@ -573,7 +573,7 @@ namespace Mono.Linker.Dataflow
 
 				case Code.Ret: {
 
-						bool hasReturnValue = GetReturnTypeWithoutModifiers (methodBody.Method.ReturnType).MetadataType != MetadataType.Void;
+						bool hasReturnValue = !methodBody.Method.ReturnsVoid ();
 
 						if (currentStack.Count != (hasReturnValue ? 1 : 0)) {
 							WarnAboutInvalidILInMethod (methodBody, operation.Offset);
@@ -917,13 +917,13 @@ namespace Mono.Linker.Dataflow
 					else
 						methodReturnValue = newObjValue;
 				} else {
-					if (GetReturnTypeWithoutModifiers (calledMethod.ReturnType).MetadataType != MetadataType.Void) {
+					if (!calledMethod.ReturnsVoid ()) {
 						methodReturnValue = UnknownValue.Instance;
 					}
 				}
 			}
 
-			if (isNewObj || GetReturnTypeWithoutModifiers (calledMethod.ReturnType).MetadataType != MetadataType.Void)
+			if (isNewObj || !calledMethod.ReturnsVoid ())
 				currentStack.Push (new StackSlot (methodReturnValue, calledMethod.ReturnType.IsByRefOrPointer ()));
 
 			foreach (var param in methodParams) {
@@ -935,14 +935,6 @@ namespace Mono.Linker.Dataflow
 			}
 		}
 
-		protected static TypeReference GetReturnTypeWithoutModifiers (TypeReference returnType)
-		{
-			while (returnType is IModifierType) {
-				returnType = ((IModifierType) returnType).ElementType;
-			}
-			return returnType;
-		}
-
 		// Array types that are dynamically accessed should resolve to System.Array instead of its element type - which is what Cecil resolves to.
 		// Any data flow annotations placed on a type parameter which receives an array type apply to the array itself. None of the members in its
 		// element type should be marked.
diff --git a/src/linker/Linker.Dataflow/MethodProxy.cs b/src/linker/Linker.Dataflow/MethodProxy.cs
index a9b95ccf3..eacdf27a8 100644
--- a/src/linker/Linker.Dataflow/MethodProxy.cs
+++ b/src/linker/Linker.Dataflow/MethodProxy.cs
@@ -32,7 +32,7 @@ namespace ILLink.Shared.TypeSystemProxy
 
 		internal partial bool IsStatic () => Method.IsStatic;
 
-		internal partial bool ReturnsVoid () => Method.ReturnType.MetadataType == MetadataType.Void;
+		internal partial bool ReturnsVoid () => Method.ReturnsVoid ();
 
 		public override string ToString () => Method.ToString ();
 	}
diff --git a/src/linker/Linker.Dataflow/ReflectionMethodBodyScanner.cs b/src/linker/Linker.Dataflow/ReflectionMethodBodyScanner.cs
index 440e1f562..e9e69fe47 100644
--- a/src/linker/Linker.Dataflow/ReflectionMethodBodyScanner.cs
+++ b/src/linker/Linker.Dataflow/ReflectionMethodBodyScanner.cs
@@ -70,7 +70,7 @@ namespace Mono.Linker.Dataflow
 		{
 			Scan (methodBody);
 
-			if (GetReturnTypeWithoutModifiers (methodBody.Method.ReturnType).MetadataType != MetadataType.Void) {
+			if (!methodBody.Method.ReturnsVoid ()) {
 				var method = methodBody.Method;
 				var methodReturnValue = GetMethodReturnValue (method);
 				if (methodReturnValue.DynamicallyAccessedMemberTypes != 0) {
@@ -311,6 +311,29 @@ namespace Mono.Linker.Dataflow
 					return handleCallAction.Invoke (calledMethodDefinition, instanceValue, parameterValues, out methodReturnValue, out _);
 				}
 
+			case IntrinsicId.None: {
+					if (calledMethodDefinition.IsPInvokeImpl) {
+						// Is the PInvoke dangerous?
+						bool comDangerousMethod = IsComInterop (calledMethodDefinition.MethodReturnType, calledMethodDefinition.ReturnType);
+						foreach (ParameterDefinition pd in calledMethodDefinition.Parameters) {
+							comDangerousMethod |= IsComInterop (pd, pd.ParameterType);
+						}
+
+						if (comDangerousMethod) {
+							analysisContext.ReportWarning (DiagnosticId.CorrectnessOfCOMCannotBeGuaranteed, calledMethodDefinition.GetDisplayName ());
+						}
+					}
+					_markStep.CheckAndReportRequiresUnreferencedCode (calledMethodDefinition);
+
+					var instanceValue = MultiValueLattice.Top;
+					IReadOnlyList<MultiValue> parameterValues = methodParams;
+					if (calledMethodDefinition.HasImplicitThis ()) {
+						instanceValue = methodParams[0];
+						parameterValues = parameterValues.Skip (1).ToImmutableList ();
+					}
+					return handleCallAction.Invoke (calledMethodDefinition, instanceValue, parameterValues, out methodReturnValue, out _);
+				}
+
 			case IntrinsicId.TypeDelegator_Ctor: {
 					// This is an identity function for analysis purposes
 					if (operation.OpCode == OpCodes.Newobj)
@@ -757,46 +780,13 @@ namespace Mono.Linker.Dataflow
 				break;
 
 			default:
-
-				if (calledMethodDefinition.IsPInvokeImpl) {
-					// Is the PInvoke dangerous?
-					bool comDangerousMethod = IsComInterop (calledMethodDefinition.MethodReturnType, calledMethodDefinition.ReturnType);
-					foreach (ParameterDefinition pd in calledMethodDefinition.Parameters) {
-						comDangerousMethod |= IsComInterop (pd, pd.ParameterType);
-					}
-
-					if (comDangerousMethod) {
-						analysisContext.ReportWarning (DiagnosticId.CorrectnessOfCOMCannotBeGuaranteed, calledMethodDefinition.GetDisplayName ());
-					}
-				}
-
-				if (requiresDataFlowAnalysis) {
-					for (int parameterIndex = 0; parameterIndex < methodParams.Count; parameterIndex++) {
-						var targetValue = GetMethodParameterValue (calledMethodDefinition, parameterIndex);
-
-						if (targetValue.DynamicallyAccessedMemberTypes != DynamicallyAccessedMemberTypes.None) {
-							RequireDynamicallyAccessedMembers (analysisContext, methodParams[parameterIndex], targetValue);
-						}
-					}
-				}
-
-				_markStep.CheckAndReportRequiresUnreferencedCode (calledMethodDefinition);
-
-				// To get good reporting of errors we need to track the origin of the value for all method calls
-				// but except Newobj as those are special.
-				if (GetReturnTypeWithoutModifiers (calledMethodDefinition.ReturnType).MetadataType != MetadataType.Void) {
-					methodReturnValue = GetMethodReturnValue (calledMethodDefinition, returnValueDynamicallyAccessedMemberTypes);
-
-					return true;
-				}
-
-				return false;
+				throw new NotImplementedException ("Unhandled instrinsic");
 			}
 
 			// If we get here, we handled this as an intrinsic.  As a convenience, if the code above
 			// didn't set the return value (and the method has a return value), we will set it to be an
 			// unknown value with the return type of the method.
-			bool returnsVoid = GetReturnTypeWithoutModifiers (calledMethod.ReturnType).MetadataType == MetadataType.Void;
+			bool returnsVoid = calledMethod.ReturnsVoid ();
 			methodReturnValue = maybeMethodReturnValue ?? (returnsVoid ?
 				MultiValueLattice.Top :
 				GetMethodReturnValue (calledMethodDefinition, returnValueDynamicallyAccessedMemberTypes));
diff --git a/src/linker/Linker/MethodReferenceExtensions.cs b/src/linker/Linker/MethodReferenceExtensions.cs
index d107ddf0a..be378a015 100644
--- a/src/linker/Linker/MethodReferenceExtensions.cs
+++ b/src/linker/Linker/MethodReferenceExtensions.cs
@@ -74,6 +74,11 @@ namespace Mono.Linker
 			return method.ReturnType;
 		}
 
+		public static bool ReturnsVoid (this IMethodSignature method)
+		{
+			return method.ReturnType.WithoutModifiers ().MetadataType == MetadataType.Void;
+		}
+
 		public static TypeReference? GetParameterType (this MethodReference method, int parameterIndex, LinkContext context)
 		{
 			if (method.DeclaringType is GenericInstanceType genericInstance)
diff --git a/test/Mono.Linker.Tests.Cases/DataFlow/LocalDataFlow.cs b/test/Mono.Linker.Tests.Cases/DataFlow/LocalDataFlow.cs
index f3da00afe..1f6347d3b 100644
--- a/test/Mono.Linker.Tests.Cases/DataFlow/LocalDataFlow.cs
+++ b/test/Mono.Linker.Tests.Cases/DataFlow/LocalDataFlow.cs
@@ -399,6 +399,7 @@ namespace Mono.Linker.Tests.Cases.DataFlow
 		// https://github.com/dotnet/linker/issues/2273
 		// Analyzer doesn't see through foreach over array at all -  will not warn
 		[ExpectedWarning ("IL2063", ProducedBy = ProducedBy.Trimmer)] // The types loaded from the array don't have annotations, so the "return" should warn
+		[ExpectedWarning ("IL2073", ProducedBy = ProducedBy.Analyzer)] // Analyzer tracks resultType as the value from IEnumerable.Current.get()
 		[return: DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)]
 		public static Type TestBackwardEdgeWithLdElem (Type[] types = null)
 		{
diff --git a/test/Mono.Linker.Tests.Cases/DataFlow/TypeBaseTypeDataFlow.cs b/test/Mono.Linker.Tests.Cases/DataFlow/TypeBaseTypeDataFlow.cs
index a9174ecf5..2edf35569 100644
--- a/test/Mono.Linker.Tests.Cases/DataFlow/TypeBaseTypeDataFlow.cs
+++ b/test/Mono.Linker.Tests.Cases/DataFlow/TypeBaseTypeDataFlow.cs
@@ -70,11 +70,9 @@ namespace Mono.Linker.Tests.Cases.DataFlow
 				derivedType.BaseType.RequiresAll ();
 			}
 
-			// This is a very special case - normally there's basically no way to "new up" a Type instance via the "new" operator
-			// so the analyzer doesn't handle this case at all - meaning it sees it as empty value.
-			// Unlike linker which sees an unknown value and thus warns that it doesn't fulfill the All annotation.
-			// It's OK for the analyzer to be more forgiving in this case, due to the very special circumstances.
-			[ExpectedWarning ("IL2062", nameof (TestAllPropagated), ProducedBy = ProducedBy.Trimmer)]
+			// This is a very special case - normally there's basically no way to "new up" a Type instance via the "new" operator.
+			// The linker and analyzer see an unknown value and thus warns that it doesn't fulfill the All annotation.
+			[ExpectedWarning ("IL2062", nameof (TestAllPropagated))]
 			public static void Test ()
 			{
 				TestAllPropagated (new TestSystemTypeBase ());