Fix linker crash on unresolved types in generic and attribute data flow handling (#1645)

Unresolved types should be handled as "unknown" value nodes. This is already the case when they appear in a method body. But when they're used either as generic arguments or in attribute instantiations linker doesn't handle this case correctly.

For generics linker would fail with an exception. For attributes linker would ignore the type.

Both cases are wrong since linker needs to make sure to issue a warning if it can't guarantee to meet the requirements posed by the data flow annotations. So ignoring unresolved types is also wrong.

The change fixes these behaviors to treat the value as "unknown" which leads into warnings when used somewhere with data flow requirements.

There's also a small change in where the attribute related warnings are reported - now they should consistently report the warning in the place where the attribute is used (and not on the attribute definition) and also correctly report which members are involved (it used to report the `this` for ctor arguments for example).

Added a new test infra capability to remove an assembly from linker input - to be able to emulate unresolved assembly.

9 files changed, 188 insertions, 26 deletions

diff --git a/src/linker/Linker.Dataflow/ReflectionMethodBodyScanner.cs b/src/linker/Linker.Dataflow/ReflectionMethodBodyScanner.cs
index 9e4aa1eb6..db83b16a2 100644
--- a/src/linker/Linker.Dataflow/ReflectionMethodBodyScanner.cs
+++ b/src/linker/Linker.Dataflow/ReflectionMethodBodyScanner.cs
@@ -87,26 +87,23 @@ namespace Mono.Linker.Dataflow
 				var annotation = _context.Annotations.FlowAnnotations.GetParameterAnnotation (method, i + paramOffset);
 				if (annotation != DynamicallyAccessedMemberTypes.None) {
 					ValueNode valueNode = GetValueNodeForCustomAttributeArgument (arguments[i]);
-					if (valueNode != null) {
-						var reflectionContext = new ReflectionPatternContext (_context, true, source, method.Parameters[i]);
-						reflectionContext.AnalyzingPattern ();
-						RequireDynamicallyAccessedMembers (ref reflectionContext, annotation, valueNode, method);
-					}
+					var methodParameter = method.Parameters[i];
+					var reflectionContext = new ReflectionPatternContext (_context, true, source, methodParameter);
+					reflectionContext.AnalyzingPattern ();
+					RequireDynamicallyAccessedMembers (ref reflectionContext, annotation, valueNode, methodParameter);
 				}
 			}
 		}
 
-		public void ProcessAttributeDataflow (FieldDefinition field, CustomAttributeArgument value)
+		public void ProcessAttributeDataflow (IMemberDefinition source, FieldDefinition field, CustomAttributeArgument value)
 		{
 			var annotation = _context.Annotations.FlowAnnotations.GetFieldAnnotation (field);
 			Debug.Assert (annotation != DynamicallyAccessedMemberTypes.None);
 
 			ValueNode valueNode = GetValueNodeForCustomAttributeArgument (value);
-			if (valueNode != null) {
-				var reflectionContext = new ReflectionPatternContext (_context, true, field.DeclaringType.Methods[0], field);
-				reflectionContext.AnalyzingPattern ();
-				RequireDynamicallyAccessedMembers (ref reflectionContext, annotation, valueNode, field);
-			}
+			var reflectionContext = new ReflectionPatternContext (_context, true, source, field);
+			reflectionContext.AnalyzingPattern ();
+			RequireDynamicallyAccessedMembers (ref reflectionContext, annotation, valueNode, field);
 		}
 
 		static ValueNode GetValueNodeForCustomAttributeArgument (CustomAttributeArgument argument)
@@ -114,7 +111,10 @@ namespace Mono.Linker.Dataflow
 			ValueNode valueNode;
 			if (argument.Type.Name == "Type") {
 				TypeDefinition referencedType = ((TypeReference) argument.Value).ResolveToMainTypeDefinition ();
-				valueNode = referencedType == null ? null : new SystemTypeValue (referencedType);
+				if (referencedType == null)
+					valueNode = UnknownValue.Instance;
+				else
+					valueNode = new SystemTypeValue (referencedType);
 			} else if (argument.Type.MetadataType == MetadataType.String) {
 				valueNode = new KnownStringValue ((string) argument.Value);
 			} else {
@@ -122,6 +122,7 @@ namespace Mono.Linker.Dataflow
 				throw new InvalidOperationException ();
 			}
 
+			Debug.Assert (valueNode != null);
 			return valueNode;
 		}
 
