[thomhurst]

Summary

A row holding only a tuple was spread across the parameters when a tuple parameter was followed by an optional parameter, so ((int, int) pair, int extra = 5) got pair = 25 and failed with a cast error in both modes.

DataUnwrapper.UnwrapWithTypesInternal now keeps the tuple whole when:

• it is already an instance of the first data parameter's tuple type, and
• every later data parameter is optional or params (CancellationToken parameters are skipped, as before).

ParameterMetadata.IsParams now also recognises C# 13 params collections (ParamCollectionAttribute). Before, it checked only ParamArrayAttribute, so params IEnumerable<T> still spread the tuple.

Rows that supply a value for every parameter ([(25, 26), 7]) and nested tuple rows (((25, 26), 7)) are spread as before. The type check runs before the optional-parameter scan, so other rows do no extra work.

Typed sources (decision)

A typed IEnumerable<(int, int)> source for the same signature is still rejected by TUnit0001. In source-gen mode the generated factory (DataSourceFactories.FromEnumerable → ToObjectArray) spreads typed tuple rows before DataUnwrapper runs and has no parameter types, so supporting typed rows needs either a new public factory overload or a change to the generated invoker. That is out of scope here and filed as #7340. A new analyzer test records the current diagnostic. The issue's "consistent in both modes" requirement still holds: object[] rows bind whole in both modes, and typed rows are rejected at compile time.

Tests

• tests/TUnit.TestProject/Bugs/_7212: 8 cases (tuple + optional, two optionals, optional + token, leading token, params array, params collection, supplied optional, nested tuple row) pass in source-gen and reflection mode. Before the fix, 5 of them failed in both modes.
• Issue7212Tests (engine test): passes.
• Regression filters, both modes: _7173, _7092, TupleUnwrappingTests, ArgsAsArrayTests, NestedTupleDataSourceTests, EnumerableTupleDataSourceDrivenTests, ClassTupleDataSourceDrivenTests all green.
• MethodDataSourceAnalyzerTests: all pass.

ClassDataSourceTupleTests fails on main too and is tracked in #7184.

Closes #7212

Summary by CodeRabbit

• Bug Fixes

  • Fixed method data binding so a tuple is correctly passed as a single argument when followed by optional or params parameters. Supplied optional values and default values are preserved, including when cancellation tokens are present.

• Tests

  • Added regression coverage for tuple data with optional parameters, cancellation tokens, and params collections.

[coderabbitai]

Important

Review skipped

We couldn't safely recover the incremental review. No full review was started, and the last reviewed checkpoint was preserved. Retry later, or explicitly request a full review by commenting @coderabbitai full review.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• Trigger review

Walkthrough

Data unwrapping now preserves a single tuple-valued row for the first tuple parameter when later data parameters are optional or params. Params detection also recognizes ParamCollectionAttribute. Regression tests cover binding behavior and typed-source diagnostics.

Changes: Tuple row binding

