Expression.Call can act as MakeGenericMethod so make it perform same validation (#1930)

Also improved validation done by MakeGenericMethod to avoid warning if no type arguments are passed in.
Added tests.

3 files changed, 162 insertions, 26 deletions

diff --git a/src/linker/Linker.Dataflow/ReflectionMethodBodyScanner.cs b/src/linker/Linker.Dataflow/ReflectionMethodBodyScanner.cs
index e0b9133c1..da95cb0bd 100644
--- a/src/linker/Linker.Dataflow/ReflectionMethodBodyScanner.cs
+++ b/src/linker/Linker.Dataflow/ReflectionMethodBodyScanner.cs
@@ -795,6 +795,7 @@ namespace Mono.Linker.Dataflow
 						}
 					}
 					break;
+
 				//
 				// System.Linq.Expressions.Expression
 				// 
@@ -804,15 +805,26 @@ namespace Mono.Linker.Dataflow
 						reflectionContext.AnalyzingPattern ();
 						BindingFlags bindingFlags = BindingFlags.Static | BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.FlattenHierarchy;
 
+						bool hasTypeArguments = (methodParams[2] as ArrayValue)?.Size.AsConstInt () != 0;
 						foreach (var value in methodParams[0].UniqueValues ()) {
 							if (value is SystemTypeValue systemTypeValue) {
 								foreach (var stringParam in methodParams[1].UniqueValues ()) {
-									// TODO: Change this as needed after deciding whether or not we are to keep
-									// all methods on a type that was accessed via reflection.
 									if (stringParam is KnownStringValue stringValue) {
-										MarkMethodsOnTypeHierarchy (ref reflectionContext, systemTypeValue.TypeRepresented, m => m.Name == stringValue.Contents, bindingFlags);
+										foreach (var method in systemTypeValue.TypeRepresented.GetMethodsOnTypeHierarchy (m => m.Name == stringValue.Contents, bindingFlags)) {
+											ValidateGenericMethodInstantiation (ref reflectionContext, method, calledMethod);
+											MarkMethod (ref reflectionContext, method);
+										}
+
 										reflectionContext.RecordHandledPattern ();
 									} else {
+										if (hasTypeArguments) {
+											// We don't know what method the `MakeGenericMethod` was called on, so we have to assume
+											// that the method may have requirements which we can't fullfil -> warn.
+											reflectionContext.RecordUnrecognizedPattern (
+												2060, string.Format (Resources.Strings.IL2060,
+													DiagnosticUtilities.GetMethodSignatureDisplayName (calledMethod)));
+										}
+
 										RequireDynamicallyAccessedMembers (
 											ref reflectionContext,
 											GetDynamicallyAccessedMemberTypesFromBindingFlagsForMethods (bindingFlags),
@@ -821,6 +833,14 @@ namespace Mono.Linker.Dataflow
 									}
 								}
 							} else {
+								if (hasTypeArguments) {
+									// We don't know what method the `MakeGenericMethod` was called on, so we have to assume
+									// that the method may have requirements which we can't fullfil -> warn.
+									reflectionContext.RecordUnrecognizedPattern (
+										2060, string.Format (Resources.Strings.IL2060,
+											DiagnosticUtilities.GetMethodSignatureDisplayName (calledMethod)));
+								}
+
 								RequireDynamicallyAccessedMembers (
 									ref reflectionContext,
 									GetDynamicallyAccessedMemberTypesFromBindingFlagsForMethods (bindingFlags),
@@ -1612,24 +1632,13 @@ namespace Mono.Linker.Dataflow
 
 						foreach (var methodValue in methodParams[0].UniqueValues ()) {
 							if (methodValue is SystemReflectionMethodBaseValue methodBaseValue) {
-								foreach (var genericParameter in methodBaseValue.MethodRepresented.GenericParameters) {
-									if (_context.Annotations.FlowAnnotations.GetGenericParameterAnnotation (genericParameter) != DynamicallyAccessedMemberTypes.None ||
-										genericParameter.HasDefaultConstructorConstraint) {
-										// There is a generic parameter which has some requirements on input types.
-										// For now we don't support tracking actual array elements, so we can't validate that the requirements are fulfilled.
-										reflectionContext.RecordUnrecognizedPattern (
-											2060, string.Format (Resources.Strings.IL2060,
-												DiagnosticUtilities.GetMethodSignatureDisplayName (calledMethod)));
-									}
-								}
-
-								// We haven't found any generic parameters with annotations, so there's nothing to validate
+								ValidateGenericMethodInstantiation (ref reflectionContext, methodBaseValue.MethodRepresented, calledMethod);
 								reflectionContext.RecordHandledPattern ();
 							} else if (methodValue == NullValue.Instance) {
 								reflectionContext.RecordHandledPattern ();
 							} else {
-								// There is a generic parameter which has some requirements on input types.
-								// For now we don't support tracking actual array elements, so we can't validate that the requirements are fulfilled.
+								// We don't know what method the `MakeGenericMethod` was called on, so we have to assume
+								// that the method may have requirements which we can't fullfil -> warn.
 								reflectionContext.RecordUnrecognizedPattern (
 									2060, string.Format (Resources.Strings.IL2060,
 										DiagnosticUtilities.GetMethodSignatureDisplayName (calledMethod)));
@@ -2137,12 +2146,6 @@ namespace Mono.Linker.Dataflow
 				MarkMethod (ref reflectionContext, ctor);
 		}
 
-		void MarkMethodsOnTypeHierarchy (ref ReflectionPatternContext reflectionContext, TypeDefinition type, Func<MethodDefinition, bool> filter, BindingFlags? bindingFlags = null)
-		{
-			foreach (var method in type.GetMethodsOnTypeHierarchy (filter, bindingFlags))
-				MarkMethod (ref reflectionContext, method);
-		}
-
 		void MarkFieldsOnTypeHierarchy (ref ReflectionPatternContext reflectionContext, TypeDefinition type, Func<FieldDefinition, bool> filter, BindingFlags? bindingFlags = BindingFlags.Default)
 		{
 			foreach (var field in type.GetFieldsOnTypeHierarchy (filter, bindingFlags))
@@ -2173,6 +2176,26 @@ namespace Mono.Linker.Dataflow
 				MarkEvent (ref reflectionContext, @event);
 		}
 
+		void ValidateGenericMethodInstantiation (
+			ref ReflectionPatternContext reflectionContext,
+			MethodDefinition genericMethod,
+			MethodReference reflectionMethod)
+		{
+			if (!genericMethod.HasGenericParameters)
+				return;
+
+			foreach (var genericParameter in genericMethod.GenericParameters) {
+				if (_context.Annotations.FlowAnnotations.GetGenericParameterAnnotation (genericParameter) != DynamicallyAccessedMemberTypes.None ||
+					genericParameter.HasDefaultConstructorConstraint) {
+					// There is a generic parameter which has some requirements on input types.
+					// For now we don't support tracking actual array elements, so we can't validate that the requirements are fulfilled.
+					reflectionContext.RecordUnrecognizedPattern (
+						2060, string.Format (Resources.Strings.IL2060,
+							DiagnosticUtilities.GetMethodSignatureDisplayName (reflectionMethod)));
+				}
+			}
+		}
+
 		static DynamicallyAccessedMemberTypes GetDynamicallyAccessedMemberTypesFromBindingFlagsForNestedTypes (BindingFlags? bindingFlags) =>
 			(HasBindingFlag (bindingFlags, BindingFlags.Public) ? DynamicallyAccessedMemberTypes.PublicNestedTypes : DynamicallyAccessedMemberTypes.None) |
 			(HasBindingFlag (bindingFlags, BindingFlags.NonPublic) ? DynamicallyAccessedMemberTypes.NonPublicNestedTypes : DynamicallyAccessedMemberTypes.None);
diff --git a/test/Mono.Linker.Tests.Cases/DataFlow/GenericParameterDataFlow.cs b/test/Mono.Linker.Tests.Cases/DataFlow/GenericParameterDataFlow.cs
index 7872462f7..08bb958cd 100644
--- a/test/Mono.Linker.Tests.Cases/DataFlow/GenericParameterDataFlow.cs
+++ b/test/Mono.Linker.Tests.Cases/DataFlow/GenericParameterDataFlow.cs
@@ -882,12 +882,14 @@ namespace Mono.Linker.Tests.Cases.DataFlow
 			{
 				TestNullMethod ();
 				TestUnknownInput (null);
+				TestUnknownMethodButNoTypeArguments (null);
 				TestWithNoArguments ();
 
 				TestWithRequirements ();
 				TestWithRequirementsFromParam (null);
 				TestWithRequirementsFromGenericParam<TestType> ();
 				TestWithRequirementsViaRuntimeMethod ();
+				TestWithRequirementsButNoTypeArguments ();
 
 				TestWithNoRequirements ();
 				TestWithNoRequirementsFromParam (null);
@@ -914,6 +916,14 @@ namespace Mono.Linker.Tests.Cases.DataFlow
 				mi.MakeGenericMethod (typeof (TestType));
 			}
 
+			[UnrecognizedReflectionAccessPattern (typeof (MethodInfo), nameof (MethodInfo.MakeGenericMethod), new Type[] { typeof (Type[]) },
+				messageCode: "IL2060")]
+			static void TestUnknownMethodButNoTypeArguments (MethodInfo mi)
+			{
+				// Thechnically linker could figure this out, but it's not worth the complexity - such call will always fail at runtime.
+				mi.MakeGenericMethod (Type.EmptyTypes);
+			}
+
 			[RecognizedReflectionAccessPattern]
 			static void TestWithNoArguments ()
 			{
@@ -957,6 +967,15 @@ namespace Mono.Linker.Tests.Cases.DataFlow
 					.MakeGenericMethod (typeof (TestType));
 			}
 
+			[UnrecognizedReflectionAccessPattern (typeof (MethodInfo), nameof (MethodInfo.MakeGenericMethod), new Type[] { typeof (Type[]) },
+				messageCode: "IL2060")]
+			static void TestWithRequirementsButNoTypeArguments ()
+			{
+				// Linker could figure out that this is not a problem, but it's not worth the complexity, since this will always throw at runtime
+				typeof (MakeGenericMethod).GetMethod (nameof (GenericWithRequirements), BindingFlags.Static)
+					.MakeGenericMethod (Type.EmptyTypes);
+			}
+
 			public static void GenericWithRequirements<[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicFields)] T> ()
 			{
 			}
diff --git a/test/Mono.Linker.Tests.Cases/Reflection/ExpressionCallString.cs b/test/Mono.Linker.Tests.Cases/Reflection/ExpressionCallString.cs
index d1a1f95b5..866fab24b 100644
--- a/test/Mono.Linker.Tests.Cases/Reflection/ExpressionCallString.cs
+++ b/test/Mono.Linker.Tests.Cases/Reflection/ExpressionCallString.cs
@@ -9,6 +9,7 @@ namespace Mono.Linker.Tests.Cases.Reflection
 {
 	[SetupCSharpCompilerToUse ("csc")]
 	[Reference ("System.Core.dll")]
+	[ExpectedNoWarnings]
 	public class ExpressionCallString
 	{
 		public static void Main ()
@@ -231,22 +232,115 @@ namespace Mono.Linker.Tests.Cases.Reflection
 			{ }
 
 			[Kept]
-			// BUG:https://github.com/mono/linker/issues/1819
-			// [ExpectedWarning("IL9999", nameof(GenericMethodWithRequirements))]
-			public static void Test ()
+			public static void GenericMethodWithRequirementsNoArguments<
+				[KeptAttributeAttribute (typeof (DynamicallyAccessedMembersAttribute))]
+			[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicProperties)] T> ()
+			{ }
+
+			[Kept]
+			static void TestWithNoTypeParameters ()
 			{
 				// Linker doesn't check if it's valid to call a generic method without generic parameters, it looks like a non-generic call
 				// so it will preserve the target method.
 				Expression.Call (typeof (TestGenericMethods), nameof (GenericMethodCalledAsNonGeneric), Type.EmptyTypes);
+			}
 
+			[Kept]
+			static void TestMethodWithoutRequirements ()
+			{
 				// This may not warn - as it's safe
 				Expression.Call (typeof (TestGenericMethods), nameof (GenericMethodWithNoRequirements), new Type[] { GetUnknownType () });
+			}
 
+			[Kept]
+			[ExpectedWarning ("IL2060", "Expression::Call")]
+			static void TestMethodWithRequirements ()
+			{
 				// This must warn - as this is dangerous
 				Expression.Call (typeof (TestGenericMethods), nameof (GenericMethodWithRequirements), new Type[] { GetUnknownType () });
 			}
 
 			[Kept]
+			[ExpectedWarning ("IL2060", "Expression::Call")]
+			static void TestMethodWithRequirementsButNoTypeArguments ()
+			{
+				// Linker could figure out that this is not a problem, but it's not worth the complexity, since this will always throw at runtime
+				Expression.Call (typeof (TestGenericMethods), nameof (GenericMethodWithRequirementsNoArguments), Type.EmptyTypes);
+			}
+
+			[Kept]
+			[KeptMember (".cctor()")]
+			class UnknownMethodWithRequirements
+			{
+				[Kept]
+				public static void GenericMethodWithRequirements<
+					[KeptAttributeAttribute (typeof (DynamicallyAccessedMembersAttribute))]
+				[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicProperties)] T> ()
+				{ }
+
+				[Kept]
+				static string _unknownMethodName = "NoMethod";
+
+				[Kept]
+				[ExpectedWarning ("IL2060")]
+				public static void TestWithTypeParameters ()
+				{
+					// Linker has no idea which method to mark - so it should warn if there are type parameters
+					Expression.Call (typeof (UnknownMethodWithRequirements), _unknownMethodName, new Type[] { GetUnknownType () });
+				}
+
+				[Kept]
+				public static void TestWithoutTypeParameters ()
+				{
+					// Linker has no idea which method to mark - so it should warn if there are type parameters
+					Expression.Call (typeof (UnknownMethodWithRequirements), _unknownMethodName, Type.EmptyTypes);
+				}
+			}
+
+			[Kept]
+			[KeptMember (".cctor()")]
+			class UnknownTypeWithRequirements
+			{
+				public static void GenericMethodWithRequirements<
+					[KeptAttributeAttribute (typeof (DynamicallyAccessedMembersAttribute))]
+				[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicProperties)] T> ()
+				{ }
+
+				[Kept]
+				[KeptAttributeAttribute (typeof (DynamicallyAccessedMembersAttribute))]
+				[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods | DynamicallyAccessedMemberTypes.NonPublicMethods)]
+				static Type _unknownType = null;
+
+				[Kept]
+				[ExpectedWarning ("IL2060")]
+				public static void TestWithTypeParameters ()
+				{
+					// Linker has no idea which method to mark - so it should warn if there are type parameters
+					Expression.Call (_unknownType, "NoMethod", new Type[] { GetUnknownType () });
+				}
+
+				[Kept]
+				public static void TestWithoutTypeParameters ()
+				{
+					// Linker has no idea which method to mark - so it should warn if there are type parameters
+					Expression.Call (_unknownType, "NoMethod", Type.EmptyTypes);
+				}
+			}
+
+			[Kept]
+			public static void Test ()
+			{
+				TestWithNoTypeParameters ();
+				TestMethodWithoutRequirements ();
+				TestMethodWithRequirements ();
+				TestMethodWithRequirementsButNoTypeArguments ();
+				UnknownMethodWithRequirements.TestWithTypeParameters ();
+				UnknownMethodWithRequirements.TestWithoutTypeParameters ();
+				UnknownTypeWithRequirements.TestWithTypeParameters ();
+				UnknownTypeWithRequirements.TestWithoutTypeParameters ();
+			}
+
+			[Kept]
 			static Type GetUnknownType () { return null; }
 		}
 	}