Redundant suppressions detection

src/ILLink.Shared/DiagnosticId.cs

@@ -185,6 +185,7 @@ public enum DiagnosticId
 		CompilerGeneratedMemberAccessedViaReflection = 2118,
 		DynamicallyAccessedMembersOnTypeReferencesCompilerGeneratedMember = 2119,
 		DynamicallyAccessedMembersOnTypeReferencesCompilerGeneratedMemberOnBase = 2120,
+		RedundantSuppression = 2121,
 
 		// Single-file diagnostic ids.
 		AvoidAssemblyLocationInSingleFile = 3000,

src/ILLink.Shared/SharedStrings.resx

@@ -1191,4 +1191,10 @@
   <data name="InvalidDependenciesFileFormatTitle" xml:space="preserve">
     <value>Unrecognized dependencies file type.</value>
   </data>
+  <data name="RedundantSuppressionMessage" xml:space="preserve">
+    <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>
+  </data>
 </root>

src/ILLink.Tasks/build/Microsoft.NET.ILLink.targets

@@ -93,6 +93,11 @@ Copyright (c) .NET Foundation. All rights reserved.
     <NoWarn>$(NoWarn);IL2118;IL2119;IL2120</NoWarn>
   </PropertyGroup>
 
+  <!-- Disable Redundant Warning Suppressions by default-->
+  <PropertyGroup Condition="'$(_TrimmerShowRedundantSuppressions)' != 'true'">
+    <NoWarn>$(NoWarn);IL2121</NoWarn>
+  </PropertyGroup>
+
   <!--
     ============================================================
                      ILLink

src/linker/Linker.Steps/CheckSuppressionsDispatcher.cs

@@ -0,0 +1,38 @@
+// 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.Collections.Generic;
+using System.Linq;
+using ILLink.Shared;
+using Mono.Cecil;
+
+namespace Mono.Linker.Steps
+{
+	public class CheckSuppressionsDispatcher : SubStepsDispatcher
+	{
+		public CheckSuppressionsDispatcher () : base (new List<ISubStep> { new CheckSuppressionsStep () })
+		{
+
+		}
+
+		public override void Process (LinkContext context)
+		{
+			base.Process (context);
+			var redundantSuppressions = context.Suppressions.GetUnusedSuppressions ();
+
+			// Suppressions targeting warning caused by anything but the linker should not be reported.
+			// Suppressions targeting RedundantSuppression warning should not be reported.
+			redundantSuppressions = redundantSuppressions
+				.Where (suppression => ((DiagnosticId) suppression.suppressMessageInfo.Id).GetDiagnosticCategory () == DiagnosticCategory.Trimming)
+				.Where (suppression => ((DiagnosticId) suppression.suppressMessageInfo.Id) != DiagnosticId.RedundantSuppression);
+
+			foreach (var (provider, suppressMessageInfo) in redundantSuppressions) {
+				var source = GetSuppresionProvider (provider);
+
+				context.LogWarning (new MessageOrigin (source), DiagnosticId.RedundantSuppression, $"IL{suppressMessageInfo.Id:0000}");
+			}
+		}
+
+		private static ICustomAttributeProvider GetSuppresionProvider (ICustomAttributeProvider provider) => provider is ModuleDefinition module ? module.Assembly : provider;
+	}
+}

src/linker/Linker.Steps/CheckSuppressionsStep.cs

@@ -0,0 +1,54 @@
+// 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 Mono.Cecil;
+
+namespace Mono.Linker.Steps
+{
+	public class CheckSuppressionsStep : BaseSubStep
+	{
+		public override SubStepTargets Targets {
+			get {
+				return SubStepTargets.Type |
+					SubStepTargets.Field |
+					SubStepTargets.Method |
+					SubStepTargets.Property;
+			}
+		}
+
+		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;
+		}
+
+		public override void ProcessType (TypeDefinition type)
+		{
+			Context.Suppressions.GatherSuppressions (type);
+		}
+
+		public override void ProcessField (FieldDefinition field)
+		{
+			Context.Suppressions.GatherSuppressions (field);
+		}
+
+		public override void ProcessMethod (MethodDefinition method)
+		{
+			if (Context.Annotations.GetAction (method) != MethodAction.ConvertToThrow)
+				Context.Suppressions.GatherSuppressions (method);
+		}
+
+		public override void ProcessProperty (PropertyDefinition property)
+		{
+			Context.Suppressions.GatherSuppressions (property);
+		}
+
+		public override void ProcessEvent (EventDefinition @event)
+		{
+			Context.Suppressions.GatherSuppressions (@event);
+		}
+	}
+}

