Skip to content

Preserve generic tuple rows from instance data sources - #7548

Merged
thomhurst merged 8 commits into
mainfrom
issue-7514-preserve-generic-tuple
Nov 4, 2026
Merged

thomhurst merged 8 commits into
mainfrom
issue-7514-preserve-generic-tuple

Conversation

@thomhurst

@thomhurst thomhurst commented Nov 4, 2026 •

Copy link
Copy Markdown
Owner

Summary

Instance method data sources lost tuple elements when generated metadata represented a generic tuple parameter as object. Preserve that row using its existing TypeInfo shape in both the runtime source reader and engine unwrapping, while retaining flattened binding for mutable tuple elements.

Add regression coverage for nested tuples with explicit generic arguments, cancellation-token injection, separate parameters, scalar binding, and single- and multi-element reference-tuple conversions. Reference tuples retain flattened binding for those conversions.

Closes #7514

Validation

  • Confirmed the wrapped-tuple and cancellation-token regressions fail before the fix with the reported argument-count error.
  • PreservedGenericTupleTests: 7 passed in each discovery mode.
  • NestedTupleBindingTests: 7 passed in each discovery mode.
  • GenericTupleBindingTests: 4 passed in each discovery mode.
  • .NET Standard 2.0 engine build passed without warnings.
  • Initial validation left engine integration to CI. Subsequent fixes change generated singleton invocation and reflection binding, as described below; no public API changes.

Review follow-up: confirmed both new multi-element reference-tuple regressions failed before the fix. All 18 targeted tuple tests pass in each discovery mode; the .NET Standard 2.0 engine build passes without warnings. Shared tuple-binding helpers keep the runtime source reader and engine unwrapper aligned.

Cross-family review follow-up: reference-tuple targets now flatten value-tuple sources using the retained target metadata in both binding paths. Both added regressions fail before the fix; all 20 targeted tuple tests pass in each discovery mode, and the .NET Standard 2.0 engine build passes without warnings. Engine integration remains for CI per repository instructions.

Null-row review follow-up: preserved singleton tuple containers containing null for reference-tuple targets. Both new regressions fail before the fix; all 22 targeted tests pass in each discovery mode. The netstandard2.0 engine build passed without warnings.

Singleton value-tuple review follow-up: the reported conversion passes without a production change. Added explicit element-type, matching-element, and null-element coverage; all 14 PreservedGenericTupleTests pass in each discovery mode.

Singleton binding follow-up: routed incompatible preserved singleton containers through element-wise conversion in the generated invoker. Added a generator snapshot, cancellation-token coverage, and reference-source coverage. All 16 focused tests, 12 existing singleton tests, and 7 nested tuple tests pass in both modes. Generator suite: 330 passed, one skipped. Corrected two unrelated stale generator snapshots for existing constructor/property output. Allocation follow-up: #7552. Engine integration remains for CI.

Nested singleton follow-up: retained outer singleton value-tuple rows through binding in both discovery modes and aligned reflection invocation with generated invocation. Four regressions fail before the fix; all 20 focused tests pass in both modes and Native AOT. Mutable (10) and nested tuple (7) tests pass in each mode; generator snapshot and netstandard2.0 build pass. Existing warnings: #7410 and #7430. Engine integration remains for CI.

Summary by CodeRabbit

  • Bug Fixes
    • Improved test-data binding for generic and single-element tuples, including nested tuples, null values, and conversions between tuple types.
    • Fixed handling of tuple arguments alongside separately supplied parameters and cancellation tokens.
  • Tests
    • Added coverage for generic tuple binding and tuple conversion scenarios.

@coderabbitai

coderabbitai Bot commented Nov 4, 2026 •

Copy link
Copy Markdown

Explain Change →

Walkthrough

This change updates tuple-row conversion and argument binding when generated metadata represents a generic tuple parameter as object. It adds rules for preserving or flattening tuple values and tests generic, nested, singleton, reference, and value tuple cases.

Changes: Generic Tuple Row Preservation

