Skip to content

Preserve null-containing tuples from static method data sources - #7469

Merged
thomhurst merged 20 commits into
mainfrom
issue-7402-static-tuple
Nov 3, 2026
Merged

thomhurst merged 20 commits into
mainfrom
issue-7402-static-tuple

Conversation

@thomhurst

@thomhurst thomhurst commented Nov 3, 2026 •

Copy link
Copy Markdown
Owner

A static [MethodDataSource] yielding Tuple.Create<string?>(null) passed null to a single tuple parameter in source-generated mode. All generated factory helpers now receive dataGeneratorMetadata, so they preserve the tuple just as reflection mode does.

Adds regressions covering scalar, enumerable, Task, ValueTask, and async-enumerable sources, plus a generator snapshot asserting each metadata-aware call. Compatible tuples stay whole; rows requiring element conversions for a single tuple parameter are spread without truncation, and both invokers reconstruct the target tuple, including tuples longer than seven elements. Existing snapshots only gain the metadata argument, including equivalent framework-specific snapshots.

Validation:

  • Original null-tuple regressions: all five failed before the generator fix. The review regression (int, int) to (long, long) also failed before the conversion fix.
  • All 11 null-tuple and numeric-conversion cases pass in source-generated and reflection modes (net10.0), including an eight-element tuple.
  • Native AOT publish succeeds on osx-arm64/net10.0; all 11 targeted cases pass in the native executable. Existing generated hook IL2075 warnings are tracked in Generated instance hooks emit IL2075 during Native AOT publishing #7430; dynamic-test IL2026 warnings describe intentionally unsupported AOT scenarios.
  • Full core generator suite: 250 passed, one skipped (net10.0).
  • TupleDataSourceDrivenTests: one passed in each mode; InstanceMethodDataSourceMemberShapeTests: 22 passed, three intended skips in each mode.
  • Only net10.0 is installed locally. net8/net9/net472 framework-specific snapshots were updated with the identical argument change and need CI validation. No public API changes. Tuple reconstruction reflection is confined to the existing reflection-only collector.

Review follow-up a32b01f:

  • Reflection tuple conversion preserves injected cancellation tokens before or after the data parameter.
  • Generated reference tuples recursively rebuild nested Rest for eight or more elements.
  • All 14 static tuple cases pass in generated and reflection modes; Native AOT publish and all 14 native cases pass (net10.0/osx-arm64).
  • Full generator suite: 250 passed, one skipped; reviewed and updated Tests7402 snapshot.
  • Reference-tuple widening analyzer limitation tracked in TUnit0001 rejects numeric widening for a single reference-tuple parameter #7480; only the runtime regression suppresses that diagnostic.

Closes #7402

Summary by CodeRabbit

  • Bug Fixes
    • Fixed test data source handling across synchronous, enumerable, task-based, and asynchronous inputs.
    • Improved tuple argument binding, including reference tuples, long tuples, generic type inference, and nullable values.
    • Improved handling of fixtures inside tuples, including initialization, dependency injection, event delivery, and disposal.
  • Tests
    • Added coverage for tuple data sources, generic inference, nullable values, long tuples, and fixture lifecycle behavior.

Review follow-up e1d8214:

Review follow-up 705e6ca:

  • Retained tuple fixtures receive property injection before initialization and remain visible to start/end event receivers. Shared tuple expansion visits nested elements once per reference, preserving the original binding arrays; ordinary arrays are returned without allocation.
  • All three lifecycle cases fail without injection traversal; with injection alone, all three still fail because start callbacks are missing. The completed fix passes all three in generated and reflection modes, verifying injection, initialization/disposal, and exactly one start/end callback.
  • Existing static tuple cases: 14 passed in each managed mode (net10.0).
  • Native AOT publish succeeds with one MSBuild node (osx-arm64/net10.0); the native executable passes all three lifecycle cases and all 14 static tuple cases.
  • Engine netstandard2.0 build succeeds with zero warnings/errors. No generator output or public API changes in this follow-up; full framework CI and fresh review remain required.

