Skip to content

Commit

Permalink
Remove Regex from analyzers (#81)
Browse files Browse the repository at this point in the history
Replace using a `Regex` on string-ified type names with the proper
Roslyn API (usually Identifier.Name) and do some 'straightening' of the
code using early-returns and pattern matching to reduce the
triangle-ness.

This is effectively a rewrite of
`AsShouldBeUsedOnlyForInterfaceAnalyzer`. There's a smaller refactor for
`CallbackSignatureShouldMatchMockedMethod*` as that code needs to be
shared between the analyzer and the code fix provider (will be handled
later)

This refactor has a few benefits:

1. It addresses the bug mentioned in #58 (testing with a newer version
is coming in another PR to keep code reviewable)
2. It improves performance a good amount using very basic Visual Studio
tracing (e.g. ~20 fewer string allocations and saves 30% [20 ms] on a
single call to Analyzer.Analyze())
3. It plumbs the `CancellationToken` into more places to be a good IDE
citizen

This PR also starts to break apart the monolithic `Helpers` class into
smaller components. In order to support the `.TryXXX` pattern I needed
to bring in a polyfill for `[NotNullWhen]` for nullability analysis. I'm
using PolySharp because, in addition to being the most popular polyfill,
it uses a source generator to add polyfills to the current assembly,
which avoid assembly loading overhead and the hassle of adding
dependencies to the analyzer.
  • Loading branch information