Layer / File(s) Summary
Erased tuple detection and row conversion
src/TUnit.Core/Helpers/DataSourceHelpers.cs, src/TUnit.Core/Attributes/TestData/MethodDataSourceAttribute.cs
The data source identifies a sole erased generic tuple parameter. Shared rules determine whether tuple values remain whole or are flattened.
Generated and reflection tuple binding
src/TUnit.Engine/Helpers/DataUnwrapper.cs, src/TUnit.Engine/Discovery/ReflectionTestDataCollector.cs, src/TUnit.Core.SourceGenerator/Generators/TestMetadataGenerator.cs
The engine and generated invokers update singleton tuple detection and tuple preservation during argument binding.
Tuple binding regression coverage
tests/TUnit.TestProject/Bugs/_7514/PreservedGenericTupleTests.cs, tests/TUnit.Core.SourceGenerator.Tests/PreservedGenericTupleTests.cs, tests/TUnit.Core.SourceGenerator.Tests/OneElementValueTupleTests.Test.verified.txt, tests/TUnit.Core.SourceGenerator.Tests/Tests7402.Test.verified.txt, tests/TUnit.Core.SourceGenerator.Tests/ClassArgumentParameterTypesTests.Test.verified.txt, tests/TUnit.Core.SourceGenerator.Tests/GenericPropertyMetadataTests.GeneratesCachedAotCompatibleGetters.verified.txt
Regression tests cover generic and nested tuple binding, null elements, cancellation tokens, and tuple conversions. Verified generated output reflects the corresponding invocation and generic factory changes.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Bug fix · Severity of issue fixed: Medium

Sequence Diagram(s)

sequenceDiagram
  participant MethodDataSourceAttribute
  participant DataSourceHelpers
  participant DataUnwrapper
  participant GeneratedTestInvoker
  MethodDataSourceAttribute->>DataSourceHelpers: choose whether to flatten tuple row
  DataUnwrapper->>DataSourceHelpers: choose whether to flatten sole tuple parameter
  DataUnwrapper->>GeneratedTestInvoker: pass preserved or unwrapped arguments
Loading

Possibly related issues

  • #7402: The change includes handling for singleton reference tuples containing null.
  • #7401: The change updates generated invocation handling for single-element value tuples.
  • #7481: The change updates preserved tuple binding after generic type inference, but does not implement the separate repeated-type inference rule.
  • #7482: The change updates tuple preservation and conversion in the reflection binding path.

Possibly related PRs

  • thomhurst/TUnit#7469: Both changes address tuple-row preservation in method data sources; this PR handles erased generic tuple parameters in the runtime reader and engine binding.
  • thomhurst/TUnit#7515: Both changes update generated tuple argument conversion, including singleton tuple handling.
  • thomhurst/TUnit#7487: Both changes address reflection binding and conversion for preserved generic tuple rows.
Pre-merge checks: Passed 3 · Failed 2

Failed Failed checks: 2 warnings

Check name Status Explanation Resolution
Out of Scope Changes check Warning Most changes relate to tuple row preservation: the DataSourceHelpers and DataUnwrapper logic, the ReflectionTestDataCollector singleton handling, the TestMetadataGenerator ITuple length check and its … Remove the unrelated snapshot corrections from GenericPropertyMetadataTests.GeneratesCachedAotCompatibleGetters.verified.txt and ClassArgumentParameterTypesTests.Test.verified.txt from this PR. If the stale snapshots must be fixed, move tha…
Docstring Coverage Warning Docstring coverage is 42.86% 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: 3 passed
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly and concisely describes the main change: preserving generic tuple rows from instance data sources.
Linked Issues check Passed Issue #7514 requires generated and reflection discovery to invoke an instance method data source test with the complete tuple and the expected generic type argument. The reproduced shape is a single g…
Full details: Linked Issues check

Explanation

Issue #7514 requires generated and reflection discovery to invoke an instance method data source test with the complete tuple and the expected generic type argument. The reproduced shape is a single generic tuple parameter erased to object. In src/TUnit.Core/Helpers/DataSourceHelpers.cs, IsErasedGenericTupleParameter detects an object parameter whose TypeInfo is a ConstructedGeneric of System.ValueTuple or System.Tuple. In src/TUnit.Engine/Helpers/DataUnwrapper.cs, UnwrapWithTypesAndCount now routes that single erased parameter through ShouldFlattenErasedTuple. For a value-tuple target with no mutable elements, the helper returns false, so the row is kept whole instead of being capped to one argument. The added regression tests in tests/TUnit.TestProject/Bugs/_7514/PreservedGenericTupleTests.cs cover the Wrapped<T> shape from the issue, and the author reports them passing in both discovery modes. I did not run the tests. The static code path matches the issue requirement. Follow-up requirements for reference-tuple and singleton conversions are also covered by the added tests and the reported results. Parent or sibling scope is not part of this assessment.

