perf(sourcegen): consolidate per-file ModuleInitializers into merged .cctor (#6226)

[thomhurst]

Fixes #6226.

Problem

Several generators still emitted a separate [ModuleInitializer] method per file/class, each costing a type load + a serial tier-0 JIT at module load:

• PropertyInjectionSourceGenerator (4 emit sites; the InitializerProperties site also built the InitializerPropertyInfo[] array eagerly inline in module init)
• AotConverterGenerator
• StaticPropertyInitializationGenerator
• DynamicTestsGenerator (one ModuleInitializer per dynamic-test method)

Fix

Apply the existing consolidation pattern (already used by test/hook registration via TUnit_TestRegistration/TUnit_HookRegistration):

1. InfrastructureGenerator declares 4 new empty partial shells in TUnit.Generated and triggers them from its single module initializer via RuntimeHelpers.RunClassConstructor:
   TUnit_PropertyRegistration, TUnit_ConverterRegistration, TUnit_StaticPropertyRegistration, TUnit_DynamicTestRegistration.
2. Each generator now contributes static readonly int _r_X = Registry.Register(...); fields to the matching shell. The compiler merges all contributions into one .cctor per concern — N per-file ModuleInitializer methods collapse into 1 merged method.
3. Five registry Register/RegisterDynamic methods now return int (dummy 0) so they can be used as static field initializers — matching the existing RegisterEntries/RegisterHook convention.

This is free aggregation via the compiler's partial-class merge — no .Collect() fan-in is added to the incremental pipeline, so incremental compilation behavior is unchanged.

Eager vs. lazy

Registrations are emitted eager-inline with no factory lambdas. A static () => … factory lambda is a new compiler-generated method (= a new JIT call) per type, so it's only worth it when it defers genuinely heavy, parallelizable work. The four consolidated concerns here are cheap (new Source(), a small InitializerPropertyInfo[], new Converter(), new DynamicSource()), so they go straight into the merged .cctor.

The test/hook generators are intentionally left as-is: they already consolidate into a merged .cctor, already batch per-class work into shared named methods (__CreateInstance/__Invoke/__Attributes), and keep a single per-class () => Source.Entries lazy factory — that one lambda is the deferral that pushes heavy ClassMetadata/MethodMetadata/Entries construction off the serial module-init path onto parallel discovery JIT.

Dual-mode

Source-gen consolidation only. Registry consumer methods (GetSource, GetProperties, GetRegisteredProperties, DiscoverInjectableProperties, TryConvert) are byte-identical, and reflection mode never calls Register. Reflection mode is unaffected.

Validation

• Builds clean: TUnit.Core, source generator, TUnit.TestProject (0 errors).
• 480 source-generator snapshot tests pass. Only Infrastructure + AotConverter output is snapshot-tested, so just 5 .verified.txt changed (4 DuplicateTypeName…Infrastructure… + the [Skip]'d AotConverterGeneratorTests.GeneratesCode).
• 4 TUnit.PublicAPI Core snapshots updated for the void→int surface change and pass on all TFMs.
• Runtime (SourceGenerated): property injection + static properties, 29 dynamic tests, AOT converters and basics all green — confirming each merged .cctor fires and registers correctly.

[codacy-production]

Not up to standards ⛔

🔴 Issues 1 minor

Alerts:
⚠ 1 issue (≤ 0 issues of at least minor severity)

Results:
1 new issue

Category	Results
CodeStyle	1 minor

View in Codacy

🟢 Metrics 0 complexity

Metric	Results
Complexity	0

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: perf(sourcegen): consolidate per-file ModuleInitializers into merged .cctor (#6226)

The approach is well-designed and correctly extends the established partial-class merge pattern already used for test and hook registration. The rationale in the PR description is thorough. A few issues worth addressing:

---

Issue 1: Potential field name collision in SafeName (StaticPropertyInitializationGenerator)

The SafeName helper maps all non-alphanumeric characters to _. This means namespace separators and existing underscores in type names are indistinguishable:

// A_B.C  →  A_B_C
// A.B_C  →  A_B_C   ← collision

If two types share the same safe name and the same property name, you get a duplicate static field in the merged .cctor, which is a compile error in the generated code.

Better approach: use a hash suffix or the stable index from the incremental pipeline, the same way the test-registration generators use a numeric suffix or the full TypeHandle-based key. Even appending GetHashCode() of the fully-qualified name (deterministic for a given build) would prevent collisions without losing readability:

private static string SafeName(string globallyQualified)
{
    var sb = new StringBuilder();
    foreach (var c in globallyQualified)
        sb.Append(char.IsLetterOrDigit(c) ? c : '_');
    // Append a short hash to avoid collisions between types whose names differ only in _ vs .
    sb.Append('_');
    sb.Append((uint)globallyQualified.GetHashCode());
    return sb.ToString();
}

---

Issue 2: Dynamic test method overloads → duplicate field names

In DynamicTestsGenerator, the contributed field name is:

_r_Dynamic_{className}_{model.MethodName}

If a class has two dynamic test source methods with the same name but different parameter types (i.e., overloads), model.MethodName would be the same string, producing duplicate fields in TUnit_DynamicTestRegistration. The className is file-scoped per method but the field lives in the cross-file partial class.

Consider incorporating the method's parameter count, arity, or a monotonic index from the incremental pipeline into the field name to guarantee uniqueness. Alternatively, use the same hash-suffix approach as above on the full method signature.

---

Minor: public API change is a break for signature-scanning tooling

The five void Register(…) → int Register(…) changes are correctly tracked in the snapshot files and are not breaking for call sites (callers discard the return value). However, they do show up as a public API delta in the TUnit public API contracts and could trip reflection-based tooling (Roslyn analyzers, source generators, ABI checkers) that depend on the exact signature. The PR description and snapshots document this clearly — it's more of a heads-up than a blocker, and the design justification (static field initializer pattern) is solid.

---

Overall

The consolidation is correct, well-motivated, and consistent with the existing TUnit_TestRegistration/TUnit_HookRegistration pattern. The incremental pipeline impact is zero (no .Collect()), and the snapshot validation is thorough. The two field-name uniqueness issues above are the main things to resolve before merging — they would produce compile errors in user projects with specific type/method naming patterns.

The file partial class → global::TUnit.SourceGenerated.{className} cross-reference in the same generated file is a clever trick and valid C#. The eager-inline approach (no lambda factory) for cheap registrations is the right call.

[claude]

Code Review

Solid performance improvement — the consolidation pattern is well-motivated and the implementation is consistent with the existing TUnit_TestRegistration/TUnit_HookRegistration approach. A few things worth flagging:

---

1. SafeName() collision risk — potential compile error

In StaticPropertyInitializationGenerator.cs, the field name for TUnit_StaticPropertyRegistration is built using:

var fieldName = $"_r_{SafeName(typeName)}_{propertyData.Property.Name}";

Where SafeName() replaces any non-alphanumeric character with _. Two distinct types whose fully-qualified names differ only by separator characters would produce the same safe name. For example, Foo.Bar.Baz and Foo_Bar.Baz both map to Foo_Bar_Baz. If both types have a property with the same name, you'd get a duplicate field name in TUnit_StaticPropertyRegistration — a compile error in user code.

A safer approach: append a stable hash of the original globallyQualified string to guarantee uniqueness even on collisions:

private static string SafeName(string globallyQualified)
{
    var sb = new StringBuilder(globallyQualified.Length + 9);
    foreach (var c in globallyQualified)
        sb.Append(char.IsLetterOrDigit(c) ? c : '_');
    // append a short stable hash to prevent collisions between types that differ only by separators
    sb.Append('_');
    sb.Append((uint)globallyQualified.GetHashCode() % 0xFFFF, "X4");
    return sb.ToString();
}

Same collision risk exists in AotConverterGenerator.cs for the _r_{converterClassName} field (though converter class names are auto-numbered AotConverter_N so collisions are unlikely there in practice).

---

2. void → int is a public API break

Five registry Register/RegisterDynamic methods on public static classes are now public static int — a breaking change if any consumer calls them directly. The PR description acknowledges this, and it's consistent with the existing RegisterEntries convention, but the dummy return 0 semantics are now permanently in the public API surface.

Consider whether an internal overload (or an [EditorBrowsable(Never)] annotation) would be cleaner than leaking this code-gen idiom to all callers. At minimum, document that the return value is intentionally meaningless:

/// <returns>Always 0 — return value exists solely to allow use as a static field initializer.</returns>
public static int Register(IAotConverter converter)

---

3. RegistrationShells coupling: new categories require two edits in one file

The RegistrationShells string array in InfrastructureGenerator now drives both the RunClassConstructor calls and the shell class declarations. This is clean, but it creates a silent coupling: if someone adds a fifth registration concern (a new generator) and forgets to add its shell name to RegistrationShells, that concern will register via its own separate .cctor at an uncontrolled time rather than through the module initializer, and there will be no compile-time error to catch it.

The existing test/hook shells were coupled to their respective generators before this PR; the new array consolidates that coupling in one place, which is better. But it's worth noting that the "add an entry to RegistrationShells" step is easy to miss when adding a new generator. A comment in RegistrationShells (or a well-placed TODO pointing to the existing generators) would help future maintainers.

---

4. Missing snapshot coverage for new pattern (informational)

The verified test snapshots only changed for AotConverterGeneratorTests and DuplicateTypeNameAcrossAssembliesTests. The new pattern for PropertyInjectionSourceGenerator, StaticPropertyInitializationGenerator, and DynamicTestsGenerator isn't snapshot-tested. The PR description confirms runtime validation passes, but a snapshot regression for those generators would have caught the SafeName() collision risk earlier (item 1 above). Not a blocker, but worth considering coverage parity with the AOT converter path.

---

Overall

The architectural direction is correct — partial-class .cctor merging is the right way to scale the registration pattern without per-file JIT overhead. The PR description is thorough and the rationale for eager vs. lazy registration is well-reasoned. The main risk is the SafeName() collision (item 1), which is a correctness issue in edge-case user assemblies that would surface as a compiler error rather than a runtime failure.

[thomhurst]

Thanks for the review — addressed in 106a842.

Issue 1 (SafeName collision): Fixed. _r_ static-property fields now append a stable per-build hash of the fully-qualified type + property name, so types differing only in . vs _ no longer collide in the merged TUnit_StaticPropertyRegistration .cctor. (Worth noting: the prior registeredFields guard would have silently dropped the second registration rather than erroring, so this is a real correctness fix, not just compile-safety.)

Issue 2 (dynamic overloads): Fixed, and a bit deeper than the field name — the generated file name Dynamic_{class}_{method}.g.cs collided too (it would throw at AddSource before the field ever mattered), as did two same-minimal-name classes in different namespaces. Both the file name and the field now carry a stable hash of the fully-qualified type + method + line, so overloaded/colliding dynamic sources now generate distinct files/fields and work instead of failing.

Minor (void→int API delta): Acknowledged — intentional, matches the existing RegisterEntries/RegisterHook convention (they already return int for exactly this static-field-initializer use), source-compatible for call sites, and tracked in the updated TUnit.PublicAPI Core snapshots.

Validation after the fix: generator builds (Debug+Release), TestProject compiles, 29 dynamic + static-property runtime tests green, full 480 source-gen snapshot suite green (static/dynamic output isn't snapshotted, so no snapshot churn from the hashes).

[claude]

Follow-up Review: Fixes Verified + Remaining Observations

The two issues flagged in the prior reviews have been addressed in commit 106a84276. Here's the status:

---

Issues from prior reviews — addressed ✓

SafeName collision (StaticPropertyInitializationGenerator) — Fixed. The field names now carry a per-build hash of the fully-qualified type + property name:

var stableHash = ((uint)$"{typeName}.{propertyData.Property.Name}".GetHashCode()).ToString("x8");
var fieldName = $"_r_{SafeName(typeName)}_{propertyData.Property.Name}_{stableHash}";

Correctly prevents the compile error that would have occurred when two types differed only in ." vs _` in their namespace separators.

Dynamic test overloads (DynamicTestsGenerator) — Fixed, and more thoroughly than required. Both the generated file hint name and the field name on TUnit_DynamicTestRegistration now incorporate a suffix derived from fully-qualified type + method name + line number. This correctly handles overloads AND distinct types that share the same minimal name.

---

Remaining observation: PropertyInjectionSourceGenerator has no hash suffix

GetSafeClassName in PropertyInjectionSourceGenerator maps the fully-qualified type name (namespace + class) to an identifier by replacing separators with _:

// Foo.Bar_MyClass → Foo_Bar_MyClass
// Foo_Bar.MyClass → Foo_Bar_MyClass  ← collision

The generated field names — _r_{SafeClassName}_PropSource, _r_{SafeTypeName}_InitProps, and their Generic variants — don't have hash suffixes. Two types whose namespace+name strings differ only in . vs _ would produce a duplicate field in the merged TUnit_PropertyRegistration .cctor, which is a compile error.

StaticPropertyInitializationGenerator had the identical problem and was fixed here. The same fix should apply to the four property-injection field names in PropertyInjectionSourceGenerator. The pre-existing class-level collision (the old {SafeClassName}_PropertyInjectionInitializer class names) is separately tracked and wasn't introduced by this PR — but the new merged-field collision is, because the partial-class context is now shared across all types.

---

Minor: GetHashCode() is randomized across process restarts

The uniqueSuffix and stableHash values are computed via string.GetHashCode(), which in .NET Core 2.1+ is randomized per process via hash seed randomization. The comment accurately says "stable within a build", but:

• In IDE scenarios (Roslyn Language Server), the generated file hint names (e.g., Dynamic_{class}_{method}_{hash}.g.cs) will differ on each server restart even for identical inputs. Roslyn would see the prior hint names as removed and the new ones as added, invalidating cached incremental state.
• A deterministic hash — e.g., FNV-1a or Marvin32 computed manually, or System.IO.Hashing.XxHash32 on .NET 7+ — would make hint names stable across process restarts and preserve the full incremental benefit.

That said, the old per-file [ModuleInitializer] approach had no incremental benefit here either, so this is an improvement regardless. The non-determinism is worth tracking as a follow-up.

---

Overall

The consolidation is architecturally sound and the implementation is correct for the tested cases. The fix for StaticPropertyInitializationGenerator and DynamicTestsGenerator is exactly right. The PropertyInjectionSourceGenerator field-name gap is the one remaining item that can produce a compile error in user assemblies with specific namespace naming patterns — recommend applying the same hash-suffix treatment before merging.

[claude]

Code Review: perf(sourcegen): consolidate per-file ModuleInitializers into merged .cctor (#6226)

This is a well-executed optimization that correctly extends the existing partial-class consolidation pattern (already used for test/hook registration) to four additional generators. The PR description is excellent — the rationale, tradeoffs, and explicit out-of-scope callouts are all documented.

What works well

• Pattern fidelity: Correctly applies the same design as TUnit_TestRegistration/TUnit_HookRegistration without reinventing it.
• FNV-1a for stable hashes: Right choice — string.GetHashCode() isn't stable across processes/platforms, so baking it into generated identifiers (hint names, field names) would break Roslyn's incremental cache. Using FileNameHelper.GetStableHashCode is correct.
• No .Collect() fan-in: Avoiding a pipeline fan-in preserves incremental compilation granularity. This is a meaningful win that's easy to overlook.
• Dual-mode unaffected: Registry consumer methods are byte-identical; reflection mode never calls Register. The scope is correctly bounded.
• Collision disambiguation via hash: The _r_{SafeName}_{stableHash} field naming pattern correctly handles the case where SafeName alone would collide (namespaces differing only in . vs _).

Issue 1 — Namespace mismatch risk in AotConverterGenerator (verify before merging)

In AotConverterGenerator.cs the new partial is emitted as:

writer.AppendLine("internal static partial class TUnit_ConverterRegistration");

without an explicit namespace block around it. The shell declared by InfrastructureGenerator is in namespace TUnit.Generated { ... }. If the converter classes are written under a file-scoped namespace declaration earlier in the same generated file (e.g., namespace TUnit.Generated;), everything naturally falls in TUnit.Generated and correctly merges. But if the file uses the global namespace, TUnit.Generated.TUnit_ConverterRegistration (the shell) would be empty and the RunClassConstructor call would fire a no-op .cctor — silently skipping all AOT converter registrations.

The runtime test passing suggests the namespace IS correct, but the other three generators explicitly emit their contributions inside namespace TUnit.Generated { ... } blocks — compare PropertyInjectionSourceGenerator.cs:

sb.AppendLine("namespace TUnit.Generated");
sb.AppendLine("{");
sb.AppendLine("internal static partial class TUnit_PropertyRegistration");

vs. AotConverterGenerator.cs which writes only:

writer.AppendLine("internal static partial class TUnit_ConverterRegistration");

Recommendation: Add the explicit namespace wrapper in AotConverterGenerator.GenerateConverters to match the pattern the other three generators follow. This eliminates the implicit coupling on whatever namespace the file started with and makes the intent unambiguous:

// close prior namespace if any, then:
writer.AppendLine("namespace TUnit.Generated");
writer.AppendLine("{");
writer.Indent();
writer.AppendLine("internal static partial class TUnit_ConverterRegistration");
writer.AppendLine("{");
// ... field initializers
writer.Unindent();
writer.AppendLine("}");
writer.Unindent();
writer.AppendLine("}");

Issue 2 — Public API surface change (void → int on 5 public methods)

AotConverterRegistry.Register, InitializerPropertyRegistry.Register, PropertySourceRegistry.Register, StaticPropertyRegistry.Register, and SourceRegistrar.RegisterDynamic are all public and have had their return type changed from void to int. Callers using these as statement expressions are unaffected, and the PublicAPI snapshots are correctly updated.

Worth noting: this matches the existing convention (RegisterEntries/RegisterHook already return int), so the pattern is consistent. The only risk is external consumers that pass these as method group delegates typed as Action<...> rather than Func<..., int>. Given these are "generated code only" APIs with no user-facing call sites, this is an acceptable tradeoff — just documenting it explicitly.

Minor: inconsistent global:: qualification in generated code

AotConverterGenerator emits fully-qualified names:

global::TUnit.Core.Converters.AotConverterRegistry.Register(new AotConverter_0())

But GenerateInitializerPropertySource in PropertyInjectionSourceGenerator emits:

InitializerPropertyRegistry.Register(typeof({model.TypeFullyQualified}), new InitializerPropertyInfo[]

This works because the generated file already has appropriate using directives, but the global:: prefix convention is more defensive in generated code (immune to namespace shadowing). Not a blocker, but worth making consistent across the four generators.

Overall

The architectural direction is correct — this is a worthwhile startup-time optimization with no incremental-build regression risk. The PR is nearly ready; the namespace explicitness in AotConverterGenerator is the one thing worth confirming or fixing before merging, and the rest are minor consistency notes.