Issue 33798: Prefer Dictionary<K, V>.TryGetValue() over guarded indexer access

src/NetAnalyzers/CSharp/Microsoft.NetCore.Analyzers/Runtime/CSharpPreferDictionaryTryGetValueFixer.cs

@@ -0,0 +1,60 @@
+// Copyright (c) Microsoft.  All Rights Reserved.  Licensed under the Apache License, Version 2.0.  See License.txt in the project root for license information.
+
+using System.Composition;
+using System.Linq;
+using System.Threading.Tasks;
+using Microsoft.CodeAnalysis;
+using Microsoft.CodeAnalysis.CodeActions;
+using Microsoft.CodeAnalysis.CodeFixes;
+using Microsoft.CodeAnalysis.CSharp;
+using Microsoft.CodeAnalysis.CSharp.Syntax;
+using Microsoft.CodeAnalysis.Editing;
+using Microsoft.NetCore.Analyzers.Runtime;
+
+namespace Microsoft.NetCore.CSharp.Analyzers.Runtime
+{
+    [ExportCodeFixProvider(LanguageNames.CSharp), Shared]
+    public sealed class CSharpPreferDictionaryTryGetValueFixer : PreferDictionaryTryGetValueFixer
+    {
+        public override async Task RegisterCodeFixesAsync(CodeFixContext context)
+        {
+            var diagnostic = context.Diagnostics.FirstOrDefault();
+            var dictionaryAccessLocation = diagnostic?.AdditionalLocations[0];
+            if (dictionaryAccessLocation is null)
+            {
+                return;
+            }
+
+            Document document = context.Document;
+            SyntaxNode root = await document.GetSyntaxRootAsync().ConfigureAwait(false);
+
+            var dictionaryAccess = root.FindNode(dictionaryAccessLocation.SourceSpan, getInnermostNodeForTie: true);
+            if (dictionaryAccess is not ElementAccessExpressionSyntax
+                || root.FindNode(context.Span) is not InvocationExpressionSyntax containsKeyInvocation
+                || containsKeyInvocation.Expression is not MemberAccessExpressionSyntax containsKeyAccess)
+            {
+                return;
+            }
+
+            var action = CodeAction.Create(PreferDictionaryTryGetValueCodeFixTitle, async ct =>
+            {
+                var editor = await DocumentEditor.CreateAsync(document, ct).ConfigureAwait(false);
+                var generator = editor.Generator;
+
+                var tryGetValueAccess = generator.MemberAccessExpression(containsKeyAccess.Expression, "TryGetValue");
+                var keyArgument = containsKeyInvocation.ArgumentList.Arguments.FirstOrDefault();
+
+                var outArgument = generator.Argument(RefKind.Out,
+                    SyntaxFactory.DeclarationExpression(SyntaxFactory.IdentifierName("var"), SyntaxFactory.SingleVariableDesignation(SyntaxFactory.Identifier("value"))));
+                var tryGetValueInvocation = generator.InvocationExpression(tryGetValueAccess, keyArgument, outArgument);
+                editor.ReplaceNode(containsKeyInvocation, tryGetValueInvocation);
+
+                editor.ReplaceNode(dictionaryAccess, generator.IdentifierName("value"));
+
+                return editor.GetChangedDocument();
+            }, PreferDictionaryTryGetValueCodeFixTitle);
+
+            context.RegisterCodeFix(action, context.Diagnostics);
+        }
+    }
+}

src/NetAnalyzers/Core/AnalyzerReleases.Unshipped.md

@@ -1,5 +1,10 @@
 ; Please do not edit this file manually, it should only be updated through code fix application.
 