Full details: Out of Scope Changes check

Explanation

Most changes relate to tuple row preservation: the DataSourceHelpers and DataUnwrapper logic, the ReflectionTestDataCollector singleton handling, the TestMetadataGenerator ITuple length check and its snapshots (OneElementValueTupleTests, Tests7402), and the _7514 regression tests. The PR description states that two generator snapshots were corrected as 'unrelated stale' snapshots for existing constructor and property output. The diff of tests/TUnit.Core.SourceGenerator.Tests/GenericPropertyMetadataTests.GeneratesCachedAotCompatibleGetters.verified.txt changes generated test-instance factories from Activator.CreateInstance to direct construction and replaces a hard-coded 42 with args[0]. ClassArgumentParameterTypesTests.Test.verified.txt changes generated property getters from dynamic member access to PropertyMetadataFactory.CreateGenericGetter. These edits concern #7499-style generic property and constructor metadata, not tuple binding. They are not connected to #7514 and are not needed by the tuple logic shown in the diffs. The PR validation section also states that no generator output changed, which conflicts with these snapshot edits.

Resolution

Remove the unrelated snapshot corrections from GenericPropertyMetadataTests.GeneratesCachedAotCompatibleGetters.verified.txt and ClassArgumentParameterTypesTests.Test.verified.txt from this PR. If the stale snapshots must be fixed, move that work to a separate PR that references its own issue. Then rerun the generator test suite to confirm that the tuple-related snapshots still pass. Update the PR description so that it does not state that generator output was unchanged.

Full details: Docstring Coverage

Explanation

Docstring coverage is 42.86% 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 7 functions across 7 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

Warning

Review coverage is incomplete: 1 file could not be fully reviewed. Findings from completed review steps are included; see review info for details.

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 keeps each tuple whole,
Then checks its shape before the role.
Nested values stay in view,
Nulls and tokens travel too.
Tests hop through each binding path.

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: 0cc8661c-d591-457a-bd01-8f1fb6bd734e
Commits

Reviewing files that changed from the base of the PR and between 10a0af7 and 6843efe.

Files selected for processing: 12
  • src/TUnit.Core.SourceGenerator/Generators/TestMetadataGenerator.cs
  • src/TUnit.Core/Attributes/TestData/MethodDataSourceAttribute.cs
  • src/TUnit.Core/Helpers/DataSourceHelpers.cs
  • src/TUnit.Engine/Discovery/ReflectionTestDataCollector.cs
  • src/TUnit.Engine/Helpers/DataUnwrapper.cs
  • tests/TUnit.Core.SourceGenerator.Tests/ClassArgumentParameterTypesTests.Test.verified.txt
  • tests/TUnit.Core.SourceGenerator.Tests/GenericPropertyMetadataTests.GeneratesCachedAotCompatibleGetters.verified.txt
  • tests/TUnit.Core.SourceGenerator.Tests/OneElementValueTupleTests.Test.verified.txt
  • tests/TUnit.Core.SourceGenerator.Tests/PreservedGenericTupleTests.Preserved_Tuples.verified.txt
  • tests/TUnit.Core.SourceGenerator.Tests/PreservedGenericTupleTests.cs
  • tests/TUnit.Core.SourceGenerator.Tests/Tests7402.Test.verified.txt
  • tests/TUnit.TestProject/Bugs/_7514/PreservedGenericTupleTests.cs
Files not fully reviewed because their changes were unavailable: 1
  • tests/TUnit.Core.SourceGenerator.Tests/PreservedGenericTupleTests.Preserved_Tuples.verified.txt

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

@thomhurst

Copy link
Copy Markdown
Owner Author

@codex review

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Nov 4, 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-04T05:12:09.498914Z 6843efe Draft marked ready
ℹ️ 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 4, 2026

Copy link
Copy Markdown
Contributor

Review of #7548

I found no blocking issues. The fix is small and targeted.

