-
-
Notifications
You must be signed in to change notification settings - Fork 108
fix: add source generation for generic types with data source properties #4434
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
When SourceRegistrar.IsEnabled is true globally but no source-generated metadata exists for a specific type (e.g., user's WebApplicationFactory), the code incorrectly used source-gen mode with an empty array instead of falling back to reflection. This caused nested IAsyncInitializer properties to not be discovered during object tracking, leading to them not being initialized before BeforeTest hooks run. The fix simplifies the logic: use source-gen mode only when plan.SourceGeneratedProperties actually has content, otherwise fall back to reflection. Fixes #4431 Co-Authored-By: Claude Opus 4.5 <[email protected]>
SummaryFixes incorrect source-gen vs reflection fallback logic in object graph discovery to properly handle types without source-generated metadata. Critical IssuesNone found ✅ Suggestions1. Consider updating the comment for clarityThe comment at line 345-348 (now lines 345-347 after the change) could be updated to better reflect the new unified behavior: // Both modes should use source-gen ONLY if we actually have source-generated properties.
// This ensures fallback to reflection when source-gen is enabled but no metadata was generated
// for a specific type (e.g., user's WebApplicationFactory that wasn't processed by the generator).This is already quite clear, but you might consider simplifying it to: // Use source-gen only when metadata is available for this specific type.
// Falls back to reflection when source-gen is globally enabled but this type
// wasn't processed by the generator (e.g., user types like WebApplicationFactory).2. Verify dual-mode behaviorAccording to TUnit's Rule #1 (Dual-Mode Implementation), changes to metadata collection should be tested in both source-gen and reflection modes. While the fix looks correct, consider:
3. Alignment with PropertyInjectionPlanBuilderThe change correctly aligns with the existing fallback logic in Verdict✅ APPROVE - The fix correctly addresses the root cause by ensuring both modes check for actual metadata presence rather than global flags. The simplification also improves code clarity by removing the dual-mode branching logic. |
When a derived class has source-gen registration for IAsyncInitializer properties, base class properties with IAsyncInitializer were being missed because the code returned early after processing the derived class. Now TraverseInitializerProperties walks up the inheritance chain and processes source-gen metadata from all types in the hierarchy. Property name tracking ensures overridden properties in derived classes take precedence. If no type in the hierarchy has source-gen registration, we fall back to reflection which already handles inheritance correctly. Co-Authored-By: Claude Opus 4.5 <[email protected]>
SummaryFixes incorrect source-gen vs reflection fallback logic and adds inheritance chain traversal for IAsyncInitializer properties. Critical IssuesNone found ✅ Suggestions1. Consider edge case: Multiple inheritance levels with mixed source-gen registrationIn Current behavior: If ANY type in the hierarchy has source-gen registration, reflection is never used (even for types without registration in the chain). Example scenario:
What happens: The loop finds Question: Is this the intended behavior? Should base class properties without source-gen metadata still be discovered via reflection when the derived class has source-gen? Or does the source generator always emit metadata for ALL inherited If the source generator handles inheritance correctly and always includes base class properties in the derived class's metadata, then this is fine. Otherwise, you might need to track 2. Property name collision handling in
|
Design document for fixing the critical gap where source generator skips generic types entirely. This breaks AOT compatibility for generic test fixtures like WebApplicationFactory<TProgram>. The solution generates metadata for concrete instantiations of generic types discovered at compile time through: - Inheritance chains - IDataSourceAttribute type arguments - Base type arguments Relates to #4431 Co-Authored-By: Claude Opus 4.5 <[email protected]>
…ties Implements three new pipelines in PropertyInjectionSourceGenerator: - Pipeline 3: Discovers concrete generic types from inheritance chains - Pipeline 4: Discovers concrete generic types from ClassDataSource<T> attributes - Pipeline 5: Generates InitializerPropertyRegistry for generic IAsyncInitializer types This enables AOT-compatible property injection for patterns like WebApplicationFactory<TProgram> with ClassDataSource properties. Fixes #4431 Co-Authored-By: Claude Opus 4.5 <[email protected]>
SummaryAdds source generation for concrete generic type instantiations to support WebApplicationFactory-style patterns in AOT scenarios. Critical IssuesNone found ✅ Analysis✅ Dual-Mode Compliance (Rule #1)PASS - Changes maintain dual-mode operation: Source Generator (TUnit.Core.SourceGenerator/Generators/PropertyInjectionSourceGenerator.cs:70-106)
Reflection Fallback (TUnit.Core/Discovery/ObjectGraphDiscoverer.cs:345-356)
✅ AOT Compatibility (Rule #5)PASS - All code is AOT-compatible:
✅ Snapshot Testing (Rule #2)N/A - No existing snapshot files modified. This PR adds new functionality without changing existing source generator output format. ✅ Performance (Rule #4)PASS - No performance concerns:
✅ No VSTest Dependencies (Rule #3)PASS - No VSTest dependencies introduced. Code Quality ObservationsStrengths
Minor Observations (Non-blocking)
Verdict✅ APPROVE - No critical issues. This is a well-designed solution to issue #4431 that properly implements TUnit's dual-mode architecture for generic types. |
Summary
Comprehensive fix for generic type support in property injection, ensuring AOT compatibility for patterns like
WebApplicationFactory<TProgram>.Changes
Source generation for generic types - Added three new pipelines to discover and generate metadata for concrete generic type instantiations:
MyTests : GenericBase<MyProgram>)ClassDataSource<T>attributesInitializerPropertyRegistryfor genericIAsyncInitializertypesFix incorrect source-gen vs reflection fallback - When
SourceRegistrar.IsEnabledis true globally but no source-generated metadata exists for a specific type, the code now properly falls back to reflectionWalk inheritance chain for IAsyncInitializer properties - When a derived class has source-gen registration, base class
IAsyncInitializerproperties are now also discoveredProblem
When using
ClassDataSourcewith generic types likeCustomWebApplicationFactory<TProgram>that haveIAsyncInitializerproperties, the source generator was skipping these types entirely because they were generic. This forced a fallback to reflection which breaks AOT compatibility.Example pattern that was broken:
Solution
The source generator now:
ClassDataSource<T>attribute type arguments for generic typesPropertySourceRegistry.RegisterandInitializerPropertyRegistry.Registercalls for these concrete typesTest plan
TUnit.Core.SourceGenerator.Testspass (376 tests)Test coverage includes:
List<string>)Fixes #4431
🤖 Generated with Claude Code