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

@@ -1,17 +1,17 @@
 <?xml version="1.0" encoding="utf-8"?>
 <root>
-  <!--
-    Microsoft ResX Schema
-
+  <!-- 
+    Microsoft ResX Schema 
+    
     Version 2.0
-
-    The primary goals of this format is to allow a simple XML format
-    that is mostly human readable. The generation and parsing of the
-    various data types are done through the TypeConverter classes
+    
+    The primary goals of this format is to allow a simple XML format 
+    that is mostly human readable. The generation and parsing of the 
+    various data types are done through the TypeConverter classes 
     associated with the data types.
-
+    
     Example:
-
+    
     ... ado.net/XML headers & schema ...
     <resheader name="resmimetype">text/microsoft-resx</resheader>
     <resheader name="version">2.0</resheader>
@@ -26,36 +26,36 @@
         <value>[base64 mime encoded string representing a byte array form of the .NET Framework object]</value>
         <comment>This is a comment</comment>
     </data>
-
-    There are any number of "resheader" rows that contain simple
+                
+    There are any number of "resheader" rows that contain simple 
     name/value pairs.
-
-    Each data row contains a name, and value. The row also contains a
-    type or mimetype. Type corresponds to a .NET class that support
-    text/value conversion through the TypeConverter architecture.
-    Classes that don't support this are serialized and stored with the
+    
+    Each data row contains a name, and value. The row also contains a 
+    type or mimetype. Type corresponds to a .NET class that support 
+    text/value conversion through the TypeConverter architecture. 
+    Classes that don't support this are serialized and stored with the 
     mimetype set.
-
-    The mimetype is used for serialized objects, and tells the
-    ResXResourceReader how to depersist the object. This is currently not
+    
+    The mimetype is used for serialized objects, and tells the 
+    ResXResourceReader how to depersist the object. This is currently not 
     extensible. For a given mimetype the value must be set accordingly
-
-    Note - application/x-microsoft.net.object.binary.base64 is the format
-    that the ResXResourceWriter will generate, however the reader can
+    
+    Note - application/x-microsoft.net.object.binary.base64 is the format 
+    that the ResXResourceWriter will generate, however the reader can 
     read any of the formats listed below.
-
+    
     mimetype: application/x-microsoft.net.object.binary.base64
-    value   : The object must be serialized with
+    value   : The object must be serialized with 
             : System.Runtime.Serialization.Formatters.Binary.BinaryFormatter
             : and then encoded with base64 encoding.
-
+    
     mimetype: application/x-microsoft.net.object.soap.base64
-    value   : The object must be serialized with
+    value   : The object must be serialized with 
             : System.Runtime.Serialization.Formatters.Soap.SoapFormatter
             : and then encoded with base64 encoding.
 
     mimetype: application/x-microsoft.net.object.bytearray.base64
-    value   : The object must be serialized into a byte array
+    value   : The object must be serialized into a byte array 
             : using a System.ComponentModel.TypeConverter
             : and then encoded with base64 encoding.
     -->
@@ -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 ()
 		{