Fix false positive in the UseConcreteType analyzer. (#6407)

The code was treating assignment to a parameter within a method as though
it was a argument to the containing method call, which was totally
incorrect. In this analyzer, we don't care about assignments to
parameters, as that doesn't influence what the analyzer is concerned
with.

Co-authored-by: Martin Taillefer <mataille@microsoft.com>

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Performance/UseConcreteTypeAnalyzer.Collector.cs

@@ -445,13 +445,6 @@ private void RecordAssignment(IOperation op, ITypeSymbol valueType)
                             break;
                         }
 
-                    case OperationKind.ParameterReference:
-                        {
-                            var paramRef = (IParameterReferenceOperation)op;
-                            RecordAssignment(paramRef.Parameter, valueType);
-                            break;
-                        }
-
                     case OperationKind.DeclarationExpression:
                         {
                             var declEx = (IDeclarationExpressionOperation)op;

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

@@ -115,6 +115,7 @@ public override void Initialize(AnalysisContext context)
                 {
                     var coll = Collector.GetInstance(voidType);
 
+                    // we accumulate a bunch of info in the collector object
                     context.RegisterOperationAction(context => coll.HandleInvocation((IInvocationOperation)context.Operation), OperationKind.Invocation);
                     context.RegisterOperationAction(context => coll.HandleSimpleAssignment((ISimpleAssignmentOperation)context.Operation), OperationKind.SimpleAssignment);
                     context.RegisterOperationAction(context => coll.HandleCoalesceAssignment((ICoalesceAssignmentOperation)context.Operation), OperationKind.CoalesceAssignment);
@@ -126,6 +127,7 @@ public override void Initialize(AnalysisContext context)
 
                     context.RegisterSymbolEndAction(context =>
                     {
+                        // based on what we've collected, spit out relevant diagnostics
                         Report(context, coll);
                         Collector.ReturnInstance(coll, context.CancellationToken);
                     });
@@ -138,6 +140,7 @@ public override void Initialize(AnalysisContext context)
         /// </summary>
         private static void Report(SymbolAnalysisContext context, Collector coll)
         {
+            // for all eligible private fields that are used as the receiver for a virtual call
             foreach (var field in coll.VirtualDispatchFields.Keys)
             {
                 if (coll.FieldAssignments.TryGetValue(field, out var assignments))
@@ -146,6 +149,7 @@ private static void Report(SymbolAnalysisContext context, Collector coll)
                 }
             }
 
+            // for all eligible local variables that are used as the receiver for a virtual call
             foreach (var local in coll.VirtualDispatchLocals.Keys)
             {
                 if (coll.LocalAssignments.TryGetValue(local, out var assignments))
@@ -154,6 +158,7 @@ private static void Report(SymbolAnalysisContext context, Collector coll)
                 }
             }
 
+            // for all eligible parameters that are used as the receiver for a virtual call
             foreach (var parameter in coll.VirtualDispatchParameters.Keys)
             {
                 if (coll.ParameterAssignments.TryGetValue(parameter, out var assignments))
@@ -168,11 +173,13 @@ private static void Report(SymbolAnalysisContext context, Collector coll)
                 }
             }
 
+            // for eligible all return types of private methods
             foreach (var pair in coll.MethodReturns)
             {
                 var method = pair.Key;
                 var returns = pair.Value;
 
+                // only report the method if it never assigned to a delegate
                 if (CanUpgrade(method))
                 {
                     Report(method, method.ReturnType, returns, UseConcreteTypeForMethodReturn);
@@ -181,10 +188,15 @@ private static void Report(SymbolAnalysisContext context, Collector coll)
 
             void Report(ISymbol sym, ITypeSymbol fromType, PooledConcurrentSet<ITypeSymbol> assignments, DiagnosticDescriptor desc)
             {
+                // a set of the values assigned to the given symbol
                 using var types = PooledHashSet<ITypeSymbol>.GetInstance(assignments, SymbolEqualityComparer.Default);
 
+                // 'void' is the magic value we use to represent null assignment
                 var assignedNull = types.Remove(coll.Void!);
 
+                // We currently only handle the case where there is only a single consistent type of value assigned to the
+                // symbol. If there are multiple different types, we could try to find the common base for these, but it doesn't
+                // seem worth the complication.
                 if (types.Count == 1)
                 {
                     var toType = types.Single();
@@ -195,6 +207,7 @@ void Report(ISymbol sym, ITypeSymbol fromType, PooledConcurrentSet<ITypeSymbol>
 
                     if (!toType.DerivesFrom(fromType.OriginalDefinition))
                     {
+                        // can readily replace fromType by toType
                         return;
                     }
 

src/NetAnalyzers/UnitTests/Microsoft.NetCore.Analyzers/Performance/UseConcreteTypeTests.cs

@@ -45,6 +45,48 @@ public class Derived2 : BaseType
             await TestCSAsync(Source);
         }
 
+        [Fact]
+        public static async Task ShouldNotTrigger2()
+        {
+            const string Source = @"
+                #nullable enable
+
+                using System.Collections.Generic;
+
+                namespace System
+                {
+                    public static partial class MemoryExtensions
+                    {
+                        public static unsafe bool SequenceEqual<T>(this ReadOnlySpan<T> span, ReadOnlySpan<T> other, IEqualityComparer<T>? comparer = null)
+                        {
+                            comparer = EqualityComparer<T>.Default;
+                            return comparer!.Equals(span[0], other[0]);
+                        }
+
+                        public static int CommonPrefixLength<T>(this Span<T> span, ReadOnlySpan<T> other, IEqualityComparer<T>? comparer)
+                        {
+                            return comparer!.Equals(span[0], other[0]) ? 0 : 1;
+                        }
+
+                        public static bool Foo()
+                        {
+                            Span<byte> s1 = stackalloc byte[2];
+                            Span<byte> s2 = stackalloc byte[2];
+                            return SequenceEqual(s1, s2, EqualityComparer<byte>.Default);
+                        }
+
+                        public static int Bar()
+                        {
+                            Span<byte> s1 = stackalloc byte[2];
+                            Span<byte> s2 = stackalloc byte[2];
+                            return CommonPrefixLength(s1, s2, EqualityComparer<byte>.Default);
+                        }
+                    }
+                }";
+
+            await TestCSAsync(Source);
+        }
+
         [Fact]
         public static async Task ShouldTrigger1()
         {