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
Generated invokers cast nested tuples directly, so a row such as (int, (int, string)) throws when a generic test accepts (T, (int, object)). Reconstruct mismatched value tuples recursively through their elements, preserving direct casts for already compatible tuples and avoiding reflection.
Cover nested and deep/long tuples, separate parameters, and preserved instance-source rows. Update the affected generator snapshots. A separate generated-metadata defect affecting preserved generic instance rows is tracked in #7514.
No actionable comments were generated in the recent review.
Walkthrough
The source generator now builds tuple cast expressions recursively. It reconstructs tuple elements when their runtime tuple shape matches the target, retains cast fallbacks, and adds tests for nested and nullable tuple binding.
Tuple argument paths now generate casts through GenerateCastExpression. The helper recursively reconstructs tuple values from ITuple elements and retains CastHelper.Cast<T> for non-tuples and fallback values.
Verified generated output covers wrapped tuples with one or more elements, cancellation-token parameters, and nested tuple shapes. Compatible tuple values retain direct-cast fallbacks.
New tests cover generic and separate parameters, instance data, nullable nested tuples, and nesting up to five levels. Verified output includes generated invocation and registration code for these cases.
thomhurst/TUnit#7469: It changes generated tuple-row preservation and reconstruction; this PR adds recursive conversion for the generated invocation path.
thomhurst/TUnit#7487: It handles incompatible nested tuple binding in reflection execution, while this PR handles conversion in source-generated invokers.
thomhurst/TUnit#7496: It routes generated constructor arguments through TupleArgumentHelper, which this PR extends with recursive tuple conversion.
Merge Risk: Minimal · up to a4b99
No actionable merge-blocking risk remains after normal checks.
Pre-merge checks: 4 · 1
Failed checks: 1 warning
Check name
Status
Explanation
Resolution
Docstring Coverage
Docstring coverage is 50.00% 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 clearly and concisely describes the main change: fixing recursive tuple argument conversions in generated code.
Linked Issues check
The PR meets the coding requirements in [#7485]. TupleArgumentHelper.GenerateCastExpression recursively reconstructs mismatched tuple elements, including nested and nullable tuples, and keeps the di…
Out of Scope Changes check
The changes stay within [#7485]. The helper, generator call-site updates, regression tests, and generated snapshots directly support recursive tuple conversion. No unrelated public API or reflection-p…
Full details: Linked Issues check
Explanation
The PR meets the coding requirements in [#7485]. TupleArgumentHelper.GenerateCastExpression recursively reconstructs mismatched tuple elements, including nested and nullable tuples, and keeps the direct CastHelper.Cast fallback for compatible values. TestMetadataGenerator uses this helper in the affected generated invocation paths. NestedTupleBindingTests covers generic nested binding, separate parameters, instance-source rows, deep nesting, and nullable values. The updated generator snapshots provide coverage for the generated output.
Full details: Out of Scope Changes check
Explanation
The changes stay within [#7485]. The helper, generator call-site updates, regression tests, and generated snapshots directly support recursive tuple conversion. No unrelated public API or reflection-path changes are shown. The separate generated-metadata defect in [#7514] is not treated as a requirement for this PR.
Full details: Docstring Coverage
Explanation
Docstring coverage is 50.00% 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 6 functions across 4 files. (6 skipped: 6 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 hops through tuples deep, Where nested values used to leap. Each element finds its proper place, With nullable shapes kept in the race. The generated paths now bind with care.
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: 29b81525-5048-4db4-8b75-bbd561b78c97
Commits
Reviewing files that changed from the base of the PR and between 149ee33 and a4b9973.
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review.
Review: nested tuple conversion in generated invokers (#7485)
The approach is sound. ValueTuple is invariant, so rebuilding a mismatched tuple from its elements is the right fix. It uses no reflection, so it is AOT-safe. It keeps the fast path (CastHelper.Cast) when the runtime type already matches. All three call sites that cast tuples now go through one helper, GenerateCastExpression. I read the generator changes only. I did not build the PR or run any tests.
Concerns
Generated code grows exponentially with nesting depth. In GenerateCastExpression, expression appears in the is ITuple check, the is not T check, the fallback cast, and in every child's expression. Each nesting level multiplies the text of the level below it by 4 or more. The PR says deep and long tuples are tested, so this may be fine for the cases covered. A tuple with several nested levels, or a chain through Rest, could still produce very large invoker bodies and slow compilation. A better shape is a small shared generic helper that takes the object once and recurses, so the expression text stays linear in depth. It would also evaluate args[i] once.
The match is by arity only.ITuple { Length: N } && not T also matches System.Tuple<...> and any custom ITuple of the same length. That is probably good, but it is a behaviour change, so say so explicitly and add a test if you intend to support it. A null element where the target element is a non-nullable value type should still fail as before. A test would pin that down.
Duplicated gating logic. In TestMetadataGenerator.cs (~L2258) the caller checks IsTupleType and falls back to a hand-built CastHelper.Cast string. GenerateCastExpression already does that check and fallback. Calling it unconditionally removes the duplicate and the risk of drift.
Minor: GenerateCastExpression is public and could be internal.
Good
Snapshot updates are limited to the affected generators.
Both execution modes are covered by the new regression tests in tests/TUnit.TestProject/Bugs/_7485.
[Medium impact] The PR appears safe to merge; no actionable new issues were found.
Summary
Generated test invokers now rebuild mismatched value tuples recursively instead of directly casting boxed tuples.
Generated tests now rebuild tuples when nested element types differ.
Diagram
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Argument and target type] --> B{Target is a tuple?}
B -->|No| C[Use CastHelper]
B -->|Yes| D{Input has matching length and needs conversion?}
D -->|No| C
D -->|Yes| E[Convert each element recursively]
E --> F[Build target tuple]
Nullable tuple targets now inspect the underlying tuple type, rebuild non-null mismatched values recursively, and preserve null through the existing cast fallback. Both nullable rows pass in generated and reflection modes.
Switch patterns bind each tuple input once, so child expressions use a local pattern variable instead of repeating the parent indexing chain. Added a five-level nesting regression.
Removed the duplicate tuple gate at the generic invoker call site and made GenerateCastExpression internal.
The existing arity-based ITuple reconstruction remains intentional: tuple inputs are consumed through their indexed elements, including reference/custom ITuple implementations. This patch preserves that policy; no additional input-type restriction was introduced. Null scalar elements still use the unchanged CastHelper fallback. Additional reference/custom ITuple and invalid scalar fixtures are optional follow-up coverage, not required for the nullable fix.
Validation: core generator suite 251 passed, 1 skipped; seven regression cases pass in each execution mode. Reviewed and accepted the four affected snapshots. No reflection or public API paths changed. Engine tests left to CI per repository guidance.
The approach is sound. Generated invokers now rebuild a mismatched ValueTuple from its elements instead of casting the boxed tuple directly. This avoids reflection, so it stays AOT and trim friendly. All the call sites that used CastHelper.Cast<tuple> now share one helper, TupleArgumentHelper.GenerateCastExpression.
I read the diff but did not build it or run any tests.
What looks good:
The is not {tuple} guard keeps the direct cast when the runtime tuple already has the exact type. That is the cheap path for the common case.
The null fallback to CastHelper.Cast<T> handles nullable tuples and nulls.
The per-depth input names (__tupleInput{depth}) avoid repeating the indexing chain at each level.
The one-element case uses new ValueTuple<T>(..), which is correct because (x) is not a tuple literal.
The PR adds regression tests for nested tuples, deep tuples, separate parameters and preserved instance-source rows. The generator snapshots are updated.
Suggestions, none blocking:
Generated code size. Every tuple parameter now emits a nested switch expression, and this is repeated across the args.Length == N and wrapped-tuple branches. For tuples whose element types contain no generic parameters, no element can be a type mismatch. A tuple like (int, string) arriving as (int, string) already hits the direct cast. The nested rebuild is only needed when some element type is a type parameter or a non-sealed type. Check how often that holds. If it is rare, add a cheap check in the helper: if no element in the tuple tree needs rebuilding, return the plain Cast<T>. That keeps snapshots and generated code small for the usual non-generic tests.
Element conversions. Elements are converted through CastHelper.Cast<T>. Check that it still applies the implicit-conversion handling the old whole-tuple cast did (for example an int boxed into a long element). If it does not, the new path could regress a case that worked before when the tuple types matched exactly.
Tuples with 8 or more elements. Roslyn flattens TRest in TupleElements, and ITuple.Length also reports the total, so this should work. The PR describes "deep/long tuples" in its tests, so confirm that a test covers 8 or more elements and not only deep nesting.
Rebased onto origin/main and resolved the shared invoker conflict, preserving main's reference-tuple and cancellation-token handling and this PR's recursive value-tuple conversion. Snapshot refresh: a4b9973.
Disposition of Claude's remaining suggestions:
Keep reconstruction for concrete scalar element types: sealed/value types can still require numeric or user-defined conversions (for example int to long). Restricting it to generic or non-sealed elements would lose those conversions. Switch pattern locals keep expression growth linear in the tuple tree; compatible tuples retain the direct-cast fallback.
Elements still use CastHelper.Cast, preserving its numeric, implicit/explicit operator, and null-to-default behavior. The 57 existing _7402 tuple-binding cases pass, including numeric and nested operator conversion cases.
DeepRow contains an eight-element nested tuple; VeryDeepRow covers five levels. All seven _7485 cases pass in generated and reflection modes.
Validation after rebase: generator suite 326 passed, 1 skipped; reviewed and accepted three snapshot refreshes; _7485 generated 7 passed and reflection 7 passed; _7402 generated 57 passed. Engine tests left to CI per repository instructions. Existing build warnings are tracked in #7410 and #7412. Keeping this PR draft for fresh reviews of the rebased head.
Review of #7515: recursive tuple reconstruction in generated invokers
I read the TupleArgumentHelper.GenerateCastExpression change. I did not build it or run the tests.
What it does well
It fixes the real problem. ValueTuple<> is invariant, so a boxed (int, (int, string)) can't be cast to (int, (int, object)). Rebuilding the tuple from its elements, with no reflection, keeps the Native AOT and trimming constraints.
The when input is not {tuple} guard sends already-compatible tuples through the direct CastHelper.Cast. Null handling for Nullable<tuple> goes through the same fallback arm.
Giving each nesting level its own pattern variable (__tupleInput{depth}) avoids repeating the indexing chain at every level.
Code size. Every tuple-typed parameter now gets a full switch expression, even when no element type can differ, such as (int, string) bound to (int, string). The snapshot diffs are large, with 571 added lines for the new test alone. The generator could emit the switch only when a rebuild is possible: a tuple element is an interface, object, a type parameter, or has a nested tuple that could vary. Otherwise it could keep the plain CastHelper.Cast<T>. That keeps the output for the common case small and easier to read. It also lowers the risk for existing users' generated code and compile time.
Move the repeated expression into a helper. The per-level switch is long inline generated text. A small runtime helper such as CastHelper.CastTuple<T>(object?) could do the same job. The cost is that a generic helper would need reflection or per-arity overloads, which is why the inline form is reasonable here. I'd record that choice in a comment near the method.
System.Tuple<> input. Matching on ITuple means a reference Tuple<,> now silently converts to a ValueTuple parameter, where before it threw. That's probably fine, but confirm it matches reflection mode's behaviour, since AGENTS.md requires both modes to behave identically.
Name collisions.__tupleInput{depth} and __tupleFallback{depth} are scoped to their switch arm. If the invoker ever hoists these into a shared scope or a lambda, two parameters at the same depth could collide. A one-line comment saying the names rely on arm scoping would protect against that.
Overall the approach is sound and the tests are good. I have no blocking concerns, assuming CI and the engine tests pass in both modes.
Current-head review disposition (a4b9973): Claude explicitly reports no blocking concerns, and Codex reports a completed review with no findings.
Claude's non-blocking suggestions are dispositioned as follows: retain reconstruction for concrete element types because numeric and user-defined conversions can still require it, as explained in the prior disposition. Keep inline generated construction to preserve reflection-free AOT support; the existing helper comments explain this choice and the per-level bindings. Pattern bindings remain scoped to their switch arms; no hoisting is introduced. System.Tuple inputs are supported by reflection's CastInvocationArgument, which recognizes both tuple families, unwraps them, and reconstructs the target tuple. Extra custom-ITuple coverage and explanatory comments are optional future improvements rather than blockers for this fix.
Marking ready for CI and the ready-for-review Claude run. Auto-merge must wait for that Claude run's verdict, per repository guidance.
Review of #7515: recursive tuple reconstruction in generated invokers
Overall this is a sound fix. I read TupleArgumentHelper.cs in the PR head. I did not build it or run the tests, and I did not look at the TestMetadataGenerator.cs change or the new tests. The snapshot files only appear in the PR's file list; I didn't inspect them.
What works
Casting a boxed ValueTuple directly fails when the element types differ, for example (int, string) to (int, object). Rebuilding the tuple from its elements, recursively and without reflection, is the right fix and keeps Native AOT and trimming intact.
The generated switch has an is not T guard, so tuples that already match still go through a plain cast.
Null, wrong-length and non-tuple inputs fall through to CastHelper.Cast<T>. They fail or pass exactly as before.
Nullable tuples and one-element tuples (new ValueTuple<T>(...)) are handled.
Depth-indexed names (__tupleInput{depth}) stop a child from shadowing its parent. Sibling elements at the same depth reuse a name, but each sits in its own switch expression, so that is legal.
Tests cover nested, deep and long tuples, separate parameters, and preserved instance-source rows, in both generated and reflection modes.
Suggestions (non-blocking)
Generated code size. Each tuple-typed parameter now emits a switch expression with a full fallback arm, repeated for every invoker case and argument count. Moving the shape into a cached generic helper would keep the invokers small and avoid duplicated IL. That helper could be CastHelper.CastTuple<T>(object?), with the generator emitting only the nested element conversion. It would also shorten the snapshots. The inline form is acceptable if you want to avoid a runtime helper.
Fast path. When the declared parameter type contains no nested tuple or element-type mismatch that can occur at runtime, the code could skip the ITuple machinery. In practice the source types are unknown at generation time, so this is only a minor note.
Name collisions.__tupleInput0 and __tupleFallback0 are pattern variables inside the generated expression. A parameter or local with the same name would be shadowed, but it would be legal. It is unlikely, so I would leave it.
I have no blocking concerns. The follow-up generic-instance-row defect is tracked separately in #7514, which is reasonable.
The earlier blocker is cleared: run 41706580978 completed successfully on a4b9973. The ready-for-review Claude verdict has no blocking concerns, Codex reports a completed review without findings on the same head, and all CI checks have passed, including engine tests and AOT on all three platforms. The latest Claude suggestions repeat the already dispositioned inline-construction and fast-path trade-offs; no new changes are needed. CodeRabbit’s docstring-coverage warning is non-blocking: the new internal helper already explains invariance, reflection-free reconstruction, input binding, and nullable fallback in comments. Additional XML documentation is optional. The nullable-tuple thread is resolved with its fix commit and has no later reviewer reply. Proceeding through the repository merge gate.
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.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
GitHub Actions
Inactive
Generated invokers cast nested tuples directly, so a row such as
(int, (int, string))throws when a generic test accepts(T, (int, object)). Reconstruct mismatched value tuples recursively through their elements, preserving direct casts for already compatible tuples and avoiding reflection.Cover nested and deep/long tuples, separate parameters, and preserved instance-source rows. Update the affected generator snapshots. A separate generated-metadata defect affecting preserved generic instance rows is tracked in #7514.
Closes #7485
Validation:
InvalidCastException.Summary by CodeRabbit
Bug Fixes
Tests