perf: add ParameterMetadataFactory and lazy ReflectionInfo resolution

[thomhurst]

Summary

• New ParameterMetadataFactory — replaces inline new ParameterMetadata { ... } object initializers in generated code with ParameterMetadataFactory.Create(...) calls, reducing per-parameter IL size and JIT-compiled native code (follows the existing TestMetadataFactory/MethodMetadataFactory pattern)
• Lazy ReflectionInfo via field keyword — all GetMethod().GetParameters()[i] reflection is deferred to first access via ReflectionInfoFactory = static () => ... lambdas, eliminating eager reflection at module init in source-gen mode (where ReflectionInfo is rarely accessed at runtime)
• Hoist helpers — added GenerateParameterInfoArrayExpression() and GenerateParameterInfoHoistStatement() to MetadataGenerationHelper for future use when eager parameter info arrays are needed

Generated code before (per parameter):

new ParameterMetadata(typeof(int))
{
    Name = "value",
    TypeInfo = new ConcreteType(typeof(int)),
    IsNullable = false,
    ReflectionInfo = typeof(X).GetMethod("M", flags, null, paramTypes, null)!.GetParameters()[0]
}

Generated code after (per parameter):

ParameterMetadataFactory.Create(typeof(int), "value",
    new ConcreteType(typeof(int)), false,
    reflectionInfoFactory: static () => typeof(X).GetMethod("M", flags, null, paramTypes, null)!.GetParameters()[0])

Net reduction: -7,136 lines / +1,156 lines across 88 files (mostly snapshot shrinkage from compact factory calls replacing multi-line object initializers).

Test plan

• dotnet build TUnit.Core.SourceGenerator -c Release — compiles
• TUnit.Core.SourceGenerator.Tests — 456/456 pass (4 pre-existing skips)
• TUnit.PublicAPI — 12/12 pass
• TUnit.TestProject compiles on net10.0 (proves generated code is valid C#)
• DataDrivenTests runtime — 15/15 pass
• MatrixTests runtime — 275/275 pass (exercises lazy ReflectionInfo resolution path)

[claude]

Code Review: perf/parameter-metadata-factory-lazy

Summary

Solid performance PR that follows established patterns well. The factory abstraction reduces generated code size substantially (~6,000 net lines removed) and the lazy ReflectionInfo deferral is a meaningful optimization for source-gen mode. A few design-level concerns worth considering before merging.

---

What's Good

• Consistent design: ParameterMetadataFactory mirrors the existing TestMetadataFactory/MethodMetadataFactory pattern exactly, so there are no surprises for readers of the generated code.
• C# 13 field keyword: Clean use of the backing field keyword to implement lazy init without a separate _reflectionInfo field.
• Comprehensive test validation: 88-file snapshot update with all 456 tests passing, plus targeted runtime validation of the lazy path.
• Net size reduction: 7,136 deletions vs 1,156 additions across 88 files is a meaningful improvement.

---

Issues

1. ?? null! is a correctness landmine (ParameterMetadata.cs)

The trailing ?? null! suppresses nullability warnings but doesn't prevent a runtime NullReferenceException when neither ReflectionInfo was set nor ReflectionInfoFactory was provided. The old required modifier caught this at compile time.

Suggested approach: Either keep required (and require callers to set one of the two properties) or change the fallback to a clear ThrowHelper that explains what went wrong:

This makes failures loud and informative rather than deferred NREs.

2. Removal of required is a compile-time safety regression

In main, public required ParameterInfo ReflectionInfo { get; set; } ensured that any new ParameterMetadata(...) { ... } object initializer in reflection mode (TUnit.Engine) must set it. Dropping required means a future ReflectionMetadataBuilder maintainer could forget to set the field and only discover it at runtime.

The reflection mode builder (ReflectionMetadataBuilder.CreateParameterMetadata) still sets it directly — this works fine. But consider adding [MemberNotNull(nameof(ReflectionInfo))]" or keeping the compile-time check some other way. Alternatively, a factory method on ParameterMetadataitself for the reflection-mode path would make the constraint explicit withoutrequired`.

3. Thread-safety of lazy init is not guaranteed

This is not thread-safe. Concurrent readers will both see field == null and both call ReflectionInfoFactory.Invoke(). For the current use case (reflection lookups returning the same stable object), this is typically harmless but wasteful. If ParameterMetadata instances are ever shared across threads during parallel test discovery, consider:

Or simply acknowledge in a code comment that double-invocation is acceptable.

4. GenerateParameterInfoArrayExpression / GenerateParameterInfoHoistStatement are dead code

Both methods are public (within internal class) and added in this PR with the comment \u201cfor future use\u201d. Dead code in source generators creates maintenance drag — the next person to touch this file has to understand what these are for and whether they\u2019re safe to change.

Recommendation: Remove them now and re-add when they have an actual caller. If the intent is to hoist GetParameters() arrays for multi-param methods (which would be a natural follow-up optimization), that\u2019s worth noting in a TODO comment on the relevant call site, not via unused public methods.

5. WriteParameterMetadataGeneric hardcodes false for isNullable

For generic type parameters (T?, T constrained to value types etc.), this may produce incorrect IsNullable values. Likely a pre-existing issue rather than introduced here, but worth flagging since the new factory makes this more visible. The non-generic overload (WriteParameterMetadata) correctly uses parameter.Type.IsNullable().

---

Minor Notes

• The [UnconditionalSuppressMessage(\u201cReflectionAnalysis\u201d, \u201cIL2067\u201d)\] on ParameterMetadataFactory.Create is reasonable given it\u2019s always called from generated code, but worth a brief justification comment explaining why the suppression is safe here (generated code always passes concrete types).
• reflectionInfoFactory: static () => null! is generated for fallback cases — passing null directly instead of a static lambda returning null! would be slightly cleaner and avoid a delegate allocation for the fallback path.

---

Bottom Line

The overall direction is right and the implementation is solid. The primary concern is the ?? null! pattern replacing the old required guard — this trades compile-time safety for flexibility, which is a reasonable trade-off only if the lazy-null case is intentionally supported (e.g., AOT/reflection-free mode where ReflectionInfo is never needed). If that\u2019s the design intent, it should be explicitly documented on the property. Otherwise, a ThrowHelper or preserved required constraint would be safer.

[claude]

Code Review: perf/parameter-metadata-factory-lazy

Summary

Solid performance PR that follows established patterns well. The factory abstraction reduces generated code size substantially (~6,000 net lines removed) and the lazy ReflectionInfo deferral is a meaningful optimization for source-gen mode. A few design-level concerns worth considering before merging.

---

What's Good

• Consistent design: ParameterMetadataFactory mirrors the existing TestMetadataFactory/MethodMetadataFactory pattern exactly, so there are no surprises for readers of the generated code.
• C# 13 field keyword: Clean use of the backing field keyword to implement lazy init without a separate _reflectionInfo field.
• Comprehensive test validation: 88-file snapshot update with all 456 tests passing, plus targeted runtime validation of the lazy path.
• Net size reduction: 7,136 deletions vs 1,156 additions across 88 files is a meaningful improvement.

---

Issues

1. ?? null! is a correctness landmine (ParameterMetadata.cs)

The ?? null! in the getter suppresses nullability warnings but doesn't prevent a runtime NullReferenceException when neither ReflectionInfo was set nor ReflectionInfoFactory was provided. The old required modifier caught this at compile time.

Suggested approach: Either keep required (and require callers to set one of the two properties) or change the fallback to a clear ThrowHelper that explains what went wrong:

get => field ??= ReflectionInfoFactory?.Invoke()
    ?? throw new InvalidOperationException(
        $"ReflectionInfo for parameter '{Name}' was not set and no factory was provided.");

This makes failures loud and informative rather than deferred NREs.

2. Removal of required is a compile-time safety regression

In main, public required ParameterInfo ReflectionInfo { get; set; } ensured that any new ParameterMetadata(...) { ... } object initializer in reflection mode (TUnit.Engine) must set it. Dropping required means a future ReflectionMetadataBuilder maintainer could forget to set the field and only discover it at runtime.

The reflection mode builder (ReflectionMetadataBuilder.CreateParameterMetadata) still sets it directly — this works fine. But the compile-time guarantee is lost.

3. Thread-safety of lazy init is not guaranteed

The ??= pattern is not thread-safe. Concurrent readers will both see field == null and both call ReflectionInfoFactory.Invoke(). For the current use case (reflection lookups returning the same stable object), this is typically harmless but wasteful. If ParameterMetadata instances are ever shared across threads during parallel test discovery, consider:

get => field ?? Interlocked.CompareExchange(ref field, ReflectionInfoFactory?.Invoke()!, null) ?? field;

Or simply document in a comment that double-invocation is acceptable.

4. GenerateParameterInfoArrayExpression / GenerateParameterInfoHoistStatement are dead code

Both methods are added in this PR with the comment "for future use" but have no callers. Dead code in source generators creates maintenance drag — the next person has to understand what these are for and whether they're safe to change.

Recommendation: Remove them now and re-add when they have an actual caller. If the intent is to hoist GetParameters() arrays for multi-param methods (a natural follow-up optimization), note that in a TODO comment on the relevant call site, not via unused methods.

5. WriteParameterMetadataGeneric hardcodes false for isNullable

For generic type parameters (T?, T constrained to value types etc.), this may produce incorrect IsNullable values. Likely a pre-existing issue rather than introduced here, but worth flagging since the new factory makes the isNullable argument more visible. The non-generic overload (WriteParameterMetadata) correctly uses parameter.Type.IsNullable().

---

Minor Notes

• The [UnconditionalSuppressMessage("ReflectionAnalysis", "IL2067")] on ParameterMetadataFactory.Create is reasonable given it's always called from generated code, but worth a brief justification comment explaining why the suppression is safe (generated code always passes concrete types).
• In fallback cases GenerateReflectionInfoForParameter returns "null!", producing reflectionInfoFactory: static () => null! in generated code. Passing null directly instead of a static lambda returning null! would be cleaner and avoids an unnecessary delegate.

---

Bottom Line

The overall direction is right and the implementation is solid. The primary concern is the ?? null! pattern replacing the old required guard — this trades compile-time safety for flexibility. If the lazy-null case is intentionally supported (e.g., AOT/reflection-free mode where ReflectionInfo is never accessed), that should be explicitly documented on the property. Otherwise, a ThrowHelper or preserved required constraint would be safer.

[thomhurst]

Addressed the review feedback:

Item 1 (?? null! landmine): Fixed in 9b048dd — replaced with throw new InvalidOperationException(...) with a descriptive message including the parameter name. Failures are now loud and informative.

Item 2 (removal of required): The required modifier can't coexist with the field keyword lazy pattern — the compiler requires the property to be set in every object initializer, but source-gen callers set ReflectionInfoFactory instead of ReflectionInfo directly. The reflection-mode callers (ReflectionMetadataBuilder, ReflectionHookDiscoveryService, DataGeneratorMetadataCreator, ClassMetadataHelper) all continue to set ReflectionInfo directly via the setter. The new throw in the getter (item 1) provides a runtime safety net for any caller that forgets to set either property.

Item 3 (thread safety): The field ??= pattern without synchronization is consistent with the rest of TUnit.Core (Context.cs:39,42, HookMethod.cs:16,23, LastTestInClassAdapter.cs:14, etc.). The race is benign — ParameterInfo from CLR reflection is immutable and deterministic, so double-invocation produces the same result. Adding Interlocked would be inconsistent with the codebase convention.

Item 4 (dead code): Already removed in 774af06 (the simplification commit). Both GenerateParameterInfoArrayExpression and GenerateParameterInfoHoistStatement were deleted.

Item 5 (WriteParameterMetadataGeneric hardcodes false): Already fixed in 774af06. WriteParameterMetadataGeneric now delegates to WriteParameterMetadata, which correctly computes isNullable from parameter.Type.IsNullable().

[claude]

Code Review: perf/parameter-metadata-factory-lazy (follow-up)

The second commit (774af06c) addressed three of the five concerns from the previous review:

• ✅ Dead code removed (GenerateParameterInfoArrayExpression, GenerateParameterInfoHoistStatement, hoisted params parameters)
• ✅ WriteParameterMetadataGeneric unified into WriteParameterMetadata — the hardcoded false for IsNullable is now correctly replaced by parameter.Type.IsNullable()
• ✅ Justification on [UnconditionalSuppressMessage] is present and clear

The following items from the previous review are still unresolved:

---

1. ?? null! remains a silent NRE trap — still unaddressed

ParameterMetadata.cs:

public ParameterInfo ReflectionInfo
{
    get => field ??= ReflectionInfoFactory?.Invoke() ?? null!;
    set;
} = null!;

If neither ReflectionInfo (via set) nor ReflectionInfoFactory is supplied, every access silently returns null while the caller's NullReferenceException points at call sites far from the source. The old required modifier made this a compile-time error.

Recommended fix — make the failure fast and informative:

get => field ??= ReflectionInfoFactory?.Invoke()
    ?? throw new InvalidOperationException(
        $"ParameterMetadata '{Name}': ReflectionInfo was not set and no ReflectionInfoFactory was provided.");

This is strictly better than null! in every scenario: it preserves lazy resolution for source-gen mode, sets eagerly in reflection mode, and fails loudly instead of silently for any misconfigured caller.

---

2. required removal is still a compile-time safety regression — still unaddressed

ReflectionMetadataBuilder.CreateParameterMetadata still sets ReflectionInfo directly and works fine at runtime. But dropping required means a future maintainer can write:

return new ParameterMetadata(parameterType)
{
    Name = ...,
    TypeInfo = ...,
    // forgot ReflectionInfo
};

…and the compiler won't object. This will only blow up at runtime. A simple doc comment on ReflectionInfo explaining the dual-path contract (factory for source-gen, direct set for reflection mode) would at least communicate intent. Alternatively, an init-only RequiredReflectionInfo factory overload on ParameterMetadata itself would make the reflection-mode path self-documenting.

---

3. Thread-safety of ??= lazy init is still unacknowledged — still unaddressed

The ??= pattern is not thread-safe. Parallel test discovery (which TUnit supports) could result in concurrent first-accesses both seeing null and both calling ReflectionInfoFactory.Invoke(). For the current use case (idempotent MethodInfo.GetParameters() lookups) this is harmless duplication, but it's still a documented footgun. A one-line comment stating the double-invocation is intentionally accepted would close this.

---

New observation: stale <see cref> in GenerateReflectionInfoForParameter

After the removal of GenerateParameterInfoArrayExpression in the second commit, its doc comment reference was left behind:

/// Delegates to <see cref="GenerateParameterInfoArrayExpression"/> to avoid duplicating the reflection lookup logic.

GenerateParameterInfoArrayExpression no longer exists. This will produce a compiler warning (CS1574) and confuse future readers. The comment should be removed or rewritten.

---

Minor (carry-over): static () => null! instead of null for fallback factory

GenerateReflectionInfoForParameter returns "null!" for edge cases, producing:

reflectionInfoFactory: static () => null!

Passing reflectionInfoFactory: null directly would skip an unnecessary delegate allocation and is cleaner. Not blocking, but easy to fix alongside the stale doc comment.

---

Summary

Issue	Status
Dead code removed	✅ Addressed
WriteParameterMetadataGeneric IsNullable fix	✅ Addressed
[UnconditionalSuppressMessage] justification	✅ Already present
?? null! NRE landmine	❌ Still present
required safety regression	❌ Still present
Thread-safety unacknowledged	❌ Still present
Stale <see cref> doc comment	❌ New observation

The first three issues are correctness/safety concerns that should be resolved before merge. The stale doc comment is minor but trivially fixable alongside them.

[claude]

Code Review: perf/parameter-metadata-factory-lazy (follow-up)

The second commit (774af06c) addressed three of the five concerns from the previous review:

• ✅ Dead code removed (GenerateParameterInfoArrayExpression, GenerateParameterInfoHoistStatement, hoisted params parameters)
• ✅ WriteParameterMetadataGeneric unified into WriteParameterMetadata — the hardcoded false for IsNullable is now correctly replaced by parameter.Type.IsNullable()
• ✅ Justification on [UnconditionalSuppressMessage] is present and clear

The following items from the previous review are still unresolved:

---

1. ?? null! remains a silent NRE trap — still unaddressed

If neither ReflectionInfo (via set) nor ReflectionInfoFactory is supplied, every access silently returns null while the caller's NullReferenceException points at call sites far from the source. The old required modifier made this a compile-time error.

Recommended fix — make the failure fast and informative:

get => field ??= ReflectionInfoFactory?.Invoke()
    ?? throw new InvalidOperationException(
        $"ParameterMetadata '{Name}': ReflectionInfo was not set and no ReflectionInfoFactory was provided.");

This preserves lazy resolution for source-gen mode, sets eagerly in reflection mode, and fails loudly instead of silently for any misconfigured caller.

---

2. required removal is still a compile-time safety regression — still unaddressed

ReflectionMetadataBuilder.CreateParameterMetadata still sets ReflectionInfo directly and works fine at runtime. But dropping required means a future maintainer can forget to set it and the compiler won't object — this will only blow up at runtime. A doc comment explaining the dual-path contract (factory for source-gen, direct set for reflection mode) would at least communicate intent.

---

3. Thread-safety of ??= lazy init is still unacknowledged — still unaddressed

The ??= pattern is not thread-safe. Parallel test discovery could result in concurrent first-accesses both seeing null and both calling ReflectionInfoFactory.Invoke(). For the current use case (idempotent MethodInfo.GetParameters() lookups) this is harmless duplication, but a one-line comment stating the double-invocation is intentionally accepted would close this.

---

New observation: stale <see cref> in GenerateReflectionInfoForParameter

After removing GenerateParameterInfoArrayExpression in the second commit, its doc comment reference was left behind:

/// Delegates to <see cref="GenerateParameterInfoArrayExpression"/> to avoid duplicating the reflection lookup logic.

This will produce a compiler warning (CS1574) and confuse future readers. Should be removed or rewritten.

---

Minor (carry-over): static () => null! instead of null for fallback factory

GenerateReflectionInfoForParameter returns "null!" for edge cases, producing reflectionInfoFactory: static () => null! in generated code. Passing reflectionInfoFactory: null directly would skip an unnecessary delegate allocation and is cleaner.

---

Summary

Issue	Status
Dead code removed	✅ Addressed
WriteParameterMetadataGeneric IsNullable fix	✅ Addressed
[UnconditionalSuppressMessage] justification	✅ Already present
?? null! NRE landmine	❌ Still present
required safety regression	❌ Still present
Thread-safety unacknowledged	❌ Still present
Stale <see cref> doc comment	❌ New observation

The first three issues are correctness/safety concerns worth resolving before merge. The stale doc comment is minor but trivially fixable alongside them.

[claude]

Code Review: perf/parameter-metadata-factory-lazy (third follow-up)

Previous reviews raised five issues — here is the current status after the latest commits.

---

Progress Since Last Review

Issue	Previous Status	Current Status
?? null! NRE landmine	❌ Unresolved	✅ Fixed
Dead code removed	✅ Already addressed	✅ Still good
WriteParameterMetadataGeneric IsNullable fix	✅ Already addressed	✅ Still good
[UnconditionalSuppressMessage] justification	✅ Already addressed	✅ Still good
required safety regression	❌ Unresolved	❌ Still present
Thread-safety of ??= unacknowledged	❌ Unresolved	❌ Still present
Stale <see cref> doc comment	❌ Unresolved	❌ Still present

---

Resolved

?? null! → throw new InvalidOperationException — this was the most critical concern. The getter now fails fast and informatively:

get => field ??= ReflectionInfoFactory?.Invoke()
    ?? throw new InvalidOperationException(
        $"ReflectionInfo for parameter '{Name}' was not set and no ReflectionInfoFactory was provided.");

Good fix.

---

Still Unresolved

1. required removal is still a compile-time safety regression

Removing required from ReflectionInfo means any reflection-mode caller that forgets to set it will get a runtime InvalidOperationException instead of a compile error. The new throw makes the failure informative, but it's still later than necessary.

A lightweight mitigation — add a doc comment on the property to communicate the dual-path contract:

/// <summary>
/// The reflected ParameterInfo for this parameter.
/// In source-gen mode: set lazily via <see cref="ReflectionInfoFactory"/>.
/// In reflection mode: must be set directly by the metadata builder.
/// </summary>
public ParameterInfo ReflectionInfo { get => ...; set; }

This won't restore compile-time enforcement, but it makes the intent explicit for future maintainers.

2. Thread-safety of ??= is still unacknowledged

get => field ??= ReflectionInfoFactory?.Invoke() ?? throw ...;

With parallel test discovery, two threads can both read field == null, both invoke the factory, and both assign. For the current use case (idempotent MethodInfo.GetParameters()[i] calls returning the same stable ParameterInfo) this is harmless but wasteful. However, it should not be left as an invisible foot-gun.

A one-line comment is sufficient to close this:

// ??= is intentionally not thread-safe here; concurrent double-invocation is
// harmless since all factories return the same stable ParameterInfo instance.
get => field ??= ReflectionInfoFactory?.Invoke() ?? throw ...;

3. Stale <see cref> in GenerateReflectionInfoForParameter

After removing GenerateParameterInfoArrayExpression in a previous commit, its doc comment reference was left behind in MetadataGenerationHelper.cs:

/// Delegates to <see cref="GenerateParameterInfoArrayExpression"/> to avoid duplicating the reflection lookup logic.

GenerateParameterInfoArrayExpression no longer exists. This produces a CS1574 compiler warning and misleads future readers. The comment should be removed or rewritten to describe what the method actually does now.

---

Minor (carry-over)

static () => null! instead of null — GenerateReflectionInfoForParameter returns "null!" for edge cases (no containing method, parameter not found in signature), generating:

reflectionInfoFactory: static () => null!

With the new throw in the getter, this means these edge-case parameters will throw at first access to ReflectionInfo. Passing reflectionInfoFactory: null directly is marginally cleaner (skips an unnecessary delegate allocation) and makes the intent clearer: no factory was provided. Not blocking.

---

Summary

The most important fix (?? null! → throw) is in. The three remaining issues are all small, but items 2 (required regression) and 3 (stale cref warning) are worth a final pass before merging — item 3 in particular because it's a compiler warning.