perf(engine): avoid string round-trip when building nested type names (#6049)

[thomhurst]

Summary

BuildClassNameForMatching (filter matching) and WriteTypeNameWithGenerics (identifier service) both formatted each nested-type segment into a scratch ValueStringBuilder, materialised a throw-away string via AsSpan().ToString(), stored it in a ValueListBuilder<string>, then re-emitted it into the outer builder.

This change collects the nested-type chain as ValueListBuilder<Type> and emits each segment directly into the outer ValueStringBuilder in reverse via a shared per-class AppendTypeNameWithGenericArgs helper. The intermediate per-segment string allocations and the string list are gone; behaviour and produced format are unchanged.

Runs on the per-test hot path during discovery and filter matching for every nested/generic class test.

Closes #6049

Test plan

• dotnet build TUnit.Engine/TUnit.Engine.csproj -c Release succeeds for all TFMs (netstandard2.0, net8.0, net9.0, net10.0).
• CI runs the existing engine + identifier tests across TFMs.

[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]

Code Review

The core optimization is correct and well-targeted. Changing ValueListBuilder<string> → ValueListBuilder<Type> and eliminating the intermediate per-segment typeVsb scratch builder removes real allocations on a hot path (discovery + filter matching for every nested/generic class test). The logic is sound, exception safety is preserved (resultVsb.ToString() materializes the string before the finally block disposes it), and the output format is unchanged.

Issue: Duplicated AppendTypeNameWithGenericArgs

The main concern is that the extraction introduced two identical private static methods — one in MetadataFilterMatcher and one in TestIdentifierService:

// MetadataFilterMatcher.cs:343
private static void AppendTypeNameWithGenericArgs(ref ValueStringBuilder vsb, Type type) { ... }

// TestIdentifierService.cs:147
private static void AppendTypeNameWithGenericArgs(ref ValueStringBuilder vsb, Type type) { ... }

The implementations are byte-for-byte identical (the only difference is a comment). The PR's stated goal is to make these two sites produce the same output — the duplication undermines that by making future divergence easy (e.g., a fix to generic arg formatting in one class silently missing the other).

Better approach: Extract to a shared internal static helper in TUnit.Engine/Helpers/, where similar utilities already live (ExpressionHelper, DisplayNameBuilder, etc.):

// TUnit.Engine/Helpers/TypeNameHelper.cs
internal static class TypeNameHelper
{
    internal static void AppendTypeNameWithGenericArgs(ref ValueStringBuilder vsb, Type type)
    {
        if (!type.IsGenericType)
        {
            vsb.Append(type.Name);
            return;
        }

        var name = type.Name;
        var backtickIndex = name.IndexOf('`');
        vsb.Append(backtickIndex > 0 ? name.AsSpan(0, backtickIndex) : name.AsSpan());

        // Use FullName for generic args to ensure uniqueness across namespaces.
        var genericArgs = type.GetGenericArguments();
        vsb.Append('<');
        for (var i = 0; i < genericArgs.Length; i++)
        {
            if (i > 0) vsb.Append(", ");
            vsb.Append(genericArgs[i].FullName ?? genericArgs[i].Name);
        }
        vsb.Append('>');
    }
}

This is consistent with the pattern in MetadataFilterMatcher's comment ("Matches TestIdentifierService.WriteTypeNameWithGenerics") — the two sites are explicitly required to produce identical output, so they should share one implementation.

Minor: [null!, null!, null!, null!] initialization

The null! pattern for initializing ValueListBuilder<Type> is a bit unusual but is fine — it mirrors the original ValueListBuilder<string> usage, and the null slots are just capacity hints for the internal span; they are overwritten before being read.

Summary

The optimization is a genuine improvement. The only structural concern is the code duplication of AppendTypeNameWithGenericArgs. Extracting it to a shared Helpers/TypeNameHelper.cs would both eliminate the duplication and formally enforce that the two formatting sites can't silently diverge.

[claude]

Follow-up Review (second commit)

The second commit addresses the duplication concern from the prior review: AppendTypeNameWithGenericArgs is now extracted to TUnit.Engine/Helpers/TypeNameHelper.cs, and both MetadataFilterMatcher.BuildClassNameForMatching and TestIdentifierService.WriteTypeNameWithGenerics delegate to it. That resolves the main structural issue.

Remaining observation: nested generic args are not expanded recursively

AppendTypeNameWithGenericArgs uses genericArgs[i].FullName ?? genericArgs[i].Name for each generic argument. Type.FullName on a constructed generic type (e.g. List<int> used as a type argument) returns a decorated assembly-qualified string like System.Collections.Generic.List\1[[System.Int32, ...]], not the display format List<System.Int32>` that this helper otherwise produces.

This is not a regression — the original pre-PR code had identical behavior with the same line — but it is worth noting as a known limitation: a type like Outer<Dictionary<string, List<int>>> will produce a different format for the outer vs. inner layers. For TUnit's current use-case (test UIDs and filter matching), this only matters if the two sides of the match (UID generation and filter matching) use the same code path, which they now do via the shared helper. So the behavior is consistent and the concern is pre-existing.

Exception safety in BuildClassNameForMatching

The resultVsb is stack-allocated and disposed in the finally block after resultVsb.ToString() has already materialized the string. That ordering is correct — ToString() copies the span content into a managed string, and the subsequent Dispose() only releases any heap overflow buffer. No issue here.

Summary

The PR is clean. The optimization is correct, the duplication concern is fully resolved, the output format is unchanged, and exception safety is preserved. The nested-generic-arg rendering limitation pre-exists this PR and is out of scope. Good to merge.