CA1853: Address issues found in PR review #6767

[mpidash]

The implementation of CA1868 (Unnecessary call to 'Contains' for sets) was based on the analyzer CA1853 (Unnecessary call to 'Dictionary.ContainsKey(key)), since they are very similar. During the review of #6767, a couple of problems were found that should also be addressed for CA1853.

This PR merges the analyzer for guarded calls, leading to the following changes for CA1853:

• Fixes CA1853: Wrong code fix is suggested when arguments of ContainsKey and Remove differ  #6781
• Support for ternary operators
• Support for guarded calls in the else branch of a conditional
• Do not report a diagnostic when the condition is negated (same behavior as the set analyzer)
• Support for variable assignments
• Support for derived types (e.g. SortedDictionary<TKey, TValue> or ImmutableSortedDictionary<TKey, TValue>.Builder)
• Support for access through interface types (IDictionary<TKey, TValue>)
• Support for IImmutableDictionary<TKey, TValue> and its derived types
• Support for guarded calls that are implemented through extension methods (e.g. System.Collections.Generic.CollectionExtensions.Remove(IDictionary<TKey, TValue>, TKey, out TValue) for SortedDictionary<TKey, TValue> (as Remove(TKey, out TValue is part of the concrete type Dictionary<TKey, TValue>))

This should also make it easier to add other guarded call diagnostics in the future.

[codecov]

Codecov Report

Merging #6791 (b50e28d) into main (28fbf11) will increase coverage by 0.02%.
Report is 48 commits behind head on main.
The diff coverage is 98.62%.

❗ Current head b50e28d differs from pull request most recent head 3496725. Consider uploading reports for the commit 3496725 to get more accurate results

@@            Coverage Diff             @@
##             main    #6791      +/-   ##
==========================================
+ Coverage   96.39%   96.41%   +0.02%     
==========================================
  Files        1403     1399       -4     
  Lines      330954   332327    +1373     
  Branches    10889    10862      -27     
==========================================
+ Hits       319023   320429    +1406     
+ Misses       9196     9169      -27     
+ Partials     2735     2729       -6     

[mpidash]

@Youssef1313 @buyaa-n @sharwell What do you think about combining the analyzers for guarded calls? This would help to apply the feedback for the set analyzer to the dictionary analyzer.

[buyaa-n]

@mpidash thank you for fixing this, overall LGTM

[buyaa-n]

Wonder if you tried/considered to populate all symbols once and register one action to check all, might be more efficient.

[mpidash]

I was thinking about this in testing as I had a bug where some guarded calls were found twice (two separate actions found them) but did not follow through.
Should I prepare a new PR where everything is consolidated into one action or should we keep it like this?

[buyaa-n]

Should I prepare a new PR where everything is consolidated into one action

That would be great, thank you!

[mpidash]

I've tested loading all symbols once and then registering one action like this:

private void OnCompilationStart(CompilationStartAnalysisContext context)
{
    var guardedCallContextWithSymbols = s_guardedCallContexts
        .Select(c => GuardedCallSymbols.TryGetSymbols(context.Compilation, c, out var symbols)
            ? (Context: c, Symbols: symbols)
            : (Context: c, Symbols: null))
        .WhereAsArray(p => p.Symbols is not null);

    if (!guardedCallContextWithSymbols.IsEmpty)
    {
        context.RegisterOperationAction(OnConditional, OperationKind.Conditional);

        void OnConditional(OperationAnalysisContext context)
        {
            foreach (var (guardedCallContext, symbols) in guardedCallContextWithSymbols)
            {
                var conditional = (IConditionalOperation)context.Operation;

                if (!symbols!.HasApplicableConditionInvocation(conditional.Condition, out var conditionInvocation, out bool containsNegated) ||
                    !symbols.HasApplicableGuardedInvocation(conditional, containsNegated, out var guardedInvocation) ||
                    !AreInvocationsOnSameInstance(conditionInvocation, guardedInvocation) ||
                    !AreInvocationArgumentsEqual(conditionInvocation, guardedInvocation))
                {
                    continue;
                }

                using var locations = ArrayBuilder<Location>.GetInstance(2);
                locations.Add(conditional.Syntax.GetLocation());
                locations.Add(guardedInvocation.Syntax.Parent!.GetLocation());

                context.ReportDiagnostic(conditionInvocation.CreateDiagnostic(
                    guardedCallContext.Rule,
                    additionalLocations: locations.ToImmutable(),
                    properties: null,
                    guardedInvocation.TargetMethod.ToDisplayString(s_symbolDisplayFormat),
                    conditionInvocation.TargetMethod.ToDisplayString(s_symbolDisplayFormat)));
            }
        }
    }
}

I've tried to measure the performance difference with AnalyzerRunner. The variance between runs of the same code (old one and the updated above) was quite high on my PC (from 5s to 11s for both, tested against dotnet/roslyn with a Release build), which makes it kinda hard to say for sure that one is more performant than the other.

Do you know a better way to compare the performance of two analyzers?

[buyaa-n]

Do you know a better way to compare the performance of two analyzers?

I haven't really tried to measure a perf of analyzers, there is some suggestions in https://github.com/dotnet/roslyn-analyzers/blob/main/docs/Performance.md, you might have already checked that.

We also have some PerformanceTests in the repo, by my understanding that runs the tests on CI, saves the results somewhere and compares them with the next PR's perf build and fails if the diff was above the threshold, not sure if adding a test for the analyzer there and use the old version as a Base could be a better way. The Perf.cmd script in the root would run all performance tests (which probably would take several hours to finish), unfortunately it seems there is no way to run only one specific perf test

Ping @mavasani for better suggestions