What I checked

  • The same erased-generic-tuple check, IsErasedGenericTupleParameter, is used on both paths. Those are the runtime reader in MethodDataSourceAttribute.ToInstanceVariant and the DataUnwrapper path in the engine. That keeps the two discovery modes consistent, as the project rules require.
  • DataUnwrapper calls RequiresFlattenedTupleBinding(values[0]!) only after the earlier IsTuple(values[0]) guard. The ! therefore can't hide a null.
  • Mutable-element tuples still go through flattened binding, so the existing boxed-struct behaviour is kept.
  • The tests cover the wrapped nested tuple, CancellationToken injection, separate parameters and scalar binding. Per the PR description, the first two fail without the fix.

Non-blocking suggestions

  1. IsErasedGenericTupleParameter matches the type name with StartsWith("System.ValueTuple")andStartsWith("System.Tuple"). This works, but comparing GenericDefinition to typeof(ValueTuple<>) and typeof(Tuple<>) would be sturdier, if ConstructedGeneric exposes a Type. If it only carries names, the current check is fine.
  2. HasSingleErasedGenericTupleParameter is evaluated once per source, outside the per-row ToRow function, so there is no hot-path cost.
  3. A short comment on the ToRow branch saying why it differs from ToObjectArrayWithTypes would help future readers. The existing comment about the outer factory only partly covers it.

Good to go once CI is green.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. 👍

Reviewed commit: 82bebedae5

ℹ️ 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 4, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerView in GreptileConfidence Score: 5/5

[Medium impact] The PR appears safe to merge; the reported nested-singleton failure is fixed.

Summary

Preserves generic tuple rows from instance data sources without losing nested elements.

  • Generic tuple rows keep their shape until the test invoker can bind them.
  • Generated snapshots match the current constructor and property output.

The numbered previous finding is fixed. The earlier reference-tuple, null-element, and singleton-conversion threads also match the current fixes. No new actionable issues were found. Tests were not rerun during this review.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  A[Instance data source] --> B[Read tuple row]
  B --> C{Mutable element needs updates?}
  C -->|Yes| D[Keep separate element boxes]
  C -->|No| E[Keep singleton container]
  D --> F[Build target tuple]
  E --> F
  F --> G[Call test]
Loading

Reviews (8) · Last reviewed commit: "Fix #7514 nested singleton row container..." · Reviewed by Greptile

@thomhurst

Copy link
Copy Markdown
Owner Author

@codex review

Fixed Greptile's confirmed reference-tuple regression in 68028a5. The preservation predicate now applies only to value tuples. Added the suggested explanation at the row branch; retained the existing name check to keep this fix focused. All 16 targeted tuple tests pass in each discovery mode. Existing analyzer/generator build warnings are tracked by #7410.

@github-actions

github-actions Bot commented Nov 4, 2026

Copy link
Copy Markdown
Contributor

Review of #7548

The fix is targeted and reads correctly. I couldn't run the full code-review skill (it errored), so this is a manual read of the diff. I didn't build the code or run the tests.

What I checked

  • IsErasedGenericTupleParameter limits the new path to parameters whose type is object, whose TypeInfo is a constructed System.ValueTuple, and which are the single non-CancellationToken data parameter. That keeps the blast radius small.
  • Reference tuples and mutable-element tuples still go through flattened binding. The ReferenceElementConversion test covers the reference-tuple case.
  • The runtime reader (MethodDataSourceAttribute) and the engine unwrapper (DataUnwrapper) apply the same rule. That matches the AGENTS.md requirement that both paths behave identically.
  • The tests cover wrapped tuples, cancellation-token injection, separate parameters, scalar binding and reference tuples.

