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

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

@@ -0,0 +1,63 @@
+// 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 System.Threading.Tasks;
+using Microsoft.CodeAnalysis;
+using Microsoft.CodeAnalysis.CodeActions;
+using Microsoft.CodeAnalysis.CodeFixes;
+using Microsoft.CodeAnalysis.CSharp.Syntax;
+using Microsoft.CodeAnalysis.Editing;
+using Microsoft.NetCore.Analyzers.Runtime;
+using static Microsoft.CodeAnalysis.CSharp.SyntaxFactory;
+
+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(context.CancellationToken).ConfigureAwait(false);
+
+            var dictionaryAccess = root.FindNode(dictionaryAccessLocation.SourceSpan, getInnermostNodeForTie: true);
+            if (dictionaryAccess is not ElementAccessExpressionSyntax
+                || root.FindNode(context.Span) is not InvocationExpressionSyntax { Expression: MemberAccessExpressionSyntax containsKeyAccess } containsKeyInvocation)
+            {
+                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,
+                    DeclarationExpression(
+                        IdentifierName(Var),
+                        SingleVariableDesignation(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

@@ -12,6 +12,7 @@ CA1850 | Performance | Info | PreferHashDataOverComputeHashAnalyzer, [Documentat
 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)
+CA1854 | Performance | Info | PreferDictionaryTryGetValueAnalyzer, [Documentation](https://docs.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1854)
 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

@@ -1933,4 +1933,16 @@
   <data name="FeatureUnsupportedWhenRuntimeMarshallingDisabledMessageDelegateUsage" xml:space="preserve">
     <value>Delegates with managed types as parameters or the return type require runtime marshalling to be enabled in the assembly where the delegate is defined.</value>
   </data>
+  <data name="PreferDictionaryTryGetValueCodeFixTitle" xml:space="preserve">
+    <value>Use 'TryGetValue(TKey, out TValue)'</value>
+  </data>
+  <data name="PreferDictionaryTryGetValueTitle" xml:space="preserve">
+    <value>Prefer the 'IDictionary.TryGetValue(TKey, out TValue)' method</value>
+  </data>
+  <data name="PreferDictionaryTryGetValueMessage" xml:space="preserve">
+    <value>Prefer a 'TryGetValue' call over a Dictionary indexer access guarded by a 'ContainsKey' check to avoid double lookup</value>
+  </data>
+  <data name="PreferDictionaryTryGetValueDescription" xml:space="preserve">
+    <value>Prefer a 'TryGetValue' call over a Dictionary indexer access guarded by a 'ContainsKey' check. 'ContainsKey' and the indexer both would lookup the key under the hood, so using 'TryGetValue' removes the extra lookup.</value>
+  </data>
 </root>

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

@@ -0,0 +1,179 @@
+// Copyright (c) Microsoft.  All Rights Reserved.  Licensed under the MIT license.  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;
+
+using static Microsoft.NetCore.Analyzers.MicrosoftNetCoreAnalyzersResources;
+
+namespace Microsoft.NetCore.Analyzers.Runtime
+{
+    [DiagnosticAnalyzer(LanguageNames.CSharp, LanguageNames.VisualBasic)]
+    public sealed class PreferDictionaryTryGetValueAnalyzer : DiagnosticAnalyzer
+    {
+        public const string RuleId = "CA1854";
+
+        private const string IndexerName = "this[]";
+        private const string IndexerNameVb = "Item";
+        private const string ContainsKey = nameof(IDictionary<dynamic, dynamic>.ContainsKey);
+
+        private static readonly LocalizableString s_localizableTitle = CreateLocalizableResourceString(nameof(PreferDictionaryTryGetValueTitle));
+        private static readonly LocalizableString s_localizableTryGetValueMessage = CreateLocalizableResourceString(nameof(PreferDictionaryTryGetValueMessage));
+        private static readonly LocalizableString s_localizableTryGetValueDescription = CreateLocalizableResourceString(nameof(PreferDictionaryTryGetValueDescription));
+
+        internal static readonly DiagnosticDescriptor ContainsKeyRule = DiagnosticDescriptorHelper.Create(
+            RuleId,
+            s_localizableTitle,
+            s_localizableTryGetValueMessage,
+            DiagnosticCategory.Performance,
+            RuleLevel.IdeSuggestion,
+            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 static void OnCompilationStart(CompilationStartAnalysisContext compilationContext)
+        {
+            var compilation = compilationContext.Compilation;
+            if (!TryGetDictionaryTypeAndMembers(compilation, out var iDictionaryType, out var containsKeySymbol, out var indexerSymbol))
+            {
+                return;
+            }
+
+            compilationContext.RegisterOperationAction(context => OnOperationAction(context, iDictionaryType, containsKeySymbol, indexerSymbol), OperationKind.PropertyReference);
+        }
+
+        private static void OnOperationAction(OperationAnalysisContext context, INamedTypeSymbol dictionaryType, IMethodSymbol containsKeySymbol, IPropertySymbol indexerSymbol)
+        {
+            var propertyReference = (IPropertyReferenceOperation)context.Operation;
+
+            if (propertyReference.Parent is IAssignmentOperation
+                || !IsDictionaryAccess(propertyReference, dictionaryType, indexerSymbol)
+                || !TryGetParentConditionalOperation(propertyReference, out var conditionalOperation)
+                || !TryGetContainsKeyGuard(conditionalOperation, containsKeySymbol, 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 TryGetDictionaryTypeAndMembers(Compilation compilation,
+            [NotNullWhen(true)] out INamedTypeSymbol? iDictionaryType,
+            [NotNullWhen(true)] out IMethodSymbol? containsKeySymbol,
+            [NotNullWhen(true)] out IPropertySymbol? indexerSymbol)
+        {
+            containsKeySymbol = null;
+            indexerSymbol = null;
+            if (!compilation.TryGetOrCreateTypeByMetadataName(WellKnownTypeNames.SystemCollectionsGenericIDictionary2, out iDictionaryType))
+            {
+                return false;
+            }
+
+            containsKeySymbol = iDictionaryType.GetMembers().OfType<IMethodSymbol>().FirstOrDefault(m => m.Name == ContainsKey);
+            indexerSymbol = iDictionaryType.GetMembers().OfType<IPropertySymbol>().FirstOrDefault(m => m.Name == IndexerName || m.Language == LanguageNames.VisualBasic && m.Name == IndexerNameVb);
+
+            return containsKeySymbol is not null && indexerSymbol is not null;
+        }
+
+        private static bool TryGetContainsKeyGuard(IConditionalOperation conditionalOperation, IMethodSymbol containsKeySymbol, [NotNullWhen(true)] out IInvocationOperation? containsKeyInvocation)
+        {
+            containsKeyInvocation = FindContainsKeyInvocation(conditionalOperation.Condition, containsKeySymbol);
+
+            return containsKeyInvocation is not null;
+        }
+
+        private static IInvocationOperation? FindContainsKeyInvocation(IOperation baseOperation, IMethodSymbol containsKeyMethod)
+        {
+            return baseOperation switch
+            {
+                IInvocationOperation i when IsContainsKeyMethod(i.TargetMethod, containsKeyMethod) => i,
+                IBinaryOperation { OperatorKind: BinaryOperatorKind.ConditionalAnd or BinaryOperatorKind.ConditionalOr } b =>
+                    FindContainsKeyInvocation(b.LeftOperand, containsKeyMethod) ?? FindContainsKeyInvocation(b.RightOperand, containsKeyMethod),
+                _ => null
+            };
+        }
+
+        private static bool IsContainsKeyMethod(IMethodSymbol suspectedContainsKeyMethod, IMethodSymbol containsKeyMethod)
+        {
+            return suspectedContainsKeyMethod.OriginalDefinition.Equals(containsKeyMethod, SymbolEqualityComparer.Default)
+                   || DoesSignatureMatch(suspectedContainsKeyMethod, containsKeyMethod);
+        }
+
+        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, IPropertySymbol indexer)
+        {
+            return propertyReference.Property.IsIndexer
+                   && IsDictionaryType(propertyReference.Property.ContainingType, dictionaryType)
+                   && (propertyReference.Property.OriginalDefinition.Equals(indexer, SymbolEqualityComparer.Default)
+                       || DoesSignatureMatch(propertyReference.Property, indexer));
+        }
+
+        private static bool TryGetParentConditionalOperation(IOperation derivedOperation, [NotNullWhen(true)] out IConditionalOperation? conditionalOperation)
+        {
+            conditionalOperation = null;
+            do
+            {
+                if (derivedOperation.Parent is IConditionalOperation conditional)
+                {
+                    conditionalOperation = conditional;
+
+                    return true;
+                }
+
+                derivedOperation = derivedOperation.Parent;
+            } while (derivedOperation.Parent != null);
+
+            return false;
+        }
+
+        private static bool IsDictionaryType(INamedTypeSymbol suspectedDictionaryType, ISymbol iDictionaryType)
+        {
+            // Either the type is the IDictionary it is a type which (indirectly) implements it.
+            return suspectedDictionaryType.OriginalDefinition.Equals(iDictionaryType, SymbolEqualityComparer.Default)
+                   || suspectedDictionaryType.AllInterfaces.Any((@interface, dictionary) => @interface.OriginalDefinition.Equals(dictionary, SymbolEqualityComparer.Default), iDictionaryType);
+        }
+
+        // Unfortunately we can't do symbol comparison, since this won't work for i.e. a method in a ConcurrentDictionary comparing against the same method in the IDictionary.
+        private static bool DoesSignatureMatch(IMethodSymbol suspected, IMethodSymbol comparator)
+        {
+            return suspected.OriginalDefinition.ReturnType.Name == comparator.ReturnType.Name
+                   && suspected.Name == comparator.Name
+                   && suspected.Parameters.Length == comparator.Parameters.Length
+                   && suspected.Parameters.Zip(comparator.Parameters, (p1, p2) => p1.OriginalDefinition.Type.Name == p2.Type.Name).All(isParameterEqual => isParameterEqual);
+        }
+
+        private static bool DoesSignatureMatch(IPropertySymbol suspected, IPropertySymbol comparator)
+        {
+            return suspected.OriginalDefinition.Type.Name == comparator.Type.Name
+                   && suspected.Name == comparator.Name
+                   && suspected.Parameters.Length == comparator.Parameters.Length
+                   && suspected.Parameters.Zip(comparator.Parameters, (p1, p2) => p1.OriginalDefinition.Type.Name == p2.Type.Name).All(isParameterEqual => isParameterEqual);
+        }
+    }
+}

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

@@ -0,0 +1,19 @@
+// 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 Microsoft.CodeAnalysis.CodeFixes;
+
+namespace Microsoft.NetCore.Analyzers.Runtime
+{
+    public abstract class PreferDictionaryTryGetValueFixer : CodeFixProvider
+    {
+        protected const string Var = "var";
+        protected const string Value = "value";
+        protected const string TryGetValue = nameof(TryGetValue);
+        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

@@ -3002,6 +3002,26 @@
         <target state="translated">{3}{0} používá typ preview {1} a vyžaduje vyjádření výslovného souhlasu s funkcemi preview. Další informace najdete v {2}.</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 'IDictionary.TryGetValue(TKey, out TValue)' method</source>
+        <target state="new">Prefer the 'IDictionary.TryGetValue(TKey, out TValue)' method</target>
+        <note />
+      </trans-unit>
+      <trans-unit id="PreferDictionaryTryGetValueMessage">
+        <source>Prefer a 'TryGetValue' call over a Dictionary indexer access guarded by a 'ContainsKey' check to avoid double lookup</source>
+        <target state="new">Prefer a 'TryGetValue' call over a Dictionary indexer access guarded by a 'ContainsKey' check to avoid double lookup</target>
+        <note />
+      </trans-unit>
+      <trans-unit id="PreferDictionaryTryGetValueDescription">
+        <source>Prefer a 'TryGetValue' call over a Dictionary indexer access guarded by a 'ContainsKey' check. 'ContainsKey' and the indexer both would lookup the key under the hood, so using 'TryGetValue' removes the extra lookup.</source>
+        <target state="new">Prefer a 'TryGetValue' call over a Dictionary indexer access guarded by a 'ContainsKey' check. 'ContainsKey' and the indexer both would lookup the key under the hood, so using 'TryGetValue' removes the extra lookup.</target>
+        <note />
+      </trans-unit>
     </body>
   </file>
 </xliff>

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

@@ -3002,6 +3002,26 @@
         <target state="translated">{3} „{0}“ verwendet den Vorschautyp „{1}“, daher müssen Vorschaufeatures abonniert werden. Weitere Informationen finden Sie unter {2}.</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 'IDictionary.TryGetValue(TKey, out TValue)' method</source>
+        <target state="new">Prefer the 'IDictionary.TryGetValue(TKey, out TValue)' method</target>
+        <note />
+      </trans-unit>
+      <trans-unit id="PreferDictionaryTryGetValueMessage">
+        <source>Prefer a 'TryGetValue' call over a Dictionary indexer access guarded by a 'ContainsKey' check to avoid double lookup</source>
+        <target state="new">Prefer a 'TryGetValue' call over a Dictionary indexer access guarded by a 'ContainsKey' check to avoid double lookup</target>
+        <note />
+      </trans-unit>
+      <trans-unit id="PreferDictionaryTryGetValueDescription">
+        <source>Prefer a 'TryGetValue' call over a Dictionary indexer access guarded by a 'ContainsKey' check. 'ContainsKey' and the indexer both would lookup the key under the hood, so using 'TryGetValue' removes the extra lookup.</source>
+        <target state="new">Prefer a 'TryGetValue' call over a Dictionary indexer access guarded by a 'ContainsKey' check. 'ContainsKey' and the indexer both would lookup the key under the hood, so using 'TryGetValue' removes the extra lookup.</target>
+        <note />
+      </trans-unit>
     </body>
   </file>
 </xliff>