perf: reduce allocations in source-gen test building hot paths

[thomhurst]

Summary

Trivial, behavior-preserving allocation reductions in the source-generated mode test-building pipeline (all code shared with reflection mode, verified in both):

• TestBuilder.cs (streaming path): replaced the Where(...).Cast<ClassConstructorAttribute>().FirstOrDefault() LINQ chain with a foreach + pattern match, matching the fix already present in the non-streaming BuildTestsFromMetadataAsync path. Saves two iterator allocations per test metadata item during streaming discovery.
• TestBuilderPipeline.cs (both dynamic-test paths): hoisted attributes.ToAttributeDictionary() and the TestClassInstanceFactory lambda out of the repeat loops. Previously each [Repeat(N)] iteration rebuilt the same attribute dictionary and allocated a new closure; now all iterations share one of each.
• DataSourceHelpers.UnwrapTupleWithTypes: the ITuple branch built a List<object?> (default capacity) then called .ToArray(), despite the length being known up front — and both branches of its inner if did the identical result.Add(element). Now writes directly into a pre-sized array. One allocation instead of two-plus per tuple data row.
• DataSourceHelpers.InvokeIfFunc: value.GetType() was called twice per invocation; now captured once.

Testing

• dotnet build TUnit.Engine -c Release clean
• TUnit.TestProject filtered runs (net10.0, Release):
  • /*/*/*Tuple*/* — 46/47 pass; the 1 failure (ClassDataSourceTupleTests.Test_ClassDataSource_UnwrapsTuples) reproduces identically on unmodified main (pre-existing, unrelated)
  • /*/*/*Repeat*/* — 27/27 pass (source-gen mode), 27/27 pass (reflection mode via TUNIT_EXECUTION_MODE=Reflection)
  • /*/*/Dynamic*/* — 16 pass / 2 skip
  • /*/*/*ClassConstructor*/* — 10/10 pass

No source generator output or public API changes, so no snapshot updates needed.

[claude]

Review

Overall this is a well-scoped, behavior-preserving performance PR. The changes are correct and the test evidence is solid. A few observations below — one warrants attention, the rest are minor notes.

---

— dead-branch removal is correct, but the commit message understates what changed

The original code had this structure inside the for loop:

if (TupleHelper.IsTupleType(expectedType) && IsTuple(element))
{
    result.Add(element); // keep nested tuple as-is
    typeIndex++;
}
else
{
    result.Add(element); // add element normally
    typeIndex++;
}

Both branches did exactly the same thing (result.Add(element); typeIndex++), so collapsing them is correct. The comment in the PR description says this correctly.

What is worth calling out: the old code used a separate typeIndex cursor that advanced independently of i, while the new code implicitly assumes i == typeIndex always (since there is no longer a skip-element branch). Since both branches always incremented typeIndex on every iteration, i == typeIndex was invariably true in the original code too. The simplification is correct — but the typeIndex variable in the original was misleadingly named since it never diverged from i. Worth a note in case anyone later wants to add a real "skip nested tuple element" path here.

---

— shared dictionary across repeat iterations

Hoisting attributesByType out of the loop is fine as long as the dictionary is treated as immutable after construction. ToAttributeDictionary returns a plain Dictionary<Type, IReadOnlyList<Attribute>> (not a ReadOnlyDictionary wrapper) when there is at least one attribute, and the values inside it can be mutable List<Attribute> objects (the second-attribute path in AttributeDictionaryHelper).

Sharing a mutable dictionary across N repeat iterations that may run in parallel via ProcessInParallel(Environment.ProcessorCount) would be a data race. In practice, nothing currently mutates the dictionary after it is built, so this is safe today. But the abstraction is not watertight — if any downstream consumer ever adds to the per-type list, the shared reference would corrupt all repeat iterations simultaneously.

A small guard would future-proof this:

// Option A: wrap in ReadOnlyDictionary before hoisting
var attributesByType = new ReadOnlyDictionary<...>(attributes.ToAttributeDictionary());

