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

@@ -8,3 +8,4 @@ CA1849 | Performance | Disabled | UseAsyncMethodInAsyncContext, [Documentation](
 CA1850 | Performance | Info | PreferHashDataOverComputeHashAnalyzer, [Documentation](https://docs.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1850)
 CA5404 | Security | Disabled | DoNotDisableTokenValidationChecks, [Documentation](https://docs.microsoft.com/visualstudio/code-quality/ca5404)
 CA5405 | Security | Disabled | DoNotAlwaysSkipTokenValidationInDelegates, [Documentation](https://docs.microsoft.com/visualstudio/code-quality/ca5405)
+CA1851 | Performance | Info | DoNotGuardDictionaryRemoveByContainsKey, [Documentation](https://docs.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1849)

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

@@ -1534,6 +1534,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>Do not guard 'Dictionary.Remove(key)' with 'Dictionary.ContainsKey(key)'</value>
+  </data>
+  <data name="RemoveRedundantGuardCallCodeFixTitle" xml:space="preserve">
+    <value>Remove redundant guard 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,87 @@
+// Copyright (c) Microsoft.  All Rights Reserved.  Licensed under the MIT license.  See License.txt in the project root for license information.
+
+using System;
+using System.Collections.Immutable;
+using System.Linq;
+using System.Threading;
+using System.Threading.Tasks;
+using Analyzer.Utilities;
+using Microsoft.CodeAnalysis;
+using Microsoft.CodeAnalysis.CodeFixes;
+using Microsoft.CodeAnalysis.Text;
+
+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();
+
+            if (!TryParseLocationInfo(diagnostic, DoNotGuardDictionaryRemoveByContainsKey.ConditionalOperation, out var conditionalOperationSpan) ||
+                !TryParseLocationInfo(diagnostic, DoNotGuardDictionaryRemoveByContainsKey.ChildStatementOperation, out var childStatementOperationSpan) ||
+                root.FindNode(conditionalOperationSpan) is not SyntaxNode conditionalSyntax ||
+                root.FindNode(childStatementOperationSpan) is not SyntaxNode childStatementSyntax)
+            {
+                return;
+            }
+
+            // we only offer a fixer if 'Remove' is the _only_ statement
+            if (!SyntaxSupportedByFixer(conditionalSyntax))
+                return;
+
+            context.RegisterCodeFix(new DoNotGuardDictionaryRemoveByContainsKeyCodeAction(_ =>
+                Task.FromResult(ReplaceConditionWithChild(context.Document, root, conditionalSyntax, childStatementSyntax))),
+                diagnostic);
+        }
+
+        protected abstract bool SyntaxSupportedByFixer(SyntaxNode conditionalSyntax);
+
+        protected abstract Document ReplaceConditionWithChild(Document document, SyntaxNode root,
+                                                              SyntaxNode conditionalOperationNode,
+                                                              SyntaxNode childOperationNode);
+
+        private static bool TryParseLocationInfo(Diagnostic diagnostic, string propertyKey, out TextSpan span)
+        {
+            span = default;
+
+            if (!diagnostic.Properties.TryGetValue(propertyKey, out var locationInfo))
+                return false;
+
+            var parts = locationInfo.Split(DoNotGuardDictionaryRemoveByContainsKey.AdditionalDocumentLocationInfoSeparatorArray, StringSplitOptions.None);
+            if (parts.Length != 2 ||
+                !int.TryParse(parts[0], out var spanStart) ||
+                !int.TryParse(parts[1], out var spanLength))
+            {
+                return false;
+            }
+
+            span = new TextSpan(spanStart, spanLength);
+            return true;
+        }
+
+        private class DoNotGuardDictionaryRemoveByContainsKeyCodeAction : DocumentChangeAction
+        {
+            public DoNotGuardDictionaryRemoveByContainsKeyCodeAction(Func<CancellationToken, Task<Document>> action)
+            : base(MicrosoftNetCoreAnalyzersResources.RemoveRedundantGuardCallCodeFixTitle, action,
+                   DoNotGuardDictionaryRemoveByContainsKey.RuleId)
+            { }
+        }
+    }
+}

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

@@ -0,0 +1,154 @@
+// 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 Microsoft.CodeAnalysis;
+using Microsoft.CodeAnalysis.Diagnostics;
+using Microsoft.CodeAnalysis.Operations;
+
+namespace Microsoft.NetCore.Analyzers.Performance
+{
+    using static MicrosoftNetCoreAnalyzersResources;
+
+    [DiagnosticAnalyzer(LanguageNames.CSharp, LanguageNames.VisualBasic)]
+    public sealed class DoNotGuardDictionaryRemoveByContainsKey : DiagnosticAnalyzer
+    {
+        internal const string RuleId = "CA1851";
+
+        public const string AdditionalDocumentLocationInfoSeparator = ";;";
+        public static readonly string[] AdditionalDocumentLocationInfoSeparatorArray = new[] { AdditionalDocumentLocationInfoSeparator };
+        public const string ConditionalOperation = nameof(ConditionalOperation);
+        public const string ChildStatementOperation = nameof(ChildStatementOperation);
+
+        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,
+            additionalCustomTags: WellKnownDiagnosticTags.Unnecessary);
+
+        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 (!TryGetDictionaryTypeAndContainsKeyeMethod(context.Compilation, out var dictionaryType, out var containsKeyMethod))
+            {
+                return;
+            }
+
+            context.RegisterOperationAction(context => AnalyzeOperation(context, dictionaryType, containsKeyMethod), OperationKind.Conditional);
+
+            static void AnalyzeOperation(OperationAnalysisContext context, INamedTypeSymbol dictionaryType, IMethodSymbol containsKeyMethod)
+            {
+                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;
+                    default:
+                        return;
+                }
+
+                if (invocationOperation == null || !invocationOperation.TargetMethod.OriginalDefinition.Equals(containsKeyMethod, SymbolEqualityComparer.Default))
+                {
+                    return;
+                }
+
+                if (conditionalOperation.WhenTrue.Children.Any())
+                {
+                    var properties = ImmutableDictionary.CreateBuilder<string, string?>();
+                    properties[ConditionalOperation] = CreateLocationInfo(conditionalOperation.Syntax);
+
+                    switch (conditionalOperation.WhenTrue.Children.First())
+                    {
+                        case IInvocationOperation childInvocationOperation:
+                            if (childInvocationOperation.TargetMethod.Name == "Remove" &&
+                                childInvocationOperation.TargetMethod.OriginalDefinition.ContainingType.Equals(dictionaryType, SymbolEqualityComparer.Default))
+                            {
+                                properties[ChildStatementOperation] = CreateLocationInfo(childInvocationOperation.Syntax.Parent);
+
+                                context.ReportDiagnostic(invocationOperation.CreateDiagnostic(Rule, properties.ToImmutable()));
+                            }
+
+                            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.Name == "Remove" &&
+                                                                op.TargetMethod.OriginalDefinition.ContainingType.Equals(dictionaryType, SymbolEqualityComparer.Default));
+
+                            if (nestedInvocationOperation != null)
+                            {
+                                properties[ChildStatementOperation] = CreateLocationInfo(nestedInvocationOperation.Syntax.Parent);
+
+                                context.ReportDiagnostic(invocationOperation.CreateDiagnostic(Rule, properties.ToImmutable()));
+                            }
+
+                            break;
+                        default:
+                            break;
+                    }
+                }
+            }
+            static bool TryGetDictionaryTypeAndContainsKeyeMethod(Compilation compilation, [NotNullWhen(true)] out INamedTypeSymbol? dictionaryType, [NotNullWhen(true)] out IMethodSymbol? containsKeyMethod)
+            {
+                containsKeyMethod = null;
+
+                if (!compilation.TryGetOrCreateTypeByMetadataName(WellKnownTypeNames.SystemCollectionsGenericDictionary2, out dictionaryType))
+                {
+                    return false;
+                }
+
+                foreach (var m in dictionaryType.GetMembers().OfType<IMethodSymbol>())
+                {
+                    if (m.ReturnType.SpecialType == SpecialType.System_Boolean &&
+                        m.Parameters.Length == 1 &&
+                        m.Name == "ContainsKey" &&
+                        m.Parameters[0].Name == "key")
+                    {
+                        containsKeyMethod = m;
+                        return true;
+                    }
+                }
+
+                return false;
+            }
+        }
+
+        private static string CreateLocationInfo(SyntaxNode syntax)
+        {
+            // see DiagnosticDescriptorCreationAnalyzer
+
+            var location = syntax.GetLocation();
+            var span = location.SourceSpan;
+
+            return $"{span.Start}{AdditionalDocumentLocationInfoSeparator}{span.Length}";
+        }
+    }
+}

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>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="DoNotHardCodeCertificate">
         <source>Do not hard-code certificate</source>
         <target state="translated">Nepoužívejte pevně zakódovaný certifikát</target>
@@ -2082,6 +2097,11 @@
         <target state="translated">Odebrat redundantní volání</target>
         <note />
       </trans-unit>
+      <trans-unit id="RemoveRedundantGuardCallCodeFixTitle">
+        <source>Remove redundant guard call</source>
+        <target state="new">Remove redundant guard 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>