agent-framework/dotnet/wf-source-gen-bp.md

Source Generator Best Practices Review

This document reviews the Workflow Executor Route Source Generator implementation against the official Roslyn Source Generator Cookbook best practices from the dotnet/roslyn repository.

Reference Documentation

• Source Generators Cookbook
• Incremental Generators Cookbook

---

Executive Summary

Category	Status	Priority
Generator Type	PASS	-
Attribute-Based Detection	FAIL	HIGH
Model Value Equality	FAIL	HIGH
Collection Equality	FAIL	HIGH
Symbol/SyntaxNode Storage	PASS	-
Code Generation Approach	PASS	-
Diagnostics	PASS	-
Pipeline Efficiency	FAIL	MEDIUM
CancellationToken Handling	PARTIAL	LOW

Overall Assessment: The generator follows several best practices but has critical performance issues that should be addressed before production use. The most significant issue is not using ForAttributeWithMetadataName, which the Roslyn team states is "at least 99x more efficient" than CreateSyntaxProvider.

---

Detailed Analysis

1. Generator Interface Selection

Best Practice: Use IIncrementalGenerator instead of the deprecated ISourceGenerator.

Our Implementation: PASS

// ExecutorRouteGenerator.cs:19
public sealed class ExecutorRouteGenerator : IIncrementalGenerator

The generator correctly implements IIncrementalGenerator, the recommended interface for new generators.

---

2. Attribute-Based Detection with ForAttributeWithMetadataName

Best Practice: Use ForAttributeWithMetadataName() for attribute-based discovery.

"This utility method is at least 99x more efficient than SyntaxProvider.CreateSyntaxProvider, and in many cases even more efficient." — Roslyn Incremental Generators Cookbook

Our Implementation: FAIL (HIGH PRIORITY)

// ExecutorRouteGenerator.cs:25-30
var executorCandidates = context.SyntaxProvider
    .CreateSyntaxProvider(
        predicate: static (node, _) => SyntaxDetector.IsExecutorCandidate(node),
        transform: static (ctx, ct) => SemanticAnalyzer.Analyze(ctx, ct, out _))

Problem: We use CreateSyntaxProvider with manual attribute detection in SyntaxDetector. This requires the generator to examine every syntax node in the compilation, whereas ForAttributeWithMetadataName uses the compiler's built-in attribute index for O(1) lookup.

Recommended Fix:

var executorCandidates = context.SyntaxProvider
    .ForAttributeWithMetadataName(
        fullyQualifiedMetadataName: "Microsoft.Agents.AI.Workflows.MessageHandlerAttribute",
        predicate: static (node, _) => node is MethodDeclarationSyntax,
        transform: static (ctx, ct) => AnalyzeMethodWithAttribute(ctx, ct))
    .Collect()
    .SelectMany((methods, _) => GroupByContainingClass(methods));

Impact: Current approach causes IDE lag on every keystroke in large projects.

---

3. Model Value Equality (Records vs Classes)

Best Practice: Use record types for pipeline models to get automatic value equality.

"Use records, rather than classes, so that value equality is generated for you." — Roslyn Incremental Generators Cookbook

Our Implementation: FAIL (HIGH PRIORITY)

// HandlerInfo.cs:28
internal sealed class HandlerInfo { ... }

// ExecutorInfo.cs:10
internal sealed class ExecutorInfo { ... }

Problem: Both HandlerInfo and ExecutorInfo are sealed class types, which use reference equality by default. The incremental generator caches results based on equality comparison—when the model equals the previous run's model, regeneration is skipped. With reference equality, every analysis produces a "new" object, defeating caching entirely.

Recommended Fix:

// HandlerInfo.cs
internal sealed record HandlerInfo(
    string MethodName,
    string InputTypeName,
    string? OutputTypeName,
    HandlerSignatureKind SignatureKind,
    bool HasCancellationToken,
    EquatableArray<string>? YieldTypes,
    EquatableArray<string>? SendTypes);

// ExecutorInfo.cs
internal sealed record ExecutorInfo(
    string? Namespace,
    string ClassName,
    string? GenericParameters,
    bool IsNested,
    string ContainingTypeChain,
    bool BaseHasConfigureRoutes,
    EquatableArray<HandlerInfo> Handlers,
    EquatableArray<string> ClassSendTypes,
    EquatableArray<string> ClassYieldTypes);

Impact: Without value equality, the generator regenerates code on every compilation even when nothing changed.

---

4. Collection Equality

Best Practice: Use custom equatable wrappers for collections since ImmutableArray<T> uses reference equality.

"Arrays, ImmutableArray<T>, and List<T> use reference equality by default. Wrap collections with custom types implementing value-based equality." — Roslyn Incremental Generators Cookbook

Our Implementation: FAIL (HIGH PRIORITY)

// ExecutorInfo.cs:46
public ImmutableArray<HandlerInfo> Handlers { get; }

