Detect injected properties in base classes

[thomhurst]

Fixes #3072

[claude]

Claude finished @thomhurst's task —— View job

---

PR Review: Detect injected properties in base classes

Reviewing this pull request that fixes issue #3072 related to detecting injected properties in base classes.

Todo List:

• Read repository guidelines (CLAUDE.md)
• Understand the bug being fixed by examining test files
• Analyze core implementation changes
• Review code quality and best practices
• Check for potential bugs or issues
• Assess performance considerations
• Evaluate security concerns
• Review test coverage
• Provide comprehensive feedback

Understanding the Issue

Bug #3072 involved a case where property injection wasn't working correctly for properties defined in base classes. The test case shows:

1. BaseClass has a required property TestData with a data source attribute
2. TestFactory inherits from BaseClass and implements IAsyncInitializer
3. In TUnit v0.57.24, TestData was null during initialization because the property injection system wasn't detecting properties from base classes

✅ Code Quality & Best Practices

Excellent improvements:

• Inheritance-aware property detection: The fix properly traverses the inheritance hierarchy using GetMembersIncludingBase() in PropertyInjectionSourceGenerator.cs:67-69
• Deduplication logic: Added processedProperties HashSet to prevent duplicate property processing (PropertyInjectionSourceGenerator.cs:65,73-76)
• Correct type targeting: Uses property.ContainingType instead of derived class type for UnsafeAccessor methods (PropertyInjectionSourceGenerator.cs:234,318-321)
• Code organization: Well-structured helper class RequiredPropertyHelper that separates concerns

Modern C# compliance:

• Uses collection initializers: new List<PropertyWithDataSourceAttribute>()
• Proper nullable annotations throughout
• Expression-bodied members where appropriate
• Clean, descriptive variable names

✅ Bug Fixes & Correctness

Primary fix implemented correctly:

• Properties from base classes are now discovered and included in source generation
• Backing field access correctly targets the property's defining type, not the derived type
• Prevents property name collisions through deduplication

Edge cases handled:

• Skip abstract, non-public, and unbound generic types (PropertyInjectionSourceGenerator.cs:36,43-52)
• Proper null checks for semantic model and interface resolution
• Handles nested types correctly in accessibility checks

✅ Performance Considerations

Performance optimizations:

• Deduplication prevents duplicate source generation (PropertyInjectionSourceGenerator.cs:147-150)
• Early filtering of unsuitable types before expensive symbol processing
• Stable naming for generated classes using deterministic hashing (PropertyInjectionSourceGenerator.cs:361-365)
• Uses SymbolEqualityComparer.Default for efficient symbol comparisons

Potential concerns addressed:

• The inheritance traversal is bounded (stops at System.Object)
• HashSet lookup for processed properties is O(1)

✅ Security Considerations

Safe reflection usage:

• UnsafeAccessor is properly constrained to .NET 8+ with preprocessor directives
• Falls back to reflection with proper binding flags for older frameworks
• No dynamic code generation or unsafe type casting beyond intended use

✅ Test Coverage

Comprehensive test coverage:

• New test case specifically for the reported bug (TUnit.TestProject/Bugs/3072/Tests.cs)
• Updated snapshot tests across multiple scenarios show the fix works correctly
• Tests cover inheritance scenarios, property injection, and async initialization
• Snapshots verify both source-generated and reflection modes maintain parity

Test quality:

• Test demonstrates actual failure scenario (null property access)
• Expected result verification with proper assertion
• Covers real-world usage pattern with data sources and async initialization

✅ Architecture Adherence

Follows TUnit principles:

• Behavioral parity: Changes maintain consistency between source-generated and reflection modes
• Performance-first: Optimizes for compile-time generation over runtime reflection
• Modern C#: Leverages latest language features appropriately

📝 Minor Suggestions

1. RequiredPropertyHelper.cs:20: Consider caching the data source interface lookup to avoid repeated compilation queries
2. Error handling: Could add more descriptive error messages for accessibility failures, though current approach is adequate
3. Documentation: The code is self-documenting, but additional XML documentation on public methods would be beneficial

✅ Overall Assessment

This is a high-quality fix that addresses the reported issue comprehensively:

• ✅ Fixes the bug: Properties from base classes are now properly detected and injected
• ✅ Maintains performance: Optimized for both compile-time and runtime efficiency
• ✅ Preserves architectural integrity: Follows TUnit's dual execution mode principles
• ✅ Comprehensive testing: Includes specific test case and updated snapshots
• ✅ Clean implementation: Well-structured, readable code following project conventions

The implementation correctly handles inheritance hierarchies while maintaining the framework's performance characteristics and architectural goals. The extensive snapshot test updates demonstrate thorough verification of the changes.

Recommendation: ✅ Approve and merge

---

[claude]