Review follow-up 47ce168:

  • Combined event receiver candidates are deduplicated by reference, including a fixture supplied directly to the constructor and inside a method tuple. Its start/end callbacks run once.
  • Tuple rows containing value-type async initializers retain flattened binding so invocation reconstructs from the initialized boxes. Cached shape checks cover nullable structs and long tuple Rest shapes. Compatible null-containing tuples remain whole. Reflection also reconstructs single-element tuples when their flattened element needs initialization.
  • New shared receiver regression and both initial value/reference struct regressions fail before the fixes. All four struct variants (value, reference, single-element, nullable) pass in generated and reflection modes, as does the shared receiver regression.
  • Existing lifecycle cases (3) and static tuple cases (14) pass in both managed modes (net10.0). Engine netstandard2.0 builds with zero warnings/errors. No generator output or public API changes.
  • Pre-existing nested struct initializer mutation loss reproduced at merge base 67870ed and tracked separately in Nested tuple struct arguments lose asynchronous initializer mutations #7484.
  • Native AOT publish succeeds with one MSBuild node (net10.0/osx-arm64); all 4 initializer, 1 shared receiver, 3 lifecycle, and 14 static tuple cases pass in the native executable. Existing hook/dynamic-test warnings remain as previously documented.

Review follow-up d43f7bb:

  • Reflection preserves actual null arguments for one-element reference tuples, fixing the failed engine-test regression.
  • Property data sources preserve tuple containers even when their elements implement struct initializers; property assignment no longer receives a flattened element. The new property regression fails before the fix in both modes.
  • Generic inference maps flattened initializer boxes to logical tuple elements, including long tuples and injected tokens. Generated generic reference tuples use the existing shared reconstruction path. Six new generic initializer cases pass in both modes; the original value/reference cases fail before the inference fix. A tuple-contained CancellationToken remains ordinary data.
  • Full core generator suite: 251 passed, one skipped (net10.0). Reviewed and committed the new generic-tuple snapshot. Engine netstandard2.0 builds with zero warnings/errors.
  • Targeted managed validation: 52 passed and three intended skips in each mode, including existing static tuple, lifecycle, mutable initializer, shared receiver, generic tuple and instance-member shape coverage.
  • Generic array elements rejected by the analyzer are tracked separately in TUnit0001 rejects a generic array element inside a tuple parameter #7489. Existing restore advisory is tracked in Mock and core generator test restores report vulnerable System.Security.Cryptography.Pkcs versions #7412. No public API changes. Only net10.0 is installed locally; framework-specific validation remains with CI.
  • Native AOT publish succeeds with one MSBuild node (net10.0/osx-arm64); the native executable passes the same 52 targeted cases with three intended skips. Existing generated-hook and dynamic-test warnings remain as previously documented.

Review follow-up 7e31eeb:

  • Rebased onto 9100521, retaining main’s null-collection guards when resolving the factory conflict.
  • Tuple rows containing struct event receivers use flattened binding so callback mutations survive reconstruction. Four new value/reference/single-element/nullable tuple regressions fail before the fix and pass afterward.
  • Tuple group: 34 passed in each of generated, reflection and native AOT modes (net10.0/osx-arm64). Native AOT publish succeeds with one MSBuild node.
  • After rebase: unit suite 747 passed/3 skipped; full core generator suite 251 passed/1 skipped with no snapshot changes. Cross-framework validation remains with CI.

Review follow-up da7fcf5:

  • Empty nullable struct initializer/receiver elements preserve their non-null tuple container. The shared policy checks actual boxed mutable values after its cached type-shape lookup; populated mutable structs still flatten for write-back.
  • Both new null-element regressions fail before the fix in reflection mode. All 36 Source-gen: static [MethodDataSource] row Tuple.Create<string?>(null) reaches a single Tuple<string?> parameter as null #7402 cases pass in generated and reflection modes and in the Native AOT executable (net10.0/osx-arm64). Native publish succeeds with one MSBuild node; no trim warnings reference the changed helpers. Engine netstandard2.0 builds with zero warnings/errors.
  • No generator output or public API changes. Cross-framework validation remains with CI; existing generated-hook/dynamic-test AOT warnings remain as previously documented.