+### New Rules
+Rule ID | Category | Severity | Notes
+--------|----------|----------|-------
+CA1839 | Performance | Warning | PreferDictionaryTryGetValueAnalyzer, [Documentation](https://docs.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1839)
+
 ### Removed Rules
 
 Rule ID | Category | Severity | Notes

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

@@ -1510,4 +1510,16 @@
     <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="PreferDictionaryTryGetValueCodeFixTitle" xml:space="preserve">
+    <value>Use 'TryGetValue(TKey, out TValue)'</value>
+  </data>
+  <data name="PreferDictionaryTryGetValueTitle" xml:space="preserve">
+    <value>Prefer the Dictionary's TryGetValue method</value>
+  </data>
+  <data name="PreferDictionaryTryGetValueMessage" xml:space="preserve">
+    <value>Prefer a 'TryGetValue' call over a Dictionary indexer access guarded by a 'ContainsKey' check.</value>
+  </data>
+  <data name="PreferDictionaryTryGetValueDescription" xml:space="preserve">
+    <value>Prefer a 'TryGetValue' call over a Dictionary indexer access guarded by a 'ContainsKey' check.</value>
+  </data>
 </root>

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/PreferDictionaryTryGetValueAnalyzer.cs

@@ -0,0 +1,150 @@
+// Copyright (c) Microsoft.  All Rights Reserved.  Licensed under the Apache License, Version 2.0.  See License.txt in the project root for license information.
+
+using System.Collections.Generic;
+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.Runtime
+{
+    [DiagnosticAnalyzer(LanguageNames.CSharp, LanguageNames.VisualBasic)]
+    public sealed class PreferDictionaryTryGetValueAnalyzer : DiagnosticAnalyzer
+    {
+        public const string RuleId = "CA1839";
+
+        private const string ContainsKeyMethodName = nameof(IDictionary<dynamic, dynamic>.ContainsKey);
+        private const string IndexerName = "this[]";
+        private const string IndexerNameVb = "Item";
+
+        private static readonly LocalizableString s_localizableTitle = CreateResource(nameof(MicrosoftNetCoreAnalyzersResources.PreferDictionaryTryGetValueTitle));
+        private static readonly LocalizableString s_localizableTryGetValueMessage = CreateResource(nameof(MicrosoftNetCoreAnalyzersResources.PreferDictionaryTryGetValueMessage));
+        private static readonly LocalizableString s_localizableTryGetValueDescription = CreateResource(nameof(MicrosoftNetCoreAnalyzersResources.PreferDictionaryTryGetValueDescription));
+
+        internal static readonly DiagnosticDescriptor ContainsKeyRule = DiagnosticDescriptorHelper.Create(
+            RuleId,
+            s_localizableTitle,
+            s_localizableTryGetValueMessage,
+            DiagnosticCategory.Performance,
+            RuleLevel.BuildWarning,
+            s_localizableTryGetValueDescription,
+            isPortedFxCopRule: false,
+            isDataflowRule: false);
+
+        public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics => ImmutableArray.Create(ContainsKeyRule);
+
+        public override void Initialize(AnalysisContext context)
+        {
+            context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None);
+            context.EnableConcurrentExecution();
+            context.RegisterCompilationStartAction(OnCompilationStart);
+        }
+
+        private void OnCompilationStart(CompilationStartAnalysisContext compilationContext)
+        {
+            var compilation = compilationContext.Compilation;
+
+            if (!compilation.TryGetOrCreateTypeByMetadataName(WellKnownTypeNames.SystemCollectionsGenericICollection1, out _))
+                return;
+            if (!compilation.TryGetOrCreateTypeByMetadataName(WellKnownTypeNames.SystemCollectionsGenericIDictionary2, out var dictionaryType))
+                return;
+            if (!compilation.TryGetOrCreateTypeByMetadataName(WellKnownTypeNames.SystemCollectionsGenericIEnumerable1, out _))
+                return;
+
+            compilationContext.RegisterOperationAction(context => OnOperationAction(context, dictionaryType), OperationKind.PropertyReference);
+        }
+
+        private static void OnOperationAction(OperationAnalysisContext context, INamedTypeSymbol dictionaryType)
+        {
+            var propertyReference = (IPropertyReferenceOperation)context.Operation;
+
+            if (propertyReference.Parent is IAssignmentOperation
+                || !IsDictionaryAccess(propertyReference, dictionaryType)
+                || !TryGetParentConditionalOperation(propertyReference, out var conditionalOperation)
+                || !TryGetContainsKeyGuard(conditionalOperation, out var containsKeyInvocation))
+            {
+                return;
+            }
+
+            if (conditionalOperation.WhenTrue is IBlockOperation blockOperation && DictionaryEntryIsModified(propertyReference, blockOperation))
+            {
+                return;
+            }
+
+            var additionalLocations = ImmutableArray.Create(propertyReference.Syntax.GetLocation());
+            context.ReportDiagnostic(Diagnostic.Create(ContainsKeyRule, containsKeyInvocation.Syntax.GetLocation(), additionalLocations));
+        }
+
+        private static bool TryGetContainsKeyGuard(IConditionalOperation conditionalOperation, [NotNullWhen(true)] out IInvocationOperation? containsKeyInvocation)
+        {
+            containsKeyInvocation = conditionalOperation.Condition as IInvocationOperation ?? FindContainsKeyInvocation(conditionalOperation.Condition);
+            if (containsKeyInvocation is not null && containsKeyInvocation.TargetMethod.Name == ContainsKeyMethodName)
+            {
+                return true;
+            }
+
+            return false;
+        }
+
+        private static IInvocationOperation? FindContainsKeyInvocation(IOperation baseOperation)
+        {
+            return baseOperation switch
+            {
+                IInvocationOperation i when i.TargetMethod.Name == ContainsKeyMethodName => i,
+                IBinaryOperation { OperatorKind: BinaryOperatorKind.ConditionalAnd or BinaryOperatorKind.ConditionalOr } b =>
+                    FindContainsKeyInvocation(b.LeftOperand) ?? FindContainsKeyInvocation(b.RightOperand),
+                _ => null
+            };
+        }
+
+        private static bool DictionaryEntryIsModified(IPropertyReferenceOperation dictionaryAccess, IBlockOperation blockOperation)
+        {
+            return blockOperation.Operations.OfType<IExpressionStatementOperation>().Any(o =>
+                o.Operation is IAssignmentOperation { Target: IPropertyReferenceOperation reference } && reference.Property.Equals(dictionaryAccess.Property, SymbolEqualityComparer.Default));
+        }
+
+        private static bool IsDictionaryAccess(IPropertyReferenceOperation propertyReference, INamedTypeSymbol dictionaryType)
+        {
+            return propertyReference.Property.IsIndexer && IsDictionaryType(propertyReference.Property.ContainingType, dictionaryType) &&
+                   (propertyReference.Property.OriginalDefinition.Name == IndexerName || propertyReference.Language == "Visual Basic" && propertyReference.Property.OriginalDefinition.Name == IndexerNameVb);
+        }
+
+        private static bool TryGetParentConditionalOperation(IOperation derivedOperation, [NotNullWhen(true)] out IConditionalOperation? conditionalOperation)
+        {
+            conditionalOperation = null;
+            do
+            {
+                if (derivedOperation.Parent is IConditionalOperation c)
+                {
+                    conditionalOperation = c;
+
+                    return true;
+                }
+
+                derivedOperation = derivedOperation.Parent;
+            } while (derivedOperation.Parent != null);
+
+            return false;
+        }
+
+        private static bool IsDictionaryType(INamedTypeSymbol derived, ISymbol dictionaryType)
+        {
+            var constructedDictionaryType = derived.GetBaseTypesAndThis()
+                .WhereAsArray(x => x.OriginalDefinition.Equals(dictionaryType, SymbolEqualityComparer.Default))
+                .SingleOrDefault() ?? derived.AllInterfaces
+                .WhereAsArray(x => x.OriginalDefinition.Equals(dictionaryType, SymbolEqualityComparer.Default))
+                .SingleOrDefault();
+
+            return constructedDictionaryType is not null;
+        }
+
+        private static LocalizableString CreateResource(string resourceName)
+        {
+            return new LocalizableResourceString(resourceName, MicrosoftNetCoreAnalyzersResources.ResourceManager, typeof(MicrosoftNetCoreAnalyzersResources));
+        }
+    }
+}

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/PreferDictionaryTryGetValueFixer.cs

@@ -0,0 +1,16 @@
+// Copyright (c) Microsoft.  All Rights Reserved.  Licensed under the Apache License, Version 2.0.  See License.txt in the project root for license information.
+
+using System.Collections.Immutable;
+using Microsoft.CodeAnalysis.CodeFixes;
+
+namespace Microsoft.NetCore.Analyzers.Runtime
+{
+    public abstract class PreferDictionaryTryGetValueFixer : CodeFixProvider
+    {
+        public override ImmutableArray<string> FixableDiagnosticIds => ImmutableArray.Create(PreferDictionaryTryGetValueAnalyzer.RuleId);
+
+        protected static string PreferDictionaryTryGetValueCodeFixTitle => MicrosoftNetCoreAnalyzersResources.PreferDictionaryTryGetValueCodeFixTitle;
+
+        public override FixAllProvider GetFixAllProvider() => WellKnownFixAllProviders.BatchFixer;
+    }
+}

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

@@ -2302,6 +2302,26 @@
         <target state="translated">Toto přetížení metody {0}.{1} může být nebezpečné. Může povolit specifikaci DTD, která může být ohrožená útoky DoS (Denial of Service) nebo může používat XmlResolver, který představuje riziko odhalení informací. Použijte místo toho přetížení, které přijímá instanci XmlReader, má zakázané zpracování DTD a nepoužívá XmlResolver.</target>
         <note />
       </trans-unit>
