Mark generic arguments of dynamically accessed types passed as string (…

…dotnet/linker#1566)

* Mark dynamically accessed generic arguments

* Clean test

* Use MarkType for keeping generic args
Enable ComplexTypeHandling tests

* Use MarkTypeVisibleToReflection for generic arguments
Clean tests

* MarkType always

* Check for array types in TypeNameResolver

* Mark System.Array instead of its element type

* Whitespace

* PR feedback

* Pattern match
Whitespace

* Add extension method ResolveToMainTypeDefinition

* Add comment

* Use ResolveToMainTypeDefinition

Commit migrated from dotnet/linker@2a4eb66

src/tools/illink/external/corert/src/System.Private.CoreLib/src/System/Reflection/Runtime/TypeParsing/TypeName.cs

@@ -140,15 +140,15 @@ internal sealed class MultiDimArrayTypeName : HasElementTypeName
 		public MultiDimArrayTypeName(TypeName elementTypeName, int rank)
 			: base(elementTypeName)
 		{
-			_rank = rank;
+			Rank = rank;
 		}
 
 		public sealed override string ToString()
 		{
-			return ElementTypeName + "[" + (_rank == 1 ? "*" : new string(',', _rank - 1)) + "]";
+			return ElementTypeName + "[" + (Rank == 1 ? "*" : new string(',', Rank - 1)) + "]";
 		}
 
-		private int _rank;
+		public int Rank { get; }
 	}
 
 	//

src/tools/illink/src/linker/Linker.Dataflow/MethodBodyScanner.cs

@@ -709,7 +709,7 @@ private static void ScanLdtoken (Instruction operation, Stack<StackSlot> current
 			}
 
 			if (operation.Operand is TypeReference typeReference) {
-				var resolvedReference = typeReference.Resolve ();
+				var resolvedReference = typeReference.ResolveToMainTypeDefinition ();
 				if (resolvedReference != null) {
 					StackSlot slot = new StackSlot (new RuntimeTypeHandleValue (resolvedReference));
 					currentStack.Push (slot);

src/tools/illink/src/linker/Linker.Dataflow/ReflectionMethodBodyScanner.cs

@@ -113,7 +113,7 @@ static ValueNode GetValueNodeForCustomAttributeArgument (CustomAttributeArgument
 		{
 			ValueNode valueNode;
 			if (argument.Type.Name == "Type") {
-				TypeDefinition referencedType = ((TypeReference) argument.Value).Resolve ();
+				TypeDefinition referencedType = ((TypeReference) argument.Value).ResolveToMainTypeDefinition ();
 				valueNode = referencedType == null ? null : new SystemTypeValue (referencedType);
 			} else if (argument.Type.MetadataType == MetadataType.String) {
 				valueNode = new KnownStringValue ((string) argument.Value);
@@ -130,22 +130,22 @@ public void ProcessGenericArgumentDataFlow (GenericParameter genericParameter, T
 			var annotation = _context.Annotations.FlowAnnotations.GetGenericParameterAnnotation (genericParameter);
 			Debug.Assert (annotation != DynamicallyAccessedMemberTypes.None);
 
-			ValueNode valueNode = GetValueNodeFromGenericArgument (genericArgument);
+			ValueNode valueNode = GetTypeValueNodeFromGenericArgument (genericArgument);
 			bool enableReflectionPatternReporting = !(source is MethodDefinition sourceMethod) || ShouldEnableReflectionPatternReporting (sourceMethod);
 
 			var reflectionContext = new ReflectionPatternContext (_context, enableReflectionPatternReporting, source, genericParameter);
 			reflectionContext.AnalyzingPattern ();
 			RequireDynamicallyAccessedMembers (ref reflectionContext, annotation, valueNode, genericParameter);
 		}
 
-		ValueNode GetValueNodeFromGenericArgument (TypeReference genericArgument)
+		ValueNode GetTypeValueNodeFromGenericArgument (TypeReference genericArgument)
 		{
 			if (genericArgument is GenericParameter inputGenericParameter) {
 				// Technically this should be a new value node type as it's not a System.Type instance representation, but just the generic parameter
 				// That said we only use it to perform the dynamically accessed members checks and for that purpose treating it as System.Type is perfectly valid.
 				return new SystemTypeForGenericParameterValue (inputGenericParameter, _context.Annotations.FlowAnnotations.GetGenericParameterAnnotation (inputGenericParameter));
 			} else {
-				TypeDefinition genericArgumentTypeDef = genericArgument.Resolve ();
+				TypeDefinition genericArgumentTypeDef = genericArgument.ResolveToMainTypeDefinition ();
 				if (genericArgumentTypeDef != null) {
 					return new SystemTypeValue (genericArgumentTypeDef);
 				} else {
@@ -752,13 +752,13 @@ public override bool HandleCall (MethodBody callingMethodBody, MethodReference c
 								if (methodParam.ParameterIndex == 0) {
 									staticType = callingMethodDefinition.DeclaringType;
 								} else {
-									staticType = callingMethodDefinition.Parameters[methodParam.ParameterIndex - 1].ParameterType.Resolve ();
+									staticType = callingMethodDefinition.Parameters[methodParam.ParameterIndex - 1].ParameterType.ResolveToMainTypeDefinition ();
 								}
 							} else {
-								staticType = callingMethodDefinition.Parameters[methodParam.ParameterIndex].ParameterType.Resolve ();
+								staticType = callingMethodDefinition.Parameters[methodParam.ParameterIndex].ParameterType.ResolveToMainTypeDefinition ();
 							}
 						} else if (methodParams[0] is LoadFieldValue loadedField) {
-							staticType = loadedField.Field.FieldType.Resolve ();
+							staticType = loadedField.Field.FieldType.ResolveToMainTypeDefinition ();
 						}
 
 						if (staticType != null) {
@@ -802,7 +802,7 @@ public override bool HandleCall (MethodBody callingMethodBody, MethodReference c
 						foreach (var typeNameValue in methodParams[0].UniqueValues ()) {
 							if (typeNameValue is KnownStringValue knownStringValue) {
 								TypeReference foundTypeRef = _context.TypeNameResolver.ResolveTypeName (knownStringValue.Contents);
-								TypeDefinition foundType = foundTypeRef?.Resolve ();
+								TypeDefinition foundType = foundTypeRef?.ResolveToMainTypeDefinition ();
 								if (foundType == null) {
 									// Intentionally ignore - it's not wrong for code to call Type.GetType on non-existing name, the code might expect null/exception back.
 									reflectionContext.RecordHandledPattern ();
@@ -1161,7 +1161,7 @@ methodParams[argsParam] is ArrayValue arrayValue &&
 						RequireDynamicallyAccessedMembers (
 							ref reflectionContext,
 							DynamicallyAccessedMemberTypes.PublicParameterlessConstructor,
-							GetValueNodeFromGenericArgument (genericCalledMethod.GenericArguments[0]),
+							GetTypeValueNodeFromGenericArgument (genericCalledMethod.GenericArguments[0]),
 							calledMethodDefinition.GenericParameters[0]);
 					}
 					break;
@@ -1340,11 +1340,13 @@ void ProcessCreateInstanceByName (ref ReflectionPatternContext reflectionContext
 								continue;
 							}
 
-							var resolvedType = _context.TypeNameResolver.ResolveTypeName (resolvedAssembly, typeNameStringValue.Contents)?.Resolve ();
-							if (resolvedType == null) {
+							var typeRef = _context.TypeNameResolver.ResolveTypeName (resolvedAssembly, typeNameStringValue.Contents);
+							var resolvedType = typeRef?.Resolve ();
+							if (resolvedType == null || typeRef is ArrayType) {
 								// It's not wrong to have a reference to non-existing type - the code may well expect to get an exception in this case
 								// Note that we did find the assembly, so it's not a linker config problem, it's either intentional, or wrong versions of assemblies
-								// but linker can't know that.
+								// but linker can't know that. In case a user tries to create an array using System.Activator we should simply ignore it, the user
+								// might expect an exception to be thrown.
 								reflectionContext.RecordHandledPattern ();
 								continue;
 							}
@@ -1571,11 +1573,13 @@ void RequireDynamicallyAccessedMembers (ref ReflectionPatternContext reflectionC
 				} else if (uniqueValue is SystemTypeValue systemTypeValue) {
 					MarkTypeForDynamicallyAccessedMembers (ref reflectionContext, systemTypeValue.TypeRepresented, requiredMemberTypes);
 				} else if (uniqueValue is KnownStringValue knownStringValue) {
-					TypeDefinition foundType = _context.TypeNameResolver.ResolveTypeName (knownStringValue.Contents)?.Resolve ();
+					TypeReference typeRef = _context.TypeNameResolver.ResolveTypeName (knownStringValue.Contents);
+					TypeDefinition foundType = typeRef?.ResolveToMainTypeDefinition ();
 					if (foundType == null) {
 						// Intentionally ignore - it's not wrong for code to call Type.GetType on non-existing name, the code might expect null/exception back.
 						reflectionContext.RecordHandledPattern ();
 					} else {
+						MarkType (ref reflectionContext, typeRef);
 						MarkTypeForDynamicallyAccessedMembers (ref reflectionContext, foundType, requiredMemberTypes);
 					}
 				} else if (uniqueValue == NullValue.Instance) {
@@ -1649,6 +1653,12 @@ void MarkTypeForDynamicallyAccessedMembers (ref ReflectionPatternContext reflect
 			}
 		}
 
+		void MarkType (ref ReflectionPatternContext reflectionContext, TypeReference typeReference)
+		{
+			var source = reflectionContext.Source;
+			reflectionContext.RecordRecognizedPattern (typeReference?.Resolve (), () => _markStep.MarkTypeVisibleToReflection (typeReference, new DependencyInfo (DependencyKind.AccessedViaReflection, source), source));
+		}
+
 		void MarkMethod (ref ReflectionPatternContext reflectionContext, MethodDefinition method)
 		{
 			var source = reflectionContext.Source;

src/tools/illink/src/linker/Linker/TypeNameResolver.cs

@@ -77,7 +77,14 @@ TypeReference ResolveTypeName (AssemblyDefinition assembly, TypeName typeName)
 				if (elementType == null)
 					return null;
 
-				return elementType;
+				return typeName switch
+				{
+					ArrayTypeName _ => new ArrayType (elementType),
+					MultiDimArrayTypeName multiDimArrayTypeName => new ArrayType (elementType, multiDimArrayTypeName.Rank),
+					ByRefTypeName _ => new ByReferenceType (elementType),
+					PointerTypeName _ => new PointerType (elementType),
+					_ => elementType
+				};
 			}
 
 			return assembly.MainModule.GetType (typeName.ToString ());

src/tools/illink/src/linker/Linker/TypeReferenceExtensions.cs

@@ -375,5 +375,17 @@ public static bool IsSubclassOf (this TypeReference type, string ns, string name
 
 			return false;
 		}
+
+		// 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.
+		public static TypeDefinition ResolveToMainTypeDefinition (this TypeReference type)
+		{
+			return type switch
+			{
+				ArrayType _ => type.Module.ImportReference (typeof (Array))?.Resolve (),
+				_ => type?.Resolve ()
+			};
+		}
 	}
 }

src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/ApplyTypeAnnotations.cs

@@ -128,9 +128,7 @@ private static void RequireCombinationOnString (
 		{
 		}
 
-		// Issue: https://github.com/mono/linker/issues/1537
-		//[Kept]
-		//[KeptMember (".ctor()")]
+		[Kept]
 		class FromStringConstantWithGenericInner
 		{
 		}
@@ -143,10 +141,57 @@ class FromStringConstantWithGeneric<T>
 			public T GetValue () { return default (T); }
 		}
 
+		[Kept]
+		class FromStringConstantWithGenericInnerInner
+		{
+			[Kept]
+			public void Method ()
+			{
+			}
+
+			int unusedField;
+		}
+
+		[Kept]
+		class FromStringConstantWithGenericInnerOne<
+		[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)]
+		[KeptAttributeAttribute (typeof (DynamicallyAccessedMembersAttribute))]
+		T>
+		{
+		}
+
+		[Kept]
+		class FromStringConstantWithGenericInnerTwo
+		{
+			void UnusedMethod ()
+			{
+			}
+		}
+
+		[Kept]
+		class FromStringConstantWitGenericInnerMultiDimArray
+		{
+		}
+
+		[Kept]
+		class FromStringConstantWithMultiDimArray
+		{
+			public void UnusedMethod () { }
+		}
+
+		[Kept]
+		[KeptMember (".ctor()")]
+		class FromStringConstantWithGenericTwoParameters<T, S>
+		{
+		}
+
 		[Kept]
 		static void TestFromStringConstantWithGeneric ()
 		{
 			RequireCombinationOnString ("Mono.Linker.Tests.Cases.DataFlow.ApplyTypeAnnotations+FromStringConstantWithGeneric`1[[Mono.Linker.Tests.Cases.DataFlow.ApplyTypeAnnotations+FromStringConstantWithGenericInner]]");
+			RequireCombinationOnString ("Mono.Linker.Tests.Cases.DataFlow.ApplyTypeAnnotations+FromStringConstantWithGenericTwoParameters`2[Mono.Linker.Tests.Cases.DataFlow.ApplyTypeAnnotations+FromStringConstantWithGenericInnerOne`1[Mono.Linker.Tests.Cases.DataFlow.ApplyTypeAnnotations+FromStringConstantWithGenericInnerInner],Mono.Linker.Tests.Cases.DataFlow.ApplyTypeAnnotations+FromStringConstantWithGenericInnerTwo]");
+			RequireCombinationOnString ("Mono.Linker.Tests.Cases.DataFlow.ApplyTypeAnnotations+FromStringConstantWithGeneric`1[[Mono.Linker.Tests.Cases.DataFlow.ApplyTypeAnnotations+FromStringConstantWitGenericInnerMultiDimArray[,]]]");
+			RequireCombinationOnString ("Mono.Linker.Tests.Cases.DataFlow.ApplyTypeAnnotations+FromStringConstantWithMultiDimArray[,]");
 		}
 
 		[Kept]

src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/ComplexTypeHandling.cs

@@ -12,7 +12,6 @@
 
 namespace Mono.Linker.Tests.Cases.DataFlow
 {
-	[IgnoreTestCase ("Active issue https://github.com/mono/linker/issues/1559")]
 	public class ComplexTypeHandling
 	{
 		public static void Main ()
@@ -33,7 +32,6 @@ class ArrayElementType
 		{
 			public ArrayElementType () { }
 
-			[Kept]
 			public void PublicMethod () { }
 
 			private int _privateField;
@@ -62,7 +60,6 @@ class ArrayElementInGenericType
 		{
 			public ArrayElementInGenericType () { }
 
-			[Kept]
 			public void PublicMethod () { }
 
 			private int _privateField;
@@ -89,6 +86,7 @@ static void TestGenericArrayOnGeneric ()
 			RequirePublicMethodsOnArrayOfGenericParameter<ArrayElementInGenericType> ();
 		}
 
+		[Kept]
 		static void RequirePublicMethodsOnArrayOfGenericParameter<T> ()
 		{
 			_ = new RequirePublicMethodsGeneric<T[]> ();
@@ -97,7 +95,7 @@ static void RequirePublicMethodsOnArrayOfGenericParameter<T> ()
 		[Kept]
 		sealed class ArrayGetTypeFromMethodParamElement
 		{
-			[Kept] // This is a bug - the method should not be marked, instead Array.* should be marked
+			// This method should not be marked, instead Array.* should be marked
 			public void PublicMethod () { }
 		}
 
@@ -116,7 +114,7 @@ static void TestArrayGetTypeFromMethodParam ()
 		[Kept]
 		sealed class ArrayGetTypeFromFieldElement
 		{
-			[Kept] // This is a bug - the method should not be marked, instead Array.* should be marked
+			// This method should not be marked, instead Array.* should be marked
 			public void PublicMethod () { }
 		}
 
@@ -132,7 +130,7 @@ static void TestArrayGetTypeFromField ()
 		[Kept]
 		sealed class ArrayTypeGetTypeElement
 		{
-			[Kept] // This is a bug - the method should not be marked, instead Array.* should be marked
+			// This method should not be marked, instead Array.* should be marked
 			public void PublicMethod () { }
 		}
 
@@ -142,10 +140,9 @@ static void TestArrayTypeGetType ()
 			RequirePublicMethods (Type.GetType ("Mono.Linker.Tests.Cases.DataFlow.ComplexTypeHandling+ArrayTypeGetTypeElement[]"));
 		}
 
-		[Kept]
+		// Nothing should be marked as CreateInstance doesn't work on arrays
 		class ArrayCreateInstanceByNameElement
 		{
-			[Kept] // This is a bug - the .ctor should not be marked - in fact in this case nothing should be marked as CreateInstance doesn't work on arrays
 			public ArrayCreateInstanceByNameElement ()
 			{
 			}
@@ -160,7 +157,7 @@ static void TestArrayCreateInstanceByName ()
 		[Kept]
 		class ArrayInAttributeParamElement
 		{
-			[Kept] // This is a bug - the method should not be marked, instead Array.* should be marked
+			// This method should not be marked, instead Array.* should be marked
 			public void PublicMethod () { }
 		}
 

src/tools/illink/test/Mono.Linker.Tests.Cases/Reflection/PropertyUsedViaReflection.cs

@@ -17,6 +17,7 @@ public static void Main ()
 			TestNullName ();
 			TestEmptyName ();
 			TestNonExistingName ();
+			TestPropertyOfArray ();
 			TestNullType ();
 			TestDataFlowType ();
 			TestIfElse (1);
@@ -82,6 +83,16 @@ static void TestNonExistingName ()
 			var property = typeof (PropertyUsedViaReflection).GetProperty ("NonExisting");
 		}
 
+		[Kept]
+		[RecognizedReflectionAccessPattern (
+			typeof (Type), nameof (Type.GetProperty), new Type[] { typeof (string) },
+			typeof (Array), nameof (Array.LongLength))]
+		static void TestPropertyOfArray ()
+		{
+			var property = typeof (int[]).GetProperty ("LongLength");
+			property.GetValue (null);
+		}
+
 		[Kept]
 		static void TestNullType ()
 		{