@@ -149,7 +150,10 @@ namespace Mono.Linker.Dataflow
 				if (genericArgumentTypeDef != null) {
 					return new SystemTypeValue (genericArgumentTypeDef);
 				} else {
-					throw new InvalidOperationException ();
+					// If we can't resolve the generic argument, it means we can't apply potential requirements on it
+					// so track it as unknown value. If we later on hit this unknown value as being used somewhere
+					// where we need to apply requirements on it, it will generate a warning.
+					return UnknownValue.Instance;
 				}
 			}
 		}
@@ -1607,8 +1611,7 @@ namespace Mono.Linker.Dataflow
 							$"Value passed to implicit 'this' parameter of method '{methodDefinition.GetDisplayName ()}' can not be statically determined and may not meet 'DynamicallyAccessedMembersAttribute' requirements.");
 						break;
 					case GenericParameter genericParameter:
-						// Note: this is currently unreachable as there's no IL way to pass unknown value to a generic parameter without using reflection.
-						// Once we support analysis of MakeGenericType/MakeGenericMethod arguments this would become reachable though.
+						// Unknown value to generic parameter - this is possible if the generic argumnet fails to resolve
 						reflectionContext.RecordUnrecognizedPattern (
 							2066,
 							$"Type passed to generic parameter '{genericParameter.Name}' of '{DiagnosticUtilities.GetGenericParameterDeclaringMemberDisplayName (genericParameter)}' can not be statically determined and may not meet 'DynamicallyAccessedMembersAttribute' requirements.");
diff --git a/src/linker/Linker.Steps/MarkStep.cs b/src/linker/Linker.Steps/MarkStep.cs
index 9f449e27c..28dec3d57 100644
--- a/src/linker/Linker.Steps/MarkStep.cs
+++ b/src/linker/Linker.Steps/MarkStep.cs
@@ -1029,8 +1029,7 @@ namespace Mono.Linker.Steps
 
 			if (property != null && _context.Annotations.FlowAnnotations.RequiresDataFlowAnalysis (property.SetMethod)) {
 				var scanner = new ReflectionMethodBodyScanner (_context, this);
-				var caCtor = (ca as CustomAttribute).Constructor.Resolve ();
-				scanner.ProcessAttributeDataflow (caCtor, property.SetMethod, new List<CustomAttributeArgument> { namedArgument.Argument });
+				scanner.ProcessAttributeDataflow (sourceLocationMember, property.SetMethod, new List<CustomAttributeArgument> { namedArgument.Argument });
 			}
 		}
 
@@ -1066,7 +1065,7 @@ namespace Mono.Linker.Steps
 
 			if (field != null && _context.Annotations.FlowAnnotations.RequiresDataFlowAnalysis (field)) {
 				var scanner = new ReflectionMethodBodyScanner (_context, this);
-				scanner.ProcessAttributeDataflow (field, namedArgument.Argument);
+				scanner.ProcessAttributeDataflow (sourceLocationMember, field, namedArgument.Argument);
 			}
 		}
 