Claude finished @thomhurst's task —— View job

---

PR Review: Detect injected properties in base classes

Reviewing this pull request that fixes issue #3072 related to detecting injected properties in base classes.

Todo List:

• Read repository guidelines (CLAUDE.md)
• Understand the bug being fixed by examining test files
• Analyze core implementation changes
• Review code quality and best practices
• Check for potential bugs or issues
• Assess performance considerations
• Evaluate security concerns
• Review test coverage
• Provide comprehensive feedback

Understanding the Issue

Bug #3072 involved a case where property injection wasn't working correctly for properties defined in base classes. The test case shows:

1. BaseClass has a required property TestData with a data source attribute
2. TestFactory inherits from BaseClass and implements IAsyncInitializer
3. In TUnit v0.57.24, TestData was null during initialization because the property injection system wasn't detecting properties from base classes

✅ Code Quality & Best Practices

Excellent improvements:

• Inheritance-aware property detection: The fix properly traverses the inheritance hierarchy using GetMembersIncludingBase() in PropertyInjectionSourceGenerator.cs:67-69
• Deduplication logic: Added processedProperties HashSet to prevent duplicate property processing (PropertyInjectionSourceGenerator.cs:65,73-76)
• Correct type targeting: Uses property.ContainingType instead of derived class type for UnsafeAccessor methods (PropertyInjectionSourceGenerator.cs:234,318-321)
• Code organization: Well-structured helper class RequiredPropertyHelper that separates concerns

Modern C# compliance:

• Uses collection initializers: new List<PropertyWithDataSourceAttribute>()
• Proper nullable annotations throughout
• Expression-bodied members where appropriate
• Clean, descriptive variable names

✅ Bug Fixes & Correctness

Primary fix implemented correctly:

• Properties from base classes are now discovered and included in source generation
• Backing field access correctly targets the property's defining type, not the derived type
• Prevents property name collisions through deduplication

Edge cases handled:

• Skip abstract, non-public, and unbound generic types (PropertyInjectionSourceGenerator.cs:36,43-52)
• Proper null checks for semantic model and interface resolution
• Handles nested types correctly in accessibility checks

✅ Performance Considerations

Performance optimizations:

• Deduplication prevents duplicate source generation (PropertyInjectionSourceGenerator.cs:147-150)
• Early filtering of unsuitable types before expensive symbol processing
• Stable naming for generated classes using deterministic hashing (PropertyInjectionSourceGenerator.cs:361-365)
• Uses SymbolEqualityComparer.Default for efficient symbol comparisons

Potential concerns addressed:

• The inheritance traversal is bounded (stops at System.Object)
• HashSet lookup for processed properties is O(1)

✅ Security Considerations

Safe reflection usage:

• UnsafeAccessor is properly constrained to .NET 8+ with preprocessor directives
• Falls back to reflection with proper binding flags for older frameworks
• No dynamic code generation or unsafe type casting beyond intended use

✅ Test Coverage

Comprehensive test coverage:

• New test case specifically for the reported bug (TUnit.TestProject/Bugs/3072/Tests.cs)
• Updated snapshot tests across multiple scenarios show the fix works correctly
• Tests cover inheritance scenarios, property injection, and async initialization
• Snapshots verify both source-generated and reflection modes maintain parity

Test quality:

• Test demonstrates actual failure scenario (null property access)
• Expected result verification with proper assertion
• Covers real-world usage pattern with data sources and async initialization

✅ Architecture Adherence

Follows TUnit principles:

• Behavioral parity: Changes maintain consistency between source-generated and reflection modes
• Performance-first: Optimizes for compile-time generation over runtime reflection
• Modern C#: Leverages latest language features appropriately

📝 Minor Suggestions

1. RequiredPropertyHelper.cs:20: Consider caching the data source interface lookup to avoid repeated compilation queries
2. Error handling: Could add more descriptive error messages for accessibility failures, though current approach is adequate
3. Documentation: The code is self-documenting, but additional XML documentation on public methods would be beneficial

✅ Overall Assessment

This is a high-quality fix that addresses the reported issue comprehensively:

• ✅ Fixes the bug: Properties from base classes are now properly detected and injected
• ✅ Maintains performance: Optimized for both compile-time and runtime efficiency
• ✅ Preserves architectural integrity: Follows TUnit's dual execution mode principles
• ✅ Comprehensive testing: Includes specific test case and updated snapshots
• ✅ Clean implementation: Well-structured, readable code following project conventions

The implementation correctly handles inheritance hierarchies while maintaining the framework's performance characteristics and architectural goals. The extensive snapshot test updates demonstrate thorough verification of the changes.

Recommendation: ✅ Approve and merge

---

[aradalvand]

@thomhurst I think this caused a bug: #3084
Probably this part, though I didn't inspect the PR too meticulously.