// Option B: rename ToAttributeDictionary to return IReadOnlyDictionary (it already does in signature)
// and ensure the implementation never leaks a mutable reference

The signature already returns IReadOnlyDictionary, so callers get the correct contract. The risk is internal mutation by something that casts the value back. Not a blocker, but worth noting.

---

closure — correct but slightly misleading name

Func<Task<object>> instanceFactory = () => Task.FromResult(metadata.InstanceFactory(Type.EmptyTypes, []));

This closure captures metadata (the outer variable), so it is not a zero-allocation static lambda — it still allocates a display class on construction. The saving is that the single closure object is shared across all repeatCount + 1 iterations instead of allocating a fresh closure object per iteration. That is a real and correct saving; just wanted to confirm the analysis is accurate.

---

Minor: streaming path — good consistency fix

Replacing the Where(...).Cast<ClassConstructorAttribute>().FirstOrDefault() chain with a foreach + pattern match is exactly the right move here: avoids two iterator allocations per metadata item, aligns the streaming path with the non-streaming path, and is clearer about the intent. No concerns.

---

Summary

• All four changes are correct and behaviour-preserving.
• The UnwrapTupleWithTypes simplification is sound (the dead branch was genuinely dead).
• The hoisted instanceFactory saves one closure allocation per additional repeat iteration.
• The hoisted attributesByType is safe today; a lightweight future-proofing step (ensure the dictionary returned by ToAttributeDictionary is truly read-only all the way through) would remove the subtle mutation hazard.

Happy to approve once the team is comfortable with the attributesByType sharing note above.

[claude]

Review

Overall this is a well-scoped, behavior-preserving performance PR. The changes are correct and the test evidence is solid. A few observations below — one warrants attention, the rest are minor notes.

---

DataSourceHelpers.UnwrapTupleWithTypes — dead-branch removal is correct, but worth a comment

The original code had this structure inside the for loop:

if (TupleHelper.IsTupleType(expectedType) && IsTuple(element))
{
    result.Add(element); // keep nested tuple as-is
    typeIndex++;
}
else
{
    result.Add(element); // add element normally
    typeIndex++;
}

Both branches did exactly the same thing (result.Add(element); typeIndex++), so collapsing them is correct.

What is worth calling out: the old code used a separate typeIndex cursor that advanced independently of i, while the new code implicitly assumes i == typeIndex always (since there is no longer a skip-element branch). Since both branches always incremented typeIndex on every iteration, i == typeIndex was invariably true in the original code too. The simplification is correct — but the typeIndex variable in the original was misleadingly named since it never diverged from i. Worth noting in case anyone later wants to add a real "skip nested tuple element" path here.

---

TestBuilderPipeline.cs — shared attributesByType dictionary across repeat iterations

Hoisting attributesByType out of the loop is fine as long as the dictionary is treated as immutable after construction. ToAttributeDictionary returns a plain Dictionary<Type, IReadOnlyList<Attribute>> (not a ReadOnlyDictionary wrapper) when there is at least one attribute, and the values inside it can be mutable List<Attribute> objects (the second-attribute path in AttributeDictionaryHelper).

Sharing a mutable dictionary across N repeat iterations that may run in parallel via ProcessInParallel(Environment.ProcessorCount) would be a data race. In practice, nothing currently mutates the dictionary after it is built, so this is safe today. But the abstraction is not watertight — if any downstream consumer ever adds to the per-type list, the shared reference would corrupt all repeat iterations simultaneously.

A small guard would future-proof this: the signature of ToAttributeDictionary already returns IReadOnlyDictionary, which is the correct contract. The risk is internal mutation by something that casts the value back. Consider wrapping in ReadOnlyDictionary at the construction site to make the immutability structural rather than conventional.

---

instanceFactory closure — correct saving, worth confirming scope

Func<Task<object>> instanceFactory = () => Task.FromResult(metadata.InstanceFactory(Type.EmptyTypes, []));

This closure captures metadata (the outer variable), so it is not a zero-allocation static lambda — it still allocates a display class on construction. The saving is that the single closure object is shared across all repeatCount + 1 iterations instead of allocating a fresh closure object per iteration. That is a real and correct saving.

