Use WellKnownTypeProvider + KnownSymbols pattern to simplify analyzers (

#245) Change adds two related types and refactors the analyzers such that the string-based `WellKnownMockTypes` can be deleted. These changes unlock additional refactoring, but I stopped to get feedback. The main changes are: ## WellKnownTypeProvider WellKnownTypeProvider comes from [roslyn-analyzers](https://github.com/[dotnet/roslyn-analyzers](https://github.com/dotnet/roslyn-analyzers)). It caches `ISymbol` lookups for a given `Compilation` to amortize costs across all the analyzers running for a given compilation. It also enforces some best practices when looking for "well known" symbols in the cases of name collisions. ## KnownSymbols KnownSymbols comes from [typeshape-csharp](https://github.com/eiriktsarpalis/typeshape-csharp) to simplify looking up types in the WellKnownTypeProvider. In this change only types are cached, the member lookups (methods, fields, etc.) are not. However, Roslyn internally caches, so it isn't clear that's needed. Using the KnownSymbols pattern cleans up the code quite a bit. It does push us away from SyntaxNodes and towards Symbols, however I don't think that's a bad thing because we also want to migrate to the IOperation-based analysis, which already has the corresponding ISymbol available at the start of analysis. The existing benchmarks show the new and old code (v0.1.0) to be within the margin of error. --------- Co-authored-by: Richard Murillo <[email protected]>
rjmurillo · Oct 29, 2024 · d25b4de · d25b4de
1 parent aabe3d1
commit d25b4de
Show file tree

Hide file tree

Showing 32 changed files with 864 additions and 338 deletions.
diff --git a/build/targets/codeanalysis/CodeAnalysis.props b/build/targets/codeanalysis/CodeAnalysis.props
@@ -26,6 +26,10 @@
       <PrivateAssets>all</PrivateAssets>
       <IncludeAssets>runtime; build; native; contentfiles; analyzers</IncludeAssets>
     </PackageReference>
+    <PackageReference Include="Microsoft.CodeAnalysis.PerformanceSensitiveAnalyzers">
+      <PrivateAssets>all</PrivateAssets>
+      <IncludeAssets>runtime; build; native; contentfiles; analyzers</IncludeAssets>
+    </PackageReference>
     <PackageReference Include="StyleCop.Analyzers">
       <PrivateAssets>all</PrivateAssets>
       <IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>

diff --git a/build/targets/codeanalysis/Packages.props b/build/targets/codeanalysis/Packages.props
@@ -3,6 +3,7 @@
     <PackageVersion Include="Meziantou.Analyzer" Version="2.0.176" />
     <PackageVersion Include="Microsoft.CodeAnalysis.Analyzers" Version="3.11.0" />
     <PackageVersion Include="Microsoft.CodeAnalysis.BannedApiAnalyzers" Version="3.11.0-beta1.24454.1" />
+    <PackageVersion Include="Microsoft.CodeAnalysis.PerformanceSensitiveAnalyzers" Version="3.11.0-beta1.24454.1" />
     <PackageVersion Include="Roslynator.Analyzers" Version="4.12.9" />
     <PackageVersion Include="StyleCop.Analyzers" Version="1.2.0-beta.556" />
     <PackageVersion Include="SonarAnalyzer.CSharp" Version="9.32.0.97167" />

diff --git a/build/targets/compiler/Compiler.props b/build/targets/compiler/Compiler.props
@@ -6,7 +6,7 @@
   </PropertyGroup>
 
   <ItemGroup>
-    <PackageReference Include="PolySharp">
+    <PackageReference Include="Polyfill">
       <PrivateAssets>all</PrivateAssets>
       <IncludeAssets>runtime; build; native; contentfiles; analyzers</IncludeAssets>
     </PackageReference>

diff --git a/build/targets/compiler/Packages.props b/build/targets/compiler/Packages.props
@@ -1,5 +1,5 @@
 <Project>
   <ItemGroup>
-    <PackageVersion Include="PolySharp" Version="1.14.1" />
-</ItemGroup>
+    <PackageVersion Include="Polyfill" Version="7.1.2" />
+  </ItemGroup>
 </Project>
diff --git a/src/Analyzers/AsShouldBeUsedOnlyForInterfaceAnalyzer.cs b/src/Analyzers/AsShouldBeUsedOnlyForInterfaceAnalyzer.cs
@@ -34,21 +34,18 @@ public override void Initialize(AnalysisContext context)
 
     private static void RegisterCompilationStartAction(CompilationStartAnalysisContext context)
     {
+        MoqKnownSymbols knownSymbols = new(context.Compilation);
+
         // Ensure Moq is referenced in the compilation
-        ImmutableArray<INamedTypeSymbol> mockTypes = context.Compilation.GetMoqMock();
-        if (mockTypes.IsEmpty)
+        if (!knownSymbols.IsMockReferenced())
         {
             return;
         }
 
         // Look for the Mock.As() method and provide it to Analyze to avoid looking it up multiple times.
-#pragma warning disable ECS0900 // Minimize boxing and unboxing
-        ImmutableArray<IMethodSymbol> asMethods = mockTypes
-            .SelectMany(mockType => mockType.GetMembers(WellKnownMoqNames.AsMethodName))
-            .OfType<IMethodSymbol>()
-            .Where(method => method.IsGenericMethod)
-            .ToImmutableArray();
-#pragma warning restore ECS0900 // Minimize boxing and unboxing
+        ImmutableArray<IMethodSymbol> asMethods = ImmutableArray.CreateRange([
+            ..knownSymbols.MockAs,
+            ..knownSymbols.Mock1As]);
 
         if (asMethods.IsEmpty)
         {

diff --git a/src/Analyzers/CallbackSignatureShouldMatchMockedMethodAnalyzer.cs b/src/Analyzers/CallbackSignatureShouldMatchMockedMethodAnalyzer.cs
@@ -31,6 +31,8 @@ public override void Initialize(AnalysisContext context)
 
     private static void Analyze(SyntaxNodeAnalysisContext context)
     {
+        MoqKnownSymbols knownSymbols = new(context.SemanticModel.Compilation);
+
         InvocationExpressionSyntax callbackOrReturnsInvocation = (InvocationExpressionSyntax)context.Node;
 
         SeparatedSyntaxList<ArgumentSyntax> callbackOrReturnsMethodArguments = callbackOrReturnsInvocation.ArgumentList.Arguments;
@@ -49,7 +51,7 @@ private static void Analyze(SyntaxNodeAnalysisContext context)
         SeparatedSyntaxList<ParameterSyntax> lambdaParameters = callbackLambda.ParameterList.Parameters;
         if (lambdaParameters.Count == 0) return;
 
-        InvocationExpressionSyntax? setupInvocation = context.SemanticModel.FindSetupMethodFromCallbackInvocation(callbackOrReturnsInvocation, context.CancellationToken);
+        InvocationExpressionSyntax? setupInvocation = context.SemanticModel.FindSetupMethodFromCallbackInvocation(knownSymbols, callbackOrReturnsInvocation, context.CancellationToken);
         InvocationExpressionSyntax? mockedMethodInvocation = setupInvocation.FindMockedMethodInvocationFromSetupMethod();
         if (mockedMethodInvocation == null) return;
 

diff --git a/src/Analyzers/ConstructorArgumentsShouldMatchAnalyzer.cs b/src/Analyzers/ConstructorArgumentsShouldMatchAnalyzer.cs
@@ -68,65 +68,42 @@ public override void Initialize(AnalysisContext context)
         return null;
     }
 
-    private static bool IsExpressionMockBehavior(SyntaxNodeAnalysisContext context, ExpressionSyntax? expression)
+    private static bool IsExpressionMockBehavior(SyntaxNodeAnalysisContext context, MoqKnownSymbols knownSymbols, ExpressionSyntax? expression)
     {
-        if (expression == null)
+        if (expression is null)
         {
             return false;
         }
 
-        if (expression is MemberAccessExpressionSyntax memberAccessExpressionSyntax)
+        SymbolInfo symbolInfo = context.SemanticModel.GetSymbolInfo(expression, context.CancellationToken);
+
+        if (symbolInfo.Symbol is null)
         {
-            if (memberAccessExpressionSyntax.Expression is IdentifierNameSyntax identifierNameSyntax
-                && string.Equals(identifierNameSyntax.Identifier.ValueText, WellKnownMoqNames.MockBehaviorTypeName, StringComparison.Ordinal))
-            {
-                return true;
-            }
+            return false;
         }
-        else if (expression is IdentifierNameSyntax identifierNameSyntax)
-        {
-            SymbolInfo symbolInfo = context.SemanticModel.GetSymbolInfo(identifierNameSyntax, context.CancellationToken);
-
-            if (symbolInfo.Symbol == null)
-            {
-                return false;
-            }
 
-            ITypeSymbol? typeSymbol = null;
-            if (symbolInfo.Symbol is IParameterSymbol parameterSymbol)
-            {
-                typeSymbol = parameterSymbol.Type;
-            }
-            else if (symbolInfo.Symbol is ILocalSymbol localSymbol)
-            {
-                typeSymbol = localSymbol.Type;
-            }
-            else if (symbolInfo.Symbol is IFieldSymbol fieldSymbol)
-            {
-                typeSymbol = fieldSymbol.Type;
-            }
-
-            if (typeSymbol != null
-                && string.Equals(typeSymbol.Name, WellKnownMoqNames.MockBehaviorTypeName, StringComparison.Ordinal))
-            {
-                return true;
-            }
+        ISymbol targetSymbol = symbolInfo.Symbol;
+        if (symbolInfo.Symbol is IParameterSymbol parameterSymbol)
+        {
+            targetSymbol = parameterSymbol.Type;
         }
-
-        // Crude fallback to check if the expression is a Moq.MockBehavior enum
-        if (expression.ToString().StartsWith(WellKnownMoqNames.FullyQualifiedMoqBehaviorTypeName, StringComparison.Ordinal))
+        else if (symbolInfo.Symbol is ILocalSymbol localSymbol)
         {
-            return true;
+            targetSymbol = localSymbol.Type;
+        }
+        else if (symbolInfo.Symbol is IFieldSymbol fieldSymbol)
+        {
+            targetSymbol = fieldSymbol.Type;
         }
 
-        return false;
+        return targetSymbol.IsInstanceOf(knownSymbols.MockBehavior);
     }
 
-    private static bool IsFirstArgumentMockBehavior(SyntaxNodeAnalysisContext context, ArgumentListSyntax? argumentList)
+    private static bool IsFirstArgumentMockBehavior(SyntaxNodeAnalysisContext context, MoqKnownSymbols knownSymbols, ArgumentListSyntax? argumentList)
     {
         ExpressionSyntax? expression = argumentList?.Arguments[0].Expression;
 
-        return IsExpressionMockBehavior(context, expression);
+        return IsExpressionMockBehavior(context, knownSymbols, expression);
     }
 
     private static void VerifyDelegateMockAttempt(
@@ -174,106 +151,87 @@ private static void AnalyzeCompilation(CompilationStartAnalysisContext context)
             return;
         }
 
+        MoqKnownSymbols knownSymbols = new(context.Compilation);
+
         // We're interested in the few ways to create mocks:
         //  - new Mock<T>()
         //  - Mock.Of<T>()
         //  - MockRepository.Create<T>()
         //
         // Ensure Moq is referenced in the compilation
-        ImmutableArray<INamedTypeSymbol> mockTypes = context.Compilation.GetMoqMock();
-        if (mockTypes.IsEmpty)
+        if (!knownSymbols.IsMockReferenced())
         {
             return;
         }
 
         // These are for classes
-        context.RegisterSyntaxNodeAction(AnalyzeNewObject, SyntaxKind.ObjectCreationExpression);
-        context.RegisterSyntaxNodeAction(AnalyzeInstanceCall, SyntaxKind.InvocationExpression);
+        context.RegisterSyntaxNodeAction(context => AnalyzeNewObject(context, knownSymbols), SyntaxKind.ObjectCreationExpression);
+        context.RegisterSyntaxNodeAction(context => AnalyzeInstanceCall(context, knownSymbols), SyntaxKind.InvocationExpression);
     }
 
-    private static void AnalyzeInstanceCall(SyntaxNodeAnalysisContext context)
+    private static void AnalyzeInstanceCall(SyntaxNodeAnalysisContext context, MoqKnownSymbols knownSymbols)
     {
         InvocationExpressionSyntax invocationExpressionSyntax = (InvocationExpressionSyntax)context.Node;
 
-        if (invocationExpressionSyntax.Expression is not MemberAccessExpressionSyntax memberAccessExpressionSyntax)
-        {
-            return;
-        }
-
-        if (memberAccessExpressionSyntax.Name is not GenericNameSyntax genericNameSyntax)
-        {
-            return;
-        }
-
-        if (genericNameSyntax.Identifier.Value is not string genericNameSyntaxIdentifierValue)
-        {
-            return;
-        }
-
-        if (string.Equals(genericNameSyntaxIdentifierValue, WellKnownMoqNames.CreateMethodName, StringComparison.Ordinal))
-        {
-            AnalyzeInvocation(context, invocationExpressionSyntax, WellKnownMoqNames.MockFactoryTypeName, true, true);
-        }
-        else if (string.Equals(genericNameSyntaxIdentifierValue, WellKnownMoqNames.OfMethodName, StringComparison.Ordinal))
-        {
-            AnalyzeInvocation(context, invocationExpressionSyntax, WellKnownMoqNames.MockTypeName, false, true);
-        }
+        AnalyzeInvocation(context, knownSymbols, invocationExpressionSyntax);
     }
 
     private static void AnalyzeInvocation(
         SyntaxNodeAnalysisContext context,
-        InvocationExpressionSyntax invocationExpressionSyntax,
-        string expectedClassName,
-        bool hasReturnedMock,
-        bool hasMockBehavior)
+        MoqKnownSymbols knownSymbols,
+        InvocationExpressionSyntax invocationExpressionSyntax)
     {
+        bool hasReturnedMock = true;
+        bool hasMockBehavior = true;
         SymbolInfo symbol = context.SemanticModel.GetSymbolInfo(invocationExpressionSyntax, context.CancellationToken);
 
         if (symbol.Symbol is not IMethodSymbol method)
         {
             return;
         }
 
-        if (!string.Equals(method.ContainingType.Name, expectedClassName, StringComparison.Ordinal))
+        if (!method.IsInstanceOf(knownSymbols.MockOf) && !method.IsInstanceOf(knownSymbols.MockRepositoryCreate))
         {
             return;
         }
 
-        ITypeSymbol returnType = method.ReturnType;
-        if (hasReturnedMock)
-        {
-            if (returnType is not INamedTypeSymbol { IsGenericType: true } typeSymbol)
-            {
-                return;
-            }
-
-            returnType = typeSymbol.TypeArguments[0];
-        }
-
         // We are calling MockRepository.Create<T> or Mock.Of<T>, determine which
         ArgumentListSyntax? argumentList = null;
-        if (WellKnownMoqNames.OfMethodName.Equals(method.Name, StringComparison.Ordinal))
+        if (method.IsInstanceOf(knownSymbols.MockOf))
         {
             // Mock.Of<T> can specify condition for construction and MockBehavior, but
             // cannot specify constructor parameters
             //
             // The only parameters that can be passed are not relevant for verification
             // to just strip them
+            hasReturnedMock = false;
         }
         else
         {
             argumentList = invocationExpressionSyntax.ArgumentList;
         }
 
-        VerifyMockAttempt(context, returnType, argumentList, hasMockBehavior);
+        ITypeSymbol returnType = method.ReturnType;
+        if (hasReturnedMock)
+        {
+            if (returnType is not INamedTypeSymbol { IsGenericType: true } typeSymbol)
+            {
+                return;
+            }
+
+            returnType = typeSymbol.TypeArguments[0];
+        }
+
+        VerifyMockAttempt(context, knownSymbols, returnType, argumentList, hasMockBehavior);
     }
 
     /// <summary>
     /// Analyzes when a Mock`1 object is created to verify the provided constructor arguments
     /// match an existing constructor of the mocked class.
     /// </summary>
     /// <param name="context">The context.</param>
-    private static void AnalyzeNewObject(SyntaxNodeAnalysisContext context)
+    /// <param name="knownSymbols">The <see cref="MoqKnownSymbols"/> used to lookup symbols against Moq types.</param>
+    private static void AnalyzeNewObject(SyntaxNodeAnalysisContext context, MoqKnownSymbols knownSymbols)
     {
         ObjectCreationExpressionSyntax newExpression = (ObjectCreationExpressionSyntax)context.Node;
 
@@ -283,33 +241,24 @@ private static void AnalyzeNewObject(SyntaxNodeAnalysisContext context)
             return;
         }
 
-        // Quick check
-        if (!string.Equals(
-                genericNameSyntax.Identifier.ValueText,
-                WellKnownMoqNames.MockTypeName,
-                StringComparison.Ordinal))
-        {
-            return;
-        }
-
-        // Full check
         SymbolInfo symbolInfo = context.SemanticModel.GetSymbolInfo(newExpression, context.CancellationToken);
 
-        if (symbolInfo.Symbol is not IMethodSymbol mockConstructorMethod
-            || mockConstructorMethod.MethodKind != MethodKind.Constructor
-            || !string.Equals(mockConstructorMethod.ContainingType.ConstructedFrom.ContainingSymbol.Name, WellKnownMoqNames.MoqSymbolName, StringComparison.Ordinal))
+        if (!symbolInfo
+            .Symbol?
+            .IsInstanceOf(knownSymbols.Mock1?.Constructors ?? ImmutableArray<IMethodSymbol>.Empty)
+            ?? false)
         {
             return;
         }
 
-        if (mockConstructorMethod.ReceiverType is not INamedTypeSymbol { IsGenericType: true } typeSymbol)
+        if (symbolInfo.Symbol?.ContainingType is not INamedTypeSymbol { IsGenericType: true } typeSymbol)
         {
             return;
         }
 
         ITypeSymbol mockedClass = typeSymbol.TypeArguments[0];
 
-        VerifyMockAttempt(context, mockedClass, newExpression.ArgumentList, true);
+        VerifyMockAttempt(context, knownSymbols, mockedClass, newExpression.ArgumentList, true);
     }
 
     /// <summary>
@@ -423,6 +372,7 @@ private static (bool IsEmpty, Location Location) ConstructorIsEmpty(
 
     private static void VerifyMockAttempt(
                     SyntaxNodeAnalysisContext context,
+                    MoqKnownSymbols knownSymbols,
                     ITypeSymbol mockedClass,
                     ArgumentListSyntax? argumentList,
                     bool hasMockBehavior)
@@ -436,7 +386,7 @@ private static void VerifyMockAttempt(
         ArgumentSyntax[] arguments = argumentList?.Arguments.ToArray() ?? [];
 #pragma warning restore ECS0900 // Consider using an alternative implementation to avoid boxing and unboxing
 
-        if (hasMockBehavior && arguments.Length > 0 && IsFirstArgumentMockBehavior(context, argumentList))
+        if (hasMockBehavior && arguments.Length > 0 && IsFirstArgumentMockBehavior(context, knownSymbols, argumentList))
         {
             // They passed a mock behavior as the first argument; ignore as Moq swallows it
             arguments = arguments.RemoveAt(0);

diff --git a/src/Analyzers/SetExplicitMockBehaviorAnalyzer.cs b/src/Analyzers/SetExplicitMockBehaviorAnalyzer.cs
@@ -34,28 +34,28 @@ public override void Initialize(AnalysisContext context)
 
     private static void RegisterCompilationStartAction(CompilationStartAnalysisContext context)
     {
+        MoqKnownSymbols knownSymbols = new(context.Compilation);
+
         // Ensure Moq is referenced in the compilation
-        ImmutableArray<INamedTypeSymbol> mockTypes = context.Compilation.GetMoqMock();
-        if (mockTypes.IsEmpty)
+        if (!knownSymbols.IsMockReferenced())
         {
             return;
         }
 
         // Look for the MockBehavior type and provide it to Analyze to avoid looking it up multiple times.
-        INamedTypeSymbol? mockBehaviorSymbol = context.Compilation.GetTypeByMetadataName(WellKnownMoqNames.FullyQualifiedMoqBehaviorTypeName);
+        INamedTypeSymbol? mockBehaviorSymbol = knownSymbols.MockBehavior;
         if (mockBehaviorSymbol is null)
         {
             return;
         }
 
         // Look for the Mock.Of() method and provide it to Analyze to avoid looking it up multiple times.
-#pragma warning disable ECS0900 // Minimize boxing and unboxing
-        ImmutableArray<IMethodSymbol> ofMethods = mockTypes
-            .SelectMany(mockType => mockType.GetMembers(WellKnownMoqNames.OfMethodName))
-            .OfType<IMethodSymbol>()
-            .Where(method => method.IsGenericMethod)
+        ImmutableArray<IMethodSymbol> ofMethods = knownSymbols.MockOf;
+
+        ImmutableArray<INamedTypeSymbol> mockTypes =
+            new INamedTypeSymbol?[] { knownSymbols.Mock1, knownSymbols.MockRepository }
+            .WhereNotNull()
             .ToImmutableArray();
-#pragma warning restore ECS0900 // Minimize boxing and unboxing
 
         context.RegisterOperationAction(
             context => AnalyzeNewObject(context, mockTypes, mockBehaviorSymbol),