// HandlerInfo.cs:58-63
public ImmutableArray<string>? YieldTypes { get; }
public ImmutableArray<string>? SendTypes { get; }

Problem: ImmutableArray<T> compares by reference, not by contents. Two arrays with identical elements are considered unequal, breaking incremental caching.

Recommended Fix: Create an EquatableArray<T> wrapper:

internal readonly struct EquatableArray<T> : IEquatable<EquatableArray<T>>, IEnumerable<T>
    where T : IEquatable<T>
{
    private readonly ImmutableArray<T> _array;

    public EquatableArray(ImmutableArray<T> array) => _array = array;

    public bool Equals(EquatableArray<T> other)
    {
        if (_array.Length != other._array.Length) return false;
        for (int i = 0; i < _array.Length; i++)
        {
            if (!_array[i].Equals(other._array[i])) return false;
        }
        return true;
    }

    public override int GetHashCode()
    {
        var hash = new HashCode();
        foreach (var item in _array) hash.Add(item);
        return hash.ToHashCode();
    }

    // ... IEnumerable implementation
}

Impact: Same as model equality—caching is completely broken for handlers and type arrays.

---

5. Symbol and SyntaxNode Storage

Best Practice: Never store ISymbol or SyntaxNode in pipeline models.

"Storing ISymbol references blocks garbage collection and roots old compilations unnecessarily. Extract only the information you need—typically string representations work well—into your equatable models." — Roslyn Incremental Generators Cookbook

Our Implementation: PASS

The models correctly store only primitive types and strings:

// HandlerInfo.cs - stores strings, not symbols
public string MethodName { get; }
public string InputTypeName { get; }
public string? OutputTypeName { get; }

// ExecutorInfo.cs - stores strings, not symbols
public string? Namespace { get; }
public string ClassName { get; }

The SemanticAnalyzer correctly extracts string representations from symbols:

// SemanticAnalyzer.cs:300-301
var inputType = methodSymbol.Parameters[0].Type;
var inputTypeName = inputType.ToDisplayString(SymbolDisplayFormat.FullyQualifiedFormat);

---

6. Code Generation Approach

Best Practice: Use StringBuilder for code generation, not SyntaxNode construction.

"Avoid constructing SyntaxNodes for output; they're complex to format correctly and NormalizeWhitespace() is expensive. Instead, use a StringBuilder wrapper that tracks indentation levels." — Roslyn Incremental Generators Cookbook

Our Implementation: PASS