---

TestBuilder.cs streaming path — good consistency fix

Replacing the Where(...).Cast<ClassConstructorAttribute>().FirstOrDefault() chain with a foreach + pattern match is exactly the right move: avoids two iterator allocations per metadata item, aligns the streaming path with the non-streaming path, and is clearer about intent. No concerns.

---

Summary

• All four changes are correct and behaviour-preserving.
• The UnwrapTupleWithTypes simplification is sound (the dead branch was genuinely dead).
• The hoisted instanceFactory saves one closure allocation per additional repeat iteration.
• The hoisted attributesByType is safe today; a lightweight future-proofing step (ensure the dictionary returned by ToAttributeDictionary is truly read-only all the way through, e.g. via a ReadOnlyDictionary wrapper) would remove the subtle mutation hazard under parallel execution.

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics -4 complexity

Metric	Results
Complexity	-4

View in Codacy

_{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.}

[claude]

Follow-up Review

Previous review comments have been addressed — the attributesByType mutation-hazard note is confirmed safe given AttributeDictionaryHelper.ToAttributeDictionary now returns a Dictionary<Type, IReadOnlyList<Attribute>> where the IReadOnlyList<Attribute> values are either a plain Attribute[] or a List<Attribute>. Nothing in the post-construction path mutates the dictionary itself, and TestDetails.AttributesByType is required init, so the sharing is correct.

A few follow-up observations now that the code is in:

---

FirstOfType<T> — consider moving to TUnit.Core

AttributeArrayExtensions.FirstOfType<T> lives in TUnit.Engine.Extensions and is internal. The identical pattern (foreach + is T) exists as inline loops in TestBuilderPipeline.cs (line 60) and TestBuilder.cs (lines 176, 1600), all now unified via this method — good. However, AttributeDictionaryHelper is in TUnit.Core (public, shared between source-gen and reflection modes), and the same attribute-array scanning need will likely arise in TUnit.Core code paths in the future. Placing the helper in TUnit.Core as an internal extension would make it available everywhere without duplication, matching where ToAttributeDictionary already lives.

---

CopyTupleElements — #if guard is broader than necessary

#if NET5_0_OR_GREATER || NETCOREAPP3_0_OR_GREATER
private static object?[] CopyTupleElements(ITuple tuple, int count)

ITuple was introduced in .NET Core 2.0 / netstandard2.1. The guard copies the existing #if from the call sites, so it is not incorrect, but the method is called from two #if NET5_0_OR_GREATER || NETCOREAPP3_0_OR_GREATER blocks and never from anywhere else. This is fine — the compiler would reject a call outside those guards anyway — but a comment explaining why the guard exists (to match the ITuple availability of the enclosing if blocks) would help future readers avoid widening the guard unnecessarily.

---

UnwrapTupleWithTypes simplification — correctness note for the record

The new code:

return CopyTupleElements(tuple, Math.Min(tuple.Length, expectedTypes.Length));

The old code's typeIndex advanced on every iteration and was therefore always equal to i, so the Math.Min cap is the correct replacement for typeIndex < expectedTypes.Length. This is already noted in the PR description and in the previous review; recording it here for completeness since the code is now merged into the review baseline.

---

instanceFactory closure — minor naming nit

Func<Task<object>> instanceFactory = () => Task.FromResult(metadata.InstanceFactory(Type.EmptyTypes, []));

The variable is named instanceFactory but it is actually a wrapper delegate around metadata.InstanceFactory. The original per-iteration lambdas were identical wrappers. The name is fine for the current use, but createInstance or instanceCreator would more clearly signal "this is called to produce a new instance each time" rather than "this is the factory itself." Minor, not a blocker.

---

Overall

The PR is correct, well-tested, and a meaningful allocation reduction in the hot-path. The previous concerns about the attributesByType race have been verified safe by reading AttributeDictionaryHelper. The CopyTupleElements extraction is a clean DRY win. Approve when ready.