New constraint on a generic argument involved in MakeGenericType/Method should warn (#2942)

For correctness linker must be able to validate that generic parameters with new constraint will have their default .ctor preserved. If that can't be guaranteed, it needs to warn.

Added tests for cases around this.

Nullable<> is the only exception to this rule, so changed the code to completely skip any validation for it.

2 files changed, 255 insertions, 66 deletions

diff --git a/src/ILLink.Shared/TrimAnalysis/HandleCallAction.cs b/src/ILLink.Shared/TrimAnalysis/HandleCallAction.cs
index ded6275f2..12d43c74f 100644
--- a/src/ILLink.Shared/TrimAnalysis/HandleCallAction.cs
+++ b/src/ILLink.Shared/TrimAnalysis/HandleCallAction.cs
@@ -808,29 +808,15 @@ namespace ILLink.Shared.TrimAnalysis
 
 				foreach (var value in instanceValue) {
 					if (value is SystemTypeValue typeValue) {
-						var genericParameterValues = GetGenericParameterValues (typeValue.RepresentedType.GetGenericParameters ());
-						if (!AnalyzeGenericInstantiationTypeArray (argumentValues[0], calledMethod, genericParameterValues)) {
-							bool hasUncheckedAnnotation = false;
-							foreach (var genericParameter in genericParameterValues) {
-								if (genericParameter.DynamicallyAccessedMemberTypes != DynamicallyAccessedMemberTypes.None ||
-									(genericParameter.GenericParameter.HasDefaultConstructorConstraint () && !typeValue.RepresentedType.IsTypeOf ("System", "Nullable`1"))) {
-									// If we failed to analyze the array, we go through the analyses again
-									// and intentionally ignore one particular annotation:
-									// Special case: Nullable<T> where T : struct
-									//  The struct constraint in C# implies new() constraints, but Nullable doesn't make a use of that part.
-									//  There are several places even in the framework where typeof(Nullable<>).MakeGenericType would warn
-									//  without any good reason to do so.
-									hasUncheckedAnnotation = true;
-									break;
-								}
-							}
-							if (hasUncheckedAnnotation) {
-								_diagnosticContext.AddDiagnostic (DiagnosticId.MakeGenericType, calledMethod.GetDisplayName ());
-							}
-						}
-
+						// Special case Nullable<T>
 						// Nullables without a type argument are considered SystemTypeValues
 						if (typeValue.RepresentedType.IsTypeOf ("System", "Nullable`1")) {
+							// Note that we're not performing any generic parameter validation
+							// Special case: Nullable<T> where T : struct
+							//  The struct constraint in C# implies new() constraint, but Nullable doesn't make a use of that part.
+							//  There are several places even in the framework where typeof(Nullable<>).MakeGenericType would warn
+							//  without any good reason to do so.
+
 							foreach (var argumentValue in argumentValues[0]) {
 								if ((argumentValue as ArrayValue)?.TryGetValueByIndex (0, out var underlyingMultiValue) == true) {
 									foreach (var underlyingValue in underlyingMultiValue) {
@@ -864,8 +850,13 @@ namespace ILLink.Shared.TrimAnalysis
 							}
 							// We want to skip adding the `value` to the return Value because we have already added Nullable<value>
 							continue;
+						} else {
+							// Any other type - perform generic parameter validation
+							var genericParameterValues = GetGenericParameterValues (typeValue.RepresentedType.GetGenericParameters ());
+							if (!AnalyzeGenericInstantiationTypeArray (argumentValues[0], calledMethod, genericParameterValues)) {
+								_diagnosticContext.AddDiagnostic (DiagnosticId.MakeGenericType, calledMethod.GetDisplayName ());
+							}
 						}
-						// We haven't found any generic parameters with annotations, so there's nothing to validate.
 					} else if (value == NullValue.Instance) {
 						// At runtime this would throw - so it has no effect on analysis
 						AddReturnValue (MultiValueLattice.Top);
@@ -1228,7 +1219,7 @@ namespace ILLink.Shared.TrimAnalysis
 		{
 			bool hasRequirements = false;
 			foreach (var genericParameter in genericParameters) {
-				if (genericParameter.DynamicallyAccessedMemberTypes != DynamicallyAccessedMemberTypes.None) {
+				if (GetGenericParameterEffectiveMemberTypes (genericParameter) != DynamicallyAccessedMemberTypes.None) {
 					hasRequirements = true;
 					break;
 				}
@@ -1265,12 +1256,26 @@ namespace ILLink.Shared.TrimAnalysis
 						// https://github.com/dotnet/linker/issues/2428
 						// We need to report the target as "this" - as that was the previous behavior
 						// but with the annotation from the generic parameter.
-						var targetValue = _annotations.GetMethodThisParameterValue (calledMethod, genericParameters[i].DynamicallyAccessedMemberTypes);
+						var targetValue = _annotations.GetMethodThisParameterValue (calledMethod, GetGenericParameterEffectiveMemberTypes (genericParameters[i]));
 						_requireDynamicallyAccessedMembersAction.Invoke (value, targetValue);
 					}
 				}
 			}
 			return true;
+
+			// Returns effective annotation of a generic parameter where it incorporates the constraint into the annotation.
+			// There are basically three cases where the constraint matters:
+			// - NeedsNew<SpecificType> - MarkStep will simply mark the default .ctor of SpecificType in this case, it has nothing to do with reflection
+			// - NeedsNew<TOuter> - this should be validated by the compiler/IL - TOuter must have matching constraints by definition, so nothing to validate
+			// - typeof(NeedsNew<>).MakeGenericType(typeOuter) - for this case we have to do it by hand as it's reflection. This is where this method helps.
+			static DynamicallyAccessedMemberTypes GetGenericParameterEffectiveMemberTypes (GenericParameterValue genericParameter)
+			{
+				DynamicallyAccessedMemberTypes result = genericParameter.DynamicallyAccessedMemberTypes;
+				if (genericParameter.GenericParameter.HasDefaultConstructorConstraint ())
+					result |= DynamicallyAccessedMemberTypes.PublicParameterlessConstructor;
+
+				return result;
+			}
 		}
 
 		void ValidateGenericMethodInstantiation (
diff --git a/test/Mono.Linker.Tests.Cases/DataFlow/MakeGenericDataFlow.cs b/test/Mono.Linker.Tests.Cases/DataFlow/MakeGenericDataFlow.cs
index 85cc5904c..262e9b3f7 100644
--- a/test/Mono.Linker.Tests.Cases/DataFlow/MakeGenericDataFlow.cs
+++ b/test/Mono.Linker.Tests.Cases/DataFlow/MakeGenericDataFlow.cs
@@ -43,9 +43,9 @@ namespace Mono.Linker.Tests.Cases.DataFlow
 
 				TestWithMultipleArgumentsWithRequirements ();
 
-				TestWithNewConstraint ();
-				TestWithStructConstraint ();
-				TestWithUnmanagedConstraint ();
+				NewConstraint.Test ();
+				StructConstraint.Test ();
+				UnmanagedConstraint.Test ();
 				TestWithNullable ();
 			}
 
@@ -182,31 +182,118 @@ namespace Mono.Linker.Tests.Cases.DataFlow
 			{
 			}
 
-			static void TestWithNewConstraint ()
+			class NewConstraint
 			{
-				typeof (GenericWithNewConstraint<>).MakeGenericType (typeof (TestType));
-			}
+				class GenericWithNewConstraint<T> where T : new()
+				{
+				}
 
-			class GenericWithNewConstraint<T> where T : new()
-			{
-			}
+				class GenericWithNewConstraintAndAnnotations<[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)] T> where T : new()
+				{
+				}
 
-			static void TestWithStructConstraint ()
-			{
-				typeof (GenericWithStructConstraint<>).MakeGenericType (typeof (TestType));
-			}
+				static void SpecificType ()
+				{
+					typeof (GenericWithNewConstraint<>).MakeGenericType (typeof (TestType));
+				}
 
-			class GenericWithStructConstraint<T> where T : struct
-			{
+				[ExpectedWarning ("IL2070")]
+				static void UnknownType (Type unknownType = null)
+				{
+					typeof (GenericWithNewConstraint<>).MakeGenericType (unknownType);
+				}
+
+				static void AnnotationMatch ([DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicParameterlessConstructor)] Type withCtor = null)
+				{
+					typeof (GenericWithNewConstraint<>).MakeGenericType (withCtor);
+				}
+
+				[ExpectedWarning ("IL2070")]
+				static void AnnotationMismatch ([DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)] Type withPublicMethods = null)
+				{
+					typeof (GenericWithNewConstraint<>).MakeGenericType (withPublicMethods);
+				}
+
+				static void AnnotationAndConstraintMatch ([DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods | DynamicallyAccessedMemberTypes.PublicConstructors)] Type withMethodsAndCtors = null)
+				{
+					typeof (GenericWithNewConstraintAndAnnotations<>).MakeGenericType (withMethodsAndCtors);
+				}
+
+				[ExpectedWarning ("IL2070")]
+				static void AnnotationAndConstraintMismatch ([DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)] Type withMethods = null)
+				{
+					typeof (GenericWithNewConstraintAndAnnotations<>).MakeGenericType (withMethods);
+				}
+
+				public static void Test ()
+				{
+					SpecificType ();
+					UnknownType ();
+					AnnotationMatch ();
+					AnnotationMismatch ();
+					AnnotationAndConstraintMatch ();
+					AnnotationAndConstraintMismatch ();
+				}
 			}
 
