Always use .OriginalDefinition uniformly in the unread-members analyzer

Author: CyrusNajmabadi

src/Analyzers/Core/Analyzers/RemoveUnusedMembers/AbstractRemoveUnusedMembersDiagnosticAnalyzer.cs

@@ -90,14 +90,32 @@ protected virtual void HandleNamedTypeSymbolStart(SymbolStartAnalysisContext con
     {
     }
 
+    internal sealed class OriginalDefinitionSymbolEqualityComparer : IEqualityComparer<ISymbol>
+    {
+        public static readonly OriginalDefinitionSymbolEqualityComparer Instance = new();
+
+        private OriginalDefinitionSymbolEqualityComparer()
+        {
+        }
+
+        bool IEqualityComparer<ISymbol>.Equals(ISymbol? x, ISymbol? y)
+            => Equals(x?.OriginalDefinition, y?.OriginalDefinition);
+
+        int IEqualityComparer<ISymbol>.GetHashCode(ISymbol obj)
+            => obj is null ? 0 : obj.OriginalDefinition.GetHashCode();
+    }
+
     private sealed class CompilationAnalyzer
     {
         private readonly object _gate = new();
 
+        private static readonly ObjectPool<HashSet<ISymbol>> s_originalDefinitionSymbolHashSetPool = new(() => new(OriginalDefinitionSymbolEqualityComparer.Instance));
+
         /// <summary>
         /// State map for candidate member symbols, with the value indicating how each symbol is used in executable code.
         /// </summary>
-        private readonly Dictionary<ISymbol, ValueUsageInfo> _symbolValueUsageStateMap = [];
+        private readonly Dictionary<ISymbol, ValueUsageInfo> _symbolValueUsageStateMap_doNotAccessDirectly = new(OriginalDefinitionSymbolEqualityComparer.Instance);
+
         /// <summary>
         /// List of properties that have a 'get' accessor usage, while the value itself is not used, e.g.:
         /// <code>
@@ -109,7 +127,7 @@ private sealed class CompilationAnalyzer
         /// </code>
         /// Here, 'get' accessor is used in an increment operation, but the result of the increment operation isn't used and 'P' itself is not used anywhere else, so it can be safely removed
         /// </summary>
-        private readonly HashSet<IPropertySymbol> _propertiesWithShadowGetAccessorUsages = [];
+        private readonly HashSet<IPropertySymbol> _propertiesWithShadowGetAccessorUsages = new(OriginalDefinitionSymbolEqualityComparer.Instance);
         private readonly INamedTypeSymbol? _taskType;
         private readonly INamedTypeSymbol? _genericTaskType;
         private readonly INamedTypeSymbol? _debuggerDisplayAttributeType;
@@ -258,7 +276,7 @@ bool ShouldAnalyze(SymbolStartAnalysisContext context, INamedTypeSymbol namedTyp
                 // Check if we have at least one candidate symbol in analysis scope.
                 foreach (var member in namedType.GetMembers())
                 {
-                    if (IsCandidateSymbol(member.OriginalDefinition)
+                    if (IsCandidateSymbol(member)
                         && context.ShouldAnalyzeLocation(GetDiagnosticLocation(member)))
                     {
                         return true;
@@ -275,19 +293,55 @@ bool ShouldAnalyze(SymbolStartAnalysisContext context, INamedTypeSymbol namedTyp
 
         private void AnalyzeSymbolDeclaration(SymbolAnalysisContext symbolContext)
         {
-            var symbol = symbolContext.Symbol.OriginalDefinition;
+            var symbol = symbolContext.Symbol;
             if (IsCandidateSymbol(symbol)
                 && symbolContext.ShouldAnalyzeLocation(GetDiagnosticLocation(symbol)))
             {
-                lock (_gate)
+                // Initialize unused state to 'ValueUsageInfo.None' to indicate that
+                // no read/write references have been encountered yet for this symbol.
+                // Note that we might receive a symbol reference (AnalyzeMemberOperation) callback before
+                // this symbol declaration callback, so even though we cannot receive duplicate callbacks for a symbol,
+                // an entry might already be present of the declared symbol here.
+                AddSymbolUsage(symbol, ValueUsageInfo.None);
+            }
+        }
+
+        private void AddSymbolUsage(ISymbol? symbol, ValueUsageInfo info)
+        {
+            if (symbol is null)
+                return;
+
+            lock (_gate)
+            {
+                _symbolValueUsageStateMap_doNotAccessDirectly.TryAdd(symbol, info);
+            }
+        }
+
+        private void UpdateSymbolUsage(ISymbol? symbol, ValueUsageInfo info)
+        {
+            if (symbol is null)
+                return;
+
+            lock (_gate)
+            {
+                if (_symbolValueUsageStateMap_doNotAccessDirectly.TryGetValue(symbol, out var currentUsageInfo))
+                    info = currentUsageInfo | info;
+
+                _symbolValueUsageStateMap_doNotAccessDirectly[symbol] = info;
+            }
+        }
+
+        private bool TryGetAndRemoveSymbolUsage(ISymbol memberSymbol, out ValueUsageInfo valueUsageInfo)
+        {
+            lock (_gate)
+            {
+                if (_symbolValueUsageStateMap_doNotAccessDirectly.TryGetValue(memberSymbol, out valueUsageInfo))
                 {
-                    // Initialize unused state to 'ValueUsageInfo.None' to indicate that
-                    // no read/write references have been encountered yet for this symbol.
-                    // Note that we might receive a symbol reference (AnalyzeMemberOperation) callback before
-                    // this symbol declaration callback, so even though we cannot receive duplicate callbacks for a symbol,
-                    // an entry might already be present of the declared symbol here.
-                    _symbolValueUsageStateMap.TryAdd(symbol, ValueUsageInfo.None);
+                    _symbolValueUsageStateMap_doNotAccessDirectly.Remove(memberSymbol);
+                    return true;
                 }
+
+                return false;
             }
         }
 
@@ -320,40 +374,15 @@ private void AnalyzeFieldInitializer(OperationAnalysisContext operationContext)
         private void OnSymbolUsage(ISymbol? memberSymbol, ValueUsageInfo usageInfo)
         {
             if (!IsCandidateSymbol(memberSymbol))
-            {
                 return;
-            }
-
-            lock (_gate)
-            {
-                // Update the usage info for the memberSymbol
-                if (_symbolValueUsageStateMap.TryGetValue(memberSymbol, out var currentUsageInfo))
-                {
-                    usageInfo = currentUsageInfo | usageInfo;
-                }
 
-                _symbolValueUsageStateMap[memberSymbol] = usageInfo;
-            }
-        }
-
-        private bool TryRemove(ISymbol memberSymbol, out ValueUsageInfo valueUsageInfo)
-        {
-            lock (_gate)
-            {
-                if (_symbolValueUsageStateMap.TryGetValue(memberSymbol, out valueUsageInfo))
-                {
-                    _symbolValueUsageStateMap.Remove(memberSymbol);
-                    return true;
-                }
-
-                return false;
-            }
+            UpdateSymbolUsage(memberSymbol, usageInfo);
         }
 
         private void AnalyzeMemberReferenceOperation(OperationAnalysisContext operationContext)
         {
             var memberReference = (IMemberReferenceOperation)operationContext.Operation;
-            var memberSymbol = memberReference.Member.OriginalDefinition;
+            var memberSymbol = memberReference.Member;
             if (IsCandidateSymbol(memberSymbol))
             {
                 // Get the value usage info.
@@ -421,16 +450,11 @@ private void AnalyzeLoopOperation(OperationAnalysisContext operationContext)
 
         private void AnalyzeInvocationOperation(OperationAnalysisContext operationContext)
         {
-            var targetMethod = ((IInvocationOperation)operationContext.Operation).TargetMethod.OriginalDefinition;
+            var targetMethod = ((IInvocationOperation)operationContext.Operation).TargetMethod;
 
             // A method invocation is considered as a read reference to the symbol
             // to ensure that we consider the method as "used".
             OnSymbolUsage(targetMethod, ValueUsageInfo.Read);
-
-            // If the invoked method is a reduced extension method, also mark the original
-            // method from which it was reduced as "used".
-            if (targetMethod.ReducedFrom != null)
-                OnSymbolUsage(targetMethod.ReducedFrom, ValueUsageInfo.Read);
         }
 
         private void AnalyzeNameOfOperation(OperationAnalysisContext operationContext)
@@ -444,7 +468,7 @@ private void AnalyzeNameOfOperation(OperationAnalysisContext operationContext)
 
             if (nameofArgument is IMemberReferenceOperation memberReference)
             {
-                OnSymbolUsage(memberReference.Member.OriginalDefinition, ValueUsageInfo.ReadWrite);
+                OnSymbolUsage(memberReference.Member, ValueUsageInfo.ReadWrite);
                 return;
             }
 
@@ -453,12 +477,12 @@ private void AnalyzeNameOfOperation(OperationAnalysisContext operationContext)
             // a bound method group/property group.
             var symbolInfo = nameofArgument.SemanticModel!.GetSymbolInfo(nameofArgument.Syntax, operationContext.CancellationToken);
             foreach (var symbol in symbolInfo.GetAllSymbols())
-                OnSymbolUsage(symbol.OriginalDefinition, ValueUsageInfo.ReadWrite);
+                OnSymbolUsage(symbol, ValueUsageInfo.ReadWrite);
         }
 
         private void AnalyzeObjectCreationOperation(OperationAnalysisContext operationContext)
         {
-            var constructor = ((IObjectCreationOperation)operationContext.Operation).Constructor?.OriginalDefinition;
+            var constructor = ((IObjectCreationOperation)operationContext.Operation).Constructor;
 
             // An object creation is considered as a read reference to the constructor
             // to ensure that we consider the constructor as "used".
@@ -481,7 +505,7 @@ private void OnSymbolEnd(SymbolAnalysisContext symbolEndContext, bool hasUnsuppo
             // Report diagnostics for unused candidate members.
             var first = true;
 
-            using var _1 = PooledHashSet<ISymbol>.GetInstance(out var symbolsReferencedInDocComments);
+            using var _1 = s_originalDefinitionSymbolHashSetPool.GetPooledObject(out var symbolsReferencedInDocComments);
             using var _2 = ArrayBuilder<string>.GetInstance(out var debuggerDisplayAttributeArguments);
 
             var entryPoint = symbolEndContext.Compilation.GetEntryPoint(cancellationToken);
@@ -499,7 +523,7 @@ private void OnSymbolEnd(SymbolAnalysisContext symbolEndContext, bool hasUnsuppo
 
                 // Check if the underlying member is neither read nor a readable reference to the member is taken.
                 // If so, we flag the member as either unused (never written) or unread (written but not read).
-                if (TryRemove(member, out var valueUsageInfo) && !valueUsageInfo.IsReadFrom())
+                if (TryGetAndRemoveSymbolUsage(member, out var valueUsageInfo) && !valueUsageInfo.IsReadFrom())
                 {
                     Debug.Assert(IsCandidateSymbol(member));
                     Debug.Assert(!member.IsImplicitlyDeclared);
@@ -649,7 +673,7 @@ private void AddCandidateSymbolsReferencedInDocComments(
                     foreach (var node in docComment.DescendantNodes().OfType<TIdentifierNameSyntax>())
                     {
                         lazyModel ??= compilation.GetSemanticModel(syntaxTree);
-                        var symbol = lazyModel.GetSymbolInfo(node, cancellationToken).Symbol?.OriginalDefinition;
+                        var symbol = lazyModel.GetSymbolInfo(node, cancellationToken).Symbol;
 
                         if (IsCandidateSymbol(symbol))
                             builder.Add(symbol);
@@ -757,8 +781,6 @@ private bool IsCandidateSymbol([NotNullWhen(true)] ISymbol? memberSymbol)
             if (memberSymbol is null)
                 return false;
 
-            Debug.Assert(memberSymbol == memberSymbol.OriginalDefinition);
-
             if (memberSymbol.DeclaredAccessibility == Accessibility.Private &&
                 !memberSymbol.IsImplicitlyDeclared)
             {