Create separate attribute for warning behavior differences (dotnet#10…

…1220) To help track differences in the warning behavior of the trimming related tools, this modifies how adds UnexpectedWarning, and requires an issue link to be provided when there is a ProducedBy argument to the constructor. To enforce that either both a ProducedBy and IssueLink is provided or neither, the ProducedBy property is removed and is provided as the second to last argument, and IssueLink is provided as the last argument. ExpectedWarning means that the correct behavior is to warn. Any attributes that expect it only from a subset of the tools must provide an issue link. (These are mostly blank strings now, though) UnexpectedWarning means that this warning is not the correct behavior. These attributes always include a ProducedBy anrdshould link to an issue. Changes Look for a Tool attribute argument in the second to last position of ExpectedWarning and Unexpected warning when a ProducedBy property is not found. Find a replace existing ExpectedWarnings to use the new constructors. Adds issue links within AttributedMembersAccessedViaReflection.cs and in some places in ArrayDataFlow.cs
matouskozak · Apr 30, 2024 · f387d8e · f387d8e
1 parent ed963a7
commit f387d8e
Show file tree

Hide file tree

Showing 79 changed files with 1,642 additions and 1,754 deletions.
diff --git a/src/coreclr/tools/aot/ILCompiler.Trimming.Tests/TestCasesRunner/ResultChecker.cs b/src/coreclr/tools/aot/ILCompiler.Trimming.Tests/TestCasesRunner/ResultChecker.cs
@@ -159,6 +159,8 @@ protected virtual void AdditionalChecking (TrimmedTestCaseResult linkResult, Ass
 
 		private static bool IsProducedByNativeAOT (CustomAttribute attr)
 		{
+			if (attr.ConstructorArguments.Count > 2 && attr.ConstructorArguments[^2].Type.Name == "Tool")
+				return ((Tool)attr.ConstructorArguments[^2].Value).HasFlag(Tool.NativeAot);
 			var producedBy = attr.GetPropertyValue ("ProducedBy");
 			return producedBy is null ? true : ((Tool) producedBy).HasFlag (Tool.NativeAot);
 		}
@@ -227,12 +229,29 @@ private void VerifyLoggedMessages (AssemblyDefinition original, TrimmingTestLogg
 						}
 						break;
 
-					case nameof (ExpectedWarningAttribute): {
+					case nameof (ExpectedWarningAttribute) or nameof(UnexpectedWarningAttribute): {
 							var expectedWarningCode = (string) attr.GetConstructorArgumentValue (0);
 							if (!expectedWarningCode.StartsWith ("IL")) {
-								Assert.Fail ($"The warning code specified in {nameof (ExpectedWarningAttribute)} must start with the 'IL' prefix. Specified value: '{expectedWarningCode}'.");
+								Assert.Fail ($"The warning code specified in {attr.AttributeType.Name} must start with the 'IL' prefix. Specified value: '{expectedWarningCode}'.");
 							}
-							var expectedMessageContains = ((CustomAttributeArgument[]) attr.GetConstructorArgumentValue (1)).Select (a => (string) a.Value).ToArray ();
+							IEnumerable<string> expectedMessageContains = attr.Constructor.Parameters switch
+							{
+								// ExpectedWarningAttribute(string warningCode, params string[] expectedMessages)
+								// ExpectedWarningAttribute(string warningCode, string[] expectedMessages, Tool producedBy, string issueLink)
+								[_, { ParameterType.IsArray: true }, ..]
+									=> ((CustomAttributeArgument[])attr.ConstructorArguments[1].Value)
+										.Select(caa => (string)caa.Value),
+								// ExpectedWarningAttribute(string warningCode, string expectedMessage1, string expectedMessage2, Tool producedBy, string issueLink)
+								[_, { ParameterType.Name: "String" }, { ParameterType.Name: "String" }, { ParameterType.Name: "Tool" }, _]
+									=> [(string)attr.GetConstructorArgumentValue(1), (string)attr.GetConstructorArgumentValue(2)],
+								// ExpectedWarningAttribute(string warningCode, string expectedMessage, Tool producedBy, string issueLink)
+								[_, { ParameterType.Name: "String" }, { ParameterType.Name: "Tool" }, _]
+									=> [(string)attr.GetConstructorArgumentValue(1)],
+								// ExpectedWarningAttribute(string warningCode, Tool producedBy, string issueLink)
+								[_, { ParameterType.Name: "Tool" }, _]
+									=> [],
+								_ => throw new UnreachableException(),
+							};
 							string fileName = (string) attr.GetPropertyValue ("FileName")!;
 							int? sourceLine = (int?) attr.GetPropertyValue ("SourceLine");
 							int? sourceColumn = (int?) attr.GetPropertyValue ("SourceColumn");

diff --git a/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/TestChecker.cs b/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/TestChecker.cs
@@ -4,6 +4,7 @@
 using System;
 using System.Collections.Generic;
 using System.Collections.Immutable;
+using System.Diagnostics;
 using System.Diagnostics.CodeAnalysis;
 using System.Linq;
 using System.Text.RegularExpressions;
@@ -66,7 +67,7 @@ public void Check (bool allowMissingWarnings)
 			}
 
 			if (message.Length > 0) {
-				Assert.Fail(message);
+				Assert.Fail (message);
 			}
 		}
 
@@ -207,51 +208,62 @@ private void ValidateDiagnostics (CSharpSyntaxNode memberSyntax, SyntaxList<Attr
 
 		static bool IsExpectedDiagnostic (AttributeSyntax attribute)
 		{
-			switch (attribute.Name.ToString ()) {
-			case "ExpectedWarning":
-			case "LogContains":
+			switch (attribute.Name.ToString () + "Attribute") {
+			case nameof (ExpectedWarningAttribute):
+			case nameof (UnexpectedWarningAttribute):
+			case nameof (LogContainsAttribute):
 				var args = LinkerTestBase.GetAttributeArguments (attribute);
 				if (args.TryGetValue ("ProducedBy", out var producedBy)) {
 					// Skip if this warning is not expected to be produced by any of the analyzers that we are currently testing.
 					return GetProducedBy (producedBy).HasFlag (Tool.Analyzer);
 				}
+				var toolArg = args.Where (arg => arg.Key.StartsWith ('#')).Count () - 2;
+				if (args.TryGetValue ($"#{toolArg}", out var maybeProducedBy) && TryGetProducedBy (maybeProducedBy, out Tool producedByTool)) {
+					return producedByTool.HasFlag (Tool.Analyzer);
+				}
 
 				return true;
 			default:
 				return false;
 			}
 
-			static Tool GetProducedBy (ExpressionSyntax expression)
+			static bool TryGetProducedBy (ExpressionSyntax expression, out Tool producedBy)
 			{
-				var producedBy = (Tool) 0x0;
+				producedBy = (Tool) 0x0;
 				switch (expression) {
-				case BinaryExpressionSyntax binaryExpressionSyntax:
+				case BinaryExpressionSyntax binaryExpressionSyntax when binaryExpressionSyntax.Kind () == SyntaxKind.BitwiseOrExpression:
 					if (!Enum.TryParse<Tool> ((binaryExpressionSyntax.Left as MemberAccessExpressionSyntax)!.Name.Identifier.ValueText, out var besProducedBy))
-						throw new ArgumentException ("Expression must be a ProducedBy value", nameof (expression));
+						return false;
 					producedBy |= besProducedBy;
 					producedBy |= GetProducedBy (binaryExpressionSyntax.Right);
 					break;
 
 				case MemberAccessExpressionSyntax memberAccessExpressionSyntax:
 					if (!Enum.TryParse<Tool> (memberAccessExpressionSyntax.Name.Identifier.ValueText, out var maeProducedBy))
-						throw new ArgumentException ("Expression must be a ProducedBy value", nameof (expression));
+						return false;
 					producedBy |= maeProducedBy;
 					break;
 
 				default:
-					break;
+					return false;
 				}
 
-				return producedBy;
+				return true;
+			}
+
+			static Tool GetProducedBy (ExpressionSyntax expression)
+			{
+				return TryGetProducedBy (expression, out var tool) ? tool : throw new ArgumentException ("Expression must be a ProducedBy value", nameof (expression));
 			}
 		}
 
 		bool TryValidateExpectedDiagnostic (AttributeSyntax attribute, List<Diagnostic> diagnostics, [NotNullWhen (true)] out int? matchIndex, [NotNullWhen (false)] out string? missingDiagnosticMessage)
 		{
-			switch (attribute.Name.ToString ()) {
-			case "ExpectedWarning":
+			switch (attribute.Name.ToString () + "Attribute") {
+			case nameof (ExpectedWarningAttribute):
+			case nameof (UnexpectedWarningAttribute):
 				return TryValidateExpectedWarningAttribute (attribute!, diagnostics, out matchIndex, out missingDiagnosticMessage);
-			case "LogContains":
+			case nameof (LogContainsAttribute):
 				return TryValidateLogContainsAttribute (attribute!, diagnostics, out matchIndex, out missingDiagnosticMessage);
 			default:
 				throw new InvalidOperationException ($"Unsupported attribute type {attribute.Name}");
@@ -268,10 +280,29 @@ private bool TryValidateExpectedWarningAttribute (AttributeSyntax attribute, Lis
 			if (!expectedWarningCode.StartsWith ("IL"))
 				throw new InvalidOperationException ($"Expected warning code should start with \"IL\" prefix.");
 
-			List<string> expectedMessages = args
-				.Where (arg => arg.Key.StartsWith ("#") && arg.Key != "#0")
-				.Select (arg => LinkerTestBase.GetStringFromExpression (arg.Value, _semanticModel))
-				.ToList ();
+			List<string> expectedMessages = ((IMethodSymbol) (_semanticModel.GetSymbolInfo (attribute).Symbol!)).Parameters switch {
+				// ExpectedWarningAttribute(string warningCode, params string[] expectedMessages)
+				[_, { IsParams: true }]
+					=> args
+						.Where (arg => arg.Key.StartsWith ('#') && arg.Key != "#0")
+						.Select (arg => LinkerTestBase.GetStringFromExpression (arg.Value, _semanticModel))
+						.ToList (),
+				// ExpectedWarningAttribute(string warningCode, string[] expectedMessages, Tool producedBy, string issueLink)
+				[_, { Type.TypeKind: TypeKind.Array }, _, _]
+					=> ((CollectionExpressionSyntax) args["#1"]).Elements
+						.Select (arg => LinkerTestBase.GetStringFromExpression (((ExpressionElementSyntax) arg).Expression, _semanticModel))
+						.ToList (),
+				// ExpectedWarningAttribute(string warningCode, string expectedMessage, Tool producedBy, string issueLink)
+				[_, { Type.SpecialType: SpecialType.System_String }, _, _]
+					=> [LinkerTestBase.GetStringFromExpression (args["#1"], _semanticModel)],
+				// ExpectedWarningAttribute(string warningCode, string expectedMessage1, string expectedMessage2, Tool producedBy, string issueLink)
+				[_, { Type.SpecialType: SpecialType.System_String }, { Type.SpecialType: SpecialType.System_String }, _, _]
+					=> [LinkerTestBase.GetStringFromExpression (args["#1"], _semanticModel), LinkerTestBase.GetStringFromExpression (args["#2"], _semanticModel)],
+				// ExpectedWarningAttribute(string warningCode, Tool producedBy, string issueLink)
+				[_, _, _]
+					=> [],
+				_ => throw new UnreachableException (),
+			};
 
 			for (int i = 0; i < diagnostics.Count; i++) {
 				if (Matches (diagnostics[i])) {
@@ -318,7 +349,7 @@ private void ValidateLogDoesNotContainAttribute (AttributeSyntax attribute, IRea
 			Assert.False (args.ContainsKey ("#1"));
 			_ = LinkerTestBase.GetStringFromExpression (arg, _semanticModel);
 			if (LogContains (attribute, diagnosticMessages, out var matchIndex, out var findText)) {
-				Assert.Fail($"LogDoesNotContain failure: Text\n\"{findText}\"\nfound in diagnostic:\n {diagnosticMessages[(int) matchIndex]}");
+				Assert.Fail ($"LogDoesNotContain failure: Text\n\"{findText}\"\nfound in diagnostic:\n {diagnosticMessages[(int) matchIndex]}");
 			}
 		}
 

diff --git a/...Analyzer.Tests.Generator/ILLink.RoslynAnalyzer.Tests.TestCaseGenerator/DataFlowTests.g.cs b/...Analyzer.Tests.Generator/ILLink.RoslynAnalyzer.Tests.TestCaseGenerator/DataFlowTests.g.cs
@@ -19,12 +19,6 @@ public Task GenericParameterDataFlowMarking ()
 			return RunTest (allowMissingWarnings: true);
 		}
 
-		[Fact]
-		public Task MakeGenericDataflowIntrinsics ()
-		{
-			return RunTest (allowMissingWarnings: true);
-		}
-
 		[Fact]
 		public Task MethodByRefParameterDataFlow ()
 		{

diff --git a/...s/illink/test/Mono.Linker.Tests.Cases.Expectations/Assertions/ExpectedWarningAttribute.cs b/...s/illink/test/Mono.Linker.Tests.Cases.Expectations/Assertions/ExpectedWarningAttribute.cs
@@ -9,8 +9,27 @@ namespace Mono.Linker.Tests.Cases.Expectations.Assertions
 		AttributeTargets.Assembly | AttributeTargets.Struct | AttributeTargets.Class | AttributeTargets.Method | AttributeTargets.Property | AttributeTargets.Constructor | AttributeTargets.Field | AttributeTargets.Interface | AttributeTargets.Event,
 		AllowMultiple = true,
 		Inherited = false)]
+	/// <summary>
+	/// An attribute applied to a member to indicate that a warning is expected in ideal behavior, and is present in all tools
+	/// </summary>
 	public class ExpectedWarningAttribute : EnableLoggerAttribute
 	{
+		public ExpectedWarningAttribute (string warningCode, string[] messageContains, Tool producedBy, string issueLinkOrReason)
+		{
+		}
+
+		public ExpectedWarningAttribute (string warningCode, string messageContains, Tool producedBy, string issueLinkOrReason)
+		{
+		}
+
+		public ExpectedWarningAttribute (string warningCode, string messageContains, string messageContains2, Tool producedBy, string issueLinkOrReason)
+		{
+		}
+
+		public ExpectedWarningAttribute (string warningCode, Tool producedBy, string issueLinkOrReason)
+		{
+		}
+
 		public ExpectedWarningAttribute (string warningCode, params string[] messageContains)
 		{
 		}
@@ -19,12 +38,6 @@ public ExpectedWarningAttribute (string warningCode, params string[] messageCont
 		public int SourceLine { get; set; }
 		public int SourceColumn { get; set; }
 
-		/// <summary>
-		/// Property used by the result checkers of trimmer and analyzers to determine whether
-		/// the tool should have produced the specified warning on the annotated member.
-		/// </summary>
-		public Tool ProducedBy { get; set; } = Tool.TrimmerAnalyzerAndNativeAot;
-
 		public bool CompilerGeneratedCode { get; set; }
 	}
 }
diff --git a/...illink/test/Mono.Linker.Tests.Cases.Expectations/Assertions/UnexpectedWarningAttribute.cs b/...illink/test/Mono.Linker.Tests.Cases.Expectations/Assertions/UnexpectedWarningAttribute.cs
@@ -0,0 +1,26 @@
+// Copyright (c) .NET Foundation and contributors. All rights reserved.
+// Licensed under the MIT license. See LICENSE file in the project root for full license information.
+
+using System;
+
+namespace Mono.Linker.Tests.Cases.Expectations.Assertions
+{
+	[AttributeUsage (
+		AttributeTargets.Assembly | AttributeTargets.Struct | AttributeTargets.Class | AttributeTargets.Method | AttributeTargets.Property | AttributeTargets.Constructor | AttributeTargets.Field | AttributeTargets.Interface | AttributeTargets.Event,
+		AllowMultiple = true,
+		Inherited = false)]
+	/// <summary>
+	/// An attribute applied to a member to indicate that a warning is raised in tests, but should not be present in ideal behavior
+	/// </summary>
+	public class UnexpectedWarningAttribute : ExpectedWarningAttribute
+	{
+		public UnexpectedWarningAttribute (string warningCode, string[] messageContains, Tool producedBy, string issueLinkOrReason)
+			: base (warningCode, messageContains, producedBy, issueLinkOrReason) { }
+		public UnexpectedWarningAttribute (string warningCode, string messageContains, Tool producedBy, string issueLinkOrReason)
+			: base (warningCode, messageContains, producedBy, issueLinkOrReason) { }
+		public UnexpectedWarningAttribute (string warningCode, string messageContains, string messageContains2, Tool producedBy, string issueLinkOrReason)
+			: base (warningCode, messageContains, messageContains2, producedBy, issueLinkOrReason) { }
+		public UnexpectedWarningAttribute (string warningCode, Tool producedBy, string issueLinkOrReason)
+			: base (warningCode, producedBy, issueLinkOrReason) { }
+	}
+}
diff --git a/src/tools/illink/test/Mono.Linker.Tests.Cases/CoreLink/InvalidIsTrimmableAttribute.cs b/src/tools/illink/test/Mono.Linker.Tests.Cases/CoreLink/InvalidIsTrimmableAttribute.cs
@@ -29,4 +29,4 @@ public static void Unused ()
 		{
 		}
 	}
-}
+}