Fix suppression check with assembly suppressions (#2180)

* Fix suppression check with assembly suppressions

The change in https://github.com/mono/linker/pull/2171 was incorrect
because it didn't account for the possibility that the suppressions
cache already contains assembly or module suppressions for the provider.

This fixes the check by re-using the cache to also track whether we have
scanned for suppression attributes on the provider. This caching is
necessary since we warn about duplicate suppressions.

Providers without any suppressions may still be scanned multiple times
since we don't cache the negative result.

* Populate cache eagerly to avoid extra state

Now whenever we add a record to the cache, we ensure that all of the
suppressions that might apply to the member have been discovered.
This way we don't need to track whether we have scanned the suppressed
member, but have to do a bit more work up-front.

* Use private accessibility

For methods only used in
UnconditionalSuppressMessageAttributeState

3 files changed, 86 insertions, 30 deletions

diff --git a/src/linker/Linker/UnconditionalSuppressMessageAttributeState.cs b/src/linker/Linker/UnconditionalSuppressMessageAttributeState.cs
index bc73836f7..e8534b06c 100644
--- a/src/linker/Linker/UnconditionalSuppressMessageAttributeState.cs
+++ b/src/linker/Linker/UnconditionalSuppressMessageAttributeState.cs
@@ -1,5 +1,6 @@
 using System;
 using System.Collections.Generic;
+using System.Diagnostics;
 using System.Linq;
 using Mono.Cecil;
 
@@ -22,24 +23,14 @@ namespace Mono.Linker
 			InitializedAssemblies = new HashSet<AssemblyDefinition> ();
 		}
 
-		void AddSuppression (CustomAttribute ca, ICustomAttributeProvider provider)
-		{
-			SuppressMessageInfo info;
-			if (!TryDecodeSuppressMessageAttributeData (ca, out info))
-				return;
-
-			AddSuppression (info, provider);
-		}
-
 		void AddSuppression (SuppressMessageInfo info, ICustomAttributeProvider provider)
 		{
 			if (!_suppressions.TryGetValue (provider, out var suppressions)) {
 				suppressions = new Dictionary<int, SuppressMessageInfo> ();
 				_suppressions.Add (provider, suppressions);
-			}
-
-			if (suppressions.ContainsKey (info.Id))
+			} else if (suppressions.ContainsKey (info.Id)) {
 				_context.LogMessage ($"Element {provider} has more than one unconditional suppression. Note that only the last one is used.");
+			}
 
 			suppressions[info.Id] = info;
 		}
@@ -68,7 +59,6 @@ namespace Mono.Linker
 				return false;
 
 			ModuleDefinition module = GetModuleFromProvider (warningOriginMember);
-			DecodeModuleLevelAndGlobalSuppressMessageAttributes (module);
 			while (warningOriginMember != null) {
 				if (IsSuppressedOnElement (id, warningOriginMember, out info))
 					return true;
@@ -93,17 +83,33 @@ namespace Mono.Linker
 			if (_suppressions.TryGetValue (provider, out var suppressions))
 				return suppressions.TryGetValue (id, out info);
 
-			if (!_context.CustomAttributes.HasAny (provider))
-				return false;
+			// Populate the cache with suppressions for this member. We need to look for suppressions on the
+			// member itself, and on the assembly/module.
 
-			foreach (var ca in _context.CustomAttributes.GetCustomAttributes (provider)) {
-				if (TypeRefHasUnconditionalSuppressions (ca.Constructor.DeclaringType) &&
-					provider is not ModuleDefinition or AssemblyDefinition)
-					AddSuppression (ca, provider);
+			var membersToScan = new HashSet<ICustomAttributeProvider> { { provider } };
+
+			// Gather assembly-level suppressions if we haven't already. To ensure that we always cache
+			// complete information for a member, we will also scan for attributes on any other members
+			// targeted by the assembly-level suppressions.
+			var module = GetModuleFromProvider (provider);
+			var assembly = module.Assembly;
+			if (InitializedAssemblies.Add (assembly)) {
+				foreach (var suppression in DecodeAssemblyAndModuleSuppressions (module)) {
+					AddSuppression (suppression.Info, suppression.Target);
+					membersToScan.Add (suppression.Target);
+				}
+			}
+
+			// Populate the cache for this member, and for any members that were targeted by assembly-level
+			// suppressions to make sure the cached info is complete.
+			foreach (var member in membersToScan) {
+				if (member is ModuleDefinition or AssemblyDefinition)
+					continue;
+				foreach (var suppressionInfo in DecodeSuppressions (member))
+					AddSuppression (suppressionInfo, member);
 			}
 
-			return _suppressions.TryGetValue (provider, out suppressions) &&
-				suppressions.TryGetValue (id, out info);
+			return _suppressions.TryGetValue (provider, out suppressions) && suppressions.TryGetValue (id, out info);
 		}
 
 		static bool TryDecodeSuppressMessageAttributeData (CustomAttribute attribute, out SuppressMessageInfo info)
@@ -167,17 +173,37 @@ namespace Mono.Linker
 			}
 		}
 
