Skip to content

refactor: PropertyInjector complexity and allocation improvements#4931

Merged
thomhurst merged 1 commit into
mainfrom
refactor/property-injector
Feb 20, 2026
Merged

refactor: PropertyInjector complexity and allocation improvements#4931
thomhurst merged 1 commit into
mainfrom
refactor/property-injector

Conversation

@thomhurst

Copy link
Copy Markdown
Owner

Summary

  • Cache PropertyInfo reflection lookups via a static ConcurrentDictionary<(Type, string), PropertyInfo?> to eliminate repeated GetProperty calls in InjectSourceGeneratedPropertyAsync and RecurseIntoNestedPropertiesCoreAsync
  • Fix duplicate reflection in CreateDataGeneratorMetadata where PropertyHelper.GetPropertyInfo was called twice for the same property (once for ReflectionInfo, once for Getter lambda) -- now resolved once and reused
  • Use factory methods from PropertyInitializationContext (ForSourceGenerated, ForReflection, ForCaching, ForReflectionCaching) instead of manual object initializers across 4 call sites, reducing duplication
  • Extract focused methods: InjectPropertiesFromPlanAsync, RecurseIntoPropertyValueAsync, CreateSourceGeneratedDataGeneratorMetadata, RentVisitedDictionary/ReturnVisitedDictionary to reduce cyclomatic complexity
  • Remove redundant condition checks where HasProperties was already verified before checking SourceGeneratedProperties.Length > 0 || ReflectionProperties.Length > 0
  • Replace foreach with indexed for loops to avoid enumerator allocations on arrays
  • Convert ResolveAndCachePropertiesCoreAsync from async to synchronous Task return to avoid unnecessary state machine allocation
  • Simplify GetInitializedDataSourceAsync with null-coalescing operator

All changes are pure refactoring -- behavior is preserved exactly.

Closes #4897

Test plan

  • dotnet build TUnit.Engine/TUnit.Engine.csproj succeeds on all TFMs (netstandard2.0, net8.0, net9.0, net10.0)
  • CI passes all existing tests
  • No snapshot test changes expected (this is engine-only, no public API or source generator changes)

@claude

claude Bot commented Feb 19, 2026

Copy link
Copy Markdown
Contributor

Code review

Overall this is a clean, well-executed refactoring. The method extractions reduce cyclomatic complexity, the async→synchronous-Task conversion in ResolveAndCachePropertiesCoreAsync correctly eliminates the state machine allocation, and the object pool encapsulation in RentVisitedDictionary/ReturnVisitedDictionary is a nice improvement. No behavioral regressions were found.

Design observation: GetCachedPropertyInfo vs PropertyHelper.GetPropertyInfo

The new GetCachedPropertyInfo calls Type.GetProperty(name) with default binding flags (public instance-only):

[UnconditionalSuppressMessage("Trimming", "IL2080", Justification = "Source-gen properties have their types preserved at compile time")]
private static PropertyInfo? GetCachedPropertyInfo(Type containingType, string propertyName)
{
return _propertyInfoCache.GetOrAdd((containingType, propertyName), key => key.Item1.GetProperty(key.Item2));
}

The codebase already has PropertyHelper.GetPropertyInfo which uses broader binding flags (Public | NonPublic | Instance | Static) to support internal, protected, and static injectable properties. Note that the original call sites this PR replaces also used default flags, so this isn't a regression introduced here — but since you're now the canonical lookup helper, it's a good opportunity to be consistent with the rest of the codebase.

Consider delegating to PropertyHelper.GetPropertyInfo inside the cache, converting the exception to null to preserve the nullable return:

[UnconditionalSuppressMessage("Trimming", "IL2080", Justification = "Source-gen properties have their types preserved at compile time")]
private static PropertyInfo? GetCachedPropertyInfo(Type containingType, string propertyName)
{
    return _propertyInfoCache.GetOrAdd((containingType, propertyName), static key =>
    {
        try { return PropertyHelper.GetPropertyInfo(key.Item1, key.Item2); }
        catch { return null; }
    });
}

This would:

  1. Be consistent with the existing PropertyHelper abstraction.
  2. Fix the pre-existing limitation for non-public/static injectable properties at these call sites.
  3. Unify the lookup semantics used in InjectSourceGeneratedPropertyAsync and RecurseIntoNestedPropertiesCoreAsync with the rest of the injection pipeline.

This is a suggestion for future improvement rather than a blocker — the PR as-is correctly preserves the existing behaviour.

@thomhurst thomhurst force-pushed the refactor/property-injector branch from 7ee0ba4 to 31bdcca Compare February 19, 2026 09:39
@thomhurst thomhurst force-pushed the refactor/property-injector branch from 31bdcca to 371889f Compare February 19, 2026 11:46
@thomhurst thomhurst force-pushed the refactor/property-injector branch from 371889f to 86bb181 Compare February 19, 2026 14:03
- Cache PropertyInfo lookups via static ConcurrentDictionary to avoid
  repeated reflection calls in source-gen paths
- Extract InjectPropertiesFromPlanAsync and RecurseIntoPropertyValueAsync
  to reduce cyclomatic complexity of InjectPropertiesRecursiveAsync
- Extract CreateSourceGeneratedDataGeneratorMetadata to fix duplicate
  PropertyHelper.GetPropertyInfo call (was called twice per property)
- Extract RentVisitedDictionary/ReturnVisitedDictionary for pool ops
- Use PropertyInitializationContext factory methods instead of manual
  object initializers, reducing duplication across 4 call sites
- Replace foreach with indexed for loops to avoid enumerator allocations
- Simplify GetInitializedDataSourceAsync with null-coalescing operator
- Remove redundant condition checks where HasProperties already implies
  properties exist
- Convert ResolveAndCachePropertiesCoreAsync from async to synchronous
  Task return to avoid unnecessary state machine allocation

Closes #4897
@thomhurst thomhurst force-pushed the refactor/property-injector branch from 466ebca to 0690fd0 Compare February 19, 2026 16:40
@thomhurst thomhurst merged commit 900a423 into main Feb 20, 2026
14 checks passed
@thomhurst thomhurst deleted the refactor/property-injector branch February 20, 2026 17:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

refactor: PropertyInjector complexity and allocation improvements

1 participant