MattKotsenas authored Jun 12, 2024
1 parent 0a7f49a commit dad8699
Show file tree
Hide file tree
Showing 14 changed files with 189 additions and 88 deletions.
1 change: 1 addition & 0 deletions Directory.Packages.props
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
<ManagePackageVersionsCentrally>true</ManagePackageVersionsCentrally>
<CentralPackageTransitivePinningEnabled>true</CentralPackageTransitivePinningEnabled>
</PropertyGroup>
<Import Project="build/targets/compiler/Packages.props" />
<Import Project="build/targets/reproducible/Packages.props" />
<Import Project="build/targets/tests/Packages.props" />
<Import Project="build/targets/codeanalysis/Packages.props" />
Expand Down
43 changes: 31 additions & 12 deletions Source/Moq.Analyzers/AsShouldBeUsedOnlyForInterfaceAnalyzer.cs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ namespace Moq.Analyzers;
[DiagnosticAnalyzer(LanguageNames.CSharp)]
public class AsShouldBeUsedOnlyForInterfaceAnalyzer : DiagnosticAnalyzer
{
private static readonly MoqMethodDescriptorBase MoqAsMethodDescriptor = new MoqAsMethodDescriptor();

private static readonly DiagnosticDescriptor Rule = new DiagnosticDescriptor(
Diagnostics.AsShouldBeUsedOnlyForInterfaceId,

Check warning on line 9 in Source/Moq.Analyzers/AsShouldBeUsedOnlyForInterfaceAnalyzer.cs

View workflow job for this annotation

GitHub Actions / build (windows-2022)

Enable analyzer release tracking for the analyzer project containing rule 'Moq1300' (https://github.com/dotnet/roslyn-analyzers/blob/main/src/Microsoft.CodeAnalysis.Analyzers/ReleaseTrackingAnalyzers.Help.md)

Check warning on line 9 in Source/Moq.Analyzers/AsShouldBeUsedOnlyForInterfaceAnalyzer.cs

View workflow job for this annotation

GitHub Actions / build (ubuntu-22.04)

Enable analyzer release tracking for the analyzer project containing rule 'Moq1300' (https://github.com/dotnet/roslyn-analyzers/blob/main/src/Microsoft.CodeAnalysis.Analyzers/ReleaseTrackingAnalyzers.Help.md)
Diagnostics.AsShouldBeUsedOnlyForInterfaceTitle,
Expand All @@ -22,20 +24,37 @@ public override void Initialize(AnalysisContext context)

private static void Analyze(SyntaxNodeAnalysisContext context)
{
InvocationExpressionSyntax? asInvocation = (InvocationExpressionSyntax)context.Node;
if (context.Node is not InvocationExpressionSyntax invocationExpression)
{
return;
}

if (invocationExpression.Expression is not MemberAccessExpressionSyntax memberAccessSyntax)
{
return;
}

if (!MoqAsMethodDescriptor.IsMatch(context.SemanticModel, memberAccessSyntax, context.CancellationToken))
{
return;
}

if (!memberAccessSyntax.Name.TryGetGenericArguments(out SeparatedSyntaxList<TypeSyntax> typeArguments))
{
return;
}

if (typeArguments.Count != 1)
{
return;
}

TypeSyntax typeArgument = typeArguments[0];
SymbolInfo symbolInfo = context.SemanticModel.GetSymbolInfo(typeArgument, context.CancellationToken);

if (asInvocation.Expression is MemberAccessExpressionSyntax memberAccessExpression
&& Helpers.IsMoqAsMethod(context.SemanticModel, memberAccessExpression)
&& memberAccessExpression.Name is GenericNameSyntax genericName
&& genericName.TypeArgumentList.Arguments.Count == 1)
if (symbolInfo.Symbol is ITypeSymbol { TypeKind: not TypeKind.Interface })
{
TypeSyntax? typeArgument = genericName.TypeArgumentList.Arguments[0];
SymbolInfo symbolInfo = context.SemanticModel.GetSymbolInfo(typeArgument, context.CancellationToken);
if (symbolInfo.Symbol is ITypeSymbol typeSymbol && typeSymbol.TypeKind != TypeKind.Interface)
{
Diagnostic? diagnostic = Diagnostic.Create(Rule, typeArgument.GetLocation());
context.ReportDiagnostic(diagnostic);
}
context.ReportDiagnostic(Diagnostic.Create(Rule, typeArgument.GetLocation()));
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ private static void Analyze(SyntaxNodeAnalysisContext context)
SeparatedSyntaxList<ParameterSyntax> lambdaParameters = callbackLambda.ParameterList.Parameters;
if (lambdaParameters.Count == 0) return;

InvocationExpressionSyntax? setupInvocation = Helpers.FindSetupMethodFromCallbackInvocation(context.SemanticModel, callbackOrReturnsInvocation);
InvocationExpressionSyntax? setupInvocation = Helpers.FindSetupMethodFromCallbackInvocation(context.SemanticModel, callbackOrReturnsInvocation, context.CancellationToken);
InvocationExpressionSyntax? mockedMethodInvocation = Helpers.FindMockedMethodInvocationFromSetupMethod(setupInvocation);
if (mockedMethodInvocation == null) return;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ private async Task<Document> FixCallbackSignatureAsync(SyntaxNode root, Document
return document;
}

InvocationExpressionSyntax? setupMethodInvocation = Helpers.FindSetupMethodFromCallbackInvocation(semanticModel, callbackInvocation);
InvocationExpressionSyntax? setupMethodInvocation = Helpers.FindSetupMethodFromCallbackInvocation(semanticModel, callbackInvocation, cancellationToken);
Debug.Assert(setupMethodInvocation != null, nameof(setupMethodInvocation) + " != null");
IMethodSymbol[]? matchingMockedMethods = Helpers.GetAllMatchingMockedMethodSymbolsFromSetupMethodInvocation(semanticModel, setupMethodInvocation).ToArray();

Expand Down
20 changes: 6 additions & 14 deletions Source/Moq.Analyzers/Helpers.cs
Original file line number Diff line number Diff line change
@@ -1,22 +1,14 @@
using System.Diagnostics;
using System.Text.RegularExpressions;

namespace Moq.Analyzers;

internal static class Helpers
{
private static readonly MoqMethodDescriptor MoqSetupMethodDescriptor = new("Setup", new Regex("^Moq\\.Mock<.*>\\.Setup\\.*"));
private static readonly MoqMethodDescriptorBase MoqSetupMethodDescriptor = new MoqSetupMethodDescriptor();

private static readonly MoqMethodDescriptor MoqAsMethodDescriptor = new("As", new Regex("^Moq\\.Mock\\.As<\\.*"), isGeneric: true);

internal static bool IsMoqSetupMethod(SemanticModel semanticModel, MemberAccessExpressionSyntax method)
{
return MoqSetupMethodDescriptor.IsMoqMethod(semanticModel, method);
}

internal static bool IsMoqAsMethod(SemanticModel semanticModel, MemberAccessExpressionSyntax method)
internal static bool IsMoqSetupMethod(SemanticModel semanticModel, MemberAccessExpressionSyntax method, CancellationToken cancellationToken)
{
return MoqAsMethodDescriptor.IsMoqMethod(semanticModel, method);
return MoqSetupMethodDescriptor.IsMatch(semanticModel, method, cancellationToken);
}

internal static bool IsCallbackOrReturnInvocation(SemanticModel semanticModel, InvocationExpressionSyntax callbackOrReturnsInvocation)
Expand Down Expand Up @@ -48,12 +40,12 @@ internal static bool IsCallbackOrReturnInvocation(SemanticModel semanticModel, I
};
}

internal static InvocationExpressionSyntax? FindSetupMethodFromCallbackInvocation(SemanticModel semanticModel, ExpressionSyntax expression)
internal static InvocationExpressionSyntax? FindSetupMethodFromCallbackInvocation(SemanticModel semanticModel, ExpressionSyntax expression, CancellationToken cancellationToken)
{
InvocationExpressionSyntax? invocation = expression as InvocationExpressionSyntax;
if (invocation?.Expression is not MemberAccessExpressionSyntax method) return null;
if (IsMoqSetupMethod(semanticModel, method)) return invocation;
return FindSetupMethodFromCallbackInvocation(semanticModel, method.Expression);
if (IsMoqSetupMethod(semanticModel, method, cancellationToken)) return invocation;
return FindSetupMethodFromCallbackInvocation(semanticModel, method.Expression, cancellationToken);
}

internal static InvocationExpressionSyntax? FindMockedMethodInvocationFromSetupMethod(InvocationExpressionSyntax? setupInvocation)
Expand Down
32 changes: 32 additions & 0 deletions Source/Moq.Analyzers/MoqAsMethodDescriptor.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
namespace Moq.Analyzers;

/// <summary>
/// A class that, given a <see cref="SemanticModel"/> and a <see cref="MemberAccessExpressionSyntax"/>, determines if
/// it is a call to the Moq `Mock.As()` method.
/// </summary>
internal class MoqAsMethodDescriptor : MoqMethodDescriptorBase
{
private const string MethodName = "As";

public override bool IsMatch(SemanticModel semanticModel, MemberAccessExpressionSyntax memberAccessSyntax, CancellationToken cancellationToken)
{
if (!IsFastMatch(memberAccessSyntax, MethodName.AsSpan()))
{
return false;
}

ISymbol? symbol = semanticModel.GetSymbolInfo(memberAccessSyntax, cancellationToken).Symbol;

if (symbol is not IMethodSymbol methodSymbol)
{
return false;
}

if (!IsContainedInMockType(methodSymbol))
{
return false;
}

return methodSymbol.Name.AsSpan().SequenceEqual(MethodName.AsSpan()) && methodSymbol.IsGenericMethod;
}
}
58 changes: 0 additions & 58 deletions Source/Moq.Analyzers/MoqMethodDescriptor.cs

This file was deleted.

38 changes: 38 additions & 0 deletions Source/Moq.Analyzers/MoqMethodDescriptorBase.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
namespace Moq.Analyzers;

/// <summary>
/// A base that that provides common functionality for identifying if a given <see cref="SyntaxNode"/>
/// is a specific Moq method.
/// </summary>
/// <remarks>
/// Currently the <see cref="IsMatch(SemanticModel, MemberAccessExpressionSyntax, CancellationToken)"/> abstract method
/// is specific to <see cref="MemberAccessExpressionSyntax"/> because that's the only type of syntax in use. I expect we'll need
/// to loosen this restriction if we start using other types of syntax.
/// </remarks>
internal abstract class MoqMethodDescriptorBase
{
private const string ContainingNamespace = "Moq";
private const string ContainingType = "Mock";

public abstract bool IsMatch(SemanticModel semanticModel, MemberAccessExpressionSyntax memberAccessSyntax, CancellationToken cancellationToken);

protected static bool IsFastMatch(MemberAccessExpressionSyntax memberAccessSyntax, ReadOnlySpan<char> methodName)
{
return memberAccessSyntax.Name.Identifier.Text.AsSpan().SequenceEqual(methodName);
}

protected static bool IsContainedInMockType(IMethodSymbol methodSymbol)
{
return IsInMoqNamespace(methodSymbol) && IsInMockType(methodSymbol);
}

private static bool IsInMoqNamespace(ISymbol symbol)
{
return symbol.ContainingNamespace.Name.AsSpan().SequenceEqual(ContainingNamespace.AsSpan());
}

private static bool IsInMockType(ISymbol symbol)
{
return symbol.ContainingType.Name.AsSpan().SequenceEqual(ContainingType.AsSpan());
}
}
32 changes: 32 additions & 0 deletions Source/Moq.Analyzers/MoqSetupMethodDescriptor.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
namespace Moq.Analyzers;

/// <summary>
/// A class that, given a <see cref="SemanticModel"/> and a <see cref="MemberAccessExpressionSyntax"/>, determines if
/// it is a call to the Moq `Mock.Setup()` method.
/// </summary>
internal class MoqSetupMethodDescriptor : MoqMethodDescriptorBase
{
private const string MethodName = "Setup";

public override bool IsMatch(SemanticModel semanticModel, MemberAccessExpressionSyntax memberAccessSyntax, CancellationToken cancellationToken)
{
if (!IsFastMatch(memberAccessSyntax, MethodName.AsSpan()))
{
return false;
}

ISymbol? symbol = semanticModel.GetSymbolInfo(memberAccessSyntax, cancellationToken).Symbol;

if (symbol is not IMethodSymbol methodSymbol)
{
return false;
}

if (!IsContainedInMockType(methodSymbol))
{
return false;
}

return methodSymbol.Name.AsSpan().SequenceEqual(MethodName.AsSpan()) && methodSymbol.IsGenericMethod;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ private static void Analyze(SyntaxNodeAnalysisContext context)
{
InvocationExpressionSyntax? setupInvocation = (InvocationExpressionSyntax)context.Node;

if (setupInvocation.Expression is MemberAccessExpressionSyntax memberAccessExpression && Helpers.IsMoqSetupMethod(context.SemanticModel, memberAccessExpression))
if (setupInvocation.Expression is MemberAccessExpressionSyntax memberAccessExpression && Helpers.IsMoqSetupMethod(context.SemanticModel, memberAccessExpression, context.CancellationToken))
{
ExpressionSyntax? mockedMemberExpression = Helpers.FindMockedMemberExpressionFromSetupMethod(setupInvocation);
if (mockedMemberExpression == null)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ private static void Analyze(SyntaxNodeAnalysisContext context)
{
InvocationExpressionSyntax? setupInvocation = (InvocationExpressionSyntax)context.Node;

if (setupInvocation.Expression is MemberAccessExpressionSyntax memberAccessExpression && Helpers.IsMoqSetupMethod(context.SemanticModel, memberAccessExpression))
if (setupInvocation.Expression is MemberAccessExpressionSyntax memberAccessExpression && Helpers.IsMoqSetupMethod(context.SemanticModel, memberAccessExpression, context.CancellationToken))
{
ExpressionSyntax? mockedMemberExpression = Helpers.FindMockedMemberExpressionFromSetupMethod(setupInvocation);
if (mockedMemberExpression == null)
Expand Down
33 changes: 33 additions & 0 deletions Source/Moq.Analyzers/SyntaxExtensions.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
using System.Diagnostics.CodeAnalysis;

namespace Moq.Analyzers;

/// <summary>
/// Extensions methods for <see cref="SyntaxNode"/>s.
/// </summary>
internal static class SyntaxExtensions
{
/// <summary>
/// Tries to get the generic arguments of a given <see cref="NameSyntax"/>.
/// </summary>
/// <param name="syntax">The syntax to inspect.</param>
/// <param name="typeArguments">The collection of <see cref="TypeSyntax"/> elements on the <paramref name="syntax"/>.</param>
/// <returns><see langword="true"/> if <paramref name="syntax"/> has generic / type parameters; <see langword="false"/> otherwise.</returns>
/// <example>
/// x.As&lt;ISampleInterface&gt;() returns <see langword="true"/> and <paramref name="typeArguments"/> will contain <c>ISampleInterface</c>.
/// </example>
/// <example>
/// x.As() returns <see langword="false"/> and <paramref name="typeArguments"/> will be empty.
/// </example>
public static bool TryGetGenericArguments(this NameSyntax syntax, [NotNullWhen(true)] out SeparatedSyntaxList<TypeSyntax> typeArguments)
{
if (syntax is GenericNameSyntax genericName)
{
typeArguments = genericName.TypeArgumentList.Arguments;
return true;
}

typeArguments = default;
return false;
}
}
7 changes: 7 additions & 0 deletions build/targets/compiler/Compiler.props
Original file line number Diff line number Diff line change
Expand Up @@ -4,4 +4,11 @@
<LangVersion>12.0</LangVersion>
<Nullable>enable</Nullable>
</PropertyGroup>

<ItemGroup>
<PackageReference Include="PolySharp">
<PrivateAssets>all</PrivateAssets>
<IncludeAssets>runtime; build; native; contentfiles; analyzers</IncludeAssets>
</PackageReference>
</ItemGroup>
</Project>
5 changes: 5 additions & 0 deletions build/targets/compiler/Packages.props
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
<Project>
<ItemGroup>
<PackageVersion Include="PolySharp" Version="1.14.1" />
</ItemGroup>
</Project>

0 comments on commit dad8699

Please sign in to comment.