Review follow-up 495f626:

  • Property-injected struct tuple elements use flattened binding and retain setter mutations, including reflection fallback when generated property metadata is unavailable. Four populated variants fail before the fix in both modes; an empty nullable control preserves its tuple container.
  • All 41 Source-gen: static [MethodDataSource] row Tuple.Create<string?>(null) reaches a single Tuple<string?> parameter as null #7402 cases pass in generated, reflection, and Native AOT modes (net10.0/osx-arm64). Native publish succeeds with one MSBuild node; no trim warnings reference the changed helper or new fixture. Existing generated-hook/dynamic-test warnings remain as previously documented. Engine netstandard2.0 builds with zero warnings/errors.
  • Warm-policy median measurements before/after: ordinary tuple 10.1/10.3 ns and 0/0 B per call; initializer tuple 44.7/46.7 ns and 88/88 B per call (five million-call samples, indicative local harness). Cached property plans avoid repeated reflection, and the initializer/receiver interface fast path is preserved.
  • No production generator output or public API changes; no received snapshots committed. Cross-framework CI and fresh review remain required.

Review follow-up b389464:

  • Reflection invokes Func-valued rows before preserving tuple-typed properties, matching the generated factory path. Added reference/value tuple Func property assertions; reflection fails before the fix with a fixture-to-tuple assignment error.
  • All 41 Source-gen: static [MethodDataSource] row Tuple.Create<string?>(null) reaches a single Tuple<string?> parameter as null #7402 cases pass in generated and reflection modes and in the Native AOT executable (net10.0/osx-arm64). Native publish succeeds with one MSBuild node; no new helper/fixture trim warnings. Engine netstandard2.0 builds with zero warnings/errors.
  • No generator output or public API changes. Cross-framework CI and fresh review remain required.

Review follow-up 999d4cc:

  • Reflection invocation checks flattened tuple element types and argument offsets for null-aware generic inference, matching discovery. Null is preserved instead of silently reconstructed as a value-type default. Four new value/reference/token regressions fail before the fix in reflection mode.
  • All 45 Source-gen: static [MethodDataSource] row Tuple.Create<string?>(null) reaches a single Tuple<string?> parameter as null #7402 cases pass in generated and reflection modes and in the Native AOT executable (net10.0/osx-arm64). Native publish succeeds with one MSBuild node. Engine netstandard2.0 builds with zero warnings/errors. No production generator output or public API changes; fresh CI/review remain required.

Review follow-up c63b4ff:

Review follow-up ace245a:

  • Retained tuple elements now receive discovery initialization before registration, using the same existing lifecycle expansion as property injection and receiver discovery. The three new value/reference/nested tuple cases fail before the fix and pass in generated and reflection modes; repeated references initialize once.
  • Full Source-gen: static [MethodDataSource] row Tuple.Create<string?>(null) reaches a single Tuple<string?> parameter as null #7402 group: 50 passed in each managed mode (net10.0). Engine netstandard2.0 builds with zero warnings/errors. No generator output or public API changes.
  • The metadata fix changes static sources from flattened rows to retained containers. The conversion, reconstruction and lifecycle changes in this PR preserve the behavior those flattened rows previously provided; shipping metadata plumbing alone loses fixture initialization, injection, callbacks or converted tuple binding in the reproduced cases.

Native validation: Release Native AOT publish succeeded on net10.0/osx-arm64 with one MSBuild node; all 50 #7402 cases pass in the native executable. No trim/AOT warnings name the changed helper or new fixture; existing hook, AutoFixture and dynamic-test warnings remain. Cross-framework validation remains with CI.