// SourceBuilder.cs:17-19
public static string Generate(ExecutorInfo info)
{
    var sb = new StringBuilder();

The SourceBuilder correctly uses StringBuilder with manual indentation tracking.

---

7. Diagnostic Reporting

Best Practice: Use ReportDiagnostic for surfacing issues to users.

Our Implementation: PASS

// ExecutorRouteGenerator.cs:44-50
context.RegisterSourceOutput(diagnosticsProvider, static (ctx, diagnostics) =>
{
    foreach (var diagnostic in diagnostics)
    {
        ctx.ReportDiagnostic(diagnostic);
    }
});

Diagnostics are well-defined with appropriate severities:

ID	Severity	Description
WFGEN001	Error	Missing IWorkflowContext parameter
WFGEN002	Error	Invalid return type
WFGEN003	Error	Class must be partial
WFGEN004	Warning	Not an Executor
WFGEN005	Error	Insufficient parameters
WFGEN006	Info	ConfigureRoutes already defined
WFGEN007	Error	Handler cannot be static

---

8. Pipeline Efficiency

Best Practice: Avoid duplicate work in the pipeline.

Our Implementation: FAIL (MEDIUM PRIORITY)

// ExecutorRouteGenerator.cs:25-41
// Pipeline 1: Get executor candidates
var executorCandidates = context.SyntaxProvider
    .CreateSyntaxProvider(
        predicate: static (node, _) => SyntaxDetector.IsExecutorCandidate(node),
        transform: static (ctx, ct) => SemanticAnalyzer.Analyze(ctx, ct, out _))
    ...

// Pipeline 2: Get diagnostics (duplicates the same work!)
var diagnosticsProvider = context.SyntaxProvider
    .CreateSyntaxProvider(
        predicate: static (node, _) => SyntaxDetector.IsExecutorCandidate(node),
        transform: static (ctx, ct) =>
        {
            SemanticAnalyzer.Analyze(ctx, ct, out var diagnostics);
            return diagnostics;
        })

Problem: The same syntax detection and semantic analysis runs twice—once for extracting ExecutorInfo and once for extracting diagnostics.

Recommended Fix: Return both in a single pipeline:

var analysisResults = context.SyntaxProvider
    .ForAttributeWithMetadataName(...)
    .Select((ctx, ct) => {
        var info = SemanticAnalyzer.Analyze(ctx, ct, out var diagnostics);
        return (Info: info, Diagnostics: diagnostics);
    });

// Split for different outputs
context.RegisterSourceOutput(
    analysisResults.Where(r => r.Info != null).Select((r, _) => r.Info!),
    GenerateSource);

context.RegisterSourceOutput(
    analysisResults.Where(r => r.Diagnostics.Length > 0).Select((r, _) => r.Diagnostics),
    ReportDiagnostics);

---

9. Base Type Chain Scanning

Best Practice: Avoid scanning indirect type relationships when possible.

"Never scan for types that indirectly implement interfaces, inherit from base types, or acquire attributes through inheritance hierarchies. This pattern forces the generator to inspect every type's AllInterfaces or base-type chain on every keystroke." — Roslyn Incremental Generators Cookbook

Our Implementation: PARTIAL CONCERN

// SemanticAnalyzer.cs:126-141
private static bool DerivesFromExecutor(INamedTypeSymbol classSymbol)
{
    var current = classSymbol.BaseType;
    while (current != null)
    {
        var fullName = current.OriginalDefinition.ToDisplayString();
        if (fullName == ExecutorTypeName || fullName.StartsWith(ExecutorTypeName + "<", ...))
        {
            return true;
        }
        current = current.BaseType;
    }
    return false;
}

Analysis: We do walk the base type chain, but this only happens after attribute filtering (classes must have [MessageHandler] methods). Since this is targeted to specific candidates rather than scanning all types, the performance impact is acceptable. However, if we switch to ForAttributeWithMetadataName, the attribute is on methods, so we'd need to check the containing class's base types—which is still targeted.

---

10. CancellationToken Handling

Best Practice: Respect CancellationToken in long-running operations.

Our Implementation: PARTIAL (LOW PRIORITY)

The CancellationToken is passed through to semantic model calls:

// SemanticAnalyzer.cs:46
var classSymbol = semanticModel.GetDeclaredSymbol(classDecl, cancellationToken);

However, there are no explicit cancellationToken.ThrowIfCancellationRequested() calls in loops like AnalyzeHandlers. For most compilations this is fine, but very large classes with many handlers might benefit from periodic checks.

---

11. File Naming Convention

Best Practice: Use descriptive generated file names with .g.cs suffix.

Our Implementation: PASS

// ExecutorRouteGenerator.cs:62-91
private static string GetHintName(ExecutorInfo info)
{
    // Produces: "Namespace.ClassName.g.cs" or "Namespace.Outer.Inner.ClassName.g.cs"
    ...
    sb.Append(".g.cs");
    return sb.ToString();
}

---

Recommended Action Plan

High Priority (Performance Critical)

1. Switch to ForAttributeWithMetadataName

   • Estimated impact: 99x+ performance improvement for attribute detection
   • Requires restructuring the pipeline to collect methods then group by class

2. Convert models to records

   • Change HandlerInfo and ExecutorInfo from sealed class to sealed record
   • Enables automatic value equality for incremental caching

3. Implement EquatableArray<T>

   • Create wrapper struct with value-based equality
   • Replace all ImmutableArray<T> usages in models

Medium Priority (Efficiency)

4. Eliminate duplicate pipeline execution
   • Combine info extraction and diagnostic collection into single pipeline
   • Split outputs using Where and Select

Low Priority (Polish)

5. Add periodic cancellation checks
   • Add ThrowIfCancellationRequested() in handler analysis loop
   • Only needed for extremely large classes

---

Compliance Matrix

Best Practice	Cookbook Reference	Status	Fix Required
Use IIncrementalGenerator	Main cookbook	PASS	No
Use ForAttributeWithMetadataName	Incremental cookbook	FAIL	Yes (High)
Use records for models	Incremental cookbook	FAIL	Yes (High)
Implement collection equality	Incremental cookbook	FAIL	Yes (High)
Don't store ISymbol/SyntaxNode	Incremental cookbook	PASS	No
Use StringBuilder for codegen	Incremental cookbook	PASS	No
Report diagnostics properly	Main cookbook	PASS	No
Avoid duplicate pipeline work	Incremental cookbook	FAIL	Yes (Medium)
Respect CancellationToken	Main cookbook	PARTIAL	Optional
Use .g.cs file suffix	Main cookbook	PASS	No
Additive-only generation	Main cookbook	PASS	No
No language feature emulation	Main cookbook	PASS	No

---

Conclusion

The source generator implementation demonstrates solid understanding of Roslyn generator fundamentals—correct interface usage, proper diagnostic reporting, and appropriate code generation patterns. However, critical performance optimizations are missing that could cause significant IDE lag in production environments.

The three high-priority fixes (ForAttributeWithMetadataName, record models, and EquatableArray) should be implemented before the generator is used in large codebases. These changes will enable proper incremental caching, reducing regeneration from "every keystroke" to "only when relevant code changes."