Explainer: Refactor to IOperation

[rjmurillo]

Refactor to IOperation explained

What's All This About?

The code analyzers and fixes in this repo directly use the C# syntax tree and the semantic model to detect some patterns and report warnings. This means the analyzers cannot be used for other .NET languages, as the syntax tree is different. This means for each language to be supported, a new analyzer must be duplicated, which increases complexity and maintenance costs. Additionally, the computational cost of traversing the syntax tree is expensive. Moving to IOperation reduces the complexity and makes the analyzers easier to understand, operationally faster, and can now be reused for other .NET languages. Neat!

Approach

Pros

• easier code to understand
• multiple language support
• faster

Cons

• Squiggles are less precise
• A subset of C# and VB.NET is representable using IOperation. Some operations may have to remain on SyntaxNode.

The change moves most of the logic that is now contained in the void Analyze(SyntaxNodeAnalysisContext) method to the callback registered during void Initialize(AnalysisContext), only invoking code to analyze when preconditions are met. This has the benefit of both simplifying to reduce issues identified in #90 as well as separating concerns about filtering and analyzing symbols.

Strategy

1. Convert to IOperation where possible
2. Where less precise squiggles are observed, improve analyzer messaging to guide user to correct action (and/or provide Code Fixes)
3. Improve analyzer messaging Review analyzer Description, Message, and Category #85
4. Add back in capabilities for precision with benchmarks to track performance

Since IOperation does not represent the full syntax tree, there may be loss of precision in the squiggle shown when writing code. This effort may require driving changes upstream with the Roslyn team.

Proposal

This may be best understood through an example. We will use the existing Mock.As<T>() analyzer as an example.

Existing

[DiagnosticAnalyzer(LanguageNames.CSharp)]
public class AsShouldBeUsedOnlyForInterfaceAnalyzer : DiagnosticAnalyzer
{
    internal const string RuleId = "Moq1300";
    private const string Title = "Moq: Invalid As type parameter";
    private const string Message = "Mock.As() should take interfaces only";

    private static readonly MoqMethodDescriptorBase MoqAsMethodDescriptor = new MoqAsMethodDescriptor();

    private static readonly DiagnosticDescriptor Rule = new(
        RuleId,
        Title,
        Message,
        DiagnosticCategory.Moq,
        DiagnosticSeverity.Error,
        isEnabledByDefault: true,
        helpLinkUri: $"https://github.com/rjmurillo/moq.analyzers/blob/main/docs/rules/{RuleId}.md");

    /// <inheritdoc />
    public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics => ImmutableArray.Create(Rule);
    public override void Initialize(AnalysisContext context)
    {
        context.EnableConcurrentExecution();
        context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None);
        context.RegisterSyntaxNodeAction(Analyze, SyntaxKind.InvocationExpression);
    }

    [System.Diagnostics.CodeAnalysis.SuppressMessage("Maintainability", "AV1500:Member or local function contains too many statements", Justification = "Tracked in https://github.com/rjmurillo/moq.analyzers/issues/90")]
    private static void Analyze(SyntaxNodeAnalysisContext context)
    {
        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 (symbolInfo.Symbol is ITypeSymbol { TypeKind: not TypeKind.Interface })
        {
            context.ReportDiagnostic(Diagnostic.Create(Rule, typeArgument.GetLocation()));
        }
    }
}

Using IOperation

[DiagnosticAnalyzer(LanguageNames.CSharp)]
public class AsShouldBeUsedOnlyForInterfaceAnalyzer2 : DiagnosticAnalyzer
{
    internal const string RuleId = "Moq1300";
    private const string Title = "Moq: Invalid As type parameter";
    private const string Message = "Mock.As() should take interfaces only";

    private static readonly DiagnosticDescriptor Rule = new(
        RuleId,
        Title,
        Message,
        DiagnosticCategory.Moq,
        DiagnosticSeverity.Error,
        isEnabledByDefault: true,
        helpLinkUri: $"https://github.com/rjmurillo/moq.analyzers/blob/main/docs/rules/{RuleId}.md");

    /// <inheritdoc />
    public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics => ImmutableArray.Create(Rule);

    /// <inheritdoc />
    public override void Initialize(AnalysisContext context)
    {
        context.EnableConcurrentExecution();
        context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None);

        context.RegisterCompilationStartAction(static context =>
        {
            ImmutableArray<INamedTypeSymbol> mockTypes = context.Compilation.GetTypesByMetadataNames([WellKnownTypeNames.MoqMock, WellKnownTypeNames.MoqMock1]);
            if (mockTypes.IsEmpty)
            {
                return;
            }
            ImmutableArray<IMethodSymbol> asMethods = mockTypes
                .SelectMany(mockType => mockType.GetMembers("As"))
                .OfType<IMethodSymbol>()
                .Where(method => method.IsGenericMethod)
                .ToImmutableArray();
            if (asMethods.IsEmpty)
            {
                return;
            }
            context.RegisterOperationAction(context => Analyze(context, asMethods), OperationKind.Invocation);
        });
    }

    private static void Analyze(OperationAnalysisContext context, ImmutableArray<IMethodSymbol> wellKnownAsMethods)
    {
        if (context.Operation is not IInvocationOperation invocationOperation)
        {
            return;
        }

        IMethodSymbol targetMethod = invocationOperation.TargetMethod;

        if (!wellKnownAsMethods.Any(asMethod => asMethod.Equals(targetMethod.OriginalDefinition, SymbolEqualityComparer.Default)))
        {
            return;
        }

        ImmutableArray<ITypeSymbol> typeArguments = targetMethod.TypeArguments;
        if (typeArguments.Length != 1)
        {
            return;
        }

        if (typeArguments[0] is ITypeSymbol { TypeKind: not TypeKind.Interface })
        {
            context.ReportDiagnostic(Diagnostic.Create(Rule, invocationOperation.Syntax.GetLocation()));
        }
    }
}

The benchmark shows an improvement between the old and new methods.

BenchmarkDotNet v0.13.12, Windows 11 (10.0.22635.3790)
Intel Core i9-10940X CPU 3.30GHz, 1 CPU, 28 logical and 14 physical cores
.NET SDK 8.0.301
  [Host] : .NET 8.0.6 (8.0.624.26715), X64 RyuJIT AVX-512F+CD+BW+DQ+VL

Job=InProcess  Toolchain=InProcessEmitToolchain  InvocationCount=1
UnrollFactor=1

Method	Mean	Error	StdDev	Ratio	RatioSD	Gen0	Gen1	Allocated	Alloc Ratio
OldMoq1300WithDiagnostics	181.6 ms	4.76 ms	13.58 ms	1.75	0.14	7000.0000	2000.0000	68.38 MB	1.39
NewMoq1300WithDiagnostics	159.9 ms	3.17 ms	8.00 ms	1.54	0.11	6000.0000	2000.0000	62.33 MB	1.26
Moq1300Baseline	104.3 ms	2.06 ms	5.02 ms	1.00	0.00	5000.0000	2000.0000	49.28 MB	1.00

Open Questions

This is a work in progress! We are patterning the initial design after Roslyn Analyzers.

The specific timing of the change is TBD. Pre-release packages will be submitted to NuGet.org to gather community feedback.