Fix suppression check with assembly suppressions (#2180)

* Fix suppression check with assembly suppressions

The change in #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

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 @@ public UnconditionalSuppressMessageAttributeState (LinkContext context)
 			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 @@ bool IsSuppressed (int id, IMemberDefinition warningOriginMember, out SuppressMe
 				return false;
 
 			ModuleDefinition module = GetModuleFromProvider (warningOriginMember);
-			DecodeModuleLevelAndGlobalSuppressMessageAttributes (module);
 			while (warningOriginMember != null) {
 				if (IsSuppressedOnElement (id, warningOriginMember, out info))
 					return true;
@@ -93,17 +83,33 @@ bool IsSuppressedOnElement (int id, ICustomAttributeProvider provider, out Suppr
 			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 @@ public static ModuleDefinition GetModuleFromProvider (ICustomAttributeProvider p
 			}
 		}
 
-		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 @@ public void LookForModuleLevelAndGlobalSuppressions (ModuleDefinition module, IC
 
 				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:

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

@@ -111,6 +111,10 @@ class TypeWithSuppression
 			public TypeWithSuppression ()
 			{
 				MethodWithRUC ();
+
+				// Triggering the suppression check a second time
+				// still shouldn't warn about duplicate suppressions.
+				MethodWithRUC ();
 			}
 
 			int _field;

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

@@ -9,6 +9,9 @@
 [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 @@ public static void Main ()
 			var warningsInMembers = new WarningsInMembers ();
 			warningsInMembers.Method ();
 			int propertyThatTriggersWarning = warningsInMembers.Property;
+
+			WarningsInMembers.MultipleWarnings ();
+			WarningsInMembers.MultipleSuppressions ();
 		}
 
 		public static Type TriggerUnrecognizedPattern ()
@@ -73,6 +79,7 @@ void Warning4 ()
 		}
 	}
 
+	[ExpectedNoWarnings]
 	public class WarningsInMembers
 	{
 		public void Method ()
@@ -86,5 +93,24 @@ public int Property {
 				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 ()
+		{
+		}
 	}
 }