You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Fix preserved generic tuple binding in reflection mode - #7487
A generic test taking (T, object) now accepts a preserved (int, string) row in reflection mode after inferring T = int. The reflection invoker rebuilds incompatible tuple arguments against the closed parameter types, recursively handles long tuple Rest shapes, and preserves already compatible tuples. Tuple constructors and their argument types are cached. The netstandard2.0 tuple fallback also flattens Rest recursively for both ValueTuple and Tuple, with cached field/property lookups; nested tuples in Item slots stay intact. No public API or generator output changes.
Original two-case regression before the fix: widened tuple failed in reflection; compatible tuple passed.
Four final cases (widened, compatible, eight-element tuple, injected token): all pass in source-generated and reflection modes on net10.0.
Existing TupleParameterWithCancellationTokenTests: 18 pass in reflection mode.
Native AOT publish succeeds with -m:1 on net10.0/osx-arm64; all four regression cases pass individually in the native executable. Existing hook IL2075 warnings are tracked in Generated instance hooks emit IL2075 during Native AOT publishing #7430; dynamic-test IL2026 warnings describe intentionally unsupported AOT paths.
Engine netstandard2.0 build: zero warnings/errors. Other framework legs require CI; Engine.Tests was not run per repository instructions.
An extra nested-tuple experiment exposed a separate source-generated conversion failure, filed as #7485; that failing experiment is excluded from this PR's regressions.
Seven DataSourceHelpersTests pass on net10.0, including direct tests of the legacy reflection fallback for multiple Rest levels and reference tuples.
A separate net10.0 harness loading the built netstandard2.0 Core assembly passes 8-element ValueTuple, 15-element ValueTuple with nested final item, and 8-element Tuple checks.
All four GenericTupleBindingTests pass in generated mode, reflection mode, and the published native executable.
.NET Framework execution remains for Windows CI; the actual netstandard2.0 library fallback was exercised locally on net10.0.
Summary by CodeRabbit
Bug Fixes
Fixed generic test data binding for tuples, including tuples with widened element types, nested long tuples, and parameters preceded by a cancellation token. Tuple values are now converted to match the expected parameter types.
Tests
Added coverage for tuple binding and preservation of elements in long value and reference tuples.
No actionable comments were generated in the recent review.
Walkthrough
The reflection invocation path now rebuilds incompatible tuple arguments when source and parameter tuple shapes have equal element counts. Tuple extraction uses cached reflection access and recursively expands Rest. Tests cover generic binding, cancellation-token positioning, and long tuples.
Changes: Tuple Binding
Layer / File(s)
Summary
Tuple element extraction src/TUnit.Core/Helpers/TupleHelper.cs, tests/TUnit.UnitTests/DataSourceHelpersTests.cs
TupleHelper caches reflection access to tuple fields or properties and recursively expands the Rest slot. Tests cover long value and reference tuples.
Reflection invocation rebuilds incompatible tuple arguments to match the parameter tuple type. Tests cover generic inference, widened tuple elements, and a preceding CancellationToken.
thomhurst/TUnit#7481 — This change adds tuple rebinding after generic inference, but does not change repeated-type inference for (T, T).
Possibly related PRs
thomhurst/TUnit#7469 — Both changes handle tuple data-source rows across discovery modes; this PR adds rebinding in reflection invocation.
thomhurst/TUnit#7467 — Both changes handle preserved tuple rows, in source-generated reconstruction and reflection rebinding, respectively.
thomhurst/TUnit#7348 — Both changes address whether tuple data is preserved as one argument or reconstructed for invocation.
Merge Risk: Minimal · up to 4f3cd
Tuple rebinding improves reflection support; no merge-blocking issue was identified.
Pre-merge checks: 4 · 1
Failed checks: 1 warning
Check name
Status
Explanation
Resolution
Docstring Coverage
Docstring coverage is 40.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 names the core change: reflection-mode binding of generic tuple parameters. The PR rebuilds incompatible tuple arguments against closed parameter types after generic inference, and adds test…
Linked Issues check
Issue #7482 requires reflection mode to normalize a preserved tuple row against the closed parameter types after generic inference, while keeping compatible tuples. The diff in src/TUnit.Engine/Discov…
Out of Scope Changes check
All changes support tuple reconstruction for the #7482 binding path. The ReflectionTestDataCollector change is the core fix. TupleHelper.cs changes the netstandard2.0 fallback (UnwrapTupleUsingReflect…
Full details: Title check
Explanation
The title names the core change: reflection-mode binding of generic tuple parameters. The PR rebuilds incompatible tuple arguments against closed parameter types after generic inference, and adds tests for the generic tuple binding case (issue #7482). The title is specific and matches the main objective. It does not describe the caching or the netstandard2.0 fallback, which is expected for a title.
Full details: Linked Issues check
Explanation
Issue #7482 requires reflection mode to normalize a preserved tuple row against the closed parameter types after generic inference, while keeping compatible tuples. The diff in src/TUnit.Engine/Discovery/ReflectionTestDataCollector.cs adds CastInvocationArgument. It returns the value unchanged when the closed parameter type already accepts it (IsInstanceOfType). When both types are tuples with the same element count, it rebuilds the target tuple through ConstructInvocationTuple. Both the regular-parameter and params-collection paths now call this helper instead of CastHelper.Cast directly. The rebuild handles the Rest slot (index 7) recursively, so long tuples are covered. The regression class tests/TUnit.TestProject/Bugs/_7482/GenericTupleBindingTests.cs covers the reported (T, object) case with a (int, string) row. Its assertions check typeof(T) == int and both element values, as the issue acceptance criteria require. A compatible (T, string) case is also covered. The repeated generic-bound inference work tracked in #7481 is not required here, and #7469 is not a closing fix. Reflection-mode and generated-mode pass results are author-reported; I did not execute the tests.
Full details: Out of Scope Changes check
Explanation
All changes support tuple reconstruction for the #7482 binding path. The ReflectionTestDataCollector change is the core fix. TupleHelper.cs changes the netstandard2.0 fallback (UnwrapTupleUsingReflection and AppendTupleElements) to flatten Rest recursively with cached member lookups. This is the same unwrapping that the fix depends on for long tuples, and the summary states that Item slots keep nested tuples intact. The DataSourceHelpersTests additions test that unwrapping directly. The GenericTupleBindingTests additions cover the reported case, a compatible case, a long tuple, and an injected CancellationToken. The new test class has an EngineTest attribute, which is consistent with the repository test pattern. No public API or generator changes were found in the reviewed summary. The #7485 nested-tuple source-generation failure is excluded, as the PR description states. The extra netstandard2.0 Rest handling is a broader change to a shared helper, but it is directly connected to the tuple unwrapping that the fix uses. No unrelated change was found.
Full details: Docstring Coverage
Explanation
Docstring coverage is 40.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 5 functions across 4 files.
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
Finishing Touches
Generate docstrings
Commit to this branch
Create a new PR
Generate unit tests (beta)
Commit to this branch
Create a new PR
Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
A rabbit checks each tuple row, And watches Rest unfold below. When types don’t match, it builds anew, With every value carried through. The tests hop in and give a nod, Then bound away across the sod.
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: 9e1734a4-d17b-4bea-a151-a748f3d36701
Commits
Reviewing files that changed from the base of the PR and between 9100521 and 4f3cdbc.
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 0 remain after this review.
The change is correct and well scoped. I found no blocking issues.
What I checked
CastInvocationArgument returns early when the value is already an instance of the target type. Compatible tuples keep their identity, and the fast path for already-correct arguments is unchanged.
Rebuilding happens only when both sides are tuples of the same logical arity. GetTupleElementTypes flattens the TRest nesting and UnwrapTupleAot (via ITuple) returns flat elements. The i == 7 recursion with a shared index therefore consumes elements in the right order, for both ValueTuple and Tuple.
Element conversion recurses through CastInvocationArgument, so nested tuples inside a tuple are handled the same way.
Constructors are cached per tuple type in a ConcurrentDictionary. This matches the guidance on caching reflection in hot paths.
The tests cover the widened, compatible, 8-element (Rest) and injected-token cases.
Minor notes (non-blocking)
UnwrapTupleAot has a pre-ITuple fallback switch for netstandard2.0, which only visibly covers up to 7 elements. On that target the 8-element case may not rebuild. The Engine is only exercised in reflection mode on modern TFMs, so this is probably unreachable. If it can be reached, ConstructInvocationTuple would index past the end of elements.
Add a cheap guard (index < elements.Length) or a comment stating the arity invariant that targetElements.Length == sourceElements.Length establishes. A malformed input would then give a clear error rather than an IndexOutOfRangeException.
The TupleConstructors cache is static and unbounded. It is bounded in practice by the number of distinct tuple types in the test assembly, so this is fine.
Nice fix, and it was good to file #7485 for the separate source-generated nested-tuple failure instead of widening this PR.
[Medium impact] The PR appears safe to merge; the previous long-tuple issue is fixed.
Summary
Fixes reflection-mode binding when a preserved tuple must match a closed generic parameter type.
Reflection mode rebuilds generic tuple rows to match test parameters.
The reflection fallback unwraps long tuples without losing their stored elements.
Diagram
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Preserved tuple row] --> B[Infer generic types]
B --> C{Argument already fits?}
C -->|Yes| D[Keep original value]
C -->|No| E[Read flat tuple elements]
E --> F[Rebuild tuple against closed parameter type]
D --> G[Invoke test]
F --> G
The fix is sound and well tested. I found no blocking issues.
What I checked
CastInvocationArgument only rebuilds a tuple when the value isn't already an instance of the target type and the arities match. Compatible rows keep their identity, and non-tuples still go through CastHelper.
ConstructInvocationTuple handles the 8th-slot Rest recursively for both ValueTuple and Tuple. It recurses with the shared index, so deep nesting works.
The netstandard2.0 fallback now flattens Rest and leaves a tuple in an Item slot intact. The old code sorted fields by name and never flattened. The cached MemberInfo[] also removes per-call reflection scanning.
The tests cover the widened, compatible, 8-element and cancellation-token cases. The unit tests exercise multiple Rest levels and reference tuples directly.
Non-blocking suggestions
Arity check.targetElements.Length == sourceElements.Length is the only guard before rebuilding. If GetTupleElementTypes returns a flattened list, this is fine. If a source and target have the same arity but an element isn't convertible, the failure is a CastHelper or constructor TargetInvocationException. Consider wrapping it with a message that names the parameter and the tuple types.
Duplicated shape logic. Tuple shape handling now lives in both TupleHelper (Core) and ReflectionTestDataCollector (Engine). A small shared internal helper, e.g. TupleHelper.GetConstructionShape(Type), would keep the i == 7 Rest rule in one place.
AOT.type.GetConstructor(arguments) on a runtime-closed generic is trim-unsafe if the tuple type was not rooted. You verified a native AOT publish for these cases. Make sure the RequiresUnreferencedCode/RequiresDynamicCode annotations cover ConstructInvocationTuple, since the closed type is built at runtime.
Cache growth.TupleConstructors and TupleMembers are static and unbounded. That is fine in practice, since the key space is the set of tuple types in the test assembly.
Overall this looks good to merge once CI is green, particularly the .NET Framework leg, which you noted hasn't run locally.
Reviewed every review surface for 4f3cdbc. Claude and Codex both report no blockers; the long-tuple thread is fixed and resolved.
Disposition of Claude’s remaining non-blocking suggestions: retain the existing conversion exception behavior, consistent with CastHelper; a richer diagnostic is optional rather than necessary for correct tuple binding. Keep reading and construction shape handling local to their respective helpers for this focused fix; both follow the same seven-items-plus-Rest rule and the regressions exercise multiple Rest levels. The new private construction methods are covered by ReflectionTestDataCollector’s existing class-level trimming/AOT suppressions (including IL2070 and IL3050); they do not add a public reflection contract. Cache growth is bounded by encountered tuple types, as the reviewer noted.
Promoting this reviewed head to ready. CI and the fresh ready-for-review Claude run will be handled by the next driver unit; auto-merge must wait for that Claude run and its findings to be checked.
Solid, well-scoped fix. Rebuilding only incompatible tuples (and leaving compatible ones untouched) is the right call, since it preserves identity and avoids needless allocation. Caching constructors and member lookups is good for the hot path. Replacing the OrderBy(f => f.Name) approach in the netstandard2.0 fallback with explicit Item1..7/Rest access also fixes a latent ordering bug (Item10 sorted before Item2). The tests cover the widened, compatible, long-tuple and token cases, and also test the legacy fallback directly.
Minor suggestions (non-blocking):
AppendTupleElements uses type.GetField(name)! / GetProperty(name)!. It is only reached after IsTupleType, so this is safe today. If that guard is ever loosened, you get an NRE instead of the old behaviour of an empty list. A short comment on the precondition would help.
ConstructInvocationTuple / CastInvocationArgument depend on UnwrapTupleAot and GetTupleElementTypes agreeing on how a nested tuple in an Item slot is flattened. A test for a nested tuple in an Item slot would pin that down. You note Generated invokers fail to convert nested tuple elements for generic tests #7485 already tracks the related source-gen failure, so this is fine to defer.
The earlier wait is complete: run 41627181292 succeeded on 4f3cdbc, and all CI legs, including Windows/.NET Framework and native AOT, passed. Codex's current-head summary is completed with no findings; Claude's ready-for-review verdict has no blockers. The inline finding remains fixed and resolved, with no later replies.
Disposition of the latest non-blocking suggestions: the tuple-member precondition is enforced immediately by UnwrapTupleUsingReflection's IsTupleType guard, and recursive calls consume only storage Rest. Keep that established guard without another comment. The legacy fallback tests already preserve a nested tuple in an Item slot; the distinct generated nested-binding limitation remains tracked in #7485. An ordinal prefix comparison would be a reasonable style improvement, but these fixed CLR tuple namespace prefixes have no demonstrated culture-dependent binding failure, so retain the existing comparison convention for this focused fix. CodeRabbit's docstring-coverage warning is advisory; the new helpers' purpose and Rest behavior are described in the code and existing API documentation, and no public API documentation is missing.
No blocking or undecided review findings remain. 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
A generic test taking
(T, object)now accepts a preserved(int, string)row in reflection mode after inferringT = int. The reflection invoker rebuilds incompatible tuple arguments against the closed parameter types, recursively handles long tuple Rest shapes, and preserves already compatible tuples. Tuple constructors and their argument types are cached. The netstandard2.0 tuple fallback also flattens Rest recursively for both ValueTuple and Tuple, with cached field/property lookups; nested tuples in Item slots stay intact. No public API or generator output changes.Closes #7482
Validation:
-m:1on net10.0/osx-arm64; all four regression cases pass individually in the native executable. Existing hook IL2075 warnings are tracked in Generated instance hooks emit IL2075 during Native AOT publishing #7430; dynamic-test IL2026 warnings describe intentionally unsupported AOT paths.An extra nested-tuple experiment exposed a separate source-generated conversion failure, filed as #7485; that failing experiment is excluded from this PR's regressions.
Follow-up validation (4f3cdbc):
Summary by CodeRabbit
Bug Fixes
Tests