Layer / File(s)	Summary
Recognize params collection parameters src/TUnit.Core/Models/TestModels/ParameterMetadata.cs	Uncached params detection now recognizes ParamArrayAttribute and ParamCollectionAttribute.
Preserve tuple rows and validate binding src/TUnit.Engine/Helpers/DataUnwrapper.cs, tests/TUnit.TestProject/Bugs/_7212/*, tests/TUnit.Engine.Tests/Issue7212Tests.cs, tests/TUnit.Analyzers.Tests/MethodDataSourceAnalyzerTests.cs	Data unwrapping preserves a tuple row when later data parameters are optional or params, and skips cancellation tokens when checking those parameters. Tests cover omitted defaults, supplied optional values, nested tuples, and typed-source diagnostics.

Estimated code review effort: 2 (Simple) | ~12 minutes

Change: Bug fix · Severity of issue fixed: Low

Merge Risk: Moderate · up to d4aa4

A valid tuple row with an omitted params collection can fail when the test runs. Fix or explicitly reject that parameter shape before merging.

_{A rabbit checks each tuple row,
And keeps it whole when defaults flow.
The tokens pass; the params wait,
The tests confirm each binding state.
Then hops away through fields of green.}

[thomhurst]

@codex review

[chatgpt-codex-connector]

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-01T21:11:50.970165Z	d4aa46e	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.

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. Another round soon, please!

Reviewed commit: 3a25a48871

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

Confidence Score: 5/5

[Medium impact] The PR appears safe to merge; the test-count mismatch is fixed.

Summary

Keeps a tuple row whole when it matches the first tuple parameter and all later data parameters can be omitted.

• A single tuple row stays whole when later parameters can be omitted.

_{Reviews (4) · Last reviewed commit: "test: expect all eight _7212 cases in Is..." · Reviewed by Greptile}

[github-actions]

Review

The change is small and correct. I read the diff and the surrounding DataUnwrapper code. I didn't build or run the tests.

• Guard order: the checks go from cheapest to costliest (dataParameterCount > 1, then the tuple-type check, then IsInstanceOfType, then the optional scan). Rows that don't match skip the extra work.
• Null and length: values.Length == 1 and IsTuple(values[0]) are already guaranteed by the early return above. firstDataParameterIndex is also always set when dataParameterCount > 1. So there is no null or index risk.
• AOT: the change uses only IsInstanceOfType and metadata flags, with no new reflection.
• Tests: they cover tuple plus optional, two optionals, a token before and after, params, a supplied optional, and a nested tuple row. The nested row confirms the old spread behaviour is unchanged.
• Analyzer test: it records the current TUnit0001 behaviour for typed rows, and Support typed tuple sources for a tuple parameter followed by optional parameters #7340 tracks the follow-up.

Optional, not blocking: the fix is applied at unwrap time, so the typed-source case (#7340) will need a second fix in the factory path. A longer-term approach is to make ToObjectArray in the source-gen factories aware of parameter types, so the spread decision is made in one place.

LGTM.

[github-actions]

Review

The fix is small and correct, and I found no blocking issues.

• The new guard in DataUnwrapper.UnwrapWithTypesInternal is narrow. It needs a single tuple row, a first data parameter that is a tuple type, an exact type match through IsInstanceOfType, and every later data parameter optional or params. Rows like [(25, 26), 7] and nested tuple rows still take the existing path.
• Skipping CancellationToken parameters when finding the first data parameter is consistent with the existing handling.
• The tests cover tuple plus optional, two optionals, optional plus token, leading token, params, a supplied optional, and a nested row. They run through an engine test, and the analyzer test records the current TUnit0001 behaviour for typed sources.
• Typed IEnumerable<(int,int)> sources are still rejected (Support typed tuple sources for a tuple parameter followed by optional parameters #7340). I agree with deferring that, but the two modes now differ in what they accept: object[] rows bind whole, while typed rows are rejected at compile time. That is worth a docs note once Support typed tuple sources for a tuple parameter followed by optional parameters #7340 is resolved.
• One small suggestion: RemainingDataParametersAreOmittable is only reached after the tuple-type check, so it adds no cost on the hot path. If you ever need to tighten it, there is a design question about a tuple whose arity differs from the declared type, but that is outside this PR.

Looks good to merge once CI is green.

[thomhurst]

@codex review

[github-actions]

Review of #7341

The fix is well-scoped. The type check runs first, so other rows pay nothing extra. Recognising ParamCollectionAttribute by name is a sensible way to handle target frameworks that lack the type. The analyzer test that records the typed-source limitation, with #7340 filed for it, is a good way to leave that gap documented.

Issue to fix before merge: the engine test count doesn't match the test project.
tests/TUnit.Engine.Tests/Issue7212Tests.cs:18-19 asserts Total == 7 and Passed == 7. TupleParameterWithOptionalParameterTests.cs has 8 test methods, and the PR description also says 8 cases. The filter //TUnit.TestProject.Bugs._7212//* selects all of them, so this engine test should fail in CI with 8 against 7. Change both assertions to 8. The PR says this test passes, but that can't be right with these values. It may have been run against an earlier version of the project.

Minor suggestions

• IsParamsParameter calls GetCustomAttributesData() on every uncached IsParams access. The impact is small, because it only runs after the tuple-type check passes. It would still be worth populating CachedIsParams at metadata creation if it isn't already.
• RemainingDataParametersAreOmittable rescans the parameters after the first loop already walked them. Folding the check into the first loop would avoid the second pass. This is optional.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 234cec439c

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

Dispositions for the latest Claude review (comment 6563348124):

• Engine test count (blocking): fixed in d4aa46e. Both assertions now expect 8. The _7212 filter runs 8 tests locally and all 8 pass.
• IsParams reflection on uncached access (minor): not changed. CachedIsParams is never set by either mode today, so this predates the PR. The new code reaches it only after the tuple-type and instance checks pass, which is a rare path. It's not worth widening this PR for.
• Second pass in RemainingDataParametersAreOmittable (optional): not changed. The scan runs only on the narrow matching path, and a separate helper keeps the guard readable.

[thomhurst]

@codex review

[github-actions]

Review

The fix is targeted and I found no blocking issues.

What I checked

• The new tuple-binding condition in DataUnwrapper has three parts. The first data parameter must be a tuple type, the row value must be an instance of it, and every later data parameter must be optional or params. Together they keep the whole-tuple binding narrow. Rows that supply every parameter, and nested tuple rows, still spread as before, and the tests cover both.
• The type check runs before the optional-parameter scan, so unrelated rows pay no extra cost.
• Matching ParamCollectionAttribute by name is a reasonable way to avoid a target-framework dependency, and the test for params IEnumerable<int> covers it.
• Documenting the typed-source limitation with an analyzer test, and filing Support typed tuple sources for a tuple parameter followed by optional parameters #7340 for it, is a sensible scope decision.

Minor suggestions (non-blocking)

1. IsParamsParameter calls GetCustomAttributesData() whenever CachedIsParams is null. That is reflection, and it allocates on every IsParams call. If source-gen mode always sets the cached value, this only affects reflection mode. Even so, caching the result in a field (as the Cached* pattern does) would keep it off hot paths and out of the AOT/trimming analysis.
2. One case stays ambiguous by design. (int, int) pair, int extra = 5 with a row [(25, 26)] now always binds the tuple whole. That is the right call given the issue, but a short note in the docs on tuple unwrapping would help users who expected a spread.

Tests cover all eight scenarios in both modes, plus the regression filters. Looks good to merge.

[chatgpt-codex-connector]

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

Reviewed commit: d4aa46e0ed

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

Dispositions for the Claude review of d4aa46e (comment 6563547811). Both notes are non-blocking.

• IsParamsParameter reflection when uncached: not changed, for the reason given in comment 6563542419. Neither mode populates CachedIsParams today, so this predates the PR. The new code reaches it only after the tuple-type and instance checks pass.
• Docs note on whole-tuple binding: deferred to Support typed tuple sources for a tuple parameter followed by optional parameters #7340. Typed-source support will change the same behaviour, so the docs should describe both paths together when Support typed tuple sources for a tuple parameter followed by optional parameters #7340 lands rather than now.

[github-actions]

Review of #7341

I found no blocking issues. The fix is narrow and well scoped.

What I checked

• The new tuple-whole check in DataUnwrapper runs only after the cheap IsTupleType and IsInstanceOfType tests. Rows that don't match skip the optional-parameter scan, so the hot path is unchanged for them.
• Rows that supply every parameter ([(25, 26), 7]) and nested tuple rows are still spread as before. The tests cover both cases.
• ParameterMetadata.IsParams matches ParamCollectionAttribute by name through GetCustomAttributesData. That avoids a dependency on a type missing from some target frameworks and is trim/AOT-safe.
• The analyzer test records the current TUnit0001 behaviour for typed tuple rows, and Support typed tuple sources for a tuple parameter followed by optional parameters #7340 tracks the follow-up. That is an honest way to leave the gap.

Suggestions (non-blocking)

1. IsParams now calls GetCustomAttributesData() on every access when CachedIsParams is null. That can allocate in reflection mode. If it's used in a hot path, cache the result in a lazy field.
2. When CachedIsParams is populated by the source generator, check that the generator also recognises C# 13 params collections. Your source-gen-mode test for params IEnumerable<int> suggests it does, but a short comment would make that explicit.
3. The ExpectedResult.Pass count of 8 in Issue7212Tests is hardcoded. That's fine, but it will need updating whenever a case is added to _7212.

The change looks good to merge once CI is green.