Merge pull request #44440 from mavasani/RemoveLegacySuppressions

Add analyzer/fixer to convert legacy FxCop format suppressions to Ros…

src/Analyzers/CSharp/Tests/CSharpAnalyzers.UnitTests.projitems

@@ -25,6 +25,7 @@
     <Compile Include="$(MSBuildThisFileDirectory)MisplacedUsingDirectives\MisplacedUsingDirectivesCodeFixProviderTests.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)OrderModifiers\OrderModifiersCompilerErrorTests.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)OrderModifiers\OrderModifiersTests.cs" />
+    <Compile Include="$(MSBuildThisFileDirectory)UpdateLegacySuppressions\UpdateLegacySuppressionsTests.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)RemoveUnnecessarySuppressions\RemoveUnnecessarySuppressionsTests.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)RemoveUnnecessaryParentheses\RemoveUnnecessaryPatternParenthesesTests.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)RemoveUnreachableCode\RemoveUnreachableCodeTests.cs" />

src/Analyzers/CSharp/Tests/UpdateLegacySuppressions/UpdateLegacySuppressionsTests.cs

@@ -0,0 +1,87 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+// See the LICENSE file in the project root for more information.
+
+#nullable enable
+
+using System.Threading.Tasks;
+using Microsoft.CodeAnalysis.RemoveUnnecessarySuppressions;
+using Microsoft.CodeAnalysis.Test.Utilities;
+using Roslyn.Test.Utilities;
+using Xunit;
+using VerifyCS = Microsoft.CodeAnalysis.Editor.UnitTests.CodeActions.CSharpCodeFixVerifier<
+    Microsoft.CodeAnalysis.CSharp.RemoveUnnecessarySuppressions.CSharpRemoveUnnecessarySuppressionsDiagnosticAnalyzer,
+    Microsoft.CodeAnalysis.UpdateLegacySuppressions.UpdateLegacySuppressionsCodeFixProvider>;
+
+namespace Microsoft.CodeAnalysis.Editor.CSharp.UnitTests.UpdateLegacySuppressions
+{
+    [Trait(Traits.Feature, Traits.Features.CodeActionsUpdateLegacySuppressions)]
+    [WorkItem(44362, "https://github.com/dotnet/roslyn/issues/44362")]
+    public class UpdateLegacySuppressionsTests
+    {
+        [Fact]
+        public void TestStandardProperties()
+            => VerifyCS.VerifyStandardProperties();
+
+        // Namespace
+        [InlineData("namespace", "N", "~N:N")]
+        // Type
+        [InlineData("type", "N.C+D", "~T:N.C.D")]
+        // Field
+        [InlineData("member", "N.C.#F", "~F:N.C.F")]
+        // Property
+        [InlineData("member", "N.C.#P", "~P:N.C.P")]
+        // Method
+        [InlineData("member", "N.C.#M", "~M:N.C.M")]
+        // Generic method with parameters
+        [InlineData("member", "N.C.#M2(!!0)", "~M:N.C.M2``1(``0)~System.Int32")]
+        // Event
+        [InlineData("member", "e:N.C.#E", "~E:N.C.E")]
+        [Theory]
+        public async Task LegacySuppressions(string scope, string target, string fixedTarget)
+        {
+            var input = $@"
+[assembly: System.Diagnostics.CodeAnalysis.SuppressMessage(""Category"", ""Id: Title"", Scope = ""{scope}"", Target = {{|#0:""{target}""|}})]
+
+namespace N
+{{
+    class C
+    {{
+        private int F;
+        public int P {{ get; set; }}
+        public void M() {{ }}
+        public int M2<T>(T t) => 0;
+        public event System.EventHandler<int> E;
+
+        class D
+        {{
+        }}
+    }}
+}}";
+
+            var expectedDiagnostic = VerifyCS.Diagnostic(AbstractRemoveUnnecessarySuppressionsDiagnosticAnalyzer.LegacyFormatTargetDescriptor)
+                                        .WithLocation(0)
+                                        .WithArguments(target);
+
+            var fixedCode = $@"
+[assembly: System.Diagnostics.CodeAnalysis.SuppressMessage(""Category"", ""Id: Title"", Scope = ""{scope}"", Target = ""{fixedTarget}"")]
+
+namespace N
+{{
+    class C
+    {{
+        private int F;
+        public int P {{ get; set; }}
+        public void M() {{ }}
+        public int M2<T>(T t) => 0;
+        public event System.EventHandler<int> E;
+
+        class D
+        {{
+        }}
+    }}
+}}";
+            await VerifyCS.VerifyCodeFixAsync(input, expectedDiagnostic, fixedCode);
+        }
+    }
+}

src/Analyzers/Core/Analyzers/AnalyzersResources.resx

@@ -316,4 +316,10 @@
   <data name="Invalid_or_missing_target_for_SuppressMessageAttribute" xml:space="preserve">
     <value>Invalid or missing target for 'SuppressMessageAttribute'</value>
   </data>
+  <data name="Avoid_legacy_format_target_in_SuppressMessageAttribute" xml:space="preserve">
+    <value>Avoid legacy format target in 'SuppressMessageAttribute'</value>
+  </data>
+  <data name="Avoid_legacy_format_target_0_in_SuppressMessageAttribute" xml:space="preserve">
+    <value>Avoid legacy format target '{0}' in 'SuppressMessageAttribute'</value>
+  </data>
 </root>

src/Analyzers/Core/Analyzers/IDEDiagnosticIds.cs

@@ -131,6 +131,7 @@ internal static class IDEDiagnosticIds
         public const string SimplifyConditionalExpressionDiagnosticId = "IDE0075";
 
         public const string InvalidSuppressMessageAttributeDiagnosticId = "IDE0076";
+        public const string LegacyFormatSuppressMessageAttributeDiagnosticId = "IDE0077";
 
         // Analyzer error Ids
         public const string AnalyzerChangedId = "IDE1001";

src/Analyzers/Core/Analyzers/RemoveUnnecessarySuppressions/AbstractRemoveUnnecessarySuppressionsDiagnosticAnalyzer.cs

@@ -10,12 +10,15 @@
 using Microsoft.CodeAnalysis.CodeQuality;
 using Microsoft.CodeAnalysis.Diagnostics;
 using Microsoft.CodeAnalysis.Shared.Extensions;
+using Roslyn.Utilities;
 
 namespace Microsoft.CodeAnalysis.RemoveUnnecessarySuppressions
 {
     internal abstract class AbstractRemoveUnnecessarySuppressionsDiagnosticAnalyzer
         : AbstractCodeQualityDiagnosticAnalyzer
     {
+        internal const string DocCommentIdKey = nameof(DocCommentIdKey);
+
         private static readonly LocalizableResourceString s_localizableTitle = new LocalizableResourceString(
            nameof(AnalyzersResources.Invalid_global_SuppressMessageAttribute), AnalyzersResources.ResourceManager, typeof(AnalyzersResources));
         private static readonly LocalizableResourceString s_localizableInvalidScopeMessage = new LocalizableResourceString(
@@ -28,8 +31,15 @@ internal abstract class AbstractRemoveUnnecessarySuppressionsDiagnosticAnalyzer
         private static readonly DiagnosticDescriptor s_invalidOrMissingTargetDescriptor = CreateDescriptor(
             IDEDiagnosticIds.InvalidSuppressMessageAttributeDiagnosticId, s_localizableTitle, s_localizableInvalidOrMissingTargetMessage, isUnnecessary: true);
 
+        private static readonly LocalizableResourceString s_localizableLegacyFormatTitle = new LocalizableResourceString(
+           nameof(AnalyzersResources.Avoid_legacy_format_target_in_SuppressMessageAttribute), AnalyzersResources.ResourceManager, typeof(AnalyzersResources));
+        private static readonly LocalizableResourceString s_localizableLegacyFormatMessage = new LocalizableResourceString(
+            nameof(AnalyzersResources.Avoid_legacy_format_target_0_in_SuppressMessageAttribute), AnalyzersResources.ResourceManager, typeof(AnalyzersResources));
+        internal static readonly DiagnosticDescriptor LegacyFormatTargetDescriptor = CreateDescriptor(
+            IDEDiagnosticIds.LegacyFormatSuppressMessageAttributeDiagnosticId, s_localizableLegacyFormatTitle, s_localizableLegacyFormatMessage, isUnnecessary: false);
+
         public AbstractRemoveUnnecessarySuppressionsDiagnosticAnalyzer()
-            : base(ImmutableArray.Create(s_invalidScopeDescriptor, s_invalidOrMissingTargetDescriptor), GeneratedCodeAnalysisFlags.None)
+            : base(ImmutableArray.Create(s_invalidScopeDescriptor, s_invalidOrMissingTargetDescriptor, LegacyFormatTargetDescriptor), GeneratedCodeAnalysisFlags.None)
         {
         }
 
@@ -66,21 +76,41 @@ public void AnalyzeAssemblyOrModuleAttribute(SyntaxNode attributeSyntax, Semanti
                     return;
                 }
 
-                DiagnosticDescriptor rule;
-                if (SuppressMessageAttributeState.HasInvalidScope(namedAttributeArguments, out var targetScope))
+                if (!SuppressMessageAttributeState.HasValidScope(namedAttributeArguments, out var targetScope))
                 {
-                    rule = s_invalidScopeDescriptor;
+                    reportDiagnostic(Diagnostic.Create(s_invalidScopeDescriptor, attributeSyntax.GetLocation()));
+                    return;
                 }
-                else if (_state.HasInvalidOrMissingTarget(namedAttributeArguments, targetScope))
+
+                if (!_state.HasValidTarget(namedAttributeArguments, targetScope, out var targetHasDocCommentIdFormat,
+                        out var targetSymbolString, out var targetValueOperation, out var resolvedSymbols))
                 {
-                    rule = s_invalidOrMissingTargetDescriptor;
+                    reportDiagnostic(Diagnostic.Create(s_invalidOrMissingTargetDescriptor, attributeSyntax.GetLocation()));
+                    return;
                 }
-                else
+
+                // We want to flag valid target which uses legacy format to update to Roslyn based DocCommentId format.
+                if (resolvedSymbols.Length > 0 && !targetHasDocCommentIdFormat)
                 {
+                    RoslynDebug.Assert(!string.IsNullOrEmpty(targetSymbolString));
+                    RoslynDebug.Assert(targetValueOperation != null);
+
+                    var properties = ImmutableDictionary<string, string?>.Empty;
+                    if (resolvedSymbols.Length == 1)
+                    {
+                        // We provide a code fix for the case where the target resolved to a single symbol.
+                        var docCommentId = DocumentationCommentId.CreateDeclarationId(resolvedSymbols[0]);
+                        if (!string.IsNullOrEmpty(docCommentId))
+                        {
+                            // Suppression target has an optional "~" prefix to distinguish it from legacy FxCop suppressions.
+                            // IDE suppression code fixes emit this prefix, so we we also add this prefix to new suppression target string.
+                            properties = properties.Add(DocCommentIdKey, "~" + docCommentId);
+                        }
+                    }
+
+                    reportDiagnostic(Diagnostic.Create(LegacyFormatTargetDescriptor, targetValueOperation.Syntax.GetLocation(), properties!, targetSymbolString));
                     return;
                 }
-
-                reportDiagnostic(Diagnostic.Create(rule, attributeSyntax.GetLocation()));
             }
         }
     }

src/Analyzers/Core/Analyzers/RemoveUnnecessarySuppressions/SuppressMessageAttributeState.cs

@@ -5,8 +5,8 @@
 #nullable enable
 
 using System;
-using System.Collections.Generic;
 using System.Collections.Immutable;
+using System.Diagnostics.CodeAnalysis;
 using System.Threading;
 using Microsoft.CodeAnalysis.Operations;
 using Microsoft.CodeAnalysis.PooledObjects;
@@ -20,7 +20,6 @@ internal partial class SuppressMessageAttributeState
         internal const string SuppressMessageTarget = "Target";
 
         private static readonly ImmutableDictionary<string, TargetScope> s_targetScopesMap = CreateTargetScopesMap();
-        private static readonly ObjectPool<List<ISymbol>> s_listPool = new ObjectPool<List<ISymbol>>(() => new List<ISymbol>());
 
         private readonly Compilation _compilation;
         private readonly INamedTypeSymbol _suppressMessageAttributeType;
@@ -82,9 +81,9 @@ simpleAssignment.Target is IPropertyReferenceOperation propertyReference &&
             return namedAttributeArguments.Length > 0;
         }
 
-        public static bool HasInvalidScope(ImmutableArray<(string name, IOperation value)> namedAttributeArguments, out TargetScope targetScope)
+        public static bool HasValidScope(ImmutableArray<(string name, IOperation value)> namedAttributeArguments, out TargetScope targetScope)
         {
-            if (!TryGetNamedArgument(namedAttributeArguments, SuppressMessageScope, out var scopeString) ||
+            if (!TryGetNamedArgument(namedAttributeArguments, SuppressMessageScope, out var scopeString, out _) ||
                 RoslynString.IsNullOrEmpty(scopeString))
             {
                 // Missing/Null/Empty scope values are treated equivalent to a compilation wide suppression.
@@ -93,46 +92,52 @@ public static bool HasInvalidScope(ImmutableArray<(string name, IOperation value
             else if (!s_targetScopesMap.TryGetValue(scopeString, out targetScope))
             {
                 targetScope = TargetScope.None;
-                return true;
+                return false;
             }
 
-            return false;
+            return true;
         }
 
-        public bool HasInvalidOrMissingTarget(ImmutableArray<(string name, IOperation value)> namedAttributeArguments, TargetScope targetScope)
+        public bool HasValidTarget(
+            ImmutableArray<(string name, IOperation value)> namedAttributeArguments,
+            TargetScope targetScope,
+            out bool targetHasDocCommentIdFormat,
+            out string? targetSymbolString,
+            out IOperation? targetValueOperation,
+            out ImmutableArray<ISymbol> resolvedSymbols)
         {
+            targetHasDocCommentIdFormat = false;
+            targetSymbolString = null;
+            targetValueOperation = null;
+            resolvedSymbols = ImmutableArray<ISymbol>.Empty;
+
             if (targetScope == TargetScope.Resource)
             {
                 // Legacy scope which we do not handle.
-                return false;
+                return true;
             }
 
-            if (!TryGetNamedArgument(namedAttributeArguments, SuppressMessageTarget, out var targetSymbolString))
+            if (!TryGetNamedArgument(namedAttributeArguments, SuppressMessageTarget, out targetSymbolString, out targetValueOperation))
             {
                 targetSymbolString = null;
             }
 
             if (targetScope == TargetScope.Module)
             {
                 // Compilation wide suppression with a non-null target is considered invalid.
-                return targetSymbolString != null;
+                return targetSymbolString == null;
             }
 
-            var resolvedSymbols = s_listPool.Allocate();
-            try
-            {
-                var resolver = new TargetSymbolResolver(_compilation, targetScope, targetSymbolString);
-                resolvedSymbols.Clear();
-                resolver.Resolve(resolvedSymbols);
-                return resolvedSymbols.Count == 0;
-            }
-            finally
-            {
-                s_listPool.Free(resolvedSymbols);
-            }
+            var resolver = new TargetSymbolResolver(_compilation, targetScope, targetSymbolString);
+            resolvedSymbols = resolver.Resolve(out targetHasDocCommentIdFormat);
+            return !resolvedSymbols.IsEmpty;
         }
 
-        private static bool TryGetNamedArgument(ImmutableArray<(string name, IOperation value)> namedAttributeArguments, string argumentName, out string? argumentValue)
+        private static bool TryGetNamedArgument(
+            ImmutableArray<(string name, IOperation value)> namedAttributeArguments,
+            string argumentName,
+            out string? argumentValue,
+            [NotNullWhen(returnValue: true)] out IOperation? argumentValueOperation)
         {
             foreach (var (name, value) in namedAttributeArguments)
             {
@@ -141,11 +146,13 @@ private static bool TryGetNamedArgument(ImmutableArray<(string name, IOperation
                     value.ConstantValue.Value is string stringValue)
                 {
                     argumentValue = stringValue;
+                    argumentValueOperation = value;
                     return true;
                 }
             }
 
             argumentValue = null;
+            argumentValueOperation = null;
             return false;
         }
     }

src/Analyzers/Core/Analyzers/xlf/AnalyzersResources.cs.xlf

@@ -37,6 +37,16 @@
         <target state="translated">Přidat kvalifikaci „this“ nebo „Me“</target>
         <note />
       </trans-unit>
+      <trans-unit id="Avoid_legacy_format_target_0_in_SuppressMessageAttribute">
+        <source>Avoid legacy format target '{0}' in 'SuppressMessageAttribute'</source>
+        <target state="new">Avoid legacy format target '{0}' in 'SuppressMessageAttribute'</target>
+        <note />
+      </trans-unit>
+      <trans-unit id="Avoid_legacy_format_target_in_SuppressMessageAttribute">
+        <source>Avoid legacy format target in 'SuppressMessageAttribute'</source>
+        <target state="new">Avoid legacy format target in 'SuppressMessageAttribute'</target>
+        <note />
+      </trans-unit>
       <trans-unit id="Avoid_unnecessary_value_assignments_in_your_code_as_these_likely_indicate_redundant_value_computations_If_the_value_computation_is_not_redundant_and_you_intend_to_retain_the_assignmentcomma_then_change_the_assignment_target_to_a_local_variable_whose_name_starts_with_an_underscore_and_is_optionally_followed_by_an_integercomma_such_as___comma__1_comma__2_comma_etc">
         <source>Avoid unnecessary value assignments in your code, as these likely indicate redundant value computations. If the value computation is not redundant and you intend to retain the assignment, then change the assignment target to a local variable whose name starts with an underscore and is optionally followed by an integer, such as '_', '_1', '_2', etc. These are treated as special discard symbol names.</source>
         <target state="translated">Vyhněte se v kódu přiřazením nepotřebných hodnot, protože ta pravděpodobně indikují nadbytečné výpočty hodnot. Pokud výpočet hodnoty není nadbytečný a chcete dané přiřazení zachovat, změňte cíl přiřazení na místní proměnnou, jejíž název začíná podtržítkem, za kterým volitelně následuje celé číslo, například _, _1, _2 atd. Tyto řetězce se považují za názvy speciálních symbolů pro vyřazení.</target>

src/Analyzers/Core/Analyzers/xlf/AnalyzersResources.de.xlf

@@ -37,6 +37,16 @@
         <target state="translated">Qualifizierung "this" oder "Me" hinzufügen.</target>
         <note />
       </trans-unit>
+      <trans-unit id="Avoid_legacy_format_target_0_in_SuppressMessageAttribute">
+        <source>Avoid legacy format target '{0}' in 'SuppressMessageAttribute'</source>
+        <target state="new">Avoid legacy format target '{0}' in 'SuppressMessageAttribute'</target>
+        <note />
+      </trans-unit>
+      <trans-unit id="Avoid_legacy_format_target_in_SuppressMessageAttribute">
+        <source>Avoid legacy format target in 'SuppressMessageAttribute'</source>
+        <target state="new">Avoid legacy format target in 'SuppressMessageAttribute'</target>
+        <note />
+      </trans-unit>
       <trans-unit id="Avoid_unnecessary_value_assignments_in_your_code_as_these_likely_indicate_redundant_value_computations_If_the_value_computation_is_not_redundant_and_you_intend_to_retain_the_assignmentcomma_then_change_the_assignment_target_to_a_local_variable_whose_name_starts_with_an_underscore_and_is_optionally_followed_by_an_integercomma_such_as___comma__1_comma__2_comma_etc">
         <source>Avoid unnecessary value assignments in your code, as these likely indicate redundant value computations. If the value computation is not redundant and you intend to retain the assignment, then change the assignment target to a local variable whose name starts with an underscore and is optionally followed by an integer, such as '_', '_1', '_2', etc. These are treated as special discard symbol names.</source>
         <target state="translated">Vermeiden Sie unnötige Wertzuweisungen in Ihrem Code, weil diese wahrscheinlich auf redundante Wertberechnungen hinweisen. Wenn die Wertberechnung nicht redundant ist und die Zuweisung beibehalten werden soll, ändern Sie das Zuweisungsziel in eine lokale Variable, deren Name mit einem Unterstrich beginnt, dem optional eine Zahl angefügt wird. Beispiel: "_", "_1", "_2" usw. Diese werden als spezielle Symbolnamen für Ausschussvariablen behandelt.</target>