Apply suggestions from code review

src/ILLink.Shared/SharedStrings.resx

@@ -1192,9 +1192,9 @@
     <value>Unrecognized dependencies file type.</value>
   </data>
   <data name="RedundantSuppressionMessage" xml:space="preserve">
-    <value>Unused UnconditionalSuppressMessageAttribute suppressing the '{0}' warning found. Consider removing the unused warning suppression.</value>
+    <value>Unused 'UnconditionalSuppressMessageAttribute' for warning '{0}'. Consider removing the unused warning suppression.</value>
   </data>
   <data name="RedundantSuppressionTitle" xml:space="preserve">
-    <value>Unused UnconditionalSuppressMessageAttribute found. Consider removing the unused warning suppression.</value>
+    <value>Unused 'UnconditionalSuppressMessageAttribute' found. Consider removing the unused warning suppression.</value>
   </data>
 </root>

src/linker/Linker.Steps/CheckSuppressionsDispatcher.cs

@@ -25,7 +25,6 @@ public override void Process (LinkContext context)
 				.Where (suppression => ((DiagnosticId) suppression.suppressMessageInfo.Id).GetDiagnosticCategory () == DiagnosticCategory.Trimming);
 
 			foreach (var (provider, suppressMessageInfo) in redundantSuppressions) {
-
 				var source = GetSuppresionProvider (provider);
 
 				context.LogWarning (new MessageOrigin (source), DiagnosticId.RedundantSuppression, $"IL{suppressMessageInfo.Id:0000}");

src/linker/Linker.Steps/CheckSuppressionsStep.cs

@@ -18,6 +18,9 @@ public override SubStepTargets Targets {
 
 		public override bool IsActiveFor (AssemblyDefinition assembly)
 		{
+			// Only process assemblies which went through marking. 
+			// The code relies on MarkStep to identify the useful suppressions.
+			// Assemblies which didn't go through marking would not produce any warnings and thus would report all suppressions as redundant.
 			var assemblyAction = Annotations.GetAction (assembly);
 			return assemblyAction == AssemblyAction.Link || assemblyAction == AssemblyAction.Copy;
 		}

src/linker/Linker/UnconditionalSuppressMessageAttributeState.cs

@@ -122,13 +122,17 @@ bool IsSuppressedOnElement (int id, ICustomAttributeProvider? provider, out Supp
 			info = default;
 
 			if (TryGetSuppressionsForProvider (provider, out var suppressions)) {
-				return TryGetSuppression (suppressions, id, out info);
+				if (suppressions != null && suppressions.TryGetValue (id, out var suppression)) {
+					suppression.Used = true;
+					info = suppression.SuppressMessageInfo;
+					return true;
+				}
 			}
 
 			return false;
 		}
 
-		public bool TryGetSuppressionsForProvider (ICustomAttributeProvider? provider, out Dictionary<int, Suppression>? suppressions)
+		bool TryGetSuppressionsForProvider (ICustomAttributeProvider? provider, out Dictionary<int, Suppression>? suppressions)
 		{
 			suppressions = null;
 			if (provider == null)
@@ -171,18 +175,6 @@ public bool TryGetSuppressionsForProvider (ICustomAttributeProvider? provider, o
 			return false;
 		}
 
-		static bool TryGetSuppression (Dictionary<int, Suppression>? suppressions, int id, out SuppressMessageInfo info)
-		{
-			info = default;
-			if (suppressions != null && suppressions.TryGetValue (id, out var suppression)) {
-				suppression.Used = true;
-				info = suppression.SuppressMessageInfo;
-				return true;
-			}
-
-			return false;
-		}
-
 		static bool TryDecodeSuppressMessageAttributeData (CustomAttribute attribute, out SuppressMessageInfo info)
 		{
 			info = default;

src/linker/Resources/Strings.resx

@@ -195,7 +195,4 @@
   <data name="IL2103" xml:space="preserve">
     <value>Value passed to the '{0}' parameter of method '{1}' cannot be statically determined as a property accessor.</value>
   </data>
-  <data name="IL2121" xml:space="preserve">
-    <value>Unused UnconditionalSuppressMessageAttribute suppressing the '{0}' warning found. Consider removing the unused warning suppression.</value>
-  </data>
 </root>

test/ILLink.RoslynAnalyzer.Tests/Warnings.WarningSuppressionTests.cs

@@ -27,11 +27,5 @@ public Task SuppressWarningsViaXml ()
 		{
 			return RunTest (allowMissingWarnings: true);
 		}
-
-		[Fact (Skip = "https://github.com/dotnet/linker/issues/2579")]
-		public Task DetectRedundantSuppressionsFromXML ()
-		{
-			return RunTest (allowMissingWarnings: true);
-		}
 	}
 }

test/ILLink.RoslynAnalyzer.Tests/generated/ILLink.RoslynAnalyzer.Tests.Generator/ILLink.RoslynAnalyzer.Tests.TestCaseGenerator/Warnings.WarningSuppressionTests.g.cs

@@ -19,6 +19,12 @@ public Task DetectRedundantSuppressionsFeatureSubstitutions ()
 			return RunTest (allowMissingWarnings: true);
 		}
 
+		[Fact]
+		public Task DetectRedundantSuppressionsFromXML ()
+		{
+			return RunTest (allowMissingWarnings: true);
+		}
+
 		[Fact]
 		public Task DetectRedundantSuppressionsInAssembly ()
 		{

test/Mono.Linker.Tests.Cases/Warnings/WarningSuppression/DetectRedundantSuppressionsFeatureSubstitutions.cs

@@ -15,7 +15,6 @@ namespace Mono.Linker.Tests.Cases.Warnings.WarningSuppression
 	[SkipKeptItemsValidation]
 	public class DetectRedundantSuppressionsFeatureSubstitutions
 	{
-		// https://github.com/dotnet/linker/issues/2920
 		public static void Main ()
 		{
 			ReportRedundantSuppressionWhenTrimmerIncompatibleCodeDisabled.Test ();
@@ -38,6 +37,12 @@ public static bool IsFeatureEnabled {
 
 		class ReportRedundantSuppressionWhenTrimmerIncompatibleCodeDisabled
 		{
+			// The test simulates the following issue.
+			// https://github.com/dotnet/linker/issues/2921
+			// The suppressed warning is issued in the 'if' branch.
+			// With feature switched to false, the linker sees only the 'else' branch.
+			// The 'else' branch contains trimmer-compatible code, the linker identifies the suppression as redundant.
+
 			[ExpectedWarning ("IL2121", "IL2072")]
 			[UnconditionalSuppressMessage ("Test", "IL2072")]
 			public static void Test ()

test/Mono.Linker.Tests.Cases/Warnings/WarningSuppression/DetectRedundantSuppressionsFromXML.cs

@@ -23,6 +23,8 @@ public static void Main ()
 		[ExpectedWarning ("IL2121", "IL2109", ProducedBy = ProducedBy.Trimmer)]
 		public class DetectRedundantSuppressions
 		{
+			// The warning should ideally point to XML.
+			// https://github.com/dotnet/linker/issues/2923
 			[ExpectedWarning ("IL2121", "IL2026", ProducedBy = ProducedBy.Trimmer)]
 			public static void Test ()
 			{

test/Mono.Linker.Tests.Cases/Warnings/WarningSuppression/DetectRedundantSuppressionsInMembersAndTypes.cs

@@ -44,7 +44,7 @@ public class RedundantSuppressionOnType
 		{
 			public static void Test ()
 			{
-				DetectRedundantSuppressionsInMembersAndTypes.TrimmerCompatibleMethod ();
+				TrimmerCompatibleMethod ();
 			}
 		}
 
@@ -54,7 +54,7 @@ public class RedundantSuppressionOnMethod
 			[UnconditionalSuppressMessage ("Test", "IL2071")]
 			public static void Test ()
 			{
-				DetectRedundantSuppressionsInMembersAndTypes.TrimmerCompatibleMethod ();
+				TrimmerCompatibleMethod ();
 			}
 		}
 
@@ -66,7 +66,7 @@ public static void Test ()
 				[UnconditionalSuppressMessage ("Test", "IL2071")]
 				void LocalMethod ()
 				{
-					DetectRedundantSuppressionsInMembersAndTypes.TrimmerCompatibleMethod ();
+					TrimmerCompatibleMethod ();
 				}
 
 				LocalMethod ();
@@ -102,7 +102,7 @@ public static string TrimmerCompatibleProperty {
 				[ExpectedWarning ("IL2121", "IL2071", ProducedBy = ProducedBy.Trimmer)]
 				[UnconditionalSuppressMessage ("Test", "IL2071")]
 				get {
-					return DetectRedundantSuppressionsInMembersAndTypes.TrimmerCompatibleMethod ();
+					return TrimmerCompatibleMethod ();
 				}
 			}
 		}
@@ -119,7 +119,7 @@ public class MultipleRedundantSuppressions
 			[UnconditionalSuppressMessage ("Test", "IL2071")]
 			public static void Test ()
 			{
-				DetectRedundantSuppressionsInMembersAndTypes.TrimmerCompatibleMethod ();
+				TrimmerCompatibleMethod ();
 			}
 		}
 
@@ -130,7 +130,7 @@ public class RedundantAndUsedSuppressions
 			[UnconditionalSuppressMessage ("Test", "IL2072")]
 			public static void Test ()
 			{
-				Expression.Call (DetectRedundantSuppressionsInMembersAndTypes.TriggerUnrecognizedPattern (), "", Type.EmptyTypes);
+				Expression.Call (TriggerUnrecognizedPattern (), "", Type.EmptyTypes);
 			}
 		}
 
@@ -139,13 +139,15 @@ public class DoNotReportNonLinkerSuppressions
 			[UnconditionalSuppressMessage ("Test", "IL3052")]
 			public static void Test ()
 			{
-				DetectRedundantSuppressionsInMembersAndTypes.TrimmerCompatibleMethod ();
+				TrimmerCompatibleMethod ();
 			}
 		}
 
-		// Methods with unreachable bodies are not processed https://github.com/dotnet/linker/issues/2921
 		public class DoNotReportSuppressionsOnMethodsConvertedToThrow
 		{
+			// The tool is unable to determine whether a suppression is redundant when it is placed on a method with unreachable body.
+			// Currently suppressions on methods with unreachable bodies should never be reported as redundant.
+			// https://github.com/dotnet/linker/issues/2920
 			public static void Test ()
 			{
 				UsedToMarkMethod (null);
@@ -158,10 +160,11 @@ static void UsedToMarkMethod (TypeWithMethodConvertedToThrow t)
 
 			class TypeWithMethodConvertedToThrow
 			{
+				// The suppression is redundant, but it should not be reported.
 				[UnconditionalSuppressMessage ("Test", "IL2072")]
 				public void MethodConvertedToThrow ()
 				{
-					DetectRedundantSuppressionsInMembersAndTypes.TrimmerCompatibleMethod ();
+					TrimmerCompatibleMethod ();
 				}
 			}
 		}
@@ -172,7 +175,7 @@ public class SuppressRedundantSuppressionWarning
 			[UnconditionalSuppressMessage ("Test", "IL2072")]
 			public static void Test ()
 			{
-				DetectRedundantSuppressionsInMembersAndTypes.TrimmerCompatibleMethod ();
+				TrimmerCompatibleMethod ();
 			}
 		}
 	}

test/Mono.Linker.Tests.Cases/Warnings/WarningSuppression/DetectRedundantSuppressionsInMembersAndTypesUsingTarget.cs

@@ -12,6 +12,9 @@
 [module: UnconditionalSuppressMessage ("Test", "IL2071", Scope = "type", Target = "T:Mono.Linker.Tests.Cases.Warnings.WarningSuppression.DetectRedundantSuppressionsInMembersAndTypesUsingTarget.RedundantSuppressionOnNestedType.NestedType")]
 [module: UnconditionalSuppressMessage ("Test", "IL2071", Scope = "member", Target = "M:Mono.Linker.Tests.Cases.Warnings.WarningSuppression.DetectRedundantSuppressionsInMembersAndTypesUsingTarget.RedundantSuppressionOnProperty.get_TrimmerCompatibleProperty")]
 
+// The IL2121 warnings are reported on the suppressions targets.
+// When the suppressions are declared on the assembly level, ideally they should also be reported on the assembly level.
+
 namespace Mono.Linker.Tests.Cases.Warnings.WarningSuppression
 {
 	[ExpectedNoWarnings]
@@ -41,7 +44,7 @@ public class RedundantSuppressionOnType
 		{
 			public static void Test ()
 			{
-				DetectRedundantSuppressionsInMembersAndTypesUsingTarget.TrimmerCompatibleMethod ();
+				TrimmerCompatibleMethod ();
 			}
 		}
 
@@ -50,7 +53,7 @@ public class RedundantSuppressionOnMethod
 			[ExpectedWarning ("IL2121", "IL2071", ProducedBy = ProducedBy.Trimmer)]
 			public static void Test ()
 			{
-				DetectRedundantSuppressionsInMembersAndTypesUsingTarget.TrimmerCompatibleMethod ();
+				TrimmerCompatibleMethod ();
 			}
 		}
 
@@ -66,7 +69,7 @@ public class NestedType
 			{
 				public static void TrimmerCompatibleMethod ()
 				{
-					DetectRedundantSuppressionsInMembersAndTypesUsingTarget.TrimmerCompatibleMethod ();
+					TrimmerCompatibleMethod ();
 				}
 			}
 		}
@@ -81,7 +84,7 @@ public static void Test ()
 			public static string TrimmerCompatibleProperty {
 				[ExpectedWarning ("IL2121", "IL2071", ProducedBy = ProducedBy.Trimmer)]
 				get {
-					return DetectRedundantSuppressionsInMembersAndTypesUsingTarget.TrimmerCompatibleMethod ();
+					return TrimmerCompatibleMethod ();
 				}
 			}
 		}

test/Mono.Linker.Tests.Cases/Warnings/WarningSuppression/SuppressWarningsInCompilerGeneratedCode.cs

@@ -537,7 +537,6 @@ IEnumerable<int> LocalFunction ()
 				}
 			}
 
-			//[UnconditionalSuppressMessage ("Test", "IL2026")] // ask
 			static async void TestIteratorLocalFunctionInAsync ()
 			{
 				await MethodAsync ();