You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Preserve generic class source rows and distinguish null collections - #7453
Null collections returned by generated data-source factories were converted into a null constructor row. Treat null collections as empty across synchronous, Task, ValueTask and async-enumerable factories, including instance variants, while retaining null elements and scalar null values.
Extract the runtime support from #7422: inferred class-source filtering and constructor-position type mapping, fresh factory values for subsequent reflection-built tests, and row-backed constraint violations. The new generated-code configuration method and optional violation sources are intentional additive public APIs on EditorBrowsable(Never) types. Keep the current generator and reflection inference rules; #7446 owns enabling the remaining inference feature. Include the independent PublicAPI scrubber correction for wrapped .cs extensions.
PublicAPI net10.0: 20 passed; reviewed and updated Core snapshots for all targets. Other runtime legs and Windows net472 remain for CI.
Core and Engine net10.0 builds passed. Engine.Tests integration cases added for CI; not run locally per repository instructions.
Native AOT publish and targeted execution: Release net10.0/osx-arm64 publish succeeded; all 7 new fixtures passed in the native executable.
Native publishing reports existing fixture warnings. Generic AutoFixture annotation warnings are tracked in #7452; generated hook warnings are already tracked in #7430. No warnings name the changed runtime paths.
Summary by CodeRabbit
Bug Fixes
Null collection results from data sources now contribute no test rows, while null elements within a collection remain valid rows.
Factory-backed test rows now receive fresh values for subsequent evaluations, helping prevent shared or already-disposed objects from affecting tests.
Generic data-source row inference and filtering now handle declared type arguments and empty sources more consistently.
Constraint violations now reflect the actual rows supplied by their data sources.
No actionable comments were generated in the recent review.
Walkthrough
The PR updates generic class-row inference, source filtering, and factory reuse; treats null collection results as no rows while preserving null elements and scalar values; and supports data-source-backed constraint violations. It also adjusts source-path matching for concatenated .cs literals.
Changes: Generic class-row handling
Layer / File(s)
Summary
Configure and filter inferred rows src/TUnit.Core/Attributes/TestData/GenericClassRowPartition.cs, src/TUnit.Core/Attributes/TestData/MethodDataSourceAttribute.cs, src/TUnit.Engine/Discovery/ReflectionTestDataCollector.cs, tests/TUnit.UnitTests/GenericClassSourceRuntimeTests.cs, tests/TUnit.PublicAPI/Tests.Core_Library_Has_No_API_Changes.*.verified.txt
Method sources can carry inferred class type arguments and filter rows. Partitioning checks declared row types and row eligibility before using runtime type inference.
Retain and invoke row factories src/TUnit.Core/Attributes/TestData/StaticDataSourceAttribute.cs, src/TUnit.Engine/Discovery/ReflectionTestDataCollector.cs, tests/TUnit.TestProject/Bugs/_7445/FreshFactoryTests.cs, tests/TUnit.Engine.Tests/Issue7445Tests.cs, tests/TUnit.UnitTests/GenericClassSourceRuntimeTests.cs
Discovery stores row factories with first-fetch values. Static sources use each first-fetch value once, then invoke the retained factory on later fetches.
Null collection results produce no rows. Null elements and null scalar results remain rows.
Represent constraint violations as data rows src/TUnit.Core/GenericConstraintViolationEntry.cs, tests/TUnit.UnitTests/GenericClassSourceRuntimeTests.cs, tests/TUnit.PublicAPI/Tests.Core_Library_Has_No_API_Changes.*.verified.txt
Constraint-violation metadata uses supplied data sources when present. Otherwise, it creates unconditional sources from violation messages.
Source-path scrubbing
Layer / File(s)
Summary
Match split source extensions tests/TUnit.PublicAPI/Tests.cs, tests/TUnit.PublicAPI/SourcePathScrubberTests.cs
The source-path pattern matches concatenated literal boundaries within .cs. The test checks path replacement and line-number retention.
sequenceDiagram
participant Collector as ReflectionTestDataCollector
participant Source as MethodDataSourceAttribute
participant Partition as GenericClassRowPartition
participant StaticSource as StaticDataSourceAttribute
Collector->>Source: read row values and factories
Collector->>Partition: evaluate rows for type arguments
Collector->>StaticSource: store first-fetch values and factories
StaticSource->>StaticSource: use first-fetch value or invoke retained factory
Loading
Possibly related PRs
thomhurst/TUnit#7422: Adds the generic class-source inference and filtering implementation that this PR extracts and updates.
thomhurst/TUnit#7339: Introduces runtime generic class-row partitioning and shared factory evaluation, which this PR continues.
thomhurst/TUnit#7336: Adds discovery for own-class method-source rows on generic classes, which this PR modifies to filter rows and retain factories.
Merge Risk: Minimal · previous review 6dc7a
This change fixes null-collection handling and fresh factory values for generic class data sources. No actionable merge-blocking risk remains; one minor edge case in the hidden constraint-violation data source API is not reachable from generated or reflection paths.
Pre-merge checks: 4 · 1
Failed checks: 1 warning
Check name
Status
Explanation
Resolution
Docstring Coverage
Docstring coverage is 19.23% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta…
Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed checks: 4 passed
Check name
Status
Explanation
Description Check
Check skipped - CodeRabbit’s high-level summary is enabled.
Title check
The title "Preserve generic class source rows and distinguish null collections" matches the main change in the objectives: null collections from generated data-source factories are treated as empty, n…
Linked Issues check
The changes satisfy the coding requirements in #7445. DataSourceFactories treats null collections as empty for synchronous, Task, ValueTask, and async-enumerable paths while retaining null eleme…
Out of Scope Changes check
The changed production files, regression fixtures, integration tests, PublicAPI baselines, and path-scrubber test have a direct connection to #7445. The path-scrubber correction is explicitly included…
Full details: Title check
Explanation
The title "Preserve generic class source rows and distinguish null collections" matches the main change in the objectives: null collections from generated data-source factories are treated as empty, null elements and scalar nulls are kept, and generic class source rows are preserved. The raw summary confirms the changes to DataSourceFactories.cs, the StaticDataSourceAttribute first-fetch handling, and the new null-collection tests. The title is short, specific, and free of file lists or vague terms. It does not mention the other runtime pieces, such as the new ConfigureInferredSource API and fresh factory values, but the task allows a title that covers only the primary change.
Full details: Linked Issues check
Explanation
The changes satisfy the coding requirements in #7445. DataSourceFactories treats null collections as empty for synchronous, Task, ValueTask, and async-enumerable paths while retaining null elements and scalar null values. GenericClassRowPartition, MethodDataSourceAttribute, StaticDataSourceAttribute, GenericConstraintViolationEntry, and ReflectionTestDataCollector implement the required filtering, constructor mapping, fresh factory values, and constraint-row behavior. New unit, integration, both-mode, PublicAPI, and Native AOT coverage addresses the issue acceptance criteria. The generated-code APIs remain hidden with EditorBrowsable(Never) as required by the PR description.
Full details: Out of Scope Changes check
Explanation
The changed production files, regression fixtures, integration tests, PublicAPI baselines, and path-scrubber test have a direct connection to #7445. The path-scrubber correction is explicitly included in the issue scope. The changes do not require the sibling generator inference feature and do not show unrelated product behavior.
Full details: Docstring Coverage
Explanation
Docstring coverage is 19.23% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 26 functions across 13 files. (4 skipped: 4 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
Finishing Touches
Generate docstrings
Commit to this branch
Create a new PR
Generate unit tests (beta)
Commit to this branch
Create a new PR
Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
A rabbit checks each row with care, Then saves a fresh-made value there. Null collections leave no track, Null elements still come back. Split .cs paths now scrub away, And test rows hop through the day.
CodeRabbit Verify
Run CodeRabbit Verify to test the changes in this pull request.
Recent review info
Run configuration
Source mode: Auto
Repository: Unavailable
Branch: Unavailable
File: No configuration file from this repository applied
Commit: Unavailable
Layers: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: 29b224e7-34ac-4e2a-90e5-99e7fe6503a2
Commits
Reviewing files that changed from the base of the PR and between e784392 and 6dc7ac9.
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 2 remain after this review.
I read the diff and found no blocking problems. The null-collection handling and the fresh-factory-values fix both look correct.
What I checked
DataSourceFactories: null collections now yield no rows for the sync, Task, ValueTask and async-enumerable variants, instance variants included. The IsDeclaredCollection<T>() guard keeps scalar and string nulls as a single null row, as intended.
The reflection-mode counterpart in MethodDataSourceAttribute (null Task<IEnumerable> results) matches the generated-mode behaviour.
StaticDataSourceAttribute.RowFactories with FirstFetchRows: the first fetch uses the values evaluated during inference, and later fetches call the original factory. Interlocked.Exchange keeps the hand-out once-only.
GenericConstraintViolationEntry.DataSources calls an existing Create overload that takes IDataSourceAttribute[], so the engine side already supports it.
The new public APIs are on EditorBrowsable(Never) types, and the snapshots were updated for all target frameworks.
Suggestions (non-blocking)
Mutation of caller-supplied sources.GenericConstraintViolationEntry.ToTestMetadata sets source.SkipIfEmpty = false on the instances in DataSources. ConfigureInferredSource mutates its argument too. Both are safe if generated code creates a fresh instance per use, as the comment says. If a caller ever shares an instance, ToTestMetadata would silently change that instance's behaviour on each call. Setting the flag where the generator builds the sources, or documenting the ownership contract on the DataSources property, would make this harder to misuse.
Per-type-argument inference logic. In GenericClassRowPartition.EvaluateAsync, the infers computation is now a three-way nested ternary. It mixes ConditionalWeakTable lookups, MethodDataSourceAttribute internals and the fallback. Pulling it into a small helper such as RowInfers(source, row, classTypeArguments) would make the precedence rules readable and testable.
Split source types.ConfigureInferredSource special-cases MethodDataSourceAttribute and uses a side ConditionalWeakTable for every other source type. A small internal interface that sources implement to carry their declared class type arguments would remove both the type check and the table.
Eager evaluation in the filter path. With FilterClassRows, the factory is invoked during enumeration to test for nulls, and the result is cached for the first fetch only. That is reasonable, but the cost is one extra factory call per row for non-first fetches. Fine for correctness; it is worth a comment if it matters for expensive factories.
The tests are good: 7 new fixtures run in both modes, the engine integration tests cover each factory shape, and the PublicAPI scrubber fix has its own test. I did not run the tests myself. I'm relying on the PR's reported results and on CI for the Engine.Tests legs.
[Medium impact] The PR appears safe to merge; no actionable defects were found.
Summary
The PR separates null collections from null data rows and preserves factory-backed constructor data.
Null collection results produce no test rows.
Generic class sources keep their rows and factory values aligned.
Generic constraint failures point to the rows that break the class rules.
PublicAPI checks recognize source paths split across wrapped strings.
Diagram
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Data source result] --> B{Null collection?}
B -->|Yes| C[No source rows]
B -->|No| D[Read rows]
D --> E[Evaluate factory for discovery]
E --> F[Keep evaluated row and original factory]
F --> G{First fetch?}
G -->|Yes| H[Return evaluated row once]
G -->|No| I[Call original factory]
Reviewed all review surfaces for 4bb1b38: Claude and Codex have positive verdicts, Greptile reports no actionable issues, and there are no inline threads.
Disposition of Claude’s nonblocking suggestions: retain the current implementation. Generated emission supplies fresh source instances, as ConfigureInferredSource documents, so mutation is confined to owned instances. The nested inference expression preserves the existing declared-type precedence; extracting a helper or introducing an interface is an optional refactor with no correctness fix needed here. First-fetch caching and subsequent factory invocation are intentional to provide fresh test objects, documented on StaticDataSourceAttribute.FirstFetchRows and RowFactories. These suggestions do not block promotion.
Marking ready for CI; draft CI skips are expected. Auto-merge must wait for the ready-for-review Claude run and its review to complete.
Rebased onto current main to resolve DataSourceFactories conflicts. Retained main’s tuple-preserving row conversion, runtime-value dispatch, and null-task/null-async-source guards; applied this PR’s null-collection guards to the shared and instance collection factories. Reviewed the range-diff for lost changes.
Validation on SDK 10.0.403/net10.0: DataSourceCollectionTests 17 passed; DataSourceFactoriesRuntimeValueTests 3 passed; GenericClassSourceRuntimeTests 8 passed. No generator output or reflection path changed by the conflict resolution. Prior integration, PublicAPI and AOT validation is recorded in the PR body; Engine.Tests remains for CI per repository instructions.
The earlier promotion was immediately reverted when GitHub finished calculating mergeability and reported conflicts. Keeping this new head draft pending fresh Claude and Codex review.
Overall this is a solid, well-tested change. The null-collection handling is consistent across sync, Task, ValueTask and async-enumerable factories. The fresh-factory-per-row behavior is covered in both modes, and the AOT validation is a good sign.
I found no blocking bugs. These are design concerns for you to weigh:
Split inference state. Inference rules now live in three places:
the ConditionalWeakTable DeclaredRowTypes side table,
the internal InferredClassTypeArguments and FilterClassRows properties on MethodDataSourceAttribute,
the fallback InfersTypeArguments.
The nested ternary in EvaluateAsync is hard to follow. Consider a small internal strategy on IDataSourceAttribute, or an internal wrapper type, that exposes "declared class type arguments". That would remove the side table and the type-switching in ConfigureInferredSource.
Mutating inputs.GenericConstraintViolationEntry.ToTestMetadata sets SkipIfEmpty = false on the caller-supplied DataSources each time it is called. That mutates objects the entry doesn't own, and it breaks if the same entry is converted twice with differently-shaped sources. Prefer applying it once at construction, or cloning the sources.
Fragile null-collection detection.IsDeclaredCollection<T>() treats null as empty only when T is statically an IEnumerable. A Task<object> that returns null still yields a null row. In reflection mode, GetTaskResultType walks the base types on every call. It could be cached per type, since this is a discovery hot path. This is a minor point.
Rows evaluated twice during filtering. With FilterClassRows, func() is invoked for filtering, and the later func() call produces fresh data. That is intended, but a factory with side effects or cost now runs one extra time per row. It is worth a doc note.
Run 41585626517 passed for current head 6dc7ac9. Re-read all conversation comments, reviews and inline threads: Claude and Codex approve this head, Greptile has no findings, and there are no review threads.
Disposition of Claude's latest nonblocking suggestions: retain the implementation. The split inference representation and helper extraction are optional refactors. Setting SkipIfEmpty=false repeatedly is idempotent; these generated-code sources are configured for row-backed violations, and reusing that entry does not change their shape. A Task null intentionally remains a scalar null row: its declared type supplies no evidence that it is a collection. Caching GetTaskResultType is a possible optimization, without measurements showing a regression here. ConfigureInferredSource already documents first-fetch reuse, and StaticDataSourceAttribute documents subsequent fresh factory invocation. Its arrays are built together by reflection discovery; no index mismatch was identified. The generator integration remains explicitly tracked by open #7446.
Promoting this reviewed head to start CI. The ready-for-review Claude run must pass and its comment must be read before auto-merge is armed.
Promotion verified: ready for review, unchanged head 6dc7ac9, mergeable. Waiting for ready-for-review Claude run 41587253901 and CI. Read that run's review before arming auto-merge; the earlier draft review passed but does not replace the promotion review. No code changes or external prerequisite are needed, and no hold label is appropriate.
I read the diff and the surrounding code. I did not run the tests. The null-collection fix and the fresh-factory change look correct, and the new fixtures cover both. I have a few design notes, none of them blocking.
What looks good
DataSourceFactories only treats a null result as 'no rows' when the declared type is a non-string IEnumerable. IsDeclaredCollection<T>() keeps scalar nulls and null elements intact, as the PR intends.
The reflection path mirrors this. GetTaskResultType walks the base types to find Task<>, so Task<IEnumerable<T>?> returning null yields no rows in both modes.
StaticDataSourceAttribute.RowFactories and the Interlocked.Exchange hand-out give each value to the first consumer only. Later method rows get fresh objects from the factory. The SingleUseResource fixture exercises exactly this.
The SourcePathPattern fix for a wrapped .cs extension comes with a regression test.
Design notes
State is spread across three places. The inferred-source behaviour is held in three separate pieces:
the internal FilterClassRows and InferredClassTypeArguments properties on MethodDataSourceAttribute
a static ConditionalWeakTable DeclaredRowTypes for every other source type
ConfigureInferredSource, which mutates its argument and returns it
The type tests in EvaluateAsync (DeclaredRowTypes.TryGetValue, then is MethodDataSourceAttribute {...}, then InfersTypeArguments) form a nested ternary that is hard to follow. A small internal interface, for example IInferredClassSource { Type[]? DeclaredTypeArguments; bool FilterRows; }, or a single wrapper data source, would drop the ConditionalWeakTable and the type switch. Each source type would then own its own inference rule. I'd at least pull the ternary into a named helper.
ToInstanceVariant copies fewer settings. It copies Arguments, SkipIfEmpty and DeferEnumeration, but not the new FilterClassRows or InferredClassTypeArguments. If an instance-variant source can be configured through ConfigureInferredSource, those settings are lost silently. The PR says instance variants are covered, so please confirm that path, or copy the fields. Adding a test for it would also help.
SkipIfEmpty is mutated in two places.ConfigureInferredSource sets it to false, and so does GenericConstraintViolationEntry.ToTestMetadata. The latter mutates caller-supplied attributes on each call. If the entry's metadata is built twice, the attributes are changed twice. This is harmless today but fragile. It would be cleaner for the generator to emit the sources with the right value, or for the entry to clone them.
Allocation on the filtering path.HasNonNullValues uses LINQ Any over UnwrapArray(row).Data. That is fine at discovery time, but a plain loop is cheap and matches the hot-path guidance in AGENTS.md.
The PublicAPI snapshots look consistent. The d__26 to d__34 rename follows from the added iterator state. Overall this is solid work. Notes 1 and 2 are the ones I'd prefer addressed before merge.
Run 41587253901 completed successfully; that earlier blocker is gone. The subsequent .NET run 41587253149 failed on four stale mock-generator snapshots on all three operating systems. Rebasing onto current main d9e9e66 removes those stale differences without changing this PR's runtime patch (range-diff is identical). All 232 mock-generator tests now pass locally.
Addressed the latest Claude review in commit 1ea0b76: extracted RowInfers with the same precedence and copied FilterClassRows/InferredClassTypeArguments in ToInstanceVariant. Added a regression proving conversion retains the settings and filters an all-null external row. The other design notes retain their prior disposition: generated sources are owned instances, repeated SkipIfEmpty assignment is idempotent, and no measured performance regression warrants changing the small LINQ check. The requested focused coverage already exists in Inferred_Null_Row_Is_Retained_Only_For_Own_Class and Constraint_Violations_Report_Only_Actual_Source_Rows; both passed again.
Validation on SDK 10.0.403/net10.0: mock generator 232 passed; GenericClassSourceRuntimeTests 9 passed; #7445 fixtures 7 passed generated and 7 passed reflection; PublicAPI 20 passed. No snapshot updates were needed. Existing build warnings are tracked by #7410, #7412, #7413, #7419 and #7438. Engine.Tests remains for CI, per repository instructions. Prior Native AOT validation is recorded in the PR body; this update adds no reflection calls or generator output changes.
The PR is back in draft for fresh reviews on head 1ea0b76. After both reviewers approve this head, promote it, read the ready-for-review Claude result, and arm auto-merge through Enable-AutoMerge.ps1.
I read the diff but did not build it or run any tests. I found no blocking issues. The change is well scoped and well tested.
What looks good
Null collections are now treated as empty across the sync, Task, ValueTask and async-enumerable factories, including the instance variants. Null elements and scalar nulls are still kept. DataSourceCollectionTests covers all 8 variants for each case.
The first-fetch handoff uses Interlocked.Exchange. It lets discovery reuse the value it already evaluated once, and later rows or repeats call the factory for a fresh object. The SingleUseResource fixture checks this directly.
The generated-mode and reflection-mode fixtures, plus the ToInstanceVariant propagation test, follow the repo rule that metadata collection must behave the same in both modes.
The new public surface is additive and EditorBrowsable(Never), and the snapshots are updated for all targets.
Suggestions (non-blocking)
Inference state is split across two places.ConfigureInferredSource puts it on MethodDataSourceAttribute as FilterClassRows and InferredClassTypeArguments. For every other source it uses the static ConditionalWeakTable DeclaredRowTypes. RowInfers then has to check both. A small internal interface, for example IInferredClassSource { Type[]? InferredClassTypeArguments }, or a field on a shared base would give one code path. It would also avoid a global weak table in a hot discovery path. This matters most if Infer generic class types from class MethodDataSource rows using shared runtime support #7446 adds more source kinds.
ConfigureInferredSource mutates the instance you pass in. The comment says generated code always creates a fresh instance per closed type, so this is safe today. A reflection-side caller that reuses an attribute would silently change its SkipIfEmpty. The same applies to GenericConstraintViolationEntry, which now writes SkipIfEmpty = false on the sources it is given. Cloning, or a note in the XML doc that the instance is mutated, would make the contract explicit.
Reflection in the null-Task check.GetTaskResultType walks BaseType at runtime to tell Task<IEnumerable<…>> apart from Task<string>. This is trim- and AOT-safe, since it only looks at Type objects. It runs once per null result, so cost is fine. If the generator already knows the declared return type, passing it in would be cheaper and clearer, as DataSourceFactories does with IsDeclaredCollection<T>().
The scrubber regex is hard to read. The new wrapped-.cs alternation is dense. A short comment, or a build-up from named fragments, would help the next person who edits it.
Overall: nice work, and the test coverage is good.
The earlier wait on run 41587253901 is resolved. Rechecked all review surfaces on head 1ea0b76: Claude's current draft run 41591895040 passed with no blocking findings, Codex's completed summary and receipt approve this exact head, Greptile reports no actionable defects, and there are no inline comments or review threads. The rework budget is not exceeded (4 Claude rounds, 2 force pushes).
Disposition of the latest nonblocking suggestions: retain the current implementation. RowInfers already extracts precedence into a named helper; replacing the inference storage with an interface is a broader optional refactor. ConfigureInferredSource documents generated-only usage and its fresh-instance ownership; repeated SkipIfEmpty=false in row-backed constraint metadata is idempotent. The null-task reflection walk occurs only for null results and has no demonstrated performance regression. SourcePathPattern already has an XML explanation and focused regression coverage, so further regex refactoring is optional. No code change is required for these suggestions.
Promoting the reviewed, mergeable head for CI. The new ready-for-review Claude run must complete and its review must be read before Enable-AutoMerge.ps1 can arm auto-merge. No external prerequisite or hold label is needed.
Promotion verified: PR is ready for review and MERGEABLE at unchanged head 1ea0b76. Ready-for-review Claude run 41593222410 is queued, and CI has started. Read that run's review after it completes, then arm auto-merge with Enable-AutoMerge.ps1 if it has no blocking findings and the current head is unchanged. The earlier run 41587253901 succeeded and is no longer the blocker. No code change or linked prerequisite is needed; this is a workflow wait, with no hold label.
I read the runtime changes in DataSourceFactories, MethodDataSourceAttribute, GenericClassRowPartition, StaticDataSourceAttribute and ReflectionTestDataCollector. I did not build or run the tests. I found no correctness bugs.
What looks right
Null-collection handling is scoped correctly. The enumerable factories (FromEnumerableCore, FromInstanceEnumerable) check result is null. The Task and ValueTask variants check IsDeclaredCollection<T>(), which excludes string. Scalar factories (FromValueCore, FromInstanceValue) are untouched, so scalar nulls still become rows.
Rows are reused only on the first fetch (Interlocked.Exchange). Later method rows and repeats call the original factory, which gives fresh objects in both modes.
The reflection-side GetTaskResultType walks BaseType to find Task<>, so it also covers Task<T> subclasses.
The new APIs are additive and EditorBrowsable(Never), and the Core snapshots are updated for all four targets.
Suggestions (non-blocking)
Weak-table side channel.ConfigureInferredSource stores the declared types for non-MethodDataSourceAttribute sources in a ConditionalWeakTable (DeclaredRowTypes). MethodDataSourceAttribute instead gets internal properties. Two mechanisms make RowInfers harder to follow. A small internal interface (for example IInferredClassSource { Type[]? InferredClassTypeArguments }) implemented by both source types would remove the weak table and the type-switch.
Duplicated null-check logic.HasNonNullValues is applied in three places: MethodDataSourceAttribute, RowInfers and ReflectionTestDataCollector. If generated and reflection mode ever diverge on the filter, it will be hard to spot. Consider one shared IsEligibleClassRow(source, row) helper. It would be a natural place to document the rule.
Allocation.HasNonNullValues uses LINQ .Any with a lambda, and it is called per row during discovery. A plain loop avoids the allocation. ReflectionTestDataCollector also builds evaluatedItems with Select(...).ToArray() for every method source, even when no row has a wrapped factory. This is minor, but discovery is a hot path.
Mutating shared attributes.GenericConstraintViolationEntry.ToTestMetadata sets SkipIfEmpty = false on the caller-supplied DataSources. If ToTestMetadata is called twice, or the sources are shared, the mutation is a surprising side effect. Setting it at construction in the generated code would be cleaner.
Overall this is a well-scoped extraction, and the tests cover both generated and reflection modes. The notes above are cleanups only.
The earlier blocker is resolved: ready-for-review Claude run 41593222410 passed on unchanged head 1ea0b76. Read its promotion review and all conversation comments, reviews, inline comments and review threads. Codex's completed summary approves this exact head, Greptile reports no actionable defects, and there are no inline comments or review threads. All current CI checks pass, including the three Engine.Tests and Native AOT legs. Rework remains within budget (5 Claude rounds, 2 force pushes).
Disposition of the promotion review's nonblocking cleanups: retain the implementation. RowInfers already names the precedence logic; replacing the inference storage with an interface is an optional refactor. HasNonNullValues is already shared by both modes; its call sites apply different source/partition rules, and the matching-mode tests now pass in CI. The LINQ and evaluatedItems suggestions identify no correctness failure or measured regression, so they do not justify a performance change without measurement. Generated-only sources are fresh instances, and repeating SkipIfEmpty=false is idempotent, as previously dispositioned. CodeRabbit's docstring percentage is an advisory tool threshold, not a repository requirement; the generated-code API has XML documentation and no actionable defect was reported.
No linked prerequisite, replacement, stale scope or human decision remains. Proceeding through Merge-Pr.ps1's fresh, head-pinned gate rather than deferring the completed workflow wait.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
GitHub Actions
Failure
Null collections returned by generated data-source factories were converted into a null constructor row. Treat null collections as empty across synchronous, Task, ValueTask and async-enumerable factories, including instance variants, while retaining null elements and scalar null values.
Extract the runtime support from #7422: inferred class-source filtering and constructor-position type mapping, fresh factory values for subsequent reflection-built tests, and row-backed constraint violations. The new generated-code configuration method and optional violation sources are intentional additive public APIs on EditorBrowsable(Never) types. Keep the current generator and reflection inference rules; #7446 owns enabling the remaining inference feature. Include the independent PublicAPI scrubber correction for wrapped
.csextensions.Closes #7445
Validation:
Native publishing reports existing fixture warnings. Generic AutoFixture annotation warnings are tracked in #7452; generated hook warnings are already tracked in #7430. No warnings name the changed runtime paths.
Summary by CodeRabbit