Review follow-up 7fd3b8b:

  • Preserves incompatible one-element tuple containers holding null while actual null arguments remain null.
  • Reuses the existing cached recursive reflection converter and removes duplicate reconstruction. Generated nested value-tuple conversions use static constructors, including nested eight-element tuples.
  • Both original review reproductions fail before the fixes in reflection mode. All 53 Source-gen: static [MethodDataSource] row Tuple.Create<string?>(null) reaches a single Tuple<string?> parameter as null #7402 cases pass in generated, reflection, and Native AOT execution on net10.0/osx-arm64. Exact-head Release native publish succeeds with one MSBuild node; no trim/AOT warnings name the changed helpers or StaticTupleDataSourceTests. Existing hook/dynamic-test warnings remain.
  • Full generator suite: 252 passed, one intended skip; reviewed three changed snapshots. Instance-member shape controls: 22 passed, three intended skips in each managed mode. Engine netstandard2.0 build has zero warnings/errors. No public API changes; cross-framework validation remains with CI.

Review follow-up 586c5fd and af0e7b5:

  • Generated nested value-tuple conversion supplies default for null inner elements before recursive extraction. The exact review reproduction fails before the fix with IndexOutOfRangeException.
  • Incompatible one-element value tuples containing null flatten for conversion; reference-tuple containers retain their distinction from actual null arguments. The new ValueTuple<string?> to ValueTuple<object?> regression fails before the fix with a boxed tuple in Item1.
  • All 55 Source-gen: static [MethodDataSource] row Tuple.Create<string?>(null) reaches a single Tuple<string?> parameter as null #7402 cases pass in generated, reflection and Native AOT modes (net10.0/osx-arm64). Release native publish succeeds with one MSBuild node; no trim/AOT warnings name the changed helpers or StaticTupleDataSourceTests. Existing hook/dynamic-test warnings remain as previously documented.
  • Full generator suite: 252 passed, one intended skip; reviewed the Tests7402 snapshot, including its existing entry-chunking threshold. Instance-member shape controls: 22 passed, three intended skips in each managed mode. No public API changes; cross-framework validation remains with CI.

Review follow-up a329e8a:

@coderabbitai

coderabbitai Bot commented Nov 3, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • Resume reviews
  • Trigger review
Walkthrough

Static data-source factories now receive dataGeneratorMetadata. Tuple binding and invocation handle flattened and preserved tuple rows, including reference tuples and nested Rest tuples. Engine discovery and registration process tuple elements. New tests cover conversion, generic inference, nullable tuple values, and lifecycle behavior.

Changes: Static data-source tuple handling