src/linker/Linker.Steps/SubStepsDispatcher.cs

@@ -51,7 +51,7 @@ public void Add (ISubStep substep)
 			substeps.Add (substep);
 		}
 
-		void IStep.Process (LinkContext context)
+		public virtual void Process (LinkContext context)
 		{
 			InitializeSubSteps (context);
 

src/linker/Linker/Driver.cs

@@ -1408,6 +1408,7 @@ static Pipeline GetStandardPipeline ()
 			p.AppendStep (new ProcessWarningsStep ());
 			p.AppendStep (new OutputWarningSuppressions ());
 			p.AppendStep (new SweepStep ());
+			p.AppendStep (new CheckSuppressionsDispatcher ());
 			p.AppendStep (new CodeRewriterStep ());
 			p.AppendStep (new CleanStep ());
 			p.AppendStep (new RegenerateGuidStep ());

src/linker/Linker/UnconditionalSuppressMessageAttributeState.cs

@@ -16,28 +16,36 @@ public class UnconditionalSuppressMessageAttributeState
 		internal const string TargetProperty = "Target";
 		internal const string MessageIdProperty = "MessageId";
 
+		public class Suppression
+		{
+			public SuppressMessageInfo SuppressMessageInfo { get; set; }
+			public bool Used { get; set; }
+		}
+
 		readonly LinkContext _context;
-		readonly Dictionary<ICustomAttributeProvider, Dictionary<int, SuppressMessageInfo>> _suppressions;
+		readonly Dictionary<ICustomAttributeProvider, Dictionary<int, Suppression>> _suppressions;
 		HashSet<AssemblyDefinition> InitializedAssemblies { get; }
 
 		public UnconditionalSuppressMessageAttributeState (LinkContext context)
 		{
 			_context = context;
-			_suppressions = new Dictionary<ICustomAttributeProvider, Dictionary<int, SuppressMessageInfo>> ();
+			_suppressions = new Dictionary<ICustomAttributeProvider, Dictionary<int, Suppression>> ();
 			InitializedAssemblies = new HashSet<AssemblyDefinition> ();
 		}
 
 		void AddSuppression (SuppressMessageInfo info, ICustomAttributeProvider provider)
 		{
+			var used = false;
 			if (!_suppressions.TryGetValue (provider, out var suppressions)) {
-				suppressions = new Dictionary<int, SuppressMessageInfo> ();
+				suppressions = new Dictionary<int, Suppression> ();
 				_suppressions.Add (provider, suppressions);
-			} else if (suppressions.ContainsKey (info.Id)) {
+			} else if (suppressions.TryGetValue (info.Id, out Suppression? value)) {
+				used = value.Used;
 				string? elementName = provider is MemberReference memberRef ? memberRef.GetDisplayName () : provider.ToString ();
 				_context.LogMessage ($"Element '{elementName}' has more than one unconditional suppression. Note that only the last one is used.");
 			}
 
-			suppressions[info.Id] = info;
+			suppressions[info.Id] = new Suppression { SuppressMessageInfo = info, Used = used };
 		}
 
 		public bool IsSuppressed (int id, MessageOrigin warningOrigin, out SuppressMessageInfo info)
@@ -69,6 +77,21 @@ public bool IsSuppressed (int id, MessageOrigin warningOrigin, out SuppressMessa
 			return false;
 		}
 
+		public void GatherSuppressions (ICustomAttributeProvider provider)
+		{
+			TryGetSuppressionsForProvider (provider, out _);
+		}
+
+		public IEnumerable<(ICustomAttributeProvider provider, SuppressMessageInfo suppressMessageInfo)> GetUnusedSuppressions ()
+		{
+			foreach (var (provider, suppressions) in _suppressions) {
+				foreach (var (_, suppression) in suppressions) {
+					if (!suppression.Used)
+						yield return (provider, suppression.SuppressMessageInfo);
+				}
+			}
+		}
+
 		bool IsSuppressed (int id, ICustomAttributeProvider warningOrigin, out SuppressMessageInfo info)
 		{
 			info = default;
@@ -97,11 +120,27 @@ bool IsSuppressed (int id, ICustomAttributeProvider warningOrigin, out SuppressM
 		bool IsSuppressedOnElement (int id, ICustomAttributeProvider? provider, out SuppressMessageInfo info)
 		{
 			info = default;
+
+			if (TryGetSuppressionsForProvider (provider, out var suppressions)) {
+				if (suppressions != null && suppressions.TryGetValue (id, out var suppression)) {
+					suppression.Used = true;
+					info = suppression.SuppressMessageInfo;
+					return true;
+				}
+			}
+
+			return false;
+		}
+
+		bool TryGetSuppressionsForProvider (ICustomAttributeProvider? provider, out Dictionary<int, Suppression>? suppressions)
+		{
+			suppressions = null;
 			if (provider == null)
 				return false;
 
-			if (_suppressions.TryGetValue (provider, out var suppressions))
-				return suppressions.TryGetValue (id, out info);
+			if (_suppressions.TryGetValue (provider, out suppressions))
+				return true;
+
 
 			// Populate the cache with suppressions for this member. We need to look for suppressions on the
 			// member itself, and on the assembly/module.
@@ -130,7 +169,7 @@ bool IsSuppressedOnElement (int id, ICustomAttributeProvider? provider, out Supp
 					AddSuppression (suppressionInfo, member);
 			}
 
-			return _suppressions.TryGetValue (provider, out suppressions) && suppressions.TryGetValue (id, out info);
+			return _suppressions.TryGetValue (provider, out suppressions);
 		}
 
 		static bool TryDecodeSuppressMessageAttributeData (CustomAttribute attribute, out SuppressMessageInfo info)

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

@@ -13,6 +13,48 @@ public Task AddSuppressionsBeforeAttributeRemoval ()
 			return RunTest (allowMissingWarnings: true);
 		}
 
+		[Fact]
+		public Task DetectRedundantSuppressionsFeatureSubstitutions ()
+		{
+			return RunTest (allowMissingWarnings: true);
+		}
+
+		[Fact]
+		public Task DetectRedundantSuppressionsFromXML ()
+		{
+			return RunTest (allowMissingWarnings: true);
+		}
+
+		[Fact]
+		public Task DetectRedundantSuppressionsInAssembly ()
+		{
+			return RunTest (allowMissingWarnings: true);
+		}
+
+		[Fact]
+		public Task DetectRedundantSuppressionsInCompilerGeneratedCode ()
+		{
+			return RunTest (allowMissingWarnings: true);
+		}
+
+		[Fact]
+		public Task DetectRedundantSuppressionsInMembersAndTypes ()
+		{
+			return RunTest (allowMissingWarnings: true);
+		}
+
+		[Fact]
+		public Task DetectRedundantSuppressionsInMembersAndTypesUsingTarget ()
+		{
+			return RunTest (allowMissingWarnings: true);
+		}
+
+		[Fact]
+		public Task DetectRedundantSuppressionsInModule ()
+		{
+			return RunTest (allowMissingWarnings: true);
+		}
+
 		[Fact]
 		public Task SuppressWarningsInAssembly ()
 		{

test/Mono.Linker.Tests.Cases/DataFlow/SuppressWarningWithLinkAttributes.xml

@@ -1,10 +1,6 @@
 <?xml version="1.0" encoding="utf-8" ?>
 <linker xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:noNamespaceSchemaLocation="../../../src/ILLink.Shared/ILLink.LinkAttributes.xsd">
   <assembly fullname="Test">
-    <attribute fullname="System.Diagnostics.CodeAnalysis.UnconditionalSuppressMessageAttribute">
-      <argument>Test</argument>
-      <argument>IL2067</argument>
-    </attribute>
     <type fullname="Mono.Linker.Tests.Cases.DataFlow.SuppressWarningWithLinkAttributes">
       <field name="_typeWithPublicParameterlessConstructor">
         <attribute fullname="System.DoesNotExistAttribute" assembly="Mono.Linker.Tests.Cases.Expectations">

test/Mono.Linker.Tests.Cases/DataFlow/XmlAnnotations.xml

@@ -1,12 +1,6 @@
 <?xml version="1.0" encoding="utf-8"?>
 <linker xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:noNamespaceSchemaLocation="../../../src/ILLink.Shared/ILLink.LinkAttributes.xsd">
 	<assembly fullname="Test">
-		<attribute fullname="System.Diagnostics.CodeAnalysis.UnconditionalSuppressMessageAttribute">
-			<argument>ILLink</argument>
-			<argument>IL2067</argument>
-			<property name="Scope">member</property>
-			<property name="Target">M:Mono.Linker.Tests.Cases.DataFlow.XmlAnnotations.ReadFromInstanceField</property>
-		</attribute>
 		<type fullname="Mono.Linker.Tests.Cases.DataFlow.XmlAnnotations">
 			<field name="_typeWithPublicParameterlessConstructor">
 				<attribute fullname="System.DoesNotExistAttribute" assembly="Mono.Linker.Tests.Cases.Expectations">