+      <trans-unit id="PreferDictionaryTryGetValueCodeFixTitle">
+        <source>Use 'TryGetValue(TKey, out TValue)'</source>
+        <target state="new">Use 'TryGetValue(TKey, out TValue)'</target>
+        <note />
+      </trans-unit>
+      <trans-unit id="PreferDictionaryTryGetValueTitle">
+        <source>Prefer the Dictionary's TryGetValue method</source>
+        <target state="new">Prefer the Dictionary's TryGetValue method</target>
+        <note />
+      </trans-unit>
+      <trans-unit id="PreferDictionaryTryGetValueMessage">
+        <source>Prefer a 'TryGetValue' call over a Dictionary indexer access guarded by a 'ContainsKey' check.</source>
+        <target state="new">Prefer a 'TryGetValue' call over a Dictionary indexer access guarded by a 'ContainsKey' check.</target>
+        <note />
+      </trans-unit>
+      <trans-unit id="PreferDictionaryTryGetValueDescription">
+        <source>Prefer a 'TryGetValue' call over a Dictionary indexer access guarded by a 'ContainsKey' check.</source>
+        <target state="new">Prefer a 'TryGetValue' call over a Dictionary indexer access guarded by a 'ContainsKey' check.</target>
+        <note />
+      </trans-unit>
     </body>
   </file>
 </xliff>

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

@@ -2302,6 +2302,26 @@
         <target state="translated">Diese Überladung der {0}.{1}-Methode ist potenziell unsicher. Sie kann die Dokumenttypdefinition (DTD) aktivieren, die für DoS-Angriff (Denial of Service) anfällig ist, oder einen XmlResolver verwenden, der anfällig für eine Offenlegung von Informationen sein kann. Verwenden Sie eine Überladung, die stattdessen eine XmlReader-Instanz nutzt, und bei der die DTD-Verarbeitung deaktiviert ist und kein XmlResolver-Vorgang ausgeführt wird.</target>
         <note />
       </trans-unit>
+      <trans-unit id="PreferDictionaryTryGetValueCodeFixTitle">
+        <source>Use 'TryGetValue(TKey, out TValue)'</source>
+        <target state="new">Use 'TryGetValue(TKey, out TValue)'</target>
+        <note />
+      </trans-unit>
+      <trans-unit id="PreferDictionaryTryGetValueTitle">
+        <source>Prefer the Dictionary's TryGetValue method</source>
+        <target state="new">Prefer the Dictionary's TryGetValue method</target>
+        <note />
+      </trans-unit>
+      <trans-unit id="PreferDictionaryTryGetValueMessage">
+        <source>Prefer a 'TryGetValue' call over a Dictionary indexer access guarded by a 'ContainsKey' check.</source>
+        <target state="new">Prefer a 'TryGetValue' call over a Dictionary indexer access guarded by a 'ContainsKey' check.</target>
+        <note />
+      </trans-unit>
+      <trans-unit id="PreferDictionaryTryGetValueDescription">
+        <source>Prefer a 'TryGetValue' call over a Dictionary indexer access guarded by a 'ContainsKey' check.</source>
+        <target state="new">Prefer a 'TryGetValue' call over a Dictionary indexer access guarded by a 'ContainsKey' check.</target>
+        <note />
+      </trans-unit>
     </body>
   </file>
 </xliff>

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

