Fix bug with nested property injection and data source caching

[thomhurst]

The source generator was incorrectly bypassing data source attribute logic by directly instantiating objects. This broke important features like caching and reuse in ClassDataSourceAttribute.

Changes:

• Remove all direct instantiation logic from DataSourceHelpersGenerator
• Delegate all property resolution to DataSourceHelpers.ResolveDataSourceForPropertyAsync
• Ensure data source attributes maintain full control over instance creation
• Fix value type comparisons for .NET Framework compatibility
• Simplify generator to only call GetDataRowsAsync on IDataSourceAttribute

This ensures that SharedType settings (PerClass, PerTestSession, etc.) work correctly and nested property injection respects the data source logic.

[claude]

Claude finished @thomhurst's task —— View job

---

PR Review Completed ✅

I've reviewed the PR focusing on code quality, best practices, potential bugs, performance, security, and test coverage.

Todo List:

• Read repository guidelines (CLAUDE.md)
• Analyze DataSourceHelpersGenerator.cs changes
• Analyze DataSourceHelpers.cs changes
• Review code quality and best practices
• Check for potential bugs or issues
• Assess performance considerations
• Review security concerns
• Evaluate test coverage implications
• Provide comprehensive feedback

---

Code Quality & Best Practices ✅

Excellent refactoring approach:

• The elimination of direct instantiation logic from the generator simplifies the architecture significantly
• Proper delegation to DataSourceHelpers.ResolveDataSourceForPropertyAsync ensures consistency
• Good separation of concerns between compile-time generation and runtime resolution

Code organization:

• Methods are well-structured with clear responsibilities
• Good use of XML documentation comments
• Consistent naming conventions throughout

Potential Issues & Bugs 🔍

1. Incomplete Implementation in ResolveDataSourcePropertyAsync

DataSourceHelpers.cs:499-504 contains a stub implementation:

public static Task<object?> ResolveDataSourcePropertyAsync(object instance, string propertyName, MethodMetadata testInformation, string testSessionId)
{
    // For now, return a default value - the runtime resolution is complex
    // In practice, this should be rare since most data sources can be resolved at compile time
    return Task.FromResult<object?>(null);
}

This method is called by the generated code (DataSourceHelpersGenerator.cs:301-302) but always returns null, which could break init-only property initialization.

2. Value Type Null Comparison

Good fix at DataSourceHelpersGenerator.cs:289-297 where value types skip null comparison:

// For value types (including tuples), we can't compare with null
if (!property.Type.IsValueType)
{
    sb.AppendLine($"        if (instance.{propertyName} == null)");
    sb.AppendLine("        {");
}

This addresses the .NET Framework compatibility issue mentioned in the PR description.

3. Reflection Access Safety

The backing field access at DataSourceHelpersGenerator.cs:303-305 could fail if the field doesn't exist:

var backingField = instance.GetType().GetField($"<{propertyName}>k__BackingField", 
    System.Reflection.BindingFlags.Instance | System.Reflection.BindingFlags.NonPublic);
backingField?.SetValue(instance, value);

The null-conditional operator ?. handles this gracefully.

Performance Considerations ⚡

Positive changes:

• Reduced code generation complexity should improve compile-time performance
• Elimination of redundant property injection logic reduces runtime overhead
• Better caching behavior through proper data source attribute delegation

Potential concerns:

• Multiple reflection calls for backing field access could be costly in hot paths
• Consider caching FieldInfo objects for frequently accessed properties

Security Concerns 🔒

Good practices:

• Proper use of DynamicallyAccessedMembersAttribute at DataSourceHelpers.cs:445
• No direct constructor invocation or unsafe operations
• Reflection usage is minimal and controlled

No significant security issues identified.

Test Coverage Assessment 🧪

Areas needing attention:

1. Init-only property initialization - The core bug fix around nested property injection should have comprehensive tests
2. Data source caching behavior - Tests should verify that SharedType settings (PerClass, PerTestSession) work correctly
3. Value type handling - Tests for the .NET Framework compatibility fix
4. Reflection fallback scenarios - Edge cases where backing field access fails

Recommended test scenarios:

• Nested classes with init-only data source properties
• Value types with data source attributes
• Different SharedType configurations
• Error handling when reflection fails

Recommendations 🎯

1. Complete the stub implementation in ResolveDataSourcePropertyAsync or add proper error handling
2. Add performance optimization by caching reflection metadata
3. Enhance test coverage for the specific scenarios this PR addresses
4. Consider adding integration tests that verify the caching behavior works end-to-end

Overall Assessment ⭐

This is a solid refactoring that addresses important architectural issues. The move away from direct instantiation to proper delegation is the right approach. The main concern is the incomplete implementation of ResolveDataSourcePropertyAsync, which could cause runtime failures.

Recommendation: Approve after addressing the stub implementation.

---