Suggestions (non-blocking)

  1. The same decision ('preserve whole unless RequiresFlattenedTupleBinding') is written out in two places. A small helper such as DataSourceHelpers.PreserveOrUnwrapErasedTuple(item) would keep them from drifting apart.
  2. HasSingleErasedGenericTupleParameter and the engine's dataParameterCount == 1 check count data parameters separately. A short comment, or a shared counting helper, would make it clearer that they must agree on how CancellationToken is treated.
  3. The name check StartsWith("System.ValueTuple")is a string comparison. It works, but comparing the generic definition'sTypewould be sturdier if one is available. I didn't check whetherTypeInfo` exposes one without reflection or AOT cost.

No blocking issues found. Engine integration tests are left to CI, as the PR description says.

🤖 Generated with Claude Code

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Keep them coming!

Reviewed commit: 68028a5418

ℹ️ 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

@codex review

Fixed the confirmed multi-element reference-tuple truncation in 330e157. Both new regressions fail before the fix; all 18 targeted tests pass in each discovery mode. The shared flattening helper also addresses Claude's suggestion to keep the two binding paths aligned. Retained the parameter-counting and TypeInfo name checks to keep this fix focused; both counting paths exclude CancellationToken.

@github-actions

github-actions Bot commented Nov 4, 2026

Copy link
Copy Markdown
Contributor

Review

The fix looks correct and well scoped. Instance method data sources now keep a generic tuple row whole when generated metadata erases the parameter to object and TypeInfo still describes a tuple. The runtime reader (MethodDataSourceAttribute.ToRow) and the engine unwrapper (DataUnwrapper) share IsErasedGenericTupleParameter and ShouldFlattenErasedTuple, so the two paths should stay aligned. Reference tuples and mutable value-tuple elements still get flattened, as before. The tests cover wrapped rows, CancellationToken injection, separate parameters, scalar binding, and single- and multi-element reference tuples.

I have no blocking concerns. Two optional suggestions:

  1. Match the tuple type by definition, not by name. IsErasedGenericTupleParameter checks FullName.StartsWith("System.ValueTuple")and"System.Tuple". Comparing GenericDefinition to the open generic types, or reusing the existing IsTuple logic, would be less brittle and avoids per-call string prefix checks. It runs in a discovery hot path, so you could also cache the result per parameter.
  2. Compute preserveGenericTuple once. In ToInstanceVariant it is computed once per source, which is good. HasSingleErasedGenericTupleParameter could move next to IsErasedGenericTupleParameter in DataSourceHelpers, so the "single data parameter, ignoring CancellationToken" rule lives in one place. DataUnwrapper uses an equivalent dataParameterCount == 1 check.

I did not run the tests. The PR description reports 18 targeted tuple tests passing in each discovery mode, and CI will run the engine suite.

@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: 330e157f12

ℹ️ 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

@codex review

Fixed both cross-family tuple findings in d8a8d8a. Both new regressions fail before the fix, and all 20 targeted tuple tests pass in each discovery mode. The netstandard2.0 engine build passes without warnings.

@github-actions

github-actions Bot commented Nov 4, 2026

Copy link
Copy Markdown
Contributor

Review of #7548: Preserve generic tuple rows from instance data sources

The fix looks correct and well targeted. I found no blocking issues.

What I checked

  • The engine unwrapper (DataUnwrapper) and the InstanceMethodDataSource row builder (MethodDataSourceAttribute.ToRow) now share IsErasedGenericTupleParameter and ShouldFlattenErasedTuple. That keeps the two binding paths aligned, which the metadata-parity rule in AGENTS.md asks for.
  • The values[0]! null-forgiving use in DataUnwrapper is safe, because the earlier IsTuple(values[0]) guard has already excluded null.
  • Checking FullName.StartsWith("System.ValueTuple")` is cheap and doesn't use reflection. It also doesn't add trimming or AOT risk.
  • The tests cover nested tuples, cancellation-token injection, separate parameters, scalar binding, and single- and multi-element reference tuples. They are the cases that failed before the fix.

Suggestions (non-blocking)

  1. IsErasedGenericTupleParameter matches names by string prefix. A small shared constant, or a helper such as IsTupleDefinitionName, would stop ValueTuple/Tuple from drifting between the two call sites and the Tuple check inside ShouldFlattenErasedTuple.
  2. In ShouldFlattenErasedTuple, the mixed &&/|| expression is easy to misread. Splitting it into named booleans (targetIsReferenceTuple, sourceIsReferenceTuple, needsMutableBoxes) would make the intent obvious.
  3. GetSingleErasedGenericTupleParameter runs on every data-generator invocation. Moving it out of the per-call path, or computing it lazily once per attribute, would be cheaper. This is a minor allocation and hot-path point.

CI still has to run the engine integration suite. Overall this looks good to merge.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Bravo.

Reviewed commit: d8a8d8abc8

ℹ️ 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

@codex review

