From 5d376b1f8963c5c0013482ec63d857d359658809 Mon Sep 17 00:00:00 2001 From: Sven Boemer Date: Wed, 11 Aug 2021 14:05:55 -0700 Subject: [PATCH] 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 --- ...onditionalSuppressMessageAttributeState.cs | 86 ++++++++++++------- .../SuppressWarningsInMembersAndTypes.cs | 4 + ...essWarningsInMembersAndTypesUsingTarget.cs | 26 ++++++ 3 files changed, 86 insertions(+), 30 deletions(-) diff --git a/src/linker/Linker/UnconditionalSuppressMessageAttributeState.cs b/src/linker/Linker/UnconditionalSuppressMessageAttributeState.cs index bc73836f7922..e8534b06c179 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 @@ public UnconditionalSuppressMessageAttributeState (LinkContext context) InitializedAssemblies = new HashSet (); } - 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 (); _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 { { 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 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: diff --git a/test/Mono.Linker.Tests.Cases/Warnings/WarningSuppression/SuppressWarningsInMembersAndTypes.cs b/test/Mono.Linker.Tests.Cases/Warnings/WarningSuppression/SuppressWarningsInMembersAndTypes.cs index d3350ea01135..7466f52389e4 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 @@ class TypeWithSuppression 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 7fcbc85a2a58..a638b365e6c5 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 @@ [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 () + { + } } }