Don't guard Dictionary<K, V>.Remove(key) by ContainsKey(key)

src/NetAnalyzers/CSharp/Microsoft.NetCore.Analyzers/Performance/CSharpDoNotGuardDictionaryRemoveByContainsKeyFixer.cs

@@ -0,0 +1,81 @@
+// Copyright (c) Microsoft.  All Rights Reserved.  Licensed under the MIT license.  See License.txt in the project root for license information.
+
+using System.Composition;
+using System.Linq;
+using Microsoft.CodeAnalysis;
+using Microsoft.CodeAnalysis.CodeFixes;
+using Microsoft.CodeAnalysis.CSharp.Syntax;
+using Microsoft.CodeAnalysis.Editing;
+using Microsoft.CodeAnalysis.Formatting;
+using Microsoft.NetCore.Analyzers.Performance;
+
+namespace Microsoft.NetCore.CSharp.Analyzers.Performance
+{
+    [ExportCodeFixProvider(LanguageNames.CSharp), Shared]
+    public sealed class CSharpDoNotGuardDictionaryRemoveByContainsKeyFixer : DoNotGuardDictionaryRemoveByContainsKeyFixer
+    {
+        protected override bool SyntaxSupportedByFixer(SyntaxNode conditionalSyntax)
+        {
+            if (conditionalSyntax is ConditionalExpressionSyntax conditionalExpressionSyntax)
+                return conditionalExpressionSyntax.WhenTrue.ChildNodes().Count() == 1;
+
+            if (conditionalSyntax is IfStatementSyntax)
+                return true;
+
+            return false;
+        }
+
+        protected override Document ReplaceConditionWithChild(Document document, SyntaxNode root,
+                                                              SyntaxNode conditionalOperationNode,
+                                                              SyntaxNode childOperationNode)
+        {
+            SyntaxNode newRoot;
+
+            // if there's a false (else) block, negate the condition and replace the single true statement with it
+
+            if (conditionalOperationNode is ConditionalExpressionSyntax conditionalExpressionSyntax &&
+                conditionalExpressionSyntax.WhenFalse.ChildNodes().Any())
+            {
+                var negatedExpression = GetNegatedExpression(document, childOperationNode);
+
+                SyntaxNode newConditionalOperationNode = conditionalExpressionSyntax
+                    .WithCondition((ExpressionSyntax)negatedExpression)
+                    .WithWhenTrue(conditionalExpressionSyntax.WhenFalse)
+                    .WithWhenFalse(null)
+                    .WithAdditionalAnnotations(Formatter.Annotation).WithTriviaFrom(conditionalOperationNode);
+
+                newRoot = root.ReplaceNode(conditionalOperationNode, newConditionalOperationNode);
+            }
+            else if (conditionalOperationNode is IfStatementSyntax ifStatementSyntax && ifStatementSyntax.Else != null)
+            {
+                var negatedExpression = GetNegatedExpression(document, childOperationNode);
+
+                SyntaxNode newConditionalOperationNode = ifStatementSyntax
+                    .WithCondition((ExpressionSyntax)negatedExpression)
+                    .WithStatement(ifStatementSyntax.Else.Statement)
+                    .WithElse(null)
+                    .WithAdditionalAnnotations(Formatter.Annotation).WithTriviaFrom(conditionalOperationNode);
+
+                newRoot = root.ReplaceNode(conditionalOperationNode, newConditionalOperationNode);
+            }
+            else
+            {
+                // preserve formatting and trivia
+
+                SyntaxNode newConditionNode = childOperationNode
+                    .WithAdditionalAnnotations(Formatter.Annotation)
+                    .WithTriviaFrom(conditionalOperationNode);
+
+                newRoot = root.ReplaceNode(conditionalOperationNode, newConditionNode);
+            }
+
+            return document.WithSyntaxRoot(newRoot);
+        }
+
+        private static SyntaxNode GetNegatedExpression(Document document, SyntaxNode newConditionNode)
+        {
+            var generator = SyntaxGenerator.GetGenerator(document);
+            return generator.LogicalNotExpression(((ExpressionStatementSyntax)newConditionNode).Expression.WithoutTrivia());
+        }
+    }
+}

src/NetAnalyzers/Core/AnalyzerReleases.Unshipped.md

