perf: eliminate AotConverterRegistry from generated code with typed casts#5529
perf: eliminate AotConverterRegistry from generated code with typed casts#5529
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This is a well-motivated refactor that addresses real problems: the old was a full-compilation scan (O(all syntax trees)) that invalidated on any keystroke, and the generated classes were verbose boilerplate. The architectural direction — inlining registrations into the per-class source and using string-based equality for incremental caching — is the right call.
Correctness concern: duplicate AOT registrations per generated file
Both and emit calls. For a typical non-generic class, collects all method symbols for the class and emits one class-level registration block. Then, is called for each test method individually and emits a second per-method registration block into the same file.
This means a type pair like will appear in both the class-level partial block and one or more per-method partial blocks in the same file. Runtime behavior depends on whether is idempotent (i.e., safe to call with the same type pair multiple times). If it isn't (e.g., it throws or double-registers), this is a silent correctness bug.
Suggestion: Either emit only at the class level (which already has all method symbols via ), or skip the per-method call in for non-generic, non-inherited cases. The two emission sites currently run independently with separate HashSets, so deduplication only works within one call, not across the pair.
Weak incremental caching tests
The new tests don't actually verify what they claim. only asserts — that's trivially true regardless of whether the pipeline cached or re-ran.
Compare to the deleted , which used:
That's the only assertion that proves caching actually happened. The new tests verify output exists, not that the pipeline didn't re-execute. They'd pass even if the string-based equality fix was reverted.
Suggestion: Use assertions (as in the deleted tests) to verify the pipeline step was cached. The helper presumably enables tracking — use it to assert for the metadata step when an unrelated type is added.
uses only
The new implementation only compares :
If the class's inherited test methods change (e.g., a attribute is added to a base class method), the incremental pipeline will not invalidate the cached metadata for this class because its fully-qualified name didn't change. This could silently produce stale generated output until a clean rebuild.
Suggestion: Consider whether changes (e.g., compilation timestamp or a hash) should also factor into equality, or document explicitly that this class relies on Roslyn's upstream invalidation to handle method-body/attribute changes.
Minor: not marked on
The new property on is nullable/optional, which is fine. But since is a , the equality and hash code are auto-generated and will include this field — so correctness is preserved. Just worth noting that this field's absence from is intentional (nullable opt-in), not an oversight.
Positive highlights
- Eliminating the fan-in was the right call. The old AOT pipeline invalidated all converters when any test changed — this was the core perf anti-pattern.
- **String-based equality in ** is a well-documented, principled fix for the ISymbol identity problem. The implementation correctly handles array-typed (the trap on arrays).
- ** reuse** removes ~200 lines of duplicated type-hierarchy walking. Good cleanup.
- Snapshot diff (1073 → 643 lines for ) shows the generated code reduction is real and significant.
The two issues above (duplicate registrations and weak incremental tests) are worth addressing before merge, but the overall architecture is a clear improvement.
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 119 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
There was a problem hiding this comment.
Code Review
This is a well-motivated refactor that addresses real problems: the old AotConverterGenerator was a full-compilation scan (O(all syntax trees)) that invalidated on any keystroke, and the generated IAotConverter classes were verbose boilerplate. The architectural direction - inlining registrations into the per-class source and using string-based equality for incremental caching - is the right call.
Correctness concern: duplicate AOT registrations per generated file
Both GeneratePerClassTestSource and GenerateTestMetadata emit EmitAotConverterRegistration calls. For a typical non-generic class, GroupMethodsByClass collects all method symbols for the class and emits one class-level registration block. Then, GenerateTestMetadata is called for each test method individually and emits a second per-method registration block into the same .g.cs file.
This means a type pair will appear in both the class-level partial block and one or more per-method partial blocks in the same file. Runtime behavior depends on whether AotConverterRegistry.Register<S,T> is idempotent (safe to call with the same type pair multiple times). If it is not (e.g., it throws or double-registers), this is a silent correctness bug.
Suggestion: Either emit only at the class level (which already has all method symbols via GroupMethodsByClass), or skip the per-method GenerateRegistrationCode call in GenerateTestMetadata for non-generic, non-inherited cases. The two emission sites currently run independently with separate seen HashSets, so deduplication only works within one call, not across the pair.
Weak incremental caching tests
The new TestMetadataGeneratorIncrementalTests tests do not actually verify what they claim. AddUnrelatedType_ShouldNotRegenerateTestMetadata only asserts result2.GeneratedTrees.Length > 0 - that is trivially true regardless of whether the pipeline cached or re-ran.
Compare to the deleted AotConverterGeneratorIncrementalTests, which used:
AssertRunReasons(driver2, IncrementalGeneratorRunReasons.Cached, 1);That is the only assertion that proves caching actually happened. The new tests verify output exists, not that the pipeline did not re-execute. They would pass even if the string-based equality fix was reverted.
Suggestion: Use IncrementalGeneratorRunReasons assertions (as in the deleted tests) to verify the pipeline step was cached. The TestHelper.GenerateTracked helper presumably enables tracking - use it to assert Cached for the metadata step when an unrelated type is added.
InheritsTestsClassMetadata.Equals uses only TypeFullyQualifiedName
The new IEquatable implementation only compares TypeFullyQualifiedName. If the class's inherited test methods change (e.g., a [Skip] attribute is added to a base class method), the incremental pipeline will not invalidate the cached metadata for this class because its fully-qualified name did not change. This could silently produce stale generated output until a clean rebuild.
Suggestion: Consider whether CompilationContext changes (e.g., a SyntaxTree hash) should also factor into equality, or document explicitly that this class relies on Roslyn's upstream invalidation to handle method-body/attribute changes.
Minor: AotConverterRegistrationCode not marked required on ClassTestGroup
The new AotConverterRegistrationCode property is nullable/optional. Since ClassTestGroup is a record, equality and hash code are auto-generated and will include this field, so correctness is preserved. Just worth noting that the absence of required is intentional (nullable opt-in).
Positive highlights
- Eliminating the .Collect() fan-in was the right call. The old AOT pipeline invalidated all converters when any test changed - that was the core perf anti-pattern.
- String-based equality in TestMethodMetadata is a well-documented, principled fix for the ISymbol identity problem. The ComputeAttributeHash implementation correctly handles array-typed TypedConstant.Values (the .Value trap on arrays).
- DataSourceAttributeHelper reuse removes ~200 lines of duplicated type-hierarchy walking. Good cleanup.
- Snapshot diff (1073 to 643 lines for DecimalArgumentTests) shows the generated code reduction is real and significant.
The two issues above (duplicate registrations and weak incremental tests) are worth addressing before merge, but the overall architecture is a clear improvement.
There was a problem hiding this comment.
Follow-up Review (addressing commit 0e7c8f5)
The second commit directly addresses the weak incremental caching tests feedback from the prior review — thank you. The changes replace the meaningless GeneratedTrees.Length > 0 assertions with proper TrackedSteps-based Cached/Modified assertions, matching the pattern from DynamicTestsGeneratorIncrementalTests. The ParseTestMetadata constant and WithTrackingName wiring are exactly right. That concern is resolved.
Still open: duplicate AOT registrations
The core correctness concern from the prior review was not addressed. The two emission paths remain independent:
GroupMethodsByClass(inGeneratePerClassTestSource) scans all methods and emits one class-level registration block with its ownseenHashSet.GenerateTestMetadatais then called for each test method and emits a second per-method registration block with a freshseenHashSet.
For a class with any test methods, both paths run, and the same AotConverterRegistry.Register<S,T> call will appear in multiple partial blocks in the same .g.cs file. The snapshot diffs confirm this — e.g., DecimalArgumentTests.Test.verified.txt shows a single class-level registration block with all 22 converter calls. But in a real compilation with multiple methods, the per-method path would also fire and produce overlapping blocks.
Whether this is a live bug depends entirely on whether AotConverterRegistry.Register<S,T> is idempotent. If it silently no-ops on duplicates, it's a latent inefficiency. If it throws or double-adds, it's a runtime failure. This should be either:
- Eliminated: emit registrations only from
GeneratePerClassTestSource(which already has all method symbols) and remove theEmitAotConverterRegistrationcall fromGenerateTestMetadata, or - Documented: add a comment explaining why duplicates are safe (with a reference to
AotConverterRegistryidempotency guarantee).
Still open: InheritsTestsClassMetadata.Equals only compares TypeFullyQualifiedName
The IEquatable implementation added in commit 1 uses only the class name for equality. This means the incremental pipeline will not invalidate cached metadata for a class if a base class method gains or loses an attribute (e.g., [Skip], [Arguments]). The class name hasn't changed, so Roslyn considers the node unchanged and skips regeneration.
This may be acceptable if upstream ForAttributeWithMetadataName invalidation cascades correctly through the inheritsTestsClassesProvider pipeline for attribute changes on base methods. But if that's the reasoning, it should be documented — otherwise it looks like an incomplete equality implementation.
Summary
| Issue | Status |
|---|---|
| Weak incremental caching tests | Resolved in commit 2 |
| Duplicate AOT registrations (correctness risk) | Still open |
| InheritsTestsClassMetadata over-caching | Still open |
The second commit is a clean fix. The two remaining issues from the first review are still worth addressing before merge.
There was a problem hiding this comment.
Code Review: perf: inline AOT converter registrations into per-class test source
The core idea is sound and the incremental caching improvement is real. Replacing ISymbol equality with string-based equality and moving away from the CompilationProvider-scoped AOT generator is the right direction. Here are my findings:
Issue 1 (Potential Bug): IsAccessibleType silently skips type-argument accessibility for public generic types
File: TUnit.Core.SourceGenerator/Generators/AotConverterHelper.cs
For a type like Wrapper<InternalType> where Wrapper is public but InternalType is internal to another assembly, the method returns true at the DeclaredAccessibility == Public check before it reaches the IsGenericType block that validates type arguments. The type-argument check is only reached if the code doesn't return early — but public types return early.
This means a converter for Wrapper<InternalType> would be emitted, producing a compile error in the generated file rather than a graceful skip. This bug was present in AotConverterGenerator before, but since the logic is being explicitly carried forward here, it's worth fixing:
if (namedType.DeclaredAccessibility == Accessibility.Public)
{
if (namedType.IsGenericType)
{
foreach (var typeArg in namedType.TypeArguments)
{
if (!IsAccessibleType(typeArg, compilation))
return false;
}
}
return true;
}Issue 2 (Potential Bug): Non-generic inherited tests may miss AOT registrations
File: TUnit.Core.SourceGenerator/Generators/TestMetadataGenerator.cs
The per-method AOT registration block guards with if (testMethod.IsGenericType || testMethod.IsGenericMethod). The comment says non-generic methods are covered by GroupMethodsByClass, but that pipeline only processes methods that are not generic and not inherited (via InheritanceDepth > 0 filtering). Non-generic inherited tests travel through GenerateInheritedTestSources → GenerateTestMetadata but hit the IsGenericType || IsGenericMethod guard and are skipped.
Repro: Base class with [Test, Arguments(5)] void Test(MyType t) where MyType has an implicit conversion from int, plus a subclass with [InheritsTests]. The inherited test won't get its AOT converter registered.
Suggestion:
if (testMethod.IsGenericType || testMethod.IsGenericMethod || testMethod.InheritanceDepth > 0)Issue 3 (Null Safety): FuncAotConverter lacks null guard for value-type conversions
File: TUnit.Core/Converters/FuncAotConverter.cs
public object? Convert(object? value) => converter((TSource)value!);When value is null and TSource is a value type, the null-suppression ! followed by the cast throws NullReferenceException. The old class-based converters had an explicit if (value == null) return null; guard. Since this PR now exclusively generates lambda-based converters (removing the old class-based approach), this path is completely unprotected.
// Option A: fix FuncAotConverter
public object? Convert(object? value) => value is null ? null : converter((TSource)value);Issue 4 (Documentation): Per-class AOT path still in .Collect() — PR claims are slightly overstated
File: TUnit.Core.SourceGenerator/Generators/TestMetadataGenerator.cs (lines 77–82)
The PR description says it "removes the .Collect() fan-in for the AOT converter branch." This is only partially true. The per-class AOT path still runs AotConverterHelper.GenerateRegistrationCode inside .Collect(), meaning it re-runs for all classes on any test method edit. The actual performance win is from the per-method path (generics), which is now correctly scoped per-method.
This is fine — the ClassTestGroup record's structural equality means Roslyn won't re-emit unchanged class files — but the claim in the PR description overstates the win. Worth a comment in code clarifying the actual caching boundary.
Issue 5 (Dead Code): Redundant condition in ScanTypedConstantForTypes
File: TUnit.Core.SourceGenerator/Generators/AotConverterHelper.cs (around line 212)
else if (constant.Kind != TypedConstantKind.Array && constant is { Value: not null, Type: not null })This is inside an else branch, so constant.Kind == TypedConstantKind.Array is already false. The Kind != TypedConstantKind.Array check is dead code. Simplify to:
else if (constant is { Value: not null, Type: not null })What's Good
-
Core incremental caching fix is correct: Using string-based equality instead of
ISymbolidentity inTestMethodMetadata.Equalsis exactly the right Roslyn incremental generator pattern. Symbols lose identity across compilations; strings don't. -
MethodAttributeHashapproach is sound: HashingTypedConstantvalues avoids cross-compilation identity issues while still detecting actual attribute changes. TheArrayArguments_ShouldNotThrowtest directly covers the knownTypedConstant.Valuepitfall for array kinds. -
Generated output is dramatically simpler: ~20 lines per converter reduced to 1 lambda per converter.
static source => (T)sourceis AOT-safe, allocates nothing, and registers inFuncAotConverter<S,T>— a clean design. -
Correct reuse of
DataSourceAttributeHelper.IsDataSourceAttribute: The oldAotConverterGeneratorduplicated this logic with a slightly different implementation. Removing the duplication is the right call. -
New incremental tests are well-targeted:
TestMetadataGeneratorIncrementalTestscovers the three most important scenarios (unrelated type add, attribute change, array argument edge case). These tests would have caught the original over-invalidation bug.
Issues 2 and 3 are the most important to address before merging — they represent behavioral regressions for non-generic inherited tests and null-value conversions respectively. Issue 1 would produce a compile error (visible and fixable) rather than silent incorrectness. Issues 4 and 5 are minor.
There was a problem hiding this comment.
Code Review
This is a well-motivated refactor with solid execution. The core ideas — inlining AOT converter registrations per-class, eliminating the fan-in .Collect(), and switching TestMethodMetadata equality to string-based identity — are all correctness and performance improvements. The diff is substantial but the changes are coherent and the snapshot test coverage provides good safety nets.
A few observations and one correctness concern follow.
Bug: IsAccessibleType incorrectly returns true for public nested types inside inaccessible containers
In AotConverterHelper.IsAccessibleType (lines 240-253), when namedType.DeclaredAccessibility == Accessibility.Public, the method returns true without checking whether the type is nested inside an inaccessible containing type:
if (namedType.DeclaredAccessibility == Accessibility.Public)
{
if (namedType.IsGenericType) { ... check type args ... }
return true; // ← BUG: ignores ContainingType
}A public nested type inside a private or protected outer class is not actually accessible from outside the outer class. The old AotConverterGenerator.cs had the same gap (it also returned true for Accessibility.Public without checking the container), so this is not a regression introduced by this PR — but the refactor is a good opportunity to fix it.
Suggested fix: After the generic-type-arg checks, add if (namedType.ContainingType != null) return IsAccessibleType(namedType.ContainingType, compilation); before the final return true, analogous to what the fallthrough path already does for other accessibility levels.
Design: seen deduplication is per-call, not per-file
AotConverterHelper.GenerateRegistrationCode creates a fresh seen HashSet on each call. For the per-class path this is fine — one call per class, one generated block. For the generic/inherited per-method path (called from GenerateTestMetadata) a fresh seen set is created per method invocation. Duplicate registrations across methods in the same file will be emitted, which is harmless at runtime because AotConverterRegistry.TryAdd silently deduplicates, but it does produce slightly noisier generated output (and marginally larger binaries in AOT builds). The comment in Initialize() acknowledges this explicitly, so this is a deliberate trade-off — worth noting for future cleanup.
Design: ComputeAttributeHash is order-sensitive but attribute order is not guaranteed
ComputeAttributeHash uses a position-dependent hash (hash = (hash * 31) ^ ...). Two methods with the same set of attributes in different orders will produce different hashes, causing unnecessary cache invalidation. Since Roslyn does not guarantee a stable ordering of GetAttributes() results across incremental compilations (though in practice it is stable), this could cause spurious cache misses. Using an order-independent combination (XOR-ing all per-attribute hashes together, or sorting by attribute name first) would be more robust:
// More cache-stable alternative
hash = attributes
.Select(attr => HashAttribute(attr))
.Aggregate(0, (acc, h) => acc ^ h);This is a minor concern because attribute order rarely changes, but it is worth considering since the entire purpose of MethodAttributeHash is incremental-cache stability.
Design: InheritsTestsClassMetadata equality only compares TypeFullyQualifiedName
The added Equals/GetHashCode for InheritsTestsClassMetadata uses only TypeFullyQualifiedName. The comment explains the intentional trade-off:
A clean rebuild handles the rare case where a base class method's attributes change without touching the subclass.
This is an acceptable and clearly documented decision. However, it means that any change to a base class's test methods (added [Category], changed [Timeout], etc.) will require a clean rebuild to take effect — IDE hot-reload will silently serve stale generated code. That behaviour should probably be surfaced in the doc comment as a known limitation rather than buried as an implementation note.
Minor: AotConverterHelper is public but appears internal-use-only
AotConverterHelper and its overloads are public static, but the class lives in the source-generator assembly and is only called from TestMetadataGenerator. Making it internal would be cleaner and consistent with DataSourceAttributeHelper which is internal static. This prevents accidental external dependencies and makes the intended scope clear.
Positive highlights
- Eliminating the
CompilationProvider.Selectfan-in for AOT converters is the right call. The old design would re-scan the entire compilation whenever any test changed — O(n) work on every keystroke. The new per-class approach correctly ties the scan to the classes that actually need it. FuncAotConverternull guard fix (value is null ? null : converter((TSource)value)) is a correctness improvement that was missing before and protects against NPEs on nullable test parameters.- String-based equality for
TestMethodMetadatais the correct pattern for Roslyn incremental generators. UsingISymbolin equality comparisons is a known anti-pattern that defeats incremental caching. - Reusing
DataSourceAttributeHelper.IsDataSourceAttributeinstead of the hand-rolled type-hierarchy walk in the old generator is a good reduction in duplicated logic. - New
TestMetadataGeneratorIncrementalTestsdirectly tests the caching behaviour; theArrayArguments_ShouldNotThrowtest case in particular guards the subtleTypedConstant.Valuethrow-on-array edge case.
Summary
The refactor is well-executed and the key goals (performance, correctness of incremental caching, reduced generated code size) are all achieved. The one item worth fixing before merge is the IsAccessibleType nested-type correctness gap. The hash ordering and internal visibility concerns are low-priority but worth tracking.
thomhurst
had a problem deploying
to
Pull Requests
GitHub Actions
Failure
thomhurst
had a problem deploying
to
Pull Requests
GitHub Actions
Failure
There was a problem hiding this comment.
Follow-up Review (commit e3e4aa1)
The latest commit directly addresses the two remaining concerns from the last review:
Resolved: IsAccessibleType nested-type fix
The fix is correct and well-placed. For public types, the method now:
- Validates generic type arguments (prevents emitting converters for
Wrapper<InternalType>) - Recursively checks the containing type (prevents emitting converters for
public Innerinsideprivate Outer)
The ordering is right — type-arg check comes before container check, so both conditions must pass, and returning false from the type-arg loop short-circuits correctly.
One minor observation: the Internal branch does not recurse into ContainingType — it only does an assembly GivesAccessTo check and returns. A nested internal type inside an inaccessible container would be incorrectly allowed through. However, this is a pre-existing edge case not introduced by this PR and not worth blocking on here.
Resolved: AotConverterHelper is now internal
Making it internal static is the right scope. It's called only from TestMetadataGenerator within the same assembly, and the internal visibility makes that contract explicit.
Remaining (pre-existing) minor concern: ComputeAttributeHash order-sensitivity
This was flagged as minor in the previous review and is not addressed here (nor should it block this PR). The XOR-based alternative would be more cache-stable, but the practical risk is negligible since Roslyn's GetAttributes() is stable within a compilation and the test suite covers the detection correctness.
Summary
All issues raised across the review cycle have been addressed:
| Issue | Status |
|---|---|
| Weak incremental caching tests | Resolved (commit 2) |
| Duplicate AOT registrations | Resolved (commit 3) |
| InheritsTestsClassMetadata over-caching | Documented with clear comment (commit 3) |
| IsAccessibleType generic type-arg gap | Resolved (commit 4) |
| Non-generic inherited tests missing AOT registration | Resolved (commit 4) |
| FuncAotConverter null guard for value types | Resolved (commit 4) |
| Dead code in ScanTypedConstantForTypes | Resolved (commit 4) |
| IsAccessibleType public nested type gap | Resolved (commit 5) |
| AotConverterHelper visibility | Resolved (commit 5) |
| ComputeAttributeHash order-sensitivity | Minor / pre-existing, does not block |
The PR is in good shape. The core goals — eliminating the CompilationProvider-scoped AOT generator, fixing ISymbol-based equality, and dramatically simplifying the generated output — are all achieved and well-tested. Good to merge.
thomhurst
changed the title
There was a problem hiding this comment.
Code Review: perf/inline-aot-converters
This is a substantial and well-executed performance improvement. Eliminating the runtime AotConverterRegistry lookup in favour of compile-time typed casts is exactly the right direction, especially for AOT compatibility. The net -361 lines (1063 deleted, 702 added) while preserving the public API surface is impressive.
Architecture Concerns
1. Constructor selection mismatch between SourceTypeAnalyzer and GetPrimaryConstructor
SourceTypeAnalyzer.GetConstructorParameterSourceTypes picks the constructor with the most parameters:
if (member is IMethodSymbol { MethodKind: MethodKind.Constructor, IsStatic: false, IsImplicitlyDeclared: false } ctor
&& (constructor == null || ctor.Parameters.Length > constructor.Parameters.Length))
{
constructor = ctor;
}But InstanceFactoryGenerator.GetPrimaryConstructor checks for [TestConstructor] first, then falls back to the most-public/most-parameters constructor. If a class has a [TestConstructor]-annotated 1-param constructor alongside a 2-param one, GetConstructorParameterSourceTypes analyses types based on the 2-param constructor's parameterCount, while the factory generates a call to the 1-param constructor. In most cases this is harmless (position 0 is still correct), but it's an internal inconsistency that could produce unexpected results for multi-constructor classes with [TestConstructor].
Suggested fix: Extract GetPrimaryConstructor to a shared location and use it in both places, so source-type analysis always targets the same constructor the factory will call.
2. Params-array optimization is incomplete and produces inconsistent generated code
The snapshot for ArgsAsArrayTests reveals this asymmetry:
// case 2: only the first element gets a typed cast
new string[] { (string)args[0], global::TUnit.Core.Helpers.CastHelper.Cast<string>(args[1]) }
// case 3: still only the first
new string[] { (string)args[0], global::TUnit.Core.Helpers.CastHelper.Cast<string>(args[1]), global::TUnit.Core.Helpers.CastHelper.Cast<string>(args[2]) }The root cause: GetMethodParameterSourceTypes sizes sourceTypes to parameterCount (1 for a params string[] method). So GetSourceTypeAt(sourceTypes, i) for i >= 1 always returns null, falling back to CastHelper.Cast. But all elements come from the same [Arguments("a", "b", "c")] attribute and have statically-known types.
The conceptual mismatch is: sourceTypes is indexed by parameter position, but params-array elements are indexed by argument position. One parameter can consume N arguments.
A possible fix would be to return a flat argument-position array from ExtractSourceTypesFromArguments (sized to max argument count across all [Arguments] instances) rather than a parameter-count-sized array — or have a separate path for params parameters that iterates across all argument positions.
This is an optimization gap, not a correctness bug, but it creates a visually confusing pattern in generated code that maintainers might find misleading.
3. Stale test class name
AotConverterGeneratorTests.GeneratesCode() is [Skip]ped and now delegates to TestMetadataGenerator.RunTest since the generator it tested was deleted. The class name is now misleading. Either rename the class to reflect what it's actually testing, or remove it entirely since the functionality moved into TestMetadataGeneratorIncrementalTests.
What Works Well
CastExpressionHelper.GenerateCastcorrectly handles the three cases (same type, compiler-resolvable conversion, unknown fallback) usingCompilation.ClassifyConversion— letting the C# compiler's own analysis drive the decision rather than reimplementing conversion logic.SourceTypeAnalyzer.ExtractSourceTypesFromArgumentscorrectly handles mixed-type[Arguments]rows (marks positions asnullwhen types are inconsistent across rows), null literals, and error constants.- Backward-compatible public API preservation:
AotConverterRegistry,IAotConverter,FuncAotConverter, andCastHelperremain for user-registered converters. The migration path is additive. - Comprehensive test coverage: 468 snapshot tests across 4 TFMs, plus targeted integration tests for the conversion scenarios.
- Incremental generator hygiene: The new
TestMetadataGeneratorIncrementalTestsproperly validates that unchanged method metadata doesn't trigger re-output — important for editor performance.
Summary
The core design is sound and the performance benefit is real. The two architectural issues (constructor selection inconsistency and params-array gap) are worth addressing before merge — especially the constructor one, since it's a latent correctness risk for the [TestConstructor] scenario. The params issue can be tracked as a follow-up if it's out of scope.
thomhurst
had a problem deploying
to
Pull Requests
GitHub Actions
Failure
There was a problem hiding this comment.
Follow-up Review (commit 1b092b7)
The single commit since the previous review updates platform-specific snapshot files for net8.0, net9.0, and net472 — six files in total, all .verified.txt. This is purely a tooling artifact from accepting snapshots generated on those TFMs and contains no logic changes.
Previous Review — Status
| Concern | Status |
|---|---|
Non-transitive comparator in SortMostDerivedFirst |
✅ Resolved (bd9e4d0) |
| Default arm used bare cast instead of CastHelper | ✅ Resolved (bd9e4d0) |
IsBoxing shortcut constrained to object/ValueType/Enum targets |
✅ Resolved (968bbb7) |
FullProjectGenerationTests rename |
✅ Resolved (6e4cd0b) |
| All other concerns from prior rounds | ✅ Resolved |
| Matrix attribute name-only checks (minor) |
Remaining Minor: Matrix attribute name-only checks
This was flagged in the previous review as non-blocking. For completeness: IsMatrixDataSourceAttribute and the per-parameter attribute name checks in ProcessMatrixDataSource still use name-only comparisons:
// SourceTypeAnalyzer.cs
private static bool IsMatrixDataSourceAttribute(AttributeData attr)
=> attr.AttributeClass?.Name is "MatrixDataSourceAttribute"; // no namespace check
foreach (var paramAttr in param.GetAttributes())
{
if (paramAttr.AttributeClass?.Name is not ("MatrixAttribute" or "MatrixMethodAttribute" or "MatrixRangeAttribute"))
continue;Compare with IsArgumentsAttribute, which validates both name and fully-qualified namespace:
private static bool IsArgumentsAttribute(AttributeData attr)
=> attr.AttributeClass?.Name is "ArgumentsAttribute"
&& attr.AttributeClass.ToDisplayString() == WellKnownFullyQualifiedClassNames.ArgumentsAttribute.WithoutGlobalPrefix;The practical risk is minimal — a user would have to define a custom attribute named exactly MatrixDataSourceAttribute for this to matter. That said, adding entries to WellKnownFullyQualifiedClassNames for the Matrix types and adding the ToDisplayString() guard would make this consistent and future-proof. Not blocking.
Snapshot Files
The six updated snapshot files (ConflictingNamespaceTests.MatrixTest_WithConflictingNamespace and Tests1889.Test for net4.7/net8/net9) are the expected diff from the matrix source type analysis added in 32999dc. They are consistent with the generated switch-cast pattern documented in the PR description and correct.
Overall
This PR is in excellent shape. All substantive concerns from the multiple review rounds have been resolved. The architecture — replacing runtime AotConverterRegistry lookups with compiler-verified typed casts generated at source-gen time — is the right long-term direction for both AOT compatibility and incremental build performance. The ITypedDataSourceAttribute<T> interface extension point means user-defined data sources automatically benefit without any action on their part.
The matrix attribute namespace check is the only remaining inconsistency, and it is minor enough that it could be addressed as a follow-up. Ready to merge.
There was a problem hiding this comment.
Follow-up Review (commit 1b092b7)
The single commit since the previous review updates platform-specific snapshot files for net8.0, net9.0, and net472 — six files total. This is purely a tooling artifact from accepting snapshots generated on those TFMs and contains no logic changes.
Previous Review — Status
All substantive concerns from the multiple review rounds remain resolved. The only previously noted outstanding item:
Matrix attribute name-only checks (minor, non-blocking)
IsMatrixDataSourceAttribute and the per-parameter attribute name checks in ProcessMatrixDataSource use name-only comparisons, unlike IsArgumentsAttribute which also validates the fully-qualified namespace via ToDisplayString(). The practical risk is minimal (a user would need to define a custom attribute named exactly MatrixDataSourceAttribute), but adding entries to WellKnownFullyQualifiedClassNames and matching the IsArgumentsAttribute pattern would make this consistent. Not blocking.
Snapshot Files
The six updated snapshot files (ConflictingNamespaceTests.MatrixTest_WithConflictingNamespace and Tests1889.Test for net4.7/net8/net9) are the expected diff from the matrix source type analysis added in 32999dc. They are consistent with the generated switch-cast pattern and correct.
Overall
This PR is in excellent shape. All substantive concerns have been addressed across multiple rounds of review. The architecture — replacing runtime AotConverterRegistry lookups with compiler-verified typed casts at source-gen time — is the right long-term direction for AOT compatibility and incremental build performance. Ready to merge.
…iles Eliminates the separate AotConverters.g.cs file and its pipeline branch (which used .Collect() — a fan-in that invalidated all converters when any test method changed). Converter registrations are now emitted inline in the same generated file as the test entries, using simple lambda-based AotConverterRegistry.Register<S,T>() calls. Key changes: - Merge AotConverterGenerator into TestMetadataGenerator as inline code - Replace IAotConverter class generation with one-line lambda registrations - Remove .Collect() fan-in for converters (was a perf anti-pattern) - Add string-based equality to TestMethodMetadata for incremental caching - Use existing DataSourceAttributeHelper instead of hand-rolled checks
Replace weak GeneratedTrees.Length assertions with proper TrackedSteps-based assertions that verify the pipeline step was actually cached/modified, matching the pattern used by DynamicTestsGeneratorIncrementalTests.
… caching - Skip AOT converter emission in per-method path for non-generic methods, since they are already covered by the per-class GroupMethodsByClass path. This eliminates the overlap for inherited non-generic test methods. - Document InheritsTestsClassMetadata equality design: upstream ForAttributeWithMetadataName caching prevents re-evaluation for base class attribute changes, so TypeFullyQualifiedName-only equality is safe.
- Fix IsAccessibleType to check type arguments of public generic types before returning true (e.g. Wrapper<InternalType> was incorrectly treated as accessible) - Include inherited tests (InheritanceDepth > 0) in per-method AOT registration path, since subclass constructors may need different converters than the base class - Add null guard in FuncAotConverter.Convert to handle null values for value-type conversions instead of throwing NullReferenceException - Remove dead TypedConstantKind.Array check in ScanTypedConstantForTypes (already excluded by prior else-if branch)
- IsAccessibleType now checks ContainingType for public nested types, preventing emission of converters for types like public Inner inside private Outer that are not actually accessible - Make AotConverterHelper internal (only used by TestMetadataGenerator)
…rRegistry from generated code Instead of scanning types for conversion operators and emitting AotConverterRegistry.Register<TSource, TTarget>() calls at compile time (which CastHelper.Cast<T> looks up at runtime), the source generator now analyzes [Arguments] attribute TypedConstants to determine source types and emits typed double-casts like (TargetType)(SourceType)args[i] that the C# compiler resolves directly. Dynamic data sources fall back to CastHelper.Cast<T>().
- Reuse InstanceFactoryGenerator.GetPrimaryConstructor in SourceTypeAnalyzer so source type analysis targets the same constructor (respecting [TestConstructor]) - Rename AotConverterGeneratorTests → FullProjectGenerationTests (stale name)
… handling - SourceTypeAnalyzer now sizes the source type array to max argument count (not parameter count) for params methods, so elements beyond position 0 get typed casts instead of CastHelper.Cast fallback - TupleArgumentHelper.GetParamsElementType now extracts element types from generic collection params (IEnumerable<T>, List<T>, etc.), not just T[], fixing a pre-existing bug where params IEnumerable<T> would try to cast a single element to the collection type instead of constructing an array
Numeric conversions are always classified as implicit or explicit, so the IsNumeric guard was unreachable.
Any conversion where IsImplicit or IsExplicit is true already implies Exists is true, making the Exists check redundant. Re: source type caching suggestion — the three GetMethodParameterSourceTypes call sites are on mutually exclusive code paths (per-class switch case, per-entry inline lambda, generic fallback), so a given method is only analyzed once per generation run. Caching in TestMethodMetadata would require storing ITypeSymbol which breaks incremental equality.
When source type is unknown for params elements beyond [Arguments] row length, default to the element type. Since SourceTypeAnalyzer already verified all data sources are [Arguments], these overflow positions use the same element type as target, producing (T)args[i] instead of CastHelper.Cast<T>(args[i]).
…entsAttribute check - When source type boxes to target (e.g., int → object), skip the cast entirely since args[i] is already object - Add namespace check to IsArgumentsAttribute to avoid matching user-defined attributes with the same name
Boxing-to-interface (e.g., int → IComparable) also sets IsBoxing=true, but returning bare args[i] would fail to compile since object? cannot be implicitly converted to an interface type. Restrict the shortcut to System.Object, System.ValueType, and System.Enum where args[i] is already the correct type.
- Explain why mixed data sources cause whole-method fallback - Skip already-inconsistent positions early in ExtractSourceTypesFromArguments - Remove firstRow variable (redundant with null check after early-continue) - Fix stale "use Math.Min" comment in dynamic count path
Tooling artifact from accepting received files on Windows — the test runner writes UTF-8 with BOM but the existing snapshots are plain ASCII.
- Remove leftover firstRow variable from ExtractSourceTypesFromArguments - Mix TypedConstant.Type into ComputeAttributeHash to distinguish int 1 from string "1" for incremental cache invalidation
- Use WellKnownFullyQualifiedClassNames for ArgumentsAttribute check instead of raw string literals - Use AsSpan().Fill(true) instead of manual loop for consistent[] init - Collapse 4 identical cast-loops into 2 using Math.Min for upper bound (argCount is int.MaxValue for dynamic count, making the branch redundant)
…taSource When a class has both [Arguments] and [ClassDataSource<T>] attributes, the generated InstanceFactory lambda is shared across all data source rows. A single static cast can't handle both argument types (e.g. int from Arguments and IntDataSource from ClassDataSource), causing InvalidCastException in AOT mode where CastHelper can't discover implicit conversions via reflection. Rewrite SourceTypeAnalyzer to collect ALL possible source types per parameter position across all data source attributes, using the ITypedDataSourceAttribute<T> marker interface (not hardcoded attribute names) so user custom data sources are supported. When multiple source types exist at a position, CastExpressionHelper generates a pattern- matching switch expression with per-type arms, compiling all necessary conversions at build time. Fixes DataSourceCountTests.ArgumentsWithClassDataSourceTests AOT failure.
…cast The DotNet8_0, DotNet9_0, and Net4_7 variants of the ConflictingNamespaceTests snapshot were not updated with the new switch expression pattern for enum casts, only DotNet10_0 was.
- Delete dead GetSourceTypeAt method - Collapse SourceTypeInfo to single GetTypes() method, removing HasSingleType/HasMultipleTypes/GetSingleType (callers use count check) - Change SourceTypeInfo from public to internal sealed - Change TupleArgumentHelper from public to internal - Eliminate GenerateElementCast dispatch duplication by adding fallbackSourceType parameter to GenerateCastForPosition - Remove wasteful .Where(IsArgumentsAttribute).ToList() allocation; GetMaxArgumentCount now filters inline - Simplify GenerateMultiSourceCast branch structure
Non-generic [ClassDataSource(typeof(T))] doesn't expose the source type to SourceTypeAnalyzer, causing fallback to CastHelper.Cast<T>() which can't discover implicit conversion operators in AOT mode (the deleted AotConverterGenerator previously auto-registered these). Convert all 7 affected test files to generic [ClassDataSource<T>] so the source type is statically known and the generator can emit typed casts. Also delete orphaned AotConverterGeneratorTests.GeneratesCode.verified.txt (its test class was renamed to FullProjectGenerationTests).
…ry comment When source and target types are identical in GenerateMultiSourceCast, emit `Type __s => __s` instead of `Type __s => (Type)__s` — the pattern match already binds the typed value, so the cast is redundant. Add comment explaining why GenerateMultiSourceCast uses single casts while GenerateCast uses double casts (pattern match performs the unbox).
…by inheritance DataSourceAttributeHelper used backtick notation (`1) for open generic matching, but InterfaceCache.GetGenericInterface compares against GloballyQualified() output which uses <> notation. This prevented the source generator from recognizing typed data source attributes, causing fallback to CastHelper.Cast<T>() which fails under AOT. Also sort multi-source switch arms most-derived-first to prevent CS8510 unreachable pattern errors when source types have inheritance relationships.
Address two review findings: - Default arm in multi-source switch now uses CastHelper.Cast<T>() instead of a bare cast, preventing NullReferenceException when args[i] is null for value-type targets. - Add deterministic string tiebreaker in SortMostDerivedFirst comparator to satisfy Introsort's total-order requirement for unrelated types.
…safe casts MatrixDataSourceAttribute was treated as an unknown data source in SourceTypeAnalyzer, causing all parameters to fall back to CastHelper.Cast. In AOT mode, CastHelper can't discover implicit operators via reflection, breaking tests like ImplicitConversion(TestEnum2 -> OneOf<TestEnum, TestEnum2>). Now ProcessMatrixDataSource examines per-parameter [Matrix] attributes to extract compile-time value types, enabling typed switch arms in generated code. Auto-generated parameters without [Matrix] use the parameter type directly, except for enums (Enum.GetValuesAsUnderlyingType boxes as int, not enum type).
There was a problem hiding this comment.
Code Review — Follow-up Pass
This is a second look after the previous review. Good news: all three issues raised previously have been addressed. Summary below.
Previous issues — resolved ✅
1. Duplicate AOT registrations
Fully resolved. The entire AotConverterRegistry.Register(new AotConverter_N()) pattern is gone. Generated code no longer emits registration calls at all — typed casts are inlined directly at call sites. The registry only exists now for user-provided FuncAotConverter scenarios.
2. Weak incremental caching tests
Fixed properly. The new TestMetadataGeneratorIncrementalTests uses AssertRunReasons(driver2, IncrementalGeneratorRunReasons.Cached) — the correct assertion that verifies the pipeline step actually cached, not just that output exists. The ChangeTestAttribute_ShouldRegenerate test verifying the positive case (re-run on change) is a nice addition.
3. FuncAotConverter null safety
Fixed: value is null ? null : converter((TSource)value). No null-reference risk for value-type TSource conversions.
InheritsTestsClassMetadata equality — acceptable design decision
The equality using only TypeFullyQualifiedName now has a detailed inline comment explaining the tradeoff and the reliance on Roslyn's upstream invalidation for base class attribute changes. The comment is accurate and sufficient — this is an acceptable design decision given how ForAttributeWithMetadataName works.
New observations
Pattern variable naming in multi-source switch expressions
The generated switch arms use __s as the pattern variable:
(args[0] switch {
global::TestEnum __s => (global::TestEnum)__s,
int __s => (global::TestEnum)__s,
_ => (global::TestEnum)args[0]
})Each arm's __s is correctly scoped and can't conflict between arms. The only theoretical risk is shadowing an outer-scope variable named __s, but since this is emitted into a generated lambda body that contains nothing else, there's no conflict in practice. Acceptable as-is, though a prefixed name like __tunit_s would be zero-risk.
Boxing special case in GenerateCast is correctly handled
The comment in CastExpressionHelper:
"Do NOT skip the cast for boxing-to-interface (e.g.
int → IComparable) —args[i]isobject?, which cannot be implicitly converted to an interface type."
This is a subtle correctness invariant. The implementation correctly gates the no-cast path on SpecialType.System_Object | System_ValueType | System_Enum only. Good catch.
SortMostDerivedFirst handles unrelated types gracefully
The sort falls back to string.Compare for types that are neither base/derived of each other. This is a correct total-order tiebreaker — Introsort requires it, and string.Compare on fully-qualified names is deterministic across compilations. No issue here.
SourceTypeInfo per-position design
Using a IReadOnlyList<ITypeSymbol>?[] indexed by parameter position is clean — null means "unknown/fallback", single element means "typed cast", multiple elements means "switch expression". The (uint)index < (uint)_typesPerPosition.Length bounds check avoids a branch for the common case. Well-designed.
Summary
All three previously raised concerns are resolved. The architectural direction — eliminating compile-time type scanning and inlining typed casts — is correct and well-executed. The snapshot diff confirms the generated code reduction is real (1376 lines of AOT converter boilerplate removed, per-method invocation code is cleaner).
This looks ready to merge.
1 participant
Summary
[Arguments]+[ClassDataSource<T>]) by generating pattern-matching switch expressions with per-type arms, so a single shared lambda can handle multiple runtime source typesITypedDataSourceAttribute<T>marker interface to discover source types — works with built-in and user-defined custom data sources, no hardcoded attribute namesCastHelper.Cast<T>()for dynamic data sources ([MethodDataSource], non-generic[ClassDataSource], etc.) where source types can't be determined at compile timeAotConverterHelper.cs(338 lines) — eliminates compile-time type scanning for conversion operators and runtimeConcurrentDictionarylookups inAotConverterRegistryAotConverterRegistry,IAotConverter,FuncAotConverter, andCastHelperremain for users who register converters manuallyBreaking change (AOT + dynamic data sources + custom operators)
If you combine
[MethodDataSource], non-generic[ClassDataSource(typeof(T))], or other non-statically-typed data sources with custom implicit/explicit conversion operators in Native AOT mode, you must now manually register converters:Previously, the source generator automatically scanned for and registered these converters at compile time. This auto-registration has been removed because the typed cast approach makes it unnecessary for the vast majority of cases.
Recommended fix: Migrate to
[ClassDataSource<T>](generic form) wherever possible — this gives the source generator full type information and generates correct typed casts without any runtime registry.This does not affect:
[Arguments(...)]— types are always known from the typed constants[ClassDataSource<T>](generic form) — type is known from the generic parameterITypedDataSourceAttribute<T>— type is known from the interfaceCastHelper.Cast<T>()uses reflection which works fine at runtimeNew/rewritten files
CastExpressionHelper.cs— usesCompilation.ClassifyConversionto decide between typed cast, multi-source switch, and CastHelper fallbackSourceTypeAnalyzer.cs— extracts compile-time source types from all data source attributes per parameter position; supports[Arguments](viaTypedConstant),ITypedDataSourceAttribute<T>(via generic type args), and unknown sources (fallback)Key changes
TupleArgumentHelper.cs— method invocation args useCastExpressionHelper.GenerateCastForPosition()instead of always emittingCastHelper.Cast<T>()InstanceFactoryGenerator.cs— constructor parameter casts use typed casts when source types are knownTestMetadataGenerator.cs— wires source type analysis into all invoke code generation paths; removes AOT converter infrastructureClassTestGroup.cs— removesAotConverterRegistrationCodepropertyGenerated code examples
Test plan
ArgumentWithImplicitConverterTests,BasicTests,DataDrivenTests,NumberArgumentTests,MatrixTests,StringArgumentTests,ClassConstructorTest[Arguments]+[ClassDataSource<T>]scenario generates correct switch expressions (fixesDataSourceCountTests.ArgumentsWithClassDataSourceTestsAOT failure)[MethodDataSource]and other dynamic sources correctly fall back toCastHelper.Cast<T>()