diff --git a/test/Mono.Linker.Tests.Cases.Expectations/Metadata/SetupCompileBeforeAttribute.cs b/test/Mono.Linker.Tests.Cases.Expectations/Metadata/SetupCompileBeforeAttribute.cs
index 54dd63027..0ede197cf 100644
--- a/test/Mono.Linker.Tests.Cases.Expectations/Metadata/SetupCompileBeforeAttribute.cs
+++ b/test/Mono.Linker.Tests.Cases.Expectations/Metadata/SetupCompileBeforeAttribute.cs
@@ -8,7 +8,7 @@ namespace Mono.Linker.Tests.Cases.Expectations.Metadata
 	[AttributeUsage (AttributeTargets.Class, AllowMultiple = true)]
 	public class SetupCompileBeforeAttribute : BaseMetadataAttribute
 	{
-		public SetupCompileBeforeAttribute (string outputName, string[] sourceFiles, string[] references = null, string[] defines = null, string[] resources = null, string additionalArguments = null, string compilerToUse = null, bool addAsReference = true)
+		public SetupCompileBeforeAttribute (string outputName, string[] sourceFiles, string[] references = null, string[] defines = null, string[] resources = null, string additionalArguments = null, string compilerToUse = null, bool addAsReference = true, bool removeFromLinkerInput = false)
 		{
 			if (sourceFiles == null)
 				throw new ArgumentNullException (nameof (sourceFiles));
@@ -17,7 +17,7 @@ namespace Mono.Linker.Tests.Cases.Expectations.Metadata
 				throw new ArgumentException ("Value cannot be null or empty.", nameof (outputName));
 		}
 
-		public SetupCompileBeforeAttribute (string outputName, Type[] typesToIncludeSourceFor, string[] references = null, string[] defines = null, string[] resources = null, string additionalArguments = null, string compilerToUse = null, bool addAsReference = true)
+		public SetupCompileBeforeAttribute (string outputName, Type[] typesToIncludeSourceFor, string[] references = null, string[] defines = null, string[] resources = null, string additionalArguments = null, string compilerToUse = null, bool addAsReference = true, bool removeFromLinkerInput = false)
 		{
 			if (typesToIncludeSourceFor == null)
 				throw new ArgumentNullException (nameof (typesToIncludeSourceFor));
diff --git a/test/Mono.Linker.Tests.Cases/DataFlow/Dependencies/UnresolvedLibrary.cs b/test/Mono.Linker.Tests.Cases/DataFlow/Dependencies/UnresolvedLibrary.cs
new file mode 100644
index 000000000..1ea25e791
--- /dev/null
+++ b/test/Mono.Linker.Tests.Cases/DataFlow/Dependencies/UnresolvedLibrary.cs
@@ -0,0 +1,17 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+// See the LICENSE file in the project root for more information.
+
+using System;
+using System.Collections.Generic;
+using System.Linq;
+using System.Text;
+using System.Threading.Tasks;
+
+namespace Mono.Linker.Tests.Cases.DataFlow.Dependencies
+{
+	public class UnresolvedType
+	{
+		public static void Method () { }
+	}
+}
diff --git a/test/Mono.Linker.Tests.Cases/DataFlow/UnresolvedMembers.cs b/test/Mono.Linker.Tests.Cases/DataFlow/UnresolvedMembers.cs
new file mode 100644
index 000000000..4bffba3f6
--- /dev/null
+++ b/test/Mono.Linker.Tests.Cases/DataFlow/UnresolvedMembers.cs
@@ -0,0 +1,130 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+// See the LICENSE file in the project root for more information.
+
+using System;
+using System.Collections.Generic;
+using System.Diagnostics.CodeAnalysis;
+using System.Linq;
+using System.Text;
+using System.Threading.Tasks;
+using Mono.Linker.Tests.Cases.Expectations.Assertions;
+using Mono.Linker.Tests.Cases.Expectations.Metadata;
+
+namespace Mono.Linker.Tests.Cases.DataFlow
+{
+	[SkipPeVerify]
+	[SetupLinkerArgument ("--skip-unresolved", "true")]
+	[SetupCompileBefore ("UnresolvedLibrary.dll", new[] { "Dependencies/UnresolvedLibrary.cs" }, removeFromLinkerInput: true)]
+	class UnresolvedMembers
+	{
+		public static void Main ()
+		{
+			UnresolvedGenericArgument ();
+			UnresolvedAttributeArgument ();
+			UnresolvedAttributePropertyValue ();
+			UnresolvedAttributeFieldValue ();
+			UnresolvedObjectGetType ();
+			UnresolvedMethodParameter ();
+		}
+
+		[Kept]
+		[KeptMember (".ctor()")]
+		class TypeWithUnresolvedGenericArgument<
+			[KeptAttributeAttribute (typeof (DynamicallyAccessedMembersAttribute))]
+		[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)] T>
+		{
+		}
+
+		[Kept]
+		static void MethodWithUnresolvedGenericArgument<
+			[KeptAttributeAttribute (typeof (DynamicallyAccessedMembersAttribute))]
+		[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)] T> ()
+		{ }
+
+		[Kept]
+		[ExpectedWarning ("IL2066", "TypeWithUnresolvedGenericArgument")]
+		[ExpectedWarning ("IL2066", nameof (MethodWithUnresolvedGenericArgument))]
+		static void UnresolvedGenericArgument ()
+		{
+			var a = new TypeWithUnresolvedGenericArgument<Dependencies.UnresolvedType> ();
+			MethodWithUnresolvedGenericArgument<Dependencies.UnresolvedType> ();
+		}
+
+		[Kept]
+		[KeptBaseType (typeof (Attribute))]
+		class AttributeWithRequirements : Attribute
+		{
+			[Kept]
+			public AttributeWithRequirements (
+				[KeptAttributeAttribute (typeof (DynamicallyAccessedMembersAttribute))]
+				[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicMethods)] Type type)
+			{ }
+
+			[Kept]
+			[KeptBackingField]
+			[KeptAttributeAttribute (typeof (DynamicallyAccessedMembersAttribute))]
+			[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)]
+			public Type PropertyWithRequirements { get; [Kept] set; }
+
+			[Kept]
+			[KeptAttributeAttribute (typeof (DynamicallyAccessedMembersAttribute))]
+			[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)]
+			public Type FieldWithRequirements;
+		}
+
+		[Kept]
+		[ExpectedWarning ("IL2062", nameof (AttributeWithRequirements))]
+		[KeptAttributeAttribute (typeof (AttributeWithRequirements))]
+		[AttributeWithRequirements (typeof (Dependencies.UnresolvedType))]
+		static void UnresolvedAttributeArgument ()
+		{
+		}
+
+		[Kept]
+		[ExpectedWarning ("IL2062", nameof (AttributeWithRequirements.PropertyWithRequirements))]
+		[KeptAttributeAttribute (typeof (AttributeWithRequirements))]
+		[AttributeWithRequirements (typeof (EmptyType), PropertyWithRequirements = typeof (Dependencies.UnresolvedType))]
+		static void UnresolvedAttributePropertyValue ()
+		{
+		}
+
+		[Kept]
+		[ExpectedWarning ("IL2064", nameof (AttributeWithRequirements.FieldWithRequirements))]
+		[KeptAttributeAttribute (typeof (AttributeWithRequirements))]
+		[AttributeWithRequirements (typeof (EmptyType), FieldWithRequirements = typeof (Dependencies.UnresolvedType))]
+		static void UnresolvedAttributeFieldValue ()
+		{
+		}
+
+		[Kept]
+		static Dependencies.UnresolvedType _unresolvedField;
+
+		[Kept]
+		[ExpectedWarning ("IL2072", nameof (Object.GetType))]
+		static void UnresolvedObjectGetType ()
+		{
+			RequirePublicMethods (_unresolvedField.GetType ());
+		}
+
+		[Kept]
+		[ExpectedWarning ("IL2072", nameof (Object.GetType))]
+		static void UnresolvedMethodParameter ()
+		{
+			RequirePublicMethods (typeof (Dependencies.UnresolvedType));
+		}
+
+		[Kept]
+		class EmptyType
+		{
+		}
+
+		[Kept]
+		static void RequirePublicMethods (
+			[KeptAttributeAttribute (typeof (DynamicallyAccessedMembersAttribute))]
+			[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)]
+			Type t)
+		{
+		}
+	}
+}
diff --git a/test/Mono.Linker.Tests/TestCasesRunner/ResultChecker.cs b/test/Mono.Linker.Tests/TestCasesRunner/ResultChecker.cs
index 7e69b201e..339da78a2 100644
--- a/test/Mono.Linker.Tests/TestCasesRunner/ResultChecker.cs
+++ b/test/Mono.Linker.Tests/TestCasesRunner/ResultChecker.cs
@@ -643,8 +643,10 @@ namespace Mono.Linker.Tests.TestCasesRunner
 
 						case nameof (ExpectedWarningAttribute): {
 								var expectedWarningCode = (string) attr.GetConstructorArgumentValue (0);
-								if (!expectedWarningCode.StartsWith ("IL"))
-									break;
+								if (!expectedWarningCode.StartsWith ("IL")) {
+									Assert.Fail ($"The warning code specified in ExpectedWarning attribute must start with the 'IL' prefix. Specified value: '{expectedWarningCode}'.");
+								}
+
 								int expectedWarningCodeNumber = int.Parse (expectedWarningCode.Substring (2));
 								var expectedMessageContains = ((CustomAttributeArgument[]) attr.GetConstructorArgumentValue (1)).Select (a => (string) a.Value).ToArray ();
 
diff --git a/test/Mono.Linker.Tests/TestCasesRunner/SetupCompileInfo.cs b/test/Mono.Linker.Tests/TestCasesRunner/SetupCompileInfo.cs
index d58da3930..7f1e92b64 100644
--- a/test/Mono.Linker.Tests/TestCasesRunner/SetupCompileInfo.cs
+++ b/test/Mono.Linker.Tests/TestCasesRunner/SetupCompileInfo.cs
@@ -13,5 +13,6 @@ namespace Mono.Linker.Tests.TestCasesRunner
 		public string AdditionalArguments;
 		public string CompilerToUse;
 		public bool AddAsReference;
+		public bool RemoveFromLinkerInput;
 	}
 }
