perf: reduce source-gen JIT overhead via metadata factory helpers

[thomhurst]

Summary

• Add TestMetadataFactory.Create<T>() and MethodMetadataFactory.Create() factory helpers to TUnit.Core
• Modify source generator to emit compact factory calls (~5 lines) instead of inline TestMetadata<T> object initializers (~38 lines each)
• Hoist shared __classMetadata and __classType locals to the top of each GetTests() method, avoiding redundant construction per test method
• Fold UseRuntimeDataGeneration(testSessionId) into the factory as an optional testSessionId parameter

Motivation

Each per-class GetTests() method previously constructed TestMetadata<T> objects inline for every test method. For a class with 10 tests, this produced ~20KB of native code per method. With 1,000 test classes, that's ~20MB of JIT-compiled native code just for metadata construction.

A generic factory method Create<T>() where T : class gets JIT'd once by the .NET runtime (reference types share native code), replacing per-class inline initializers with calls to a shared factory.

Benchmark (10,000 tests, net10.0)

Mode	Before	After	Improvement
Source-gen (cold)	5.42s	4.26s	21% faster
Source-gen (warm)	3.30s	2.64s	20% faster
Reflection (reference)	2.00s	2.00s	n/a

Gap to reflection mode cut in half: 1.3s → 0.64s.

Test plan

• Full solution builds with 0 errors across all TFMs (netstandard2.0, net8.0, net9.0, net10.0)
• Source generator snapshot tests pass (456 passed, 4 skipped, 0 failed)
• Public API snapshot tests pass (12/12)
• Runtime verification: BasicTests, DataDrivenTests, MethodDataSource, PropertySetter, DependsOn tests all pass
• Tested on both net8.0 and net10.0

[claude]

Code Review: perf/metadata-factory-helpers

This is a well-motivated performance optimization. The core insight — that Create<T>() where T : class gets JIT'd once for all reference-type instantiations — is sound and the 20-21% cold-start improvement is meaningful. The PR is generally in good shape, but there are a few issues worth addressing.

---

🔴 Architecture Concern: String Parsing Hack in PreGeneratePropertyInjectionsExpression

The most significant problem is in TestMetadataGenerator.cs, PreGeneratePropertyInjectionsExpression:

// Use the existing method to generate, then extract the expression
var writer = new CodeWriter(includeHeader: false);
GeneratePropertyInjections(writer, typeSymbol, className);
var output = writer.ToString().Trim();

// Output format: "PropertyInjections = <expression>,"
// Strip the prefix and trailing comma
const string prefix = "PropertyInjections = ";
if (!output.StartsWith(prefix))
    return null;

var expr = output[prefix.Length..].TrimEnd();
if (expr.EndsWith(","))
    expr = expr[..^1].TrimEnd();

if (expr.Contains("Array.Empty<"))
    return null;

This is fragile output-format parsing — the method generates a string using one code path, then manually strips the prefix and trailing comma from that string. This creates invisible coupling between GeneratePropertyInjections's output format and this parser. If anyone ever changes the output format of GeneratePropertyInjections (say, the assignment format, whitespace, or the specific Array.Empty< sentinel), this silently produces wrong results.

Better approach: The PreGeneratePropertyDataSourcesExpression pattern is the right model — build the expression directly in a CodeWriter without reparsing existing methods. Either:

1. Refactor GeneratePropertyInjections to return the expression string directly (separate the "is empty?" check from the "generate expression" logic), or
2. Duplicate the generation logic (it's not large) to build the expression independently

The Array.Empty< string sniffing as a "return null" sentinel is particularly concerning — it ties correctness to an implementation detail of a different method.

---

🟡 Double Type Hierarchy Traversal in PreGeneratePropertyDataSourcesExpression

Both PreGeneratePropertyDataSourcesExpression and PreGeneratePropertyInjectionsExpression traverse the entire class hierarchy twice: once to check if any qualifying properties exist, then again (after clearing the set) to generate the output. This is O(2n) when O(n) is possible:

// First pass: detect existence
while (tempType != null) { /* traverse */ }
if (processedProperties.Count == 0) return null;

processedProperties.Clear();  // throw away the work

// Second pass: generate
while (currentType != null) { /* traverse again */ }

Better approach: Collect the relevant (IPropertySymbol, AttributeData) pairs in a single pass into a list, then return null if empty or generate the expression from the list.

---

🟡 Library.props: Overly Broad AD0001 Suppression

<NoWarn Condition="'$(TargetFramework)' == 'net9.0'">$(NoWarn);AD0001</NoWarn>

This suppresses all AD0001 analyzer crash diagnostics for every project targeting net9.0. AD0001 is the catch-all "analyzer threw an exception" warning, so this could hide unrelated analyzer bugs silently. Consider adding a link to the tracked SDK issue in the comment so future maintainers know when it's safe to remove, and ideally scope the suppression to only the affected project(s) rather than the shared Library.props.

---

🟡 filePath Always Emitted Even When Empty

In both PreGenerateMetadataBlock and PreGenerateMaterializerMethod:

// Always emit filePath (it's almost never empty)
writer.AppendLine(",");
writer.AppendLine($"filePath: @\"{filePath}\",");
writer.Append("testSessionId: testSessionId");

The comment says "almost never empty", but the snapshot output shows filePath: @"" being emitted when there is no file path. Since the factory default for filePath is "", emitting filePath: @"" explicitly is dead weight in the generated code. The conditional should be:

if (!string.IsNullOrEmpty(filePath))
{
    writer.AppendLine(",");
    writer.AppendLine($"filePath: @\"{filePath}\",");
}
writer.Append("testSessionId: testSessionId");

This is a minor but easy win that makes the generated code slightly smaller and more honest.

---

✅ What's Done Well

• The JIT sharing insight is correct and well-documented. The comment in TestMetadataFactory.cs explaining why this works (reference types share JIT-compiled native code) is excellent — this is non-obvious and the explanation will help future contributors.
• Hoisting __classMetadata and __classType avoids reconstructing the ClassMetadata per test method, which is a real win for classes with many tests.
• Optional-parameter pattern for rare fields (dependencies, data sources, repeat count) keeps the common case compact while supporting all configurations.
• Snapshot tests all pass — 456 passing with correct .verified.txt files committed. The behavioral equivalence with the old inline initializer approach is well-validated.
• UseRuntimeDataGeneration folded cleanly into the factory's testSessionId parameter. The ?? Guid.NewGuid().ToString() fallback correctly matches the old TestSessionId { get; set; } = Guid.NewGuid().ToString() default.
• MetadataGenerationHelper public extensions (GenerateClassMetadataGetOrAddWithParentExpression, GenerateParameterMetadataArrayForMethodExpression) are clean additions with proper XML docs.
• AOT suppressions are justified — the [UnconditionalSuppressMessage] attributes have accurate justifications since these factories are only ever called from source-generated code with concrete types.
• Benchmark numbers are compelling — cutting the reflection-vs-source-gen gap in half (1.3s → 0.64s) is a real UX improvement for large test suites.

---

Summary

The two main architectural issues to address before merging:

1. Replace the string-parsing hack in PreGeneratePropertyInjectionsExpression with direct expression generation
2. Eliminate the double-traversal pattern in the property helper methods

The Library.props and filePath issues are lower priority but worth fixing while the code is being touched.

[claude]

Code Review — perf: reduce source-gen JIT overhead via metadata factory helpers

This is a well-motivated, well-benchmarked performance improvement. The core insight — that a single generic factory method Create<T>() where T : class gets JIT'd once for all reference types, eliminating per-class specialisation of ~38-line object initialisers — is correct and the 20–21% startup improvement backs it up nicely. The secondary win of hoisting __classMetadata / __classType out of the per-test loop is clean. Good work overall.

---

Issues found

1. Indentation inconsistency in generated __Materialize_* methods (bug, cosmetic)

In PreGenerateMaterializerMethod, the ClassMetadata object-initialiser fields are rendered at the wrong indent level:

// __Materialize_* (WRONG — 4-space indent for fields)
        var __classMetadata = global::TUnit.Core.ClassMetadata.GetOrAdd(..., new global::TUnit.Core.ClassMetadata
    {
    Type = typeof(...),
    ...
    });

// GetTests() (correct — 8-space indent for fields)
        var __classMetadata = global::TUnit.Core.ClassMetadata.GetOrAdd(..., new global::TUnit.Core.ClassMetadata
        {
        Type = typeof(...),
        ...
        });

Root cause: GenerateClassMetadataGetOrAddWithParentExpression(testMethod.TypeSymbol, writer.IndentLevel) in PreGenerateMaterializerMethod passes the outer writer.IndentLevel (which is 2 = 8 spaces, inside the method body) to a fresh inner CodeWriter. However, when the resulting multi-line string is embedded as $"var __classMetadata = {classMetadataExpr};" the first line is placed after var __classMetadata = on the same line, so subsequent lines are indented relative to column 0 of the inner writer — not relative to the outer indentation context. PreGenerateSharedLocals uses writer.IndentLevel of a fresh writer (indentLevel = 0), so there's no mismatch there.

This is purely cosmetic (the code compiles and runs fine), but it makes the generated output visually inconsistent and harder to debug.

Suggested fix: in PreGenerateMaterializerMethod, either pass 0 (instead of writer.IndentLevel) to match how PreGenerateSharedLocals works, or adjust the inner CodeWriter to start at 0 and let the outer writer handle indentation of the whole block.

---

2. Public factory API has no discoverability guard

TestMetadataFactory and MethodMetadataFactory are both public — meaning they appear in Intellisense for every TUnit user — but they are purely infrastructure for the source generator. Nothing prevents a user from calling TestMetadataFactory.Create directly (possibly with incorrect arguments) or being confused by its presence.

Suggested improvement (minor but nice-to-have):

[EditorBrowsable(EditorBrowsableState.Never)]
[Obsolete("Infrastructure only — called by TUnit's source generator. Do not use directly.")]
public static class TestMetadataFactory { ... }

This hides the types from Intellisense autocompletion while keeping them accessible from generated code in user assemblies (where internal wouldn't work without InternalsVisibleTo). The Obsolete message also serves as documentation for anyone who stumbles across them via reflection or browsing the API.

---

3. TestMetadata<T>.TestSessionId — factory falls back to Guid.NewGuid() when testSessionId is null

// TestMetadataFactory.cs
TestSessionId = testSessionId ?? Guid.NewGuid().ToString(),

This is a behavioural parity with the old UseRuntimeDataGeneration, but it means that any future call-site where testSessionId is accidentally null (e.g., a new code path in the generator) will silently produce a random GUID rather than surfacing an obvious error. The non-nullable string testSessionId parameter in the generated call gives confidence the compiler will catch it at generator-time, but the runtime fallback is a silent trap.

Consider accepting string testSessionId (non-nullable, no default) to make the call-site contract explicit, or at least ArgumentNullException.ThrowIfNull(testSessionId) before the null-coalescing. The generated code always provides a real session ID today, so the Guid.NewGuid() branch should never execute in practice.

---

Observations (no action required)

• Single-pass property injection — the refactor of GeneratePropertyInjections → GeneratePropertyInjectionsExpression is a clean improvement; the old double-pass (detect → reset → generate) was a code smell waiting for a bug.

• testName and testMethodName duplicated in factory call — in virtually all generated call sites these two strings are identical (e.g., "Test1", "Test1"). The separation is intentional (display names can differ), but it makes the positional call-site slightly noisy. Not a problem worth changing now.

• AD0001 suppression in Library.props — well-documented with upstream dotnet/runtime issue links. Good practice.

• ClassTestGroup.SharedLocalsCode required property — clean model-level change. Since ClassTestGroup is source-generator–internal, the required keyword enforces correctness at compile time.

---

Summary

The performance motivation is solid and the implementation is largely clean. The two items worth addressing before merge are:

1. The indentation inconsistency in generated __Materialize_* blocks (purely cosmetic but visible in diffs).
2. Adding [EditorBrowsable(EditorBrowsableState.Never)] / [Obsolete] to the two factory classes to keep the public API surface tidy.

Neither is a blocker, but both are low-effort to fix.

[claude]

Code Review — perf: reduce source-gen JIT overhead via metadata factory helpers

The original commit is a well-motivated and well-benchmarked optimisation. The two follow-up commits (7a18dfc4, ca4f28e8) address several issues raised by earlier reviews cleanly:

• ✅ String-parsing hack replaced with direct expression generation in GeneratePropertyInjectionsExpression
• ✅ Double type-hierarchy traversals collapsed into single-pass collection
• ✅ Conditional filePath emission (no longer emits filePath: @"" when empty)
• ✅ testSessionId made required — Guid.NewGuid() silent fallback removed
• ✅ AD0001 suppression comment extended with upstream dotnet/runtime issue links

Two items from the earlier reviews remain open:

---

🔴 Indentation inconsistency in __Materialize_* generated code (still present)

The __classMetadata initialiser in __Materialize_* methods is still at the wrong indent level:

// __Materialize_* (wrong — { and fields at 4-space indent)
        var __classMetadata = global::TUnit.Core.ClassMetadata.GetOrAdd(..., new global::TUnit.Core.ClassMetadata
    {
    Type = typeof(global::TUnit.TestProject.AbstractTests.ConcreteClass2),
    ...
    });

// GetTests() (correct — { and fields at 8-space indent, matching method body)
        var __classMetadata = global::TUnit.Core.ClassMetadata.GetOrAdd(..., new global::TUnit.Core.ClassMetadata
        {
        Type = typeof(global::TUnit.TestProject.AbstractTests.ConcreteClass2),
        ...
        });

Root cause is in PreGenerateMaterializerMethod: it calls MetadataGenerationHelper.GenerateClassMetadataGetOrAddWithParentExpression(testMethod.TypeSymbol, writer.IndentLevel), passing the outer writer's current indent level (2 = 8 spaces) to the inner CodeWriter. But when that multi-line string is embedded via writer.AppendLine($"var __classMetadata = {classMetadataExpr};"), only the first line aligns with the outer indent context — subsequent lines are at the inner writer's absolute indent level rather than relative to the outer indentation.

PreGenerateSharedLocals (used in GetTests()) works correctly because it uses writer.IndentLevel of a fresh writer at indent 0, so the generated string is already at baseline and gets indented correctly when the outer writer appends it.

Fix: pass 0 instead of writer.IndentLevel in PreGenerateMaterializerMethod:

// Before
var classMetadataExpr = MetadataGenerationHelper.GenerateClassMetadataGetOrAddWithParentExpression(testMethod.TypeSymbol, writer.IndentLevel);

// After
var classMetadataExpr = MetadataGenerationHelper.GenerateClassMetadataGetOrAddWithParentExpression(testMethod.TypeSymbol, 0);

The generated code compiles and runs correctly regardless (C# doesn't care about whitespace), but the inconsistency appears across every .verified.txt snapshot and will continue showing up in diffs.

---

🟡 Factory classes are public with no discoverability guard

TestMetadataFactory and MethodMetadataFactory must be public so generated code in user assemblies can call them, but they're pure infrastructure and should not appear in Intellisense for end users. They remain without any discoverability suppression:

// Suggested addition to both factory classes
[System.ComponentModel.EditorBrowsable(System.ComponentModel.EditorBrowsableState.Never)]
[Obsolete("Infrastructure only — called by TUnit source generator. Do not use directly.", error: false)]
public static class TestMetadataFactory { ... }

This hides them from Intellisense autocompletion while keeping them accessible from generated code, and provides a clear signal to anyone who finds them via reflection or API browsing. (Non-blocking — the same omission exists for other infrastructure types in TUnit.Core — but worth adding while touching this file.)

---

Summary

The core design is sound and all significant functional issues have been addressed in the follow-up commits. The indentation bug is cosmetic (generated code is functionally correct) and the EditorBrowsable suggestion is a nice-to-have. Both are easy fixes if you want a clean final state before merge.

[claude]

Code Review: perf/metadata-factory-helpers (Follow-up)

The previous review's main concerns have been addressed. Here's a summary of what's fixed and what needs attention.

---

✅ Previous Issues Addressed

String-parsing hack in PreGeneratePropertyInjectionsExpression — Fixed. The method now uses single-pass collection into qualifyingProperties, generates the expression directly, and no longer does Array.Empty< string-sniffing as a sentinel. Clean.

Double type-hierarchy traversal — Fixed in both PreGeneratePropertyDataSourcesExpression and GeneratePropertyInjectionsExpression. Both now collect into a List<(...)> in one pass and return null if empty before generating output.

filePath always emitted — Fixed. Both PreGenerateMetadataBlock and PreGenerateMaterializerMethod now gate on !string.IsNullOrEmpty(filePath), so the common case (empty path in tests) produces cleaner output.

---

🔴 Bug: Public API Snapshot Does Not Match Source

The TUnit.PublicAPI/Tests.Core_Library_Has_No_API_Changes.*.verified.txt snapshots show:

public static .TestMetadata<T> Create<T>(
    ...
    .MethodMetadata methodMetadata,
    string filePath = ,           // <-- immediately after methodMetadata
    int inheritanceDepth = 0,
    ...
    string? testSessionId = null)   // <-- last parameter, optional, nullable

But the actual TUnit.Core/TestMetadataFactory.cs source declares:

public static TestMetadata<T> Create<T>(
    ...
    MethodMetadata methodMetadata,
    string testSessionId,           // <-- required, position 8, before filePath
    string filePath = ,
    ...
)

These are fundamentally different APIs: testSessionId is required at position 8 in the source but optional/nullable/last in the snapshot. The PR description says the intent was to fold it in as an "optional testSessionId parameter", which matches the snapshot — but the code contradicts that.

This will cause the Public API snapshot tests to fail in CI. The snapshots need to be regenerated once the source is in its final form. If testSessionId is truly meant to be required here (since every call site always passes it), then the snapshots need updating. If it was meant to be optional, the source needs updating.

---

🟡 Minor: Generated ClassMetadata Initializer Loses Indentation

In the generated GetTests() output (visible in all snapshot diffs), the ClassMetadata initializer properties have no indentation relative to the {:

var __classMetadata = global::TUnit.Core.ClassMetadata.GetOrAdd("...", new global::TUnit.Core.ClassMetadata
{
Type = typeof(global::TUnit.TestProject.BasicTests),        // <-- should be indented
TypeInfo = new global::TUnit.Core.ConcreteType(...),

This happens because PreGenerateSharedLocals creates an outer CodeWriter at indent level 0, then passes writer.IndentLevel to GenerateClassMetadataGetOrAddWithParentExpression, which generates the block starting at level 0. When the multi-line result is inlined into var __classMetadata = {classMetadataExpr}; in the outer writer (which writes at level 2 inside the method body), the continuation lines don't inherit the outer indentation.

The fix is straightforward: pass writer.IndentLevel + 1 (or however many levels the inline expression continues at) so the inner properties are indented relative to the opening {. It's not a functional bug but makes the generated code harder to read/debug.

---

✅ Architecture and Optimization Quality

The dual-path design is clean:

• Non-generic, non-inherited tests → GeneratePerClassTestSource (new factory path) — early-returns from GenerateTestMethodSource via the guard at line 302
• Generic types, generic methods, inherited tests → GenerateTestMethodSource (old inline path)

The JIT sharing claim is correct: Create<T>() with a class constraint gets JIT-compiled once for all reference-type instantiations. The hoisting of __classMetadata and __classType prevents redundant ClassMetadata.GetOrAdd calls (which involve string hashing) for every test in the class. The benchmarks are credible.

The AD0001 suppression in Library.props is scoped to net9.0 only and has referenced issue links — acceptable as a workaround for a known SDK bug.

---

Summary: Fix the public API snapshot inconsistency (the testSessionId parameter position/optionality) and optionally address the indentation cosmetic. Everything else looks solid.

[claude]

Code Review — perf: reduce source-gen JIT overhead via metadata factory helpers (5th commit follow-up)

The 5th commit (16d3ade1) fixes the remaining indentation issue from the previous review. Here's where things stand:

---

✅ All Previous Issues Now Resolved

Issue	Status
String-parsing hack in property injections	✅ Fixed (commit 2)
Double type-hierarchy traversal	✅ Fixed (commit 2)
Conditional filePath emission	✅ Fixed (commit 2)
testSessionId made required (no Guid.NewGuid() fallback)	✅ Fixed (commit 3)
ClassMetadata field indentation in __Materialize_* methods	✅ Fixed (commits 4 & 5)
[EditorBrowsable(Never)] on factory classes	✅ Fixed (commit 4)

---

🔴 Still Open: Public API Snapshot Mismatch

This was flagged in the previous review and is still present. The TUnit.PublicAPI/Tests.Core_Library_Has_No_API_Changes.*.verified.txt snapshots record the Create<T> signature as:

.MethodMetadata methodMetadata,
string filePath = ,          // ← position 8, optional
int inheritanceDepth = 0,
...
string? testSessionId = null   // ← last parameter, optional/nullable

But the actual TestMetadataFactory.cs source (unchanged in commits 3–5) declares:

MethodMetadata methodMetadata,
string testSessionId,          // ← position 8, REQUIRED, non-nullable
string filePath = ,
int inheritanceDepth = 0,
...
// no testSessionId at the end

These are materially different APIs. The snapshot was generated against an older shape of the method (before commit 3 made testSessionId required) and was never regenerated. The Public API snapshot tests will fail in CI when they compare the recorded signature against the live binary.

Fix: regenerate the public API snapshots after the source is in its final form by running the snapshot test with VERIFY_SNAPSHOTS=update (or your project's equivalent accept-all command), then commit the updated *.verified.txt files.

---

Summary

The implementation is solid and all functional concerns from earlier reviews have been addressed. Only one item remains before merge: regenerate the public API snapshots to reflect the current string testSessionId required parameter position in TestMetadataFactory.Create<T>.

[claude]

Code Review: Follow-up on perf/metadata-factory-helpers

This revision has addressed all three items from the previous review. Here's the status:

---

✅ All Previous Issues Resolved

String-parsing hack — Fixed. GeneratePropertyInjectionsExpression now does a clean single-pass collection of qualifying properties and generates the expression directly. No more fragile output-format sniffing.

Double type hierarchy traversal — Fixed. Both PreGeneratePropertyDataSourcesExpression and GeneratePropertyInjectionsExpression now collect qualifying (Property, ...) pairs in a single pass, then return null if empty or generate from the collected list.

filePath always emitted when empty — Fixed. There's now a proper if (!string.IsNullOrEmpty(filePath)) guard before emitting the filePath: named argument, matching the snapshot tests.

Library.props AD0001 comment — The GitHub issue links (dotnet/runtime#109352 and dotnet/runtime#120728) are now in the comment, which is exactly what was requested. The suppression remains global to all net9.0 targets in Library.props, but given this is a genuine SDK-level bug affecting any project that opts into AOT analysis on net9.0, that scope is arguably correct.

---

🟡 Remaining Concern: PreGeneratePropertyInjectionsExpression Is a Redundant Wrapper

After the refactor, this method is now a one-liner:

private static string? PreGeneratePropertyInjectionsExpression(INamedTypeSymbol typeSymbol, string className)
{
    return GeneratePropertyInjectionsExpression(typeSymbol, className);
}

It's called in exactly one place. This wrapper adds no abstraction value and creates a ghost in the call stack — if someone later wants to understand what PropertyInjections generation does, they have to trace through an extra indirection. Consider inlining the call at the single call site:

var propInj = GeneratePropertyInjectionsExpression(testMethod.TypeSymbol, className);

Minor, but worth cleaning up since it's a leftover from the prior string-parsing architecture.

---

🟡 Dual Write*/Generate* Pattern in MetadataGenerationHelper.cs

MetadataGenerationHelper.cs now contains parallel implementations for the same operations:

• WriteMethodMetadata(ICodeWriter writer, IMethodSymbol, INamedTypeSymbol)
• GenerateMethodMetadata(IMethodSymbol, string classMetadataExpression, int indentLevel) — labelled "for backward compat"
• WriteParameterMetadataArrayForMethod + GenerateParameterMetadataArrayForMethod
• WriteParameterMetadataArrayForConstructor + GenerateParameterMetadataArrayForConstructor
• WritePropertyMetadataArray + GeneratePropertyMetadataArray

The "backward compat" comment is concerning — if these methods are only used within the generator's own source, there's no external API compatibility to maintain. The dual pattern means that two implementations of the same logic exist and can drift. The root cause seems to be that WriteMethodMetadata takes an INamedTypeSymbol (so it can recurse for class metadata), while GenerateMethodMetadata takes a pre-generated classMetadataExpression string.

Better approach: The cleanest resolution is to make the Write* path universal by supporting both cases — e.g., have WriteClassMetadataGetOrAddWithParent accept an overload that writes directly rather than serializing to a string first. The current recursive writer pattern in WriteClassMetadataGetOrAddWithParent already does this internally:

// Line 157: creates intermediate string writer just to pass to another method
var parentWriter = new CodeWriter("", includeHeader: false).SetIndentLevel(writer.IndentLevel);
WriteClassMetadataGetOrAddWithParent(parentWriter, typeSymbol.ContainingType);
WriteClassMetadataGetOrAdd(writer, typeSymbol, parentWriter.ToString());

This intermediate serialization-then-pass-as-string pattern leaks through the design. Not a blocking issue, but worth tracking as tech debt.

---

✅ What's Done Well

The core changes are well-executed:

• The __classMetadata and __classType hoisting via PreGenerateSharedLocals is clean — the shared locals are pre-computed once per class group at analysis time and emitted verbatim, avoiding ClassMetadata.GetOrAdd calls on every test in the class.
• GenerateMethodMetadataFactoryCall using StringBuilder directly (rather than CodeWriter) is appropriate for a single-line expression generation.
• The attribute body deduplication in GroupMethodsByClass (using bodyToIndex/distinctBodies) is a nice additional optimization beyond the original PR description.
• [EditorBrowsable(EditorBrowsableState.Never)] on both factory types is the right call — these are infrastructure types that generated code calls, not user-facing API.
• The testSessionId is now a required, non-nullable parameter (no Guid.NewGuid() fallback), which correctly makes the implicit contract explicit.

The benchmark numbers (21% cold-start improvement, gap to reflection cut in half) make this PR worthwhile. The remaining concerns are tech debt rather than correctness issues. LGTM with the minor cleanup suggestions.

[claude]

Code Review — perf/metadata-factory-helpers (Final follow-up)

This follow-up picks up from where the previous review left off (LGTM with minor cleanup suggestions). Here is the status of the two outstanding items.

---

✅ PreGeneratePropertyInjectionsExpression redundant wrapper — Resolved

The previous review flagged PreGeneratePropertyInjectionsExpression as a one-liner that added no abstraction value. It has been eliminated: GeneratePropertyInjections now calls GeneratePropertyInjectionsExpression directly, and the expression method itself does the full single-pass collection + generation. The call chain is clean.

---

🟡 Dual Write*/Generate* pattern in MetadataGenerationHelper.cs — Still present, still tech debt

MetadataGenerationHelper now exposes:

• WriteClassMetadataGetOrAddWithParent (writes to a CodeWriter)
• GenerateClassMetadataGetOrAddWithParentExpression (returns a string)
• WriteParameterMetadataArrayForMethod / GenerateParameterMetadataArrayForMethodExpression

These are parallel implementations of the same logic. This was flagged in the previous review and noted as tech debt. It hasn't been addressed, and that's a reasonable call — it's a design smell, not a correctness issue. Worth tracking for a follow-up cleanup but not a blocker here.

---

✅ All Previous Issues Fully Resolved

Issue	Status
String-parsing hack in property injections	✅ Fixed
Double type-hierarchy traversal	✅ Fixed
Conditional filePath emission in factory calls	✅ Fixed
testSessionId made required (no Guid.NewGuid() fallback)	✅ Fixed
ClassMetadata field indentation in __Materialize_* methods	✅ Fixed
[EditorBrowsable(EditorBrowsableState.Never)] on factory classes	✅ Fixed
Public API snapshot mismatch	✅ Fixed
Redundant PreGeneratePropertyInjectionsExpression wrapper	✅ Fixed

---

Observations (no action needed)

• FilePath = @"" in EnumerateTestDescriptors — The empty filepath is still emitted unconditionally in TestDescriptor object initializers (not the factory call). This wasn't part of the factory optimization scope and is consistent with the old code path. Low-priority follow-up if desired.
• testName / testMethodName positional duplication — Factory calls still pass both as identical strings in nearly all cases (e.g., "Test1", "Test1"). The separation is intentional (display names can differ). Not worth changing.
• Snapshot coverage is thorough — 456 snapshot tests passing across all TFMs, including edge cases like matrix tests, async data sources, property setters, and inherited tests.

---

Summary

All functional and architectural concerns raised across the previous review iterations have been addressed. The implementation is clean, the benchmark improvement (21% cold-start, gap to reflection halved) is well-validated, and the generated code is readable. The only remaining item is the Write*/Generate* dual-pattern tech debt which is a known, tracked concern.

LGTM. Ready to merge.