-			static void TestWithUnmanagedConstraint ()
+			class StructConstraint
 			{
-				typeof (GenericWithUnmanagedConstraint<>).MakeGenericType (typeof (TestType));
+				class GenericWithStructConstraint<T> where T : struct
+				{
+				}
+
+				static void SpecificType ()
+				{
+					typeof (GenericWithStructConstraint<>).MakeGenericType (typeof (TestType));
+				}
+
+				static void AnnotationMatch ([DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicParameterlessConstructor)] Type withCtor = null)
+				{
+					typeof (GenericWithStructConstraint<>).MakeGenericType (withCtor);
+				}
+
+				[ExpectedWarning ("IL2070")]
+				static void AnnotationMismatch ([DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)] Type withPublicMethods = null)
+				{
+					typeof (GenericWithStructConstraint<>).MakeGenericType (withPublicMethods);
+				}
+
+				public static void Test ()
+				{
+					SpecificType ();
+					AnnotationMatch ();
+					AnnotationMismatch ();
+				}
 			}
 
-			class GenericWithUnmanagedConstraint<T> where T : unmanaged
+			class UnmanagedConstraint
 			{
+				class GenericWithUnmanagedConstraint<T> where T : unmanaged
+				{
+				}
+
+				static void SpecificType ()
+				{
+					typeof (GenericWithUnmanagedConstraint<>).MakeGenericType (typeof (TestType));
+				}
+
+				static void AnnotationMatch ([DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicParameterlessConstructor)] Type withCtor = null)
+				{
+					typeof (GenericWithUnmanagedConstraint<>).MakeGenericType (withCtor);
+				}
+
+				[ExpectedWarning ("IL2070")]
+				static void AnnotationMismatch ([DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)] Type withPublicMethods = null)
+				{
+					typeof (GenericWithUnmanagedConstraint<>).MakeGenericType (withPublicMethods);
+				}
+
+				public static void Test ()
+				{
+					SpecificType ();
+					AnnotationMatch ();
+					AnnotationMismatch ();
+				}
 			}
 
 			static void TestWithNullable ()