diff --git a/test/Mono.Linker.Tests/TestCasesRunner/TestCaseCompiler.cs b/test/Mono.Linker.Tests/TestCasesRunner/TestCaseCompiler.cs
index 76dad5711..d16becb12 100644
--- a/test/Mono.Linker.Tests/TestCasesRunner/TestCaseCompiler.cs
+++ b/test/Mono.Linker.Tests/TestCasesRunner/TestCaseCompiler.cs
@@ -42,7 +42,8 @@ namespace Mono.Linker.Tests.TestCasesRunner
 
 			Prepare (outputDirectory);
 
-			var compiledReferences = CompileBeforeTestCaseAssemblies (outputDirectory, originalCommonReferences, originalDefines).ToArray ();
+			var removeFromLinkerInputAssemblies = new List<NPath> ();
+			var compiledReferences = CompileBeforeTestCaseAssemblies (outputDirectory, originalCommonReferences, originalDefines, removeFromLinkerInputAssemblies).ToArray ();
 			var allTestCaseReferences = originalCommonReferences
 				.Concat (compiledReferences)
 				.Concat (mainAssemblyReferences.Select (r => r.ToNPath ()))
@@ -61,8 +62,12 @@ namespace Mono.Linker.Tests.TestCasesRunner
 			// The compile after step is used by tests to mess around with the input to the linker.  Generally speaking, it doesn't seem like we would ever want to mess with the
 			// expectations assemblies because this would undermine our ability to inspect them for expected results during ResultChecking.  The UnityLinker UnresolvedHandling tests depend on this
 			// behavior of skipping the after test compile
-			if (outputDirectory != _sandbox.ExpectationsDirectory)
+			if (outputDirectory != _sandbox.ExpectationsDirectory) {
+				foreach (var assemblyToRemove in removeFromLinkerInputAssemblies)
+					assemblyToRemove.DeleteIfExists ();
+
 				CompileAfterTestCaseAssemblies (outputDirectory, originalCommonReferences, originalDefines);
+			}
 
 			return testAssembly;
 		}
@@ -100,7 +105,7 @@ namespace Mono.Linker.Tests.TestCasesRunner
 			};
 		}
 
-		private IEnumerable<NPath> CompileBeforeTestCaseAssemblies (NPath outputDirectory, NPath[] references, string[] defines)
+		private IEnumerable<NPath> CompileBeforeTestCaseAssemblies (NPath outputDirectory, NPath[] references, string[] defines, IList<NPath> removeFromLinkerInputAssemblies)
 		{
 			foreach (var setupCompileInfo in _metadataProvider.GetSetupCompileAssembliesBefore ()) {
 				var options = CreateOptionsForSupportingAssembly (
@@ -111,6 +116,10 @@ namespace Mono.Linker.Tests.TestCasesRunner
 					defines,
 					CollectSetupBeforeResourcesFiles (setupCompileInfo));
 				var output = CompileAssembly (options);
+
+				if (setupCompileInfo.RemoveFromLinkerInput)
+					removeFromLinkerInputAssemblies.Add (output);
+
 				if (setupCompileInfo.AddAsReference)
 					yield return output;
 			}
diff --git a/test/Mono.Linker.Tests/TestCasesRunner/TestCaseMetadaProvider.cs b/test/Mono.Linker.Tests/TestCasesRunner/TestCaseMetadaProvider.cs
index 59f49c4ee..e1474914d 100644
--- a/test/Mono.Linker.Tests/TestCasesRunner/TestCaseMetadaProvider.cs
+++ b/test/Mono.Linker.Tests/TestCasesRunner/TestCaseMetadaProvider.cs
@@ -332,7 +332,8 @@ namespace Mono.Linker.Tests.TestCasesRunner
 				Resources = ((CustomAttributeArgument[]) ctorArguments[4].Value)?.Select (arg => MakeSourceTreeFilePathAbsolute (arg.Value.ToString ())).ToArray (),
 				AdditionalArguments = (string) ctorArguments[5].Value,
 				CompilerToUse = (string) ctorArguments[6].Value,
-				AddAsReference = ctorArguments.Count >= 8 ? (bool) ctorArguments[7].Value : true
+				AddAsReference = ctorArguments.Count >= 8 ? (bool) ctorArguments[7].Value : true,
+				RemoveFromLinkerInput = ctorArguments.Count >= 9 ? (bool) ctorArguments[8].Value : false
 			};
 		}