@@ -11,6 +11,7 @@ CA1849 | Performance | Disabled | UseAsyncMethodInAsyncContext, [Documentation](
 CA1850 | Performance | Info | PreferHashDataOverComputeHashAnalyzer, [Documentation](https://docs.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1850)
 CA1851 | Performance | Disabled | AvoidMultipleEnumerations, [Documentation](https://docs.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1851)
 CA1852 | Performance | Hidden | SealInternalTypes, [Documentation](https://docs.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1852)
+CA1853 | Performance | Info | DoNotGuardDictionaryRemoveByContainsKey, [Documentation](https://docs.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1853)
 CA2019 | Reliability | Info | UseThreadStaticCorrectly, [Documentation](https://docs.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca2019)
 CA2259 | Usage | Warning | UseThreadStaticCorrectly, [Documentation](https://docs.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca2259)
 CA5404 | Security | Disabled | DoNotDisableTokenValidationChecks, [Documentation](https://docs.microsoft.com/visualstudio/code-quality/ca5404)

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/MicrosoftNetCoreAnalyzersResources.resx

@@ -1552,6 +1552,18 @@
     <value>and all other platforms</value>
     <comment>This call site is reachable on: 'windows' 10.0.2000 and later, and all other platforms</comment>
   </data>
+  <data name="DoNotGuardDictionaryRemoveByContainsKeyDescription" xml:space="preserve">
+    <value>Do not guard 'Dictionary.Remove(key)' with 'Dictionary.ContainsKey(key)'. The former already checks whether the key exists, and will not throw if it does not.</value>
+  </data>
+  <data name="DoNotGuardDictionaryRemoveByContainsKeyMessage" xml:space="preserve">
+    <value>Do not guard 'Dictionary.Remove(key)' with 'Dictionary.ContainsKey(key)'</value>
+  </data>
+  <data name="DoNotGuardDictionaryRemoveByContainsKeyTitle" xml:space="preserve">
+    <value>Unnecessary call to 'Dictionary.ContainsKey(key)'</value>
+  </data>
+  <data name="RemoveRedundantGuardCallCodeFixTitle" xml:space="preserve">
+    <value>Remove unnecessary call</value>
+  </data>
   <data name="BufferBlockCopyLengthMessage" xml:space="preserve">
     <value>'Buffer.BlockCopy' expects the number of bytes to be copied for the 'count' argument. Using 'Array.Length' may not match the number of bytes that needs to be copied.</value>
   </data>

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Performance/DoNotGuardDictionaryRemoveByContainsKey.Fixer.cs

@@ -0,0 +1,56 @@
+// Copyright (c) Microsoft.  All Rights Reserved.  Licensed under the MIT license.  See License.txt in the project root for license information.
+
+using System.Collections.Immutable;
+using System.Linq;
+using System.Threading.Tasks;
+using Microsoft.CodeAnalysis;
+using Microsoft.CodeAnalysis.CodeActions;
+using Microsoft.CodeAnalysis.CodeFixes;
+
+namespace Microsoft.NetCore.Analyzers.Performance
+{
+    public abstract class DoNotGuardDictionaryRemoveByContainsKeyFixer : CodeFixProvider
+    {
+        public override ImmutableArray<string> FixableDiagnosticIds { get; } =
+            ImmutableArray.Create(DoNotGuardDictionaryRemoveByContainsKey.RuleId);
+
+        public sealed override FixAllProvider GetFixAllProvider()
+        {
+            return WellKnownFixAllProviders.BatchFixer;
+        }
+
+        public sealed override async Task RegisterCodeFixesAsync(CodeFixContext context)
+        {
+            var root = await context.Document.GetSyntaxRootAsync(context.CancellationToken).ConfigureAwait(false);
+            var node = root.FindNode(context.Span, getInnermostNodeForTie: true);
+
+            if (node is null)
+            {
+                return;
+            }
+
+            var diagnostic = context.Diagnostics.FirstOrDefault();
+            var conditionalOperationSpan = diagnostic.AdditionalLocations[0];
+            var childLocation = diagnostic.AdditionalLocations[1];
+            if (root.FindNode(conditionalOperationSpan.SourceSpan) is not SyntaxNode conditionalSyntax ||
+                root.FindNode(childLocation.SourceSpan) is not SyntaxNode childStatementSyntax)
+            {
+                return;
+            }
+
+            // we only offer a fixer if 'Remove' is the _only_ statement
+            if (!SyntaxSupportedByFixer(conditionalSyntax))
+                return;
+
+            context.RegisterCodeFix(CodeAction.Create(MicrosoftNetCoreAnalyzersResources.RemoveRedundantGuardCallCodeFixTitle, ct =>
+                    Task.FromResult(ReplaceConditionWithChild(context.Document, root, conditionalSyntax, childStatementSyntax)), MicrosoftNetCoreAnalyzersResources.RemoveRedundantGuardCallCodeFixTitle),
+                diagnostic);
+        }
+
+        protected abstract bool SyntaxSupportedByFixer(SyntaxNode conditionalSyntax);
+
+        protected abstract Document ReplaceConditionWithChild(Document document, SyntaxNode root,
+                                                              SyntaxNode conditionalOperationNode,
+                                                              SyntaxNode childOperationNode);
+    }
+}

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Performance/DoNotGuardDictionaryRemoveByContainsKey.cs

@@ -0,0 +1,159 @@
+// Copyright (c) Microsoft.  All Rights Reserved.  Licensed under the MIT license.  See License.txt in the project root for license information.
+
+using System.Collections.Immutable;
+using System.Diagnostics.CodeAnalysis;
+using System.Linq;
+using Analyzer.Utilities;
+using Analyzer.Utilities.Extensions;
+using Analyzer.Utilities.PooledObjects;
+using Microsoft.CodeAnalysis;
+using Microsoft.CodeAnalysis.Diagnostics;
+using Microsoft.CodeAnalysis.Operations;
+
+namespace Microsoft.NetCore.Analyzers.Performance
+{
+    using static MicrosoftNetCoreAnalyzersResources;
+    /// <summary>
+    /// Do not guard 'Dictionary.Remove(key)' with 'Dictionary.ContainsKey(key)'
+    /// </summary>
+    [DiagnosticAnalyzer(LanguageNames.CSharp, LanguageNames.VisualBasic)]
+    public sealed class DoNotGuardDictionaryRemoveByContainsKey : DiagnosticAnalyzer
+    {
+        internal const string RuleId = "CA1853";
+        private const string Remove = nameof(Remove);
+        private const string ContainsKey = nameof(ContainsKey);
+
+        internal static readonly DiagnosticDescriptor Rule = DiagnosticDescriptorHelper.Create(
+            RuleId,
+            CreateLocalizableResourceString(nameof(DoNotGuardDictionaryRemoveByContainsKeyTitle)),
+            CreateLocalizableResourceString(nameof(DoNotGuardDictionaryRemoveByContainsKeyMessage)),
+            DiagnosticCategory.Performance,
+            RuleLevel.IdeSuggestion,
+            CreateLocalizableResourceString(nameof(DoNotGuardDictionaryRemoveByContainsKeyDescription)),
+            isPortedFxCopRule: false,
+            isDataflowRule: false);
+
+        public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics { get; } = ImmutableArray.Create(Rule);
+
+        public override void Initialize(AnalysisContext context)
+        {
+            context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None);
+            context.EnableConcurrentExecution();
+            context.RegisterCompilationStartAction(OnCompilationStart);
+        }
+
+        private static void OnCompilationStart(CompilationStartAnalysisContext context)
+        {
+            if (!TryGetDictionaryTypeAndMethods(context.Compilation, out var containsKey, out var remove1Param, out var remove2Param))
+            {
+                return;
+            }
+
+            context.RegisterOperationAction(context => AnalyzeOperation(context, containsKey, remove1Param, remove2Param), OperationKind.Conditional);
+
+            static void AnalyzeOperation(OperationAnalysisContext context, IMethodSymbol containsKeyMethod, IMethodSymbol remove1Param, IMethodSymbol? remove2Param)
+            {
+                var conditionalOperation = (IConditionalOperation)context.Operation;
+
+                IInvocationOperation? invocationOperation = null;
+
+                switch (conditionalOperation.Condition)
+                {
+                    case IInvocationOperation iOperation:
+                        invocationOperation = iOperation;
+                        break;
+                    case IUnaryOperation unaryOperation when unaryOperation.OperatorKind == UnaryOperatorKind.Not:
+                        if (unaryOperation.Operand is IInvocationOperation operand)
+                            invocationOperation = operand;
+                        break;
+                    case IParenthesizedOperation parenthesizedOperation:
+                        if (parenthesizedOperation.Operand is IInvocationOperation invocation)
+                            invocationOperation = invocation;
+                        break;
+                    default:
+                        return;
+                }
+
+                if (invocationOperation == null || !invocationOperation.TargetMethod.OriginalDefinition.Equals(containsKeyMethod, SymbolEqualityComparer.Default))
+                {
+                    return;
+                }
+
+                if (conditionalOperation.WhenTrue.Children.Any())
+                {
+                    using var additionalLocation = ArrayBuilder<Location>.GetInstance(2);
+                    additionalLocation.Add(conditionalOperation.Syntax.GetLocation());
+
+                    switch (conditionalOperation.WhenTrue.Children.First())
+                    {
+                        case IInvocationOperation childInvocationOperation:
+                            if (childInvocationOperation.TargetMethod.OriginalDefinition.Equals(remove1Param, SymbolEqualityComparer.Default) ||
+                                childInvocationOperation.TargetMethod.OriginalDefinition.Equals(remove2Param, SymbolEqualityComparer.Default))
+                            {
+                                additionalLocation.Add(childInvocationOperation.Syntax.Parent.GetLocation());
+                                context.ReportDiagnostic(invocationOperation.CreateDiagnostic(Rule, additionalLocations: additionalLocation.ToImmutable(), null));
+                            }
+
+                            break;
+                        case IExpressionStatementOperation childStatementOperation:
+                            /*
+                             * If the if statement contains a block, only proceed if one of the methods calls Remove.
+                             * However, a fixer is only offered if there is a single method in the block.
+                             */
+
+                            var nestedInvocationOperation = childStatementOperation.Children.OfType<IInvocationOperation>()
+                                                            .FirstOrDefault(op => op.TargetMethod.OriginalDefinition.Equals(remove1Param, SymbolEqualityComparer.Default) ||
+                                                                                  op.TargetMethod.OriginalDefinition.Equals(remove2Param, SymbolEqualityComparer.Default));
+
+                            if (nestedInvocationOperation != null)
+                            {
+                                additionalLocation.Add(nestedInvocationOperation.Syntax.Parent.GetLocation());
+                                context.ReportDiagnostic(invocationOperation.CreateDiagnostic(Rule, additionalLocations: additionalLocation.ToImmutable(), null));
+                            }
+
+                            break;
+                        default:
+                            break;
+                    }
+                }
+            }
+            static bool TryGetDictionaryTypeAndMethods(Compilation compilation, [NotNullWhen(true)] out IMethodSymbol? containsKey,
+                            [NotNullWhen(true)] out IMethodSymbol? remove1Param, out IMethodSymbol? remove2Param)
+            {
+                containsKey = null;
+                remove1Param = null;
+                remove2Param = null;
+
+                if (!compilation.TryGetOrCreateTypeByMetadataName(WellKnownTypeNames.SystemCollectionsGenericDictionary2, out var dictionary))
+                {
+                    return false;
+                }
+
+                foreach (var m in dictionary.GetMembers().OfType<IMethodSymbol>())
+                {
+                    if (m.ReturnType.SpecialType == SpecialType.System_Boolean)
+                    {
+                        switch (m.Parameters.Length)
+                        {
+                            case 1:
+                                switch (m.Name)
+                                {
+                                    case ContainsKey: containsKey = m; break;
+                                    case Remove: remove1Param = m; break;
+                                }
+                                break;
+                            case 2:
+                                if (m.Name == Remove)
+                                {
+                                    remove2Param = m;
+                                }
+                                break;
+                        }
+                    }
+                }
+
+                return containsKey != null && remove1Param != null;
+            }
+        }
+    }
+}

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/xlf/MicrosoftNetCoreAnalyzersResources.cs.xlf

@@ -682,6 +682,21 @@
         <target state="translated">Nezakazujte ServicePointManagerSecurityProtocols</target>
         <note />
       </trans-unit>
+      <trans-unit id="DoNotGuardDictionaryRemoveByContainsKeyDescription">
+        <source>Do not guard 'Dictionary.Remove(key)' with 'Dictionary.ContainsKey(key)'. The former already checks whether the key exists, and will not throw if it does not.</source>
+        <target state="new">Do not guard 'Dictionary.Remove(key)' with 'Dictionary.ContainsKey(key)'. The former already checks whether the key exists, and will not throw if it does not.</target>
+        <note />
+      </trans-unit>
+      <trans-unit id="DoNotGuardDictionaryRemoveByContainsKeyMessage">
+        <source>Do not guard 'Dictionary.Remove(key)' with 'Dictionary.ContainsKey(key)'</source>
+        <target state="new">Do not guard 'Dictionary.Remove(key)' with 'Dictionary.ContainsKey(key)'</target>
+        <note />
+      </trans-unit>
+      <trans-unit id="DoNotGuardDictionaryRemoveByContainsKeyTitle">
+        <source>Unnecessary call to 'Dictionary.ContainsKey(key)'</source>
+        <target state="new">Unnecessary call to 'Dictionary.ContainsKey(key)'</target>
+        <note />
+      </trans-unit>
       <trans-unit id="DoNotHardCodeCertificate">
         <source>Do not hard-code certificate</source>
         <target state="translated">Nepoužívejte pevně zakódovaný certifikát</target>
@@ -2142,6 +2157,11 @@
         <target state="translated">Odebrat redundantní volání</target>
         <note />
       </trans-unit>
+      <trans-unit id="RemoveRedundantGuardCallCodeFixTitle">
+        <source>Remove unnecessary call</source>
+        <target state="new">Remove unnecessary call</target>
+        <note />
+      </trans-unit>
       <trans-unit id="ReplaceStringLiteralWithCharLiteralCodeActionTitle">
         <source>Replace string literal with char literal</source>
         <target state="translated">Nahradit řetězcový literál s literálem char</target>