-		void DecodeModuleLevelAndGlobalSuppressMessageAttributes (ModuleDefinition module)
+		IEnumerable<SuppressMessageInfo> DecodeSuppressions (ICustomAttributeProvider provider)
+		{
+			Debug.Assert (provider is not ModuleDefinition or AssemblyDefinition);
+
+			if (!_context.CustomAttributes.HasAny (provider))
+				yield break;
+
+			foreach (var ca in _context.CustomAttributes.GetCustomAttributes (provider)) {
+				if (!TypeRefHasUnconditionalSuppressions (ca.Constructor.DeclaringType))
+					continue;
+
+				if (!TryDecodeSuppressMessageAttributeData (ca, out var info))
+					continue;
+
+				yield return info;
+			}
+		}
+
+		IEnumerable<(SuppressMessageInfo Info, ICustomAttributeProvider Target)> DecodeAssemblyAndModuleSuppressions (ModuleDefinition module)
 		{
 			AssemblyDefinition assembly = module.Assembly;
-			if (InitializedAssemblies.Add (assembly)) {
-				LookForModuleLevelAndGlobalSuppressions (module, assembly);
-				foreach (var _module in assembly.Modules)
-					LookForModuleLevelAndGlobalSuppressions (_module, _module);
+			foreach (var suppression in DecodeGlobalSuppressions (module, assembly))
+				yield return suppression;
+
+			foreach (var _module in assembly.Modules) {
+				foreach (var suppression in DecodeGlobalSuppressions (_module, _module))
+					yield return suppression;
 			}
 		}
 
-		public void LookForModuleLevelAndGlobalSuppressions (ModuleDefinition module, ICustomAttributeProvider provider)
+		IEnumerable<(SuppressMessageInfo Info, ICustomAttributeProvider Target)> DecodeGlobalSuppressions (ModuleDefinition module, ICustomAttributeProvider provider)
 		{
 			var attributes = _context.CustomAttributes.GetCustomAttributes (provider).
 					Where (a => TypeRefHasUnconditionalSuppressions (a.AttributeType));
@@ -188,19 +214,19 @@ namespace Mono.Linker
 
 				var scope = info.Scope?.ToLower ();
 				if (info.Target == null && (scope == "module" || scope == null)) {
-					AddSuppression (info, provider);
+					yield return (info, provider);
 					continue;
 				}
 
 				switch (scope) {
 				case "module":
-					AddSuppression (info, provider);
+					yield return (info, provider);
 					break;
 
 				case "type":
 				case "member":
 					foreach (var result in DocumentationSignatureParser.GetMembersForDocumentationSignature (info.Target, module))
-						AddSuppression (info, result);
+						yield return (info, result);
 
 					break;
 				default:
diff --git a/test/Mono.Linker.Tests.Cases/Warnings/WarningSuppression/SuppressWarningsInMembersAndTypes.cs b/test/Mono.Linker.Tests.Cases/Warnings/WarningSuppression/SuppressWarningsInMembersAndTypes.cs
index d3350ea01..7466f5238 100644
--- a/test/Mono.Linker.Tests.Cases/Warnings/WarningSuppression/SuppressWarningsInMembersAndTypes.cs
+++ b/test/Mono.Linker.Tests.Cases/Warnings/WarningSuppression/SuppressWarningsInMembersAndTypes.cs
@@ -111,6 +111,10 @@ namespace Mono.Linker.Tests.Cases.Warnings.WarningSuppression
 			public TypeWithSuppression ()
 			{
 				MethodWithRUC ();
+
+				// Triggering the suppression check a second time
+				// still shouldn't warn about duplicate suppressions.
+				MethodWithRUC ();
 			}
 
 			int _field;
diff --git a/test/Mono.Linker.Tests.Cases/Warnings/WarningSuppression/SuppressWarningsInMembersAndTypesUsingTarget.cs b/test/Mono.Linker.Tests.Cases/Warnings/WarningSuppression/SuppressWarningsInMembersAndTypesUsingTarget.cs
index 7fcbc85a2..a638b365e 100644
--- a/test/Mono.Linker.Tests.Cases/Warnings/WarningSuppression/SuppressWarningsInMembersAndTypesUsingTarget.cs
+++ b/test/Mono.Linker.Tests.Cases/Warnings/WarningSuppression/SuppressWarningsInMembersAndTypesUsingTarget.cs
@@ -9,6 +9,9 @@ using Mono.Linker.Tests.Cases.Expectations.Metadata;
 [module: UnconditionalSuppressMessage ("Test", "IL2072", Scope = "type", Target = "T:Mono.Linker.Tests.Cases.Warnings.WarningSuppression.WarningsInType")]
 [module: UnconditionalSuppressMessage ("Test", "IL2072", Scope = "member", Target = "M:Mono.Linker.Tests.Cases.Warnings.WarningSuppression.WarningsInMembers.Method")]
 [module: UnconditionalSuppressMessage ("Test", "IL2072", Scope = "member", Target = "M:Mono.Linker.Tests.Cases.Warnings.WarningSuppression.WarningsInMembers.get_Property")]
+[module: UnconditionalSuppressMessage ("Test", "IL2072", Scope = "member", Target = "M:Mono.Linker.Tests.Cases.Warnings.WarningSuppression..get_Property")]
+[module: UnconditionalSuppressMessage ("Test", "IL2072", Scope = "member", Target = "M:Mono.Linker.Tests.Cases.Warnings.WarningSuppression.WarningsInMembers.MultipleWarnings")]
+[module: UnconditionalSuppressMessage ("Test", "IL2026", Scope = "member", Target = "M:Mono.Linker.Tests.Cases.Warnings.WarningSuppression.WarningsInMembers.MultipleSuppressions")]
 
 namespace Mono.Linker.Tests.Cases.Warnings.WarningSuppression
 {
@@ -30,6 +33,9 @@ namespace Mono.Linker.Tests.Cases.Warnings.WarningSuppression
 			var warningsInMembers = new WarningsInMembers ();
 			warningsInMembers.Method ();
 			int propertyThatTriggersWarning = warningsInMembers.Property;
+
+			WarningsInMembers.MultipleWarnings ();
+			WarningsInMembers.MultipleSuppressions ();
 		}
 
 		public static Type TriggerUnrecognizedPattern ()
@@ -73,6 +79,7 @@ namespace Mono.Linker.Tests.Cases.Warnings.WarningSuppression
 		}
 	}
 
+	[ExpectedNoWarnings]
 	public class WarningsInMembers
 	{
 		public void Method ()
@@ -86,5 +93,24 @@ namespace Mono.Linker.Tests.Cases.Warnings.WarningSuppression
 				return 0;
 			}
 		}
+
+		[UnconditionalSuppressMessage ("Test", "IL2026")]
+		public static void MultipleWarnings ()
+		{
+			Expression.Call (SuppressWarningsInMembersAndTypesUsingTarget.TriggerUnrecognizedPattern (), "", Type.EmptyTypes);
+			RUCMethod ();
+		}
+
+		[LogContains (nameof (MultipleSuppressions) + "() has more than one unconditional suppression")]
+		[UnconditionalSuppressMessage ("Test", "IL2026")]
+		public static void MultipleSuppressions ()
+		{
+			RUCMethod ();
+		}
+
+		[RequiresUnreferencedCode ("--RUCMethod--")]
+		static void RUCMethod ()
+		{
+		}
 	}
 }