Fix overreporting of warnings due to RequiresUnreferencedCode (#1798)

Linker has intrinsic handling for some methods which are annotated with `RequiresUnreferencedCodeAttribute`. The intrinsic handling will sometimes recognize the pattern and avoid generating a warning, at other times it will generate a better more specific warning. In cases where this happens the generic handler for `RequiresUnreferencedCodeAttribute` should not kick in, as it would still produce a warning.

The change:
* For direct calls, virtual calls and `.ctor` calls rely on `ReflectionMethodBodyScanner` to handle all warnings caused by `RequiresUnreferencedCodeAttribute`. It has knowledge about intrinsic handling and can handle all non-intrisic calls as well.
* Limit the generic handler in `MarkMethod` to all other cases (typically reflection or other indirect references to the method)

Test changes:
* We already have test cases which trigger these conditions, but we don't have a way to validate that extra warnings were not produced
* Added `ExpectedNoWarnings` test attribute which validates no warnings other than those explicitly stated by the test are produced.
* Used it on two test cases which trigger the interesting condition - and fixed the tests to correctly expect all warnings

2 files changed, 86 insertions, 33 deletions

diff --git a/test/Mono.Linker.Tests/TestCases/Dependencies/SortedWarnings.txt b/test/Mono.Linker.Tests/TestCases/Dependencies/SortedWarnings.txt
index ff53d8753..67d635f2f 100644
--- a/test/Mono.Linker.Tests/TestCases/Dependencies/SortedWarnings.txt
+++ b/test/Mono.Linker.Tests/TestCases/Dependencies/SortedWarnings.txt
@@ -3,13 +3,8 @@ ILLink: Trim analysis warning IL2072: Mono.Linker.Tests.Cases.Warnings.Dependenc
 ILLink: Trim analysis warning IL2072: Mono.Linker.Tests.Cases.Warnings.Dependencies.TriggerWarnings_Lib.Warning2.get: '#0' argument does not satisfy 'DynamicallyAccessedMemberTypes.PublicMethods', 'DynamicallyAccessedMemberTypes.NonPublicMethods' in call to 'System.Linq.Expressions.MethodCallExpression System.Linq.Expressions.Expression::Call(System.Type,System.String,System.Type[],System.Linq.Expressions.Expression[])'. The return value of method 'Mono.Linker.Tests.Cases.Warnings.Dependencies.TriggerWarnings_Lib.TriggerUnrecognizedPattern()' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to.
 ILLink: Trim analysis warning IL2072: Mono.Linker.Tests.Cases.Warnings.Dependencies.TriggerWarnings_Lib.Main(): '#0' argument does not satisfy 'DynamicallyAccessedMemberTypes.PublicMethods', 'DynamicallyAccessedMemberTypes.NonPublicMethods' in call to 'System.Linq.Expressions.MethodCallExpression System.Linq.Expressions.Expression::Call(System.Type,System.String,System.Type[],System.Linq.Expressions.Expression[])'. The return value of method 'Mono.Linker.Tests.Cases.Warnings.Dependencies.TriggerWarnings_Lib.TriggerUnrecognizedPattern()' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to.
 ILLink: Trim analysis warning IL2072: Mono.Linker.Tests.Cases.Warnings.Dependencies.TriggerWarnings_Lib.Warning1(): '#0' argument does not satisfy 'DynamicallyAccessedMemberTypes.PublicMethods', 'DynamicallyAccessedMemberTypes.NonPublicMethods' in call to 'System.Linq.Expressions.MethodCallExpression System.Linq.Expressions.Expression::Call(System.Type,System.String,System.Type[],System.Linq.Expressions.Expression[])'. The return value of method 'Mono.Linker.Tests.Cases.Warnings.Dependencies.TriggerWarnings_Lib.TriggerUnrecognizedPattern()' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to.
-ILLink: Trim analysis warning IL2026: Mono.Linker.Tests.Cases.Warnings.Individual.WarningsAreSorted.A.X(): 'System.Type.GetType(String)' method has 'RequiresUnreferencedCodeAttribute' which can break functionality when trimming application code. The type might be removed.
 ILLink: Trim analysis warning IL2075: Mono.Linker.Tests.Cases.Warnings.Individual.WarningsAreSorted.A.X(): 'this' argument does not satisfy 'DynamicallyAccessedMemberTypes.PublicMethods' in call to 'System.Type.GetMethod(String)'. The return value of method 'System.Type.GetType(String)' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to.
-ILLink: Trim analysis warning IL2026: Mono.Linker.Tests.Cases.Warnings.Individual.WarningsAreSorted.A.Y(): 'System.Type.GetType(String)' method has 'RequiresUnreferencedCodeAttribute' which can break functionality when trimming application code. The type might be removed.
 ILLink: Trim analysis warning IL2075: Mono.Linker.Tests.Cases.Warnings.Individual.WarningsAreSorted.A.Y(): 'this' argument does not satisfy 'DynamicallyAccessedMemberTypes.PublicMethods' in call to 'System.Type.GetMethod(String)'. The return value of method 'System.Type.GetType(String)' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to.
-ILLink: Trim analysis warning IL2026: Mono.Linker.Tests.Cases.Warnings.Individual.WarningsAreSorted.B.X(): 'System.Type.GetType(String)' method has 'RequiresUnreferencedCodeAttribute' which can break functionality when trimming application code. The type might be removed.
 ILLink: Trim analysis warning IL2075: Mono.Linker.Tests.Cases.Warnings.Individual.WarningsAreSorted.B.X(): 'this' argument does not satisfy 'DynamicallyAccessedMemberTypes.PublicMethods' in call to 'System.Type.GetMethod(String)'. The return value of method 'System.Type.GetType(String)' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to.
-ILLink: Trim analysis warning IL2026: Mono.Linker.Tests.Cases.Warnings.Individual.WarningsAreSorted.B.Y(): 'System.Type.GetType(String)' method has 'RequiresUnreferencedCodeAttribute' which can break functionality when trimming application code. The type might be removed.
 ILLink: Trim analysis warning IL2075: Mono.Linker.Tests.Cases.Warnings.Individual.WarningsAreSorted.B.Y(): 'this' argument does not satisfy 'DynamicallyAccessedMemberTypes.PublicMethods' in call to 'System.Type.GetMethod(String)'. The return value of method 'System.Type.GetType(String)' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to.
-ILLink: Trim analysis warning IL2026: Mono.Linker.Tests.Cases.Warnings.Individual.WarningsAreSorted.B.Z(): 'System.Type.GetType(String)' method has 'RequiresUnreferencedCodeAttribute' which can break functionality when trimming application code. The type might be removed.
 ILLink: Trim analysis warning IL2075: Mono.Linker.Tests.Cases.Warnings.Individual.WarningsAreSorted.B.Z(): 'this' argument does not satisfy 'DynamicallyAccessedMemberTypes.PublicMethods' in call to 'System.Type.GetMethod(String)'. The return value of method 'System.Type.GetType(String)' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to.
\ No newline at end of file
diff --git a/test/Mono.Linker.Tests/TestCasesRunner/ResultChecker.cs b/test/Mono.Linker.Tests/TestCasesRunner/ResultChecker.cs
index 32177b27c..03fe04760 100644
--- a/test/Mono.Linker.Tests/TestCasesRunner/ResultChecker.cs
+++ b/test/Mono.Linker.Tests/TestCasesRunner/ResultChecker.cs
@@ -606,6 +606,7 @@ namespace Mono.Linker.Tests.TestCasesRunner
 		void VerifyLoggedMessages (AssemblyDefinition original, LinkerTestLogger logger, bool checkRemainingErrors)
 		{
 			List<MessageContainer> loggedMessages = logger.GetLoggedMessages ();
+			List<(IMemberDefinition, CustomAttribute)> expectedNoWarningsAttributes = new List<(IMemberDefinition, CustomAttribute)> ();
 			foreach (var testType in original.AllDefinedTypes ()) {
 				foreach (var attrProvider in testType.AllMembers ().Append (testType)) {
 					foreach (var attr in attrProvider.CustomAttributes) {
@@ -644,60 +645,117 @@ namespace Mono.Linker.Tests.TestCasesRunner
 						case nameof (ExpectedWarningAttribute): {
 								var expectedWarningCode = (string) attr.GetConstructorArgumentValue (0);
 								if (!expectedWarningCode.StartsWith ("IL")) {
-									Assert.Fail ($"The warning code specified in ExpectedWarning attribute must start with the 'IL' prefix. Specified value: '{expectedWarningCode}'.");
+									Assert.Fail ($"The warning code specified in {nameof (ExpectedWarningAttribute)} 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 ();
-
 								string fileName = (string) attr.GetPropertyValue ("FileName");
 								int? sourceLine = (int?) attr.GetPropertyValue ("SourceLine");
 								int? sourceColumn = (int?) attr.GetPropertyValue ("SourceColumn");
 
+								int expectedWarningCodeNumber = int.Parse (expectedWarningCode.Substring (2));
 								var actualMethod = attrProvider as MethodDefinition;
 
-								Assert.IsTrue (
-									logger.GetLoggedMessages ().Any (mc => {
-										if (mc.Category != MessageCategory.Warning || mc.Code != expectedWarningCodeNumber)
+								var matchedMessages = loggedMessages.Where (mc => {
+									if (mc.Category != MessageCategory.Warning || mc.Code != expectedWarningCodeNumber)
+										return false;
+
+									foreach (var expectedMessage in expectedMessageContains)
+										if (!mc.Text.Contains (expectedMessage))
 											return false;
 
-										foreach (var expectedMessage in expectedMessageContains)
-											if (!mc.Text.Contains (expectedMessage))
-												return false;
+									if (fileName != null) {
+										// Note: string.Compare(string, StringComparison) doesn't exist in .NET Framework API set
+										if (mc.Origin?.FileName?.IndexOf (fileName, StringComparison.OrdinalIgnoreCase) < 0)
+											return false;
 
-										if (fileName != null) {
-											// Note: string.Compare(string, StringComparison) doesn't exist in .NET Framework API set
-											if (mc.Origin?.FileName?.IndexOf (fileName, StringComparison.OrdinalIgnoreCase) < 0)
-												return false;
+										if (sourceLine != null && mc.Origin?.SourceLine != sourceLine.Value)
+											return false;
 
-											if (sourceLine != null && mc.Origin?.SourceLine != sourceLine.Value)
-												return false;
+										if (sourceColumn != null && mc.Origin?.SourceColumn != sourceColumn.Value)
+											return false;
+									} else {
+										if (mc.Origin?.MemberDefinition?.FullName == attrProvider.FullName)
+											return true;
 
-											if (sourceColumn != null && mc.Origin?.SourceColumn != sourceColumn.Value)
-												return false;
-										} else {
-											if (mc.Origin?.MemberDefinition?.FullName == attrProvider.FullName)
-												return true;
+										if (loggedMessages.Any (m => m.Text.Contains (attrProvider.FullName)))
+											return true;
 
-											if (loggedMessages.Any (m => m.Text.Contains (attrProvider.FullName)))
-												return true;
+										return false;
+									}
 
-											return false;
-										}
+									return true;
+								}).ToList ();
 
-										return true;
-									}),
+								Assert.IsTrue (
+									matchedMessages.Count > 0,
 									$"Expected to find warning: {(fileName != null ? (fileName + (sourceLine != null ? $"({sourceLine},{sourceColumn})" : "")) + ": " : "")}" +
 									$"warning {expectedWarningCode}: {(fileName == null ? (actualMethod?.GetDisplayName () ?? attrProvider.FullName) + ": " : "")}" +
 									$"and message containing {string.Join (" ", expectedMessageContains.Select (m => "'" + m + "'"))}, " +
 									$"but no such message was found.{Environment.NewLine}Logged messages:{Environment.NewLine}{string.Join (Environment.NewLine, loggedMessages)}");
+
+								foreach (var matchedMessage in matchedMessages)
+									loggedMessages.Remove (matchedMessage);
+							}
+							break;
+
+						// These are validated in VerifyRecordedReflectionPatterns as well, but we need to remove any warnings these might refer to from the list
+						// so that we can correctly validate presense of warnings
+						case nameof (UnrecognizedReflectionAccessPatternAttribute): {
+								string expectedWarningCode = null;
+								if (attr.ConstructorArguments.Count >= 5) {
+									expectedWarningCode = (string) attr.ConstructorArguments[4].Value;
+									if (expectedWarningCode != null && !expectedWarningCode.StartsWith ("IL"))
+										Assert.Fail ($"The warning code specified in {nameof (UnrecognizedReflectionAccessPatternAttribute)} must start with the 'IL' prefix. Specified value: '{expectedWarningCode}'");
+								}
+
+								if (expectedWarningCode != null) {
+									int expectedWarningCodeNumber = int.Parse (expectedWarningCode.Substring (2));
+
+									var matchedMessages = loggedMessages.Where (mc => mc.Category == MessageCategory.Warning && mc.Code == expectedWarningCodeNumber).ToList ();
+									foreach (var matchedMessage in matchedMessages)
+										loggedMessages.Remove (matchedMessage);
+								}
 							}
 							break;
+
+						case nameof (ExpectedNoWarningsAttribute):
+							// Postpone processing of negative checks, to make it possible to mark some warnings as expected (will be removed from the list above)
+							// and then do the negative check on the rest.
+							expectedNoWarningsAttributes.Add ((attrProvider, attr));
+							break;
 						}
 					}
 				}
 			}
 
+			foreach ((var attrProvider, var attr) in expectedNoWarningsAttributes) {
+				var unexpectedWarningCode = attr.ConstructorArguments.Count == 0 ? (string) null : (string) attr.GetConstructorArgumentValue (0);
+				if (unexpectedWarningCode != null && !unexpectedWarningCode.StartsWith ("IL")) {
+					Assert.Fail ($"The warning code specified in ExpectedWarning attribute must start with the 'IL' prefix. Specified value: '{unexpectedWarningCode}'.");
+				}
+
+				int? unexpectedWarningCodeNumber = unexpectedWarningCode == null ? null : int.Parse (unexpectedWarningCode.Substring (2));
+
+				MessageContainer? unexpectedWarningMessage = null;
+				foreach (var mc in logger.GetLoggedMessages ()) {
+					if (mc.Category != MessageCategory.Warning)
+						continue;
+
+					if (unexpectedWarningCodeNumber != null && unexpectedWarningCodeNumber.Value != mc.Code)
+						continue;
+
+					// This is a hacky way to say anything in the "subtree" of the attrProvider
+					if (mc.Origin?.MemberDefinition?.FullName.Contains (attrProvider.FullName) != true)
+						continue;
+
+					unexpectedWarningMessage = mc;
+					break;
+				}
+
+				Assert.IsNull (unexpectedWarningMessage,
+					$"Unexpected warning found: {unexpectedWarningMessage}");
+			}
+
 			if (checkRemainingErrors) {
 				var remainingErrors = loggedMessages.Where (m => Regex.IsMatch (m.ToString (), @".*(error | warning): \d{4}.*"));
 				Assert.IsEmpty (remainingErrors, $"Found unexpected errors:{Environment.NewLine}{string.Join (Environment.NewLine, remainingErrors)}");
@@ -811,7 +869,7 @@ namespace Mono.Linker.Tests.TestCasesRunner
 							var codeString = (string) attr.ConstructorArguments[4].Value;
 							if (codeString != null) {
 								if (!codeString.StartsWith ("IL"))
-									throw new InvalidOperationException ($"invalid message code {codeString}");
+									Assert.Fail ($"The warning code specified in {nameof (UnrecognizedReflectionAccessPatternAttribute)} must start with the 'IL' prefix. Specified value: '{codeString}'");
 								expectedMessageCode = int.Parse (codeString.Substring (2));
 							}
 						}