Fixed in 0285bc3. Confirmed both value-tuple and reference-tuple singleton null rows failed before the fix. The shared helper now preserves their containers for single-element reference-tuple targets, allowing the existing invoker to distinguish a tuple containing null from a null row. All 22 targeted tuple tests pass in each discovery mode; netstandard2.0 engine build passed with zero warnings. Engine integration remains for CI.

Claude suggestions: the shared decision now uses a named reference-target boolean. Retaining the existing name matching and invocation-time parameter lookup keeps the null-row fix focused; no caching or performance change is claimed.

@github-actions

github-actions Bot commented Nov 4, 2026

Copy link
Copy Markdown
Contributor

Review of #7548

I read the diff only. I didn't build the PR or run the tests. The fix targets the right problem. Generated metadata erases a generic tuple parameter to object, and both the runtime source reader and DataUnwrapper now use the retained TypeInfo to keep the row intact. Putting the decision in the shared helpers IsErasedGenericTupleParameter and ShouldFlattenErasedTuple keeps the two paths in step. The regression tests cover many cases: nested tuples, cancellation-token injection, separate parameters, scalar binding, reference-tuple conversions and null rows.

Suggestions, none blocking:

  1. Name matching on a hot path. IsErasedGenericTupleParameter and ShouldFlattenErasedTuple run FullName.StartsWith(...) on every row. AGENTS.md asks for cached reflection and fewer allocations in data-generation paths. Compare GenericDefinition against a cached set of tuple generic definitions instead, or resolve the answer once per parameter and cache it. ShouldFlattenErasedTuple also calls UnwrapTupleAot just to test for [null], which allocates an array on every singleton Tuple``1 row. A direct check on Item1 would avoid that.

  2. Special cases in ShouldFlattenErasedTuple. The Tuple1/null-row rule and the RequiresFlattenedTupleBinding` rule look like workarounds for a general problem: how a row binds to a target whose closed type isn't known yet. If more of these cases show up, bind late, at the invoker once the type is closed, instead of adding more flatten-or-preserve rules at each layer.

  3. Test breadth. The tests exercise both discovery modes. A case with more than 7 tuple elements (a nested TRest) and one with a non-singleton Tuple target that has a null element would also be worth adding.

Overall this is a solid, well-tested fix. Please confirm the engine integration CI is green before merging.

@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: 0285bc375c

ℹ️ 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

@codex review

Please confirm the singleton value-tuple disposition: #7548 (comment) . Added three tests; all 14 tests in this class pass in each discovery mode.

@github-actions

github-actions Bot commented Nov 4, 2026

Copy link
Copy Markdown
Contributor

Review of #7548 (preserve erased generic tuple rows)

I read the diff and found no correctness bugs. I didn't build the PR or run any tests.

What I checked

  • DataUnwrapper only reaches the new branch after the IsTuple(values[0]) guard, so values[0]! can't be null there.
  • The runtime reader (MethodDataSourceAttribute.ToRow) and the engine unwrapper use the same ShouldFlattenErasedTuple helper. That keeps the two metadata paths consistent, as AGENTS.md requires.
  • The tests cover both discovery modes. They include the awkward cases: a singleton tuple holding null, value-tuple sources feeding reference-tuple targets, and CancellationToken injection.

Design suggestions (non-blocking)

  1. IsErasedGenericTupleParameter and ShouldFlattenErasedTuple match tuple types with FullName.StartsWith("System.ValueTuple")andStartsWith("System.Tuple"). This runs per row. Resolving the check once per data source, and caching it, would avoid repeated string comparisons. The generator could also set an explicit "is tuple" flag on TypeInfo so no name matching is needed.
  2. ShouldFlattenErasedTuple calls UnwrapTupleAot for the Tuple1null check, andRequiresFlattenedTupleBinding` calls it again. Each call allocates an array on a hot path. Unwrap once and pass the result to both checks.
  3. The flatten rules are now spread across ToRow, DataUnwrapper, and the helper. Each one carries its own comment explaining the rules. A single "tuple binding policy" method that returns flatten or preserve would be easier to reason about. It would also be where the next edge case gets added.

Overall this is a solid fix with good regression coverage.

This was referenced Nov 5, 2026

This branch was previously deployed

1 inactive deployment
Pull Requests — 6843efec Deployed Nov 4, 2026 by thomhurst via modularpipeline (windows-latest) #21123
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.

Generated discovery drops elements from preserved tuples bound to generic parameters

1 participant