@@ -264,9 +351,9 @@ namespace Mono.Linker.Tests.Cases.DataFlow
 
 				TestWithMultipleArgumentsWithRequirements ();
 
-				TestWithNewConstraint ();
-				TestWithStructConstraint ();
-				TestWithUnmanagedConstraint ();
+				NewConstraint.Test ();
+				StructConstraint.Test ();
+				UnmanagedConstraint.Test ();
 
 				TestGetMethodFromHandle ();
 				TestGetMethodFromHandleWithWarning ();
@@ -663,39 +750,136 @@ namespace Mono.Linker.Tests.Cases.DataFlow
 			{
 			}
 
-			static void TestWithNewConstraint ()
+			class NewConstraint
 			{
-				typeof (MakeGenericMethod).GetMethod (nameof (GenericWithNewConstraint), BindingFlags.Static | BindingFlags.NonPublic)
-					.MakeGenericMethod (typeof (TestType));
-			}
+				static void GenericWithNewConstraint<T> () where T : new()
+				{
+					var t = new T ();
+				}
 
-			static void GenericWithNewConstraint<T> () where T : new()
-			{
-				var t = new T ();
-			}
+				static void GenericWithNewConstraintAndAnnotations<[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)] T> () where T : new()
+				{
+				}
 
-			static void TestWithStructConstraint ()
-			{
-				typeof (MakeGenericMethod).GetMethod (nameof (GenericWithStructConstraint), BindingFlags.Static | BindingFlags.NonPublic)
-					.MakeGenericMethod (typeof (TestType));
-			}
+				static void SpecificType ()
+				{
+					typeof (NewConstraint).GetMethod (nameof (GenericWithNewConstraint), BindingFlags.Static | BindingFlags.NonPublic)
+						.MakeGenericMethod (typeof (TestType));
+				}
 