@@ -2302,6 +2302,26 @@
         <target state="translated">Es posible que la sobrecarga del método "{0}.{1}" no sea segura. Puede habilitar la definición de tipo de documento (DTD), que puede ser vulnerable a los ataques por denegación de servicio, o bien usar un elemento XmlResolver que puede ser vulnerable a la divulgación de información. Use una sobrecarga que tome una instancia de XmlReader, con el procesamiento de DTD deshabilitado y sin XmlResolver.</target>
         <note />
       </trans-unit>
+      <trans-unit id="PreferDictionaryTryGetValueCodeFixTitle">
+        <source>Use 'TryGetValue(TKey, out TValue)'</source>
+        <target state="new">Use 'TryGetValue(TKey, out TValue)'</target>
+        <note />
+      </trans-unit>
+      <trans-unit id="PreferDictionaryTryGetValueTitle">
+        <source>Prefer the Dictionary's TryGetValue method</source>
+        <target state="new">Prefer the Dictionary's TryGetValue method</target>
+        <note />
+      </trans-unit>
+      <trans-unit id="PreferDictionaryTryGetValueMessage">
+        <source>Prefer a 'TryGetValue' call over a Dictionary indexer access guarded by a 'ContainsKey' check.</source>
+        <target state="new">Prefer a 'TryGetValue' call over a Dictionary indexer access guarded by a 'ContainsKey' check.</target>
+        <note />
+      </trans-unit>
+      <trans-unit id="PreferDictionaryTryGetValueDescription">
+        <source>Prefer a 'TryGetValue' call over a Dictionary indexer access guarded by a 'ContainsKey' check.</source>
+        <target state="new">Prefer a 'TryGetValue' call over a Dictionary indexer access guarded by a 'ContainsKey' check.</target>
+        <note />
+      </trans-unit>
     </body>
   </file>
 </xliff>

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

@@ -2302,6 +2302,26 @@
         <target state="translated">Cette surcharge de la méthode '{0}.{1}' est potentiellement non sécurisée. Elle peut activer le traitement DTD (définition de type de document), qui est vulnérable aux attaques par déni de service, ou utiliser une opération XmlResolver, qui est vulnérable à la divulgation d'informations. Employez une surcharge qui accepte plutôt une instance de XmlReader, avec désactivation du traitement DTD et de XmlResolver.</target>
         <note />
       </trans-unit>
+      <trans-unit id="PreferDictionaryTryGetValueCodeFixTitle">
+        <source>Use 'TryGetValue(TKey, out TValue)'</source>
+        <target state="new">Use 'TryGetValue(TKey, out TValue)'</target>
+        <note />
+      </trans-unit>
+      <trans-unit id="PreferDictionaryTryGetValueTitle">
+        <source>Prefer the Dictionary's TryGetValue method</source>
+        <target state="new">Prefer the Dictionary's TryGetValue method</target>
+        <note />
+      </trans-unit>
+      <trans-unit id="PreferDictionaryTryGetValueMessage">
+        <source>Prefer a 'TryGetValue' call over a Dictionary indexer access guarded by a 'ContainsKey' check.</source>
+        <target state="new">Prefer a 'TryGetValue' call over a Dictionary indexer access guarded by a 'ContainsKey' check.</target>
+        <note />
+      </trans-unit>
+      <trans-unit id="PreferDictionaryTryGetValueDescription">
+        <source>Prefer a 'TryGetValue' call over a Dictionary indexer access guarded by a 'ContainsKey' check.</source>
+        <target state="new">Prefer a 'TryGetValue' call over a Dictionary indexer access guarded by a 'ContainsKey' check.</target>
+        <note />
+      </trans-unit>
     </body>
   </file>
 </xliff>