Layer / File(s) Summary
Factory metadata and tuple-row selection
src/TUnit.Core.SourceGenerator/Generators/TestMetadataGenerator.cs, src/TUnit.Core/Helpers/DataSourceFactories.cs, src/TUnit.Core/Attributes/TestData/MethodDataSourceAttribute.cs, tests/TUnit.Core.SourceGenerator.Tests/*.verified.txt
Generated static data-source factories now pass dataGeneratorMetadata. Factory helpers use tuple binding modes to preserve or spread rows. Runtime-value sources preserve tuple rows for a single tuple-valued property.
Tuple inference, conversion, and lifecycle processing
src/TUnit.Core/Helpers/DataSourceHelpers.cs, src/TUnit.Engine/Helpers/*, src/TUnit.Engine/Discovery/ReflectionTestDataCollector.cs, src/TUnit.Engine/Services/TestGenericTypeResolver.cs, src/TUnit.Engine/Discovery/ObjectGraphDiscoverer.cs, src/TUnit.Engine/Extensions/TestContextExtensions.cs, src/TUnit.Engine/Services/TestArgumentRegistrationService.cs
Engine code infers flattened tuple element types, reconstructs tuple arguments, and expands tuple elements during registration and discovery.
Regression coverage
tests/TUnit.Core.SourceGenerator.Tests/Bugs/7402/*, tests/TUnit.Core.SourceGenerator.Tests/Tests7402.Generic_Tuples_Use_Shared_Binder.verified.txt, tests/TUnit.Core.SourceGenerator.Tests/Tests7402.Test.verified.txt, tests/TUnit.TestProject/Bugs/_7402/*
Tests cover generated factory calls, tuple conversions, generic inference, nullable reference tuples, property binding, and fixture lifecycle behavior.

Estimated code review effort: 4 (Complex) | ~45 minutes

Change: Bug fix · Severity of issue fixed: Medium

Sequence Diagram(s)

sequenceDiagram
  participant Generator as TestMetadataGenerator
  participant Factory as DataSourceFactories
  participant Binding as DataSourceHelpers
  participant Invoker as ReflectionTestDataCollector
  participant Discovery as ObjectGraphDiscoverer
  Generator->>Factory: Pass dataGeneratorMetadata
  Factory->>Binding: Convert a data-source row
  Binding->>Invoker: Preserve tuple or provide flattened values
  Invoker->>Invoker: Infer element types and reconstruct tuple
  Discovery->>Discovery: Traverse tuple elements
Loading

Possibly related issues

Possibly related PRs

Merge Risk: Low · previous review d43f7

Some tuple property rows can bind differently across execution paths. The narrow issue is suitable for owner follow-up.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 40.63% 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 Passed checks: 4 passed
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title "Preserve null-containing tuples from static method data sources" matches the main change in the changeset. The linked issue #7402 asks that static [MethodDataSource] rows holding a one-elem…
Linked Issues check Passed [#7402] Requirement: a static [MethodDataSource] row of Tuple.Create<string?>(null) must reach a single Tuple<string?> parameter as the tuple, not as null, in source-generated mode. The generator chan…
Out of Scope Changes check Passed The changes in the whole pull request concern tuple-row handling for data sources, which #7402 covers. Changes in the latest follow-up d43f7bb are also tuple-row handling. ReflectionTestDataCollect…
Full details: Title check

Explanation

The title "Preserve null-containing tuples from static method data sources" matches the main change in the changeset. The linked issue #7402 asks that static [MethodDataSource] rows holding a one-element tuple with a null value reach a single tuple parameter as a tuple. The diff passes dataGeneratorMetadata to static DataSourceFactories calls, adds tuple-preservation logic in DataSourceFactories, DataUnwrapper, and DataSourceHelpers, and updates the snapshots and tests that cover this case. The title is specific and concise. It does not mention the broader tuple binding work, such as generic tuple inference and tuple expansion in the engine, but it does not need to cover every detail.

Full details: Linked Issues check

Explanation

[#7402] Requirement: a static [MethodDataSource] row of Tuple.Create<string?>(null) must reach a single Tuple<string?> parameter as the tuple, not as null, in source-generated mode. The generator change in TestMetadataGenerator.EmitDataSourceFactoryInvocation passes dataGeneratorMetadata to all static data-source factory calls. The prior conditional metadata argument was removed. Tests7402 and the affected generator snapshots verify the metadata-aware calls. DataSourceFactories selects the tuple row binding from the metadata. DataUnwrapper keeps a single tuple whole when the expected type accepts the runtime tuple. The regression suite (StaticTupleDataSourceTests, covering value, enumerable, Task, ValueTask, and async-enumerable sources) covers the reported case. The PR description reports that these cases pass in source-generated and reflection modes on net10.0, and in Native AOT. This assessment relies on the reported net10.0 results and the source changes. The net8, net9, and net472 snapshots carry the same argument change; their behavior is pending CI validation. Tuple-element conversion, nested and long tuples, and lifecycle handling extend the same tuple-row behavior and are not separate requirements of #7402. Related issues #7348, #7480, #7481, #7482, #7484, #7489, and #7412 are not requirements of this pull request.

Full details: Out of Scope Changes check

Explanation

The changes in the whole pull request concern tuple-row handling for data sources, which #7402 covers. Changes in the latest follow-up d43f7bb are also tuple-row handling. ReflectionTestDataCollector preserves one-element reference-tuple null arguments. DataSourceFactories and MethodDataSourceAttribute preserve tuple containers for property data sources. TupleParameterTypes and TestGenericTypeResolver infer generic arguments from flattened tuple elements. The generic test cases are in GenericMutableTupleInitializerTests. ObjectGraphDiscoverer, TupleArguments, TestContextExtensions, and TestArgumentRegistrationService keep fixtures inside preserved tuples injected, initialized, disposed, and visible to event receivers. Without this work, preserving tuples would skip initialization of those fixtures. The lifecycle and receiver tests verify that behavior. Snapshot edits add the metadata argument and the new generated tuple cases. The analyzer limitation for reference-tuple widening is tracked in #7480, and only a test-level diagnostic suppression depends on it. Work tracked in #7481, #7482, #7484, #7489, #7412, #7468, and #7348 is not included here. No public API change or unrelated feature is shown.

Full details: Docstring Coverage

Explanation

Docstring coverage is 40.63% 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 32 functions across 18 files. (1 skipped: 1 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.

Share

A rabbit checks each tuple row,
And watches nulls stay in their place.
Eight elements hop through nested Rest,
While tokens keep their proper space.
Fixtures wake, then rest at end,
And generated rows reach their tests.

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: 4576a0c8-9c78-4665-ab50-9927d230f09a
Commits

Reviewing files that changed from the base of the PR and between 47ce168 and d43f7bb.

Files selected for processing: 10
  • src/TUnit.Core.SourceGenerator/Generators/TestMetadataGenerator.cs
  • src/TUnit.Core/Attributes/TestData/MethodDataSourceAttribute.cs
  • src/TUnit.Core/Helpers/DataSourceFactories.cs
  • src/TUnit.Engine/Discovery/ReflectionTestDataCollector.cs
  • src/TUnit.Engine/Helpers/TupleParameterTypes.cs
  • src/TUnit.Engine/Services/TestGenericTypeResolver.cs
  • tests/TUnit.Core.SourceGenerator.Tests/Bugs/7402/Tests7402.cs
  • tests/TUnit.Core.SourceGenerator.Tests/Tests7402.Generic_Tuples_Use_Shared_Binder.verified.txt
  • tests/TUnit.TestProject/Bugs/_7402/GenericMutableTupleInitializerTests.cs
  • tests/TUnit.TestProject/Bugs/_7402/TuplePropertyBindingTests.cs

Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 6 remain after this review.

@thomhurst

Copy link
Copy Markdown
Owner Author

@codex review

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Nov 3, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-11-03T15:21:48.815857Z a329e8a New commits
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@github-actions

github-actions Bot commented Nov 3, 2026

Copy link
Copy Markdown
Contributor

Review

The fix looks correct and minimal. EmitDataSourceFactoryInvocation now always passes dataGeneratorMetadata as the first argument. I checked DataSourceFactories.cs and a metadata-taking overload exists for every helper the generator can emit: FromValue, FromEnumerable, FromTask<T>, FromValueTask<T>, FromAsyncEnumerable<T>, all the FromInstance* variants, and both runtime-value helpers. So the generated code compiles and no public API changes. This also brings source-generated mode in line with reflection mode, as the PR description says.

The five regressions cover scalar, enumerable, Task, ValueTask and async-enumerable sources, and the snapshot test pins each call shape.

Minor, non-blocking:

  • The non-metadata FromValue, FromEnumerable, FromTask, FromValueTask and FromAsyncEnumerable overloads are now unused by the generator. If nothing else references them, they could be removed or marked as legacy to keep the helper surface from drifting. They are public, so check compatibility first.
  • metadataArgument is now a constant used in every branch. You could drop the constant and write the argument inline, or have the helper-name builder add it.
  • The net8, net9 and net472 snapshots were edited by hand, as the PR notes. CI needs to confirm them.

Looks good to merge once CI is green.

🤖 Generated with Claude Code

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Already looking forward to the next diff.

Reviewed commit: f951b885a9

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@greptile-apps

greptile-apps Bot commented Nov 3, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerView in GreptileConfidence Score: 5/5

[Medium impact] The changes appear safe to merge; no new actionable issue was found.

Summary

Preserves null-containing tuple rows while keeping element conversion and fixture lifecycle behavior.

  • Method data sources keep tuple rows whole when a test expects one tuple.
  • Tuple rows convert their elements and rebuild the requested tuple.
  • Tuple elements stay available for fixture setup, callbacks, and property assignment.

Reviews (20) · Last reviewed commit: "Preserve custom nested tuple conversions..." · Reviewed by Greptile

@thomhurst

Copy link
Copy Markdown
Owner Author

Reviewed all review surfaces for f951b88: Claude and Codex both report no blockers; Greptile also reports safe to merge, and there are no inline threads. Disposition of Claude’s optional suggestions: retain the existing public factory overloads to preserve compatibility, and retain the metadataArgument constant to keep the repeated emitted argument consistent. Neither needs a change for this fix. Framework-specific snapshot validation remains assigned to CI as documented in the PR body.

Promoting this reviewed head to ready. The ready_for_review Claude run and full CI must complete before auto-merge can be armed.

@github-actions

github-actions Bot commented Nov 3, 2026

Copy link
Copy Markdown
Contributor

Review

The fix is small and correct. EmitDataSourceFactoryInvocation now always passes dataGeneratorMetadata. Every helper kind the generator emits (FromValue, FromEnumerable, FromTask<T>, FromValueTask<T>, FromAsyncEnumerable<T>, and the instance/runtime variants) already has a metadata-taking overload in DataSourceFactories.cs, so the generated code will compile. Making metadataArgument a const and deleting the later reassignment removes a special case, and source-generated mode now matches reflection mode.

  • Tests: the five regressions cover scalar, enumerable, Task, ValueTask and async-enumerable sources. The snapshot test pins the generated calls. The repo guidance about running both modes is followed.
  • Snapshots: the snapshot diffs only add the dataGeneratorMetadata, argument. The net8/net9/net472 snapshots could only be checked in CI, as the description says.
  • Cost: the metadata-less FromValue, FromEnumerable, FromTask, FromValueTask and FromAsyncEnumerable overloads probably have no generator callers left now. If so, they could be removed later, though they are public API, so that needs a deliberate decision. The extra argument is a reference to an existing local, so it adds no allocation, and there is no new reflection, so AOT is unaffected.

No blocking issues. Approve once CI is green on the framework-specific snapshots.

@coderabbitai coderabbitai Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Fix comments: Fix CodeRabbit comments on this PR

@github-actions

github-actions Bot commented Nov 3, 2026

Copy link
Copy Markdown
Contributor

Review of #7469

The fix looks sound. Always passing dataGeneratorMetadata to every generated factory helper makes source-generated mode match reflection mode for a single tuple parameter, including a one-element tuple holding null. The new Tests7402 snapshot checks that all five source kinds (Value, Enumerable, Task, ValueTask, AsyncEnumerable) get the metadata argument. The regression cases are in TUnit.TestProject.

I did not build or run anything. I only read the diff and confirmed that the metadata-taking overloads exist in DataSourceFactories.

Points to check:

  • Per-call cost: Every method data source now carries the metadata argument, where only tuple-sensitive ones did before. This should be cheap, since it is one extra parameter on a static lambda. Still, the guidance in AGENTS.md is to measure hot paths, so a quick check of the discovery/data-generation benchmark would back that up.
  • Duplicated reconstruction logic: The tuple handling now exists twice, in the generated invoker and in ReflectionTestDataCollector.ReconstructTuple. It is handled consistently, including the nested Rest tuple for elements past the seventh. A shared helper in TUnit.Core would stop the two copies drifting apart. That is not a blocker.
  • DataUnwrapper: When the tuple is not an instance of the target type, it now spreads the elements via UnwrapTupleAot. That is what allows (int,int) to (long,long). The reflection invoker only rebuilds the tuple when args.Length == tupleElements.Length. A mismatched length falls through to the old paths, which looks intended.
  • CI: The net8, net9 and net472 snapshots were edited by hand, so they depend on CI to confirm.

I have no blocking concerns. Approving in spirit; please confirm CI is green, especially the framework-specific snapshots.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 43703c13e5

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@thomhurst

Copy link
Copy Markdown
Owner Author

Pushed e452541 addressing both new findings and resolved their threads. The earlier blocker run 41608974207 succeeded. Validation: generic tuple regression fails before the inference fix and passes afterward in generated and reflection modes; all 11 existing static tuple cases pass in both modes; regenerated Tests7402 snapshot passes (net10.0). No public API or reflection invocation changes. Fresh CI and reviews are required on this head.

Disposition of Claude’s optional suggestions: metadata passing changes no allocation and is required for correctness; a benchmark project is outside this corrective change. Sharing tuple reconstruction remains optional: the generated code emits statically typed construction for AOT, whereas reflection construction stays in its reflection-only collector. Retaining these paths preserves that boundary.

@github-actions

github-actions Bot commented Nov 3, 2026

Copy link
Copy Markdown
Contributor

Review of #7469

The generator fix looks correct. EmitDataSourceFactoryInvocation now always passes dataGeneratorMetadata. That gives source-generated mode the same single-tuple-parameter handling as reflection mode, so a one-element tuple holding null stays a tuple. I read the full generator diff and the ReflectionTestDataCollector hunk. I did not build or run anything. I did not read the DataUnwrapper change closely.

Earlier review points are addressed or answered: the metadata-less overloads stay for public-API compatibility, and the const metadataArgument is fine. The (int,int) to (long,long) conversion and the generic tuple regression were fixed in e452541.

Non-blocking:

  • Duplicated tuple reconstruction. The logic now lives in the generated invoker and in ReflectionTestDataCollector.ReconstructTuple, including the nested Rest handling past seven elements. The two copies can drift apart. I accept the AOT reasoning (statically typed construction in generated code, reflection only in the collector). I'd still add a test that runs the same eight-element and widening cases through both modes, which I believe the PR already does, so the two stay locked together.
  • Length guard. args.Length == tupleElements.Length silently falls through to the old paths on a mismatch. A short comment on why that is safe would help the next reader.
  • Hand-edited snapshots. The net8, net9 and net472 snapshots were edited by hand, so CI is the real check for them.

No blockers from me. Merge once CI is green.

@coderabbitai coderabbitai Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Fix comments: Fix CodeRabbit comments on this PR

@thomhurst

Copy link
Copy Markdown
Owner Author

Pushed a32b01f and resolved both new CodeRabbit findings. Run 41608974207 succeeded, so the earlier blocker is gone. All 14 static tuple regressions pass in generated and reflection modes; the Native AOT executable passes all 14. The full generator suite passes (250 passed, one skipped). Reference-tuple analyzer widening is tracked separately in #7480.

Claude’s remaining optional points: the eight-element and widening cases run in both modes. The tuple reconstruction length guard intentionally accepts only an exact flattened row shape; other shapes retain the existing wrapped-tuple/argument-validation paths. No additional change is needed for that non-blocking suggestion. Framework-specific snapshots still need CI. Fresh CI and automated reviews must complete on this new head.

This was referenced Nov 4, 2026

This branch was previously deployed

1 inactive deployment
Pull Requests — a329e8a8 Deployed Nov 3, 2026 by thomhurst via modularpipeline (ubuntu-latest) #20987
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Source-gen: static [MethodDataSource] row Tuple.Create<string?>(null) reaches a single Tuple<string?> parameter as null

1 participant