Enhance property injection to support inherited data source attributes

[thomhurst]

Fixes #2955

[claude]

Claude finished @thomhurst's task —— View job

---

Code Review for PR #3144

✅ Overall Assessment: Looks Good with Minor Considerations

This PR successfully addresses issue #2955 by adding support for inherited data source attributes in property injection. The implementation is solid and follows TUnit's architectural patterns.

✅ Strengths

1. Clean Architecture & Design

• Clean separation between source-generated and reflection modes in PropertyInjectionPlanBuilder.cs:19-33 vs PropertyInjectionPlanBuilder.cs:38-82
• Proper use of BindingFlags.DeclaredOnly to walk inheritance hierarchy correctly (PropertyInjectionPlanBuilder.cs:50)
• Smart property override handling with HashSet<string> to respect derived class overrides (PropertyInjectionPlanBuilder.cs:43,56-59)

2. Comprehensive Test Coverage

• Excellent reproduction test case for the original issue (InheritedDataSourceTests.cs:50-84)
• Multi-level inheritance testing (MultiLevelInheritanceTests.cs:104-120)
• Both positive and control test cases included

3. AOT/Trimming Compatibility

• Proper suppression attributes for reflection usage (PropertyInjectionPlanBuilder.cs:38-39)
• Follows TUnit's AOT-friendly patterns

⚠️ Areas for Consideration

1. Performance Impact

• The inheritance chain traversal (PropertyInjectionPlanBuilder.cs:46-73) adds O(n) complexity per inheritance level
• Consider caching reflection results for types that are used repeatedly
• Recommendation: Since TUnit emphasizes performance, consider adding a static concurrent dictionary to cache property injection plans by type

2. Source Generator Parity

• The enhancement only affects reflection mode (BuildReflectionPlan)
• Critical Question: Does the source generator also need to support inherited attributes to maintain behavioral parity per TUnit's Critical Rule Repeat attributes  #1?
• Recommendation: Verify that the source generator handles inheritance correctly or update it accordingly

3. Minor Code Quality

// Line 51: This condition could be simplified
.Where(p => p.CanWrite || p.SetMethod?.IsPublic == false)
// Consider: .Where(p => p.CanWrite || p.SetMethod != null)

🔍 Security & Reliability

No Security Concerns Identified

• Proper use of reflection with appropriate trimming suppressions
• No exposure of sensitive data or unvalidated input

Thread Safety Considerations

• Static methods are inherently thread-safe for read operations
• The inheritance traversal creates new collections each time, avoiding shared state issues

📋 Testing Recommendations

Consider Adding:

1. Edge Case Tests: Empty inheritance chains, interfaces, abstract classes
2. Performance Tests: Measure impact on test discovery/execution time
3. Snapshot Tests: Run dotnet test TUnit.Core.SourceGenerator.Tests to ensure source generator output hasn't changed

🏁 Before Merge Checklist

• Verify source generator handles inheritance (behavioral parity requirement)
• Run performance benchmarks to quantify impact
• Consider adding property injection plan caching for frequently used types

📖 Files Reviewed

• TUnit.Core/PropertyInjection/PropertyInjectionPlanBuilder.cs - Core implementation
• TUnit.TestProject/Bugs/2955/InheritedDataSourceTests.cs - Test coverage

This is a well-implemented fix that addresses the reported issue while maintaining code quality standards. The main consideration is ensuring the source generator maintains behavioral parity with this reflection enhancement.