-			static void GenericWithStructConstraint<T> () where T : struct
-			{
-				var t = new T ();
+				[ExpectedWarning ("IL2070")]
+				static void UnknownType (Type unknownType = null)
+				{
+					typeof (NewConstraint).GetMethod (nameof (GenericWithNewConstraint), BindingFlags.Static | BindingFlags.NonPublic)
+						.MakeGenericMethod (unknownType);
+				}
+
+				static void AnnotationMatch ([DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicParameterlessConstructor)] Type withCtor = null)
+				{
+					typeof (NewConstraint).GetMethod (nameof (GenericWithNewConstraint), BindingFlags.Static | BindingFlags.NonPublic)
+						.MakeGenericMethod (withCtor);
+				}
+
+				[ExpectedWarning ("IL2070")]
+				static void AnnotationMismatch ([DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)] Type withPublicMethods = null)
+				{
+					typeof (NewConstraint).GetMethod (nameof (GenericWithNewConstraint), BindingFlags.Static | BindingFlags.NonPublic)
+						.MakeGenericMethod (withPublicMethods);
+				}
+
+				static void AnnotationAndConstraintMatch ([DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods | DynamicallyAccessedMemberTypes.PublicConstructors)] Type withMethodsAndCtors = null)
+				{
+					typeof (NewConstraint).GetMethod (nameof (GenericWithNewConstraintAndAnnotations), BindingFlags.Static | BindingFlags.NonPublic)
+						.MakeGenericMethod (withMethodsAndCtors);
+				}
+
+				[ExpectedWarning ("IL2070")]
+				static void AnnotationAndConstraintMismatch ([DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)] Type withMethods = null)
+				{
+					typeof (NewConstraint).GetMethod (nameof (GenericWithNewConstraintAndAnnotations), BindingFlags.Static | BindingFlags.NonPublic)
+						.MakeGenericMethod (withMethods);
+				}
+
+				public static void Test ()
+				{
+					SpecificType ();
+					UnknownType ();
+					AnnotationMatch ();
+					AnnotationMismatch ();
+					AnnotationAndConstraintMatch ();
+					AnnotationAndConstraintMismatch ();
+				}
 			}
 
-			static void TestWithUnmanagedConstraint ()
+			class StructConstraint
 			{
-				typeof (MakeGenericMethod).GetMethod (nameof (GenericWithUnmanagedConstraint), BindingFlags.Static | BindingFlags.NonPublic)
-					.MakeGenericMethod (typeof (TestType));
+				static void GenericWithStructConstraint<T> () where T : struct
+				{
+					var t = new T ();
+				}
+
+				static void SpecificType ()
+				{
+					typeof (StructConstraint).GetMethod (nameof (GenericWithStructConstraint), BindingFlags.Static | BindingFlags.NonPublic)
+						.MakeGenericMethod (typeof (TestType));
+				}
+
+				static void AnnotationMatch ([DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicParameterlessConstructor)] Type withCtor = null)
+				{
+					typeof (StructConstraint).GetMethod (nameof (GenericWithStructConstraint), BindingFlags.Static | BindingFlags.NonPublic)
+						.MakeGenericMethod (withCtor);
+				}
+
+				[ExpectedWarning ("IL2070")]
+				static void AnnotationMismatch ([DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)] Type withPublicMethods = null)
+				{
+					typeof (StructConstraint).GetMethod (nameof (GenericWithStructConstraint), BindingFlags.Static | BindingFlags.NonPublic)
+						.MakeGenericMethod (withPublicMethods);
+				}
+
+				public static void Test ()
+				{
+					SpecificType ();
+					AnnotationMatch ();
+					AnnotationMismatch ();
+				}
 			}
 
-			static void GenericWithUnmanagedConstraint<T> () where T : unmanaged
+			class UnmanagedConstraint
 			{
-				var t = new T ();
+				static void GenericWithUnmanagedConstraint<T> () where T : unmanaged
+				{
+					var t = new T ();
+				}
+
+				static void SpecificType ()
+				{
+					typeof (UnmanagedConstraint).GetMethod (nameof (GenericWithUnmanagedConstraint), BindingFlags.Static | BindingFlags.NonPublic)
+						.MakeGenericMethod (typeof (TestType));
+				}
+
+				static void AnnotationMatch ([DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicParameterlessConstructor)] Type withCtor = null)
+				{
+					typeof (UnmanagedConstraint).GetMethod (nameof (GenericWithUnmanagedConstraint), BindingFlags.Static | BindingFlags.NonPublic)
+						.MakeGenericMethod (withCtor);
+				}
+
+				[ExpectedWarning ("IL2070")]
+				static void AnnotationMismatch ([DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)] Type withPublicMethods = null)
+				{
+					typeof (UnmanagedConstraint).GetMethod (nameof (GenericWithUnmanagedConstraint), BindingFlags.Static | BindingFlags.NonPublic)
+						.MakeGenericMethod (withPublicMethods);
+				}
+
+				public static void Test ()
+				{
+					SpecificType ();
+					AnnotationMatch ();
+					AnnotationMismatch ();
+				}
 			}
 
+
 			static void TestGetMethodFromHandle (Type unknownType = null)
 			{
 				MethodInfo m = (MethodInfo) MethodInfo.GetMethodFromHandle (typeof (MakeGenericMethod).GetMethod (nameof (GenericWithNoRequirements)).MethodHandle);