perf: eliminate closure and Func creation

[TimothyMakkison]

Following on from #4479

Use function overloads to pass state into update methods.

I've used UnconditionalSuppressMessage to suppress the following error,
IL2111 - Method with parameters or return value with `DynamicallyAccessedMembersAttribute` is accessed via reflection. Trimmer can't guarantee availability of the requirements of the method.

Before

After

[thomhurst]

Summary

Performance optimization that eliminates closure and Func allocations in hot paths by converting to static lambdas with explicit state parameters.

Critical Issues

None found ✅

Suggestions

Consider the IL2111 Suppression Carefully

The [UnconditionalSuppressMessage("ReflectionAnalysis", "IL2111")] suppression in ContextProvider.GetOrCreateClassContext warrants careful consideration:

Current approach: The PR adds the suppression with the justification "Type parameter is annotated at the method boundary."

Why this is reasonable:

• The static lambda's type parameter IS properly annotated with [DynamicallyAccessedMembers(...)]
• The warning occurs because ConcurrentDictionary.GetOrAdd internally uses the type, and the trimmer conservatively warns about reflection access
• The actual runtime behavior is safe since the type flows directly from the method parameter to the lambda parameter, both with proper annotations

Recommendation: The suppression appears justified. The alternative would be to keep the closure (defeating the performance goal) or restructure the code in a more complex way. The justification message could be slightly more detailed:

[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2111",
    Justification = "Type parameter is properly annotated with DynamicallyAccessedMembers at both the method parameter and static lambda parameter. The trimmer warning is due to ConcurrentDictionary.GetOrAdd's internal reflection usage, but the type flows safely from the annotated method parameter.")]

However, this is a minor improvement and not blocking.

Verify Performance Impact

The performance improvement shown in the screenshots looks excellent (from 1087 width to 619 width in the allocations). Consider:

• Running before/after benchmarks if available
• Verifying this is indeed a hot path (which it appears to be, given it's in test discovery)

Verdict

✅ APPROVE - No critical issues

This is a solid performance optimization that aligns perfectly with TUnit's "Performance First" principle. The closure elimination in hot paths (test discovery and context creation) will reduce allocations without changing behavior. The IL2111 suppression is reasonable given the proper annotations on both the method and lambda parameters.

[thomhurst]

Summary

Eliminates closure and Func allocations by using state-passing overloads in GetOrAdd and ImmutableInterlocked.Update.

Critical Issues

None found ✅

Suggestions

Minor: Consider IL2111 suppression placement

The UnconditionalSuppressMessage for IL2111 on GetOrCreateClassContext (TUnit.Core/ContextProvider.cs:64-65) is appropriate and follows the same pattern as PropertyCacheManager.GetCachedProperties.

However, I noticed that ReflectionTestDataCollector already has a class-level IL2111 suppression (line 25), so the changes in that file don't require any additional suppressions - they're already covered.

The suppression justification is correct: the DynamicallyAccessedMembers annotation on the method parameter ensures the type passed to the lambda has the required annotations, even though the trimmer can't statically verify this through the lambda boundary.

Minor: Pattern consistency verification

This change makes GetOrCreateClassContext consistent with GetOrCreateAssemblyContext (which already uses the static lambda + state pattern). Good consistency!

The transformation is correct:

• Before: Lambda captures classType from outer scope → closure allocation
• After: Static lambda receives type and state parameters → no closure

Both transformations in ReflectionTestDataCollector.cs correctly pass the test/dynamicTest as state to avoid capturing loop variables.

Previous Review Status

No previous review comments found.

Verdict

✅ APPROVE - No critical issues

This is a clean performance optimization that:

• Aligns with TUnit Rule 4 (Performance First)
• Follows established patterns in the codebase
• Correctly annotates AOT/trimming concerns
• Improves performance in hot paths (test discovery)

The closure elimination should reduce allocations during test discovery, which is particularly valuable given TUnit's focus on processing millions of tests efficiently.

[thomhurst]

Summary

Performance optimization that eliminates closure and Func allocations in hot paths by using static lambdas with state-passing overloads.

Critical Issues

None found ✅

Suggestions

IL2111 Suppression Pattern - Consider Alignment with PropertyCacheManager

The IL2111 suppression you've added to GetOrCreateClassContext (TUnit.Core/ContextProvider.cs:64-65) is correct and necessary. However, I noticed a small inconsistency with the existing pattern in PropertyCacheManager.GetCachedProperties:

Your justification:

Justification = "Type parameter is annotated at the method boundary."

PropertyCacheManager pattern (line 41-42):

Justification = "CreatePropertyArray is called with a type that has the required DynamicallyAccessedMembers annotation from the caller."

Both are correct, but for consistency across the codebase, consider using a more explicit justification that matches the established pattern:

[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2111",
    Justification = "Static lambda receives type parameter with required DynamicallyAccessedMembers annotation from the method boundary.")]

This makes it clearer why it's safe (the lambda receives the annotated parameter) rather than just stating that the parameter is annotated.

Previous Review Status

Two previous reviews by thomhurst already approved this PR. All points raised were informational/confirmatory rather than requesting changes.

Verdict

✅ APPROVE - No critical issues

This is an excellent performance optimization that:

• Aligns with TUnit Rule 4 (Performance First): Eliminates allocations in hot paths (test discovery and context creation)
• Follows existing patterns: Matches the static lambda + state pattern already used in GetOrCreateAssemblyContext (line 54-58)
• Proper AOT annotations: Correctly annotates the lambda parameter with DynamicallyAccessedMembers
• Already covered suppressions: The ReflectionTestDataCollector changes are covered by the class-level IL2111 suppression (line 25)

The transformation is textbook-perfect:

• Before: Lambda captures classType → closure allocation
• After: Static lambda + state parameter → zero closures

Great work on following #4479's pattern!

[thomhurst]

@TimothyMakkison if you rebase off of the latest main, is Sourcy still broken?

[TimothyMakkison]

Manullay updated API change tests and renamed category from "ReflectionAnalysis" to "Trimming"

@TimothyMakkison if you rebase off of the latest main, is Sourcy still broken?

I could try from the terminal or VS but it doesn't look like it was fixed 😢