Skip to content

Conversation

@TimothyMakkison
Copy link
Contributor

@TimothyMakkison TimothyMakkison commented Jan 26, 2026

Add incremental caching for MethodAssertionGenerator.

  • Uses ImmutableEquatableArray for structural equality on collections
  • I have created tests to verify this, as usual I can't run any projects with Sourcy so I created a new test project see Method assertion incre test #4565
  • Plan is to do this for all generators to improve ide responsiveness, should help with repeated build times 👍

@TimothyMakkison TimothyMakkison marked this pull request as draft January 26, 2026 19:14
@TimothyMakkison TimothyMakkison force-pushed the incre_cach_method_assertion branch 2 times, most recently from 8476626 to ef20be6 Compare January 26, 2026 20:29
@TimothyMakkison TimothyMakkison marked this pull request as ready for review January 26, 2026 20:29
@TimothyMakkison TimothyMakkison changed the title Incre cach method assertion feat: make MethodAssertionGenerator incremental Jan 26, 2026
@TimothyMakkison TimothyMakkison force-pushed the incre_cach_method_assertion branch from ef20be6 to 7b1c1c3 Compare January 26, 2026 21:45
@thomhurst
Copy link
Owner

Summary

Adds incremental caching to MethodAssertionGenerator by introducing ImmutableEquatableArray<T> for structural equality on collections, improving IDE responsiveness and build performance.

Critical Issues

None found ✅

Suggestions

1. Missing snapshot test updates

Impact: Medium
The PR modifies source generator output by restructuring internal data models (e.g., AssertionMethodData now uses new record types like MethodData, TargetTypeData, and ParameterData). While the generated code output may be identical, source generator changes typically require snapshot test verification.

Action: Run dotnet test TUnit.Assertions.SourceGenerator.Tests to verify no snapshot differences. If .received.txt files are generated, review and commit .verified.txt files per CLAUDE.md instructions.

2. Unused field BuildAssertion

Location: MethodAssertionGenerator.cs:20

public static string BuildAssertion = "MethodAssertionData";

This field is only used once in line 71 with .WithTrackingName(BuildAssertion). Consider:

  • Making it const instead of a field (no runtime allocation)
  • Or inlining the string literal directly if it's only used once

Minor optimization: Avoids unnecessary static field allocation for a constant value.

3. Record types for incremental caching

Observation: The new record types (ParameterData, MethodData, TargetTypeData, ContainingTypeData) all implement value equality, which is correct for incremental generation. However, they contain string fields that reference fully-qualified type names and expressions.

Question: Have you verified that these strings produce stable output across compilations? Type display strings can sometimes include metadata tokens or ordering differences that break incremental caching.

Test recommendation: Add a test that verifies incremental compilation works correctly by running the generator twice with identical input and confirming no regeneration occurs.

4. ImmutableEquatableArray source attribution

The comment at the top credits dotnet/runtime. Consider adding a license header or more explicit attribution if required by the source repository's license (MIT in this case allows it, but attribution is good practice).

Previous Review Status

No previous review comments.

Verdict

APPROVE - No critical issues

This is a well-executed performance improvement. The refactoring to value-based records and ImmutableEquatableArray follows best practices for incremental source generators. The code is clean and the approach aligns with TUnit's performance-first principles (Rule 4).

Minor suggestions above are optional improvements, not blockers.

@TimothyMakkison
Copy link
Contributor Author

TimothyMakkison commented Jan 26, 2026

  1. Missing snapshot test updates

Shouldn't alter the output (I hope) 🤞

  1. Unused field BuildAssertion

See separate PR for the incremental tests.

  1. Record types for incremental caching

Should be fine, I rewrote Mapperly, Mediator and Refit with the same technique 👍

Should I add the tests to TUnit.Assertion.SourceGenerator.Tests in a different PR? Once it's added, running them will be a pain for me due to the sourcy issue.

@TimothyMakkison
Copy link
Contributor Author

Okay, looking at TestMetadataGenerator I see a lot of dead or unused code, is it okay if I make a pr removing this, or are you keeping it for future plans?

I'm pretty sure InterfaceCache is creating a memory leak as ITypeSymbol is always different between runs, at the very least it keeps at least on stale ITypeSymbol permanently alive

@thomhurst thomhurst disabled auto-merge January 27, 2026 00:44
@thomhurst thomhurst merged commit 5460395 into thomhurst:main Jan 27, 2026
6 of 10 checks passed
@thomhurst
Copy link
Owner

Okay, looking at TestMetadataGenerator I see a lot of dead or unused code, is it okay if I make a pr removing this, or are you keeping it for future plans?

I'm pretty sure InterfaceCache is creating a memory leak as ITypeSymbol is always different between runs, at the very least it keeps at least on stale ITypeSymbol permanently alive

If it's dead yeah go for it! 🔥

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants