Skip to content

Accept numeric widening for single reference-tuple data parameters - #7510

Merged
thomhurst merged 13 commits into
mainfrom
issue-7480-reference-tuple
Nov 3, 2026
Merged

thomhurst merged 13 commits into
mainfrom
issue-7480-reference-tuple

Conversation

@thomhurst

@thomhurst thomhurst commented Nov 3, 2026 •

Copy link
Copy Markdown
Owner

A method data source returning Tuple<int, int> now binds to a single Tuple<long, long> test parameter without TUnit0001. The analyzer checks implicit element conversions and handles flattened Rest elements, while retaining exact-type binding and tuple spreading.

Adds positive regressions for direct and enumerable rows, one- and two-element tuples, eight- and fifteen-element tuples, and unchanged nested tuple elements. Unsupported nested reference-tuple element widening retains TUnit0001. Negative regressions cover narrowing, incompatible Rest elements, and shorter/longer tuples. Runtime conversion shipped in #7469; this PR delivers the analyzer scope tracked separately in #7480.

Validation: the three initial widening cases failed before the fix. All 15 reference-tuple cases and all 134 method-data-source analyzer cases pass on net10.0 (SDK 10.0.403, macOS arm64). Only the net10.0 runtime is installed; other frameworks require CI. Existing build warnings are tracked in #7410. No generator implementation or public API changes; the Tests7402 snapshot includes the added runtime regressions.

Closes #7480

Review follow-up cdcdb7d:

  • Rebased onto main after Preserve null-containing tuples from static method data sources #7469 merged; run 41662664099 completed successfully, clearing the prerequisite.
  • Removed per-element metadata-name construction/lookups, and fixed the test formatting nit.
  • Added boxing/reference-conversion analyzer cases: all 17 reference-tuple cases pass on net10.0.
  • Removed the obsolete TUnit0001 suppression from the long reference-tuple runtime regression; it builds and passes in generated and reflection modes on net10.0.
  • No generator implementation or public API changes; the Tests7402 snapshot includes the added runtime regressions. Fresh review is required after this push.

Summary by CodeRabbit

  • Bug Fixes
    • Method data sources now correctly recognize reference tuples supplied as complete test-argument rows, including long tuples.
    • Supported whole-tuple conversions, including numeric widening and exact user-defined conversions to nullable tuples, are accepted.
    • Incompatible tuple conversions, including unsupported constructor widening and composed user-defined conversions, continue to produce diagnostics.
  • Tests
    • Added coverage for reference-tuple conversions from direct and enumerable data sources, including nullable tuple cases.

Review follow-up: confirmed the generated runtime does not reconstruct nested reference-tuple Items. Removed analyzer recursion into those Items while retaining flattened Rest support; added direct and enumerable negative regressions and retained positive coverage for unchanged nested Items. This keeps #7480 within its runtime-supported scope.

Review follow-up a9205da: restricted reference-tuple reconstruction acceptance to test-method binding. Constructor binding retains TUnit0001 for widened tuple types; exact constructor tuple binding remains supported. Added direct/enumerable constructor regressions. All 21 reference-tuple and all 149 method-data-source analyzer cases pass on net10.0. Existing warnings remain tracked in #7410 and #7412.

Review follow-up dd1b937: added analyzer and end-to-end char-to-decimal tuple regressions to verify the CodeRabbit report. Runtime test passes in generated and reflection modes without production changes; all 150 method-data-source analyzer cases pass on net10.0.

Review follow-up 4aadab8: reject nullable value-tuple element widening and composed user-defined conversions unsupported by CastHelper. Added direct/enumerable negative regressions (both reported shapes failed before the fix), a nested nullable-tuple regression, and positive coverage retaining exact user-defined operators, numeric nullable widening and unchanged tuple lifting. All 29 reference-tuple and all 157 method-data-source analyzer cases pass on net10.0. No runtime, generator implementation or public API changes in this follow-up; the PR snapshot includes earlier runtime regressions. Source-only AOT converter discovery remains tracked in #7511.

The earlier wait is cleared: run 41707540291 succeeded. Pushed 5b33813 after rebasing onto main 5d9086b and fixed/resolved the new Greptile nullable-tuple operator finding. The exact-operator regression failed before the fix. All 158 method-data-source analyzer cases, 24 StaticTupleDataSourceTests in each of generated/reflection modes, both Tests7402 generator snapshot tests, and six SharedObjectDisposeSpanTests pass on net10.0.

CI diagnosis: Windows/macOS failed because the prior char-to-decimal runtime regression had no snapshot update. Reviewed and committed the updated Tests7402.Test.verified.txt, including the new operator cases. Ubuntu's duplicate disposal span failure is recorded on #7435; this base includes the newer session ownership fix #7509, and fresh CI must validate net8.0. No tests were skipped or weakened, and no CI rerun was requested. Existing build warnings are tracked in #7410/#7412. Only net10.0 is installed locally; other frameworks and native publishing remain for CI.

Claude's non-blocking drift suggestion is now addressed with a CastHelper cross-reference and analyzer/runtime coverage for exact nullable-tuple operators. Fresh CI and Claude/Codex verdicts are required for this head before auto-merge; no external blocker remains.

Review follow-up fcb4d4a: reject native-integer to decimal tuple element conversions unsupported by CastHelper, including nullable targets and nested value tuples. All seven new direct/enumerable regression cases failed before the fix; all 178 method-data-source analyzer cases pass on net10.0 (SDK 10.0.403). No runtime or generator implementation changes in this follow-up. Other frameworks remain for CI. Conversion-parity coverage requested by Claude is tracked in #7530.

Review follow-up b73bcad: reject small-integral element conversions to native integers that CastHelper cannot execute. All 13 new negative direct/enumerable cases failed before the fix; added four positive exact-operator cases. All 67 reference-tuple and 221 method-related analyzer cases pass on net10.0. A separate built-CastHelper check confirms 16 unsupported casts fail and four exact int/uint operators succeed. Includes nullable source/target and nested value-tuple coverage. No runtime, generator implementation or public API changes in this follow-up. Existing warnings remain tracked in #7410/#7412; other frameworks remain for CI. Run 41707540291 succeeded and its earlier wait is cleared. Fresh CI and both reviewer verdicts are needed for this head; auto-merge remains disabled.

@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

The analyzer now accepts supported implicit element conversions when binding reference-tuple method data sources. It can preserve a compatible reference tuple as one parameter. Analyzer and runtime tests cover accepted conversions, rejected bindings, and generated test entries.

Changes: Reference tuple conversion

Layer / File(s) Summary
Reference tuple binding checks
src/TUnit.Analyzers/TestDataAnalyzer.cs, src/TUnit.Core/Helpers/CastHelper.cs
The analyzer checks supported implicit conversions between reference-tuple elements for method symbols. It preserves a compatible reference tuple as one parameter. The cast helper change adds a comment; executable behavior is unchanged.
Analyzer binding regressions
tests/TUnit.Analyzers.Tests/ReferenceTupleMethodDataSourceTests.cs
Tests cover exact tuple types, supported widening, nullable and nested tuples, long tuple forms, incompatible shapes, and user-defined conversions.
Converted tuple runtime coverage
tests/TUnit.TestProject/Bugs/_7402/StaticTupleDataSourceTests.cs, tests/TUnit.Core.SourceGenerator.Tests/Tests7402.Test.verified.txt
Project tests add character-to-decimal and nullable tuple cases and remove two warning suppressions. Generated test output adds entries and updates method indices and source lines.

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

Change: Bug fix · Severity of issue fixed: Medium

Possibly related issues

  • thomhurst/TUnit#7374: The issue concerns System.Tuple data-source binding and spreading. This change also updates reference-tuple row handling in the analyzer.
  • thomhurst/TUnit#7465: The issue concerns typed-row parameter binding rules. This change adds reference-tuple conversion checks to that binding analysis.

Possibly related PRs

  • thomhurst/TUnit#7469: This PR adds runtime reconstruction for single reference-tuple rows. This change adds analyzer acceptance for supported element conversions used by that binding.
  • thomhurst/TUnit#7457: This PR adds analyzer support for spreading System.Tuple rows and preserving one tuple parameter. This change extends the same binding path to supported implicit element conversions.
Merge Risk: Low · previous review 5393c

Some accepted tuple conversions can fail when test data is bound, including in native AOT. Workarounds exist for the affected cases.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 12.50% 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 "Accept numeric widening for single reference-tuple data parameters" matches the main change. The PR changes analyzer binding so TUnit0001 accepts a single reference-tuple test-method parame…
Linked Issues check Passed Active issue #7480 requires compatible widening for one reference-tuple test-method parameter, including flattened nested Rest, negative diagnostics, exact binding, and tuple spreading. `TestDataAna…
Out of Scope Changes check Passed The changes remain within issue #7480. Analyzer changes implement the requested binding rules. Analyzer and runtime tests provide regression coverage for supported conversions and rejected boundaries.…
Full details: Title check

Explanation

The title "Accept numeric widening for single reference-tuple data parameters" matches the main change. The PR changes analyzer binding so TUnit0001 accepts a single reference-tuple test-method parameter whose elements have compatible implicit numeric conversions. The diff adds analyzer logic in TestDataAnalyzer.cs, analyzer tests, and updated runtime tests and snapshot output. The title is specific and concise. It does not mention the narrower limits (test-method binding only, no nested Items, no constructor or nullable value-tuple widening), but the title does not need to cover every detail.

Full details: Linked Issues check

Explanation

Active issue #7480 requires compatible widening for one reference-tuple test-method parameter, including flattened nested Rest, negative diagnostics, exact binding, and tuple spreading. TestDataAnalyzer.cs adds whole-row conversion checks for test methods, recursively validates flattened elements, and retains constructor diagnostics. ReferenceTupleMethodDataSourceTests.cs covers direct and enumerable sources, short, eight-element, and nested Rest tuples, incompatible types, narrowing, length mismatches, exact binding, and spreading. Closed issue #7509 adds no requirements.

Full details: Out of Scope Changes check

Explanation

The changes remain within issue #7480. Analyzer changes implement the requested binding rules. Analyzer and runtime tests provide regression coverage for supported conversions and rejected boundaries. The CastHelper.cs change is a supporting cross-reference comment, and the generated snapshot updates support the added runtime tests. The evidence shows no unrelated production behavior, generator output, or public API change.

Full details: Docstring Coverage

Explanation

Docstring coverage is 12.50% 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 8 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.

Share

A rabbit checks each tuple row,
And sorts the types from high to low.
A nested pair can pass the test,
While mismatched shapes are put to rest.
The generated cases join the flow,
And carrots mark the green lights glow.

@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-03T22:56:18.939486Z b73bcad 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.

@chatgpt-codex-connector

Copy link
Copy Markdown

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

Reviewed commit: 6360bb3dbb

ℹ️ 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 PR appears safe to merge; no new blocking issue was found.

Summary

The PR lets a method data source bind a reference tuple to one test parameter when its elements can be converted.

  • Test methods can receive converted reference tuples as one parameter.

Reviews (12) · Last reviewed commit: "Reject unsupported native-integer tuple ..." · Reviewed by Greptile

@github-actions

github-actions Bot commented Nov 3, 2026

Copy link
Copy Markdown
Contributor

Review of #7510

The analyzer change is small, targeted and well tested. It recurses through nested tuples and Rest, restricts the check to System.Tuple arities 1-8, and leaves the exact-type and spreading paths alone. Tests cover direct and enumerable rows, nested and Rest tuples, and narrowing and arity mismatches.

Concerns

  1. The analyzer may now accept code that fails at runtime. The description says runtime conversion is still tracked in Preserve null-containing tuples from static method data sources #7469. If the engine does not yet convert Tuple<int,int> to Tuple<long,long>, TUnit0001 disappears and users get a runtime binding failure instead, which is worse. Please confirm runtime handling exists. If it doesn't, land this with Preserve null-containing tuples from static method data sources #7469 or add an end-to-end test in TUnit.TestProject (targeted filter) showing the widened tuple works.

  2. ClassifyConversion(...).IsImplicit is broader than numeric widening. It also accepts user-defined implicit conversions, boxing and reference conversions (e.g. Tuple<string> to Tuple<object>). Unless the runtime follows the same rules, analyzer and runtime can disagree. Share one conversion predicate between them, or narrow the analyzer to what the runtime supports.

  3. GetTypeByMetadataName runs on every check. IsReferenceTuple builds a metadata-name string and looks it up per element at every recursion level. Compare ContainingNamespace and Name == "Tuple" on OriginalDefinition, or cache the eight symbols per compilation.

Nit: missing blank line before the first new [Test] and an extra blank line before the closing brace in ReferenceTupleMethodDataSourceTests.cs.

Overall good; concern 1 is the one to resolve before merge.

🤖 Generated with Claude Code

@thomhurst

thomhurst commented Nov 3, 2026 •

Copy link
Copy Markdown
Owner Author

Claude concern 1 is blocking: runtime reconstruction is still in open #7469, so this analyzer change must follow it. GitHub rejects native blocked_by relationships involving pull requests (HTTP 422: source and target may only be issues), so #7469 is documented here as the prerequisite. Moving to it under its own lock. Concerns 2 and 3 and the formatting nit remain undispositioned; verify the conversion predicate against the final runtime implementation before promoting this draft.

@thomhurst

Copy link
Copy Markdown
Owner Author

@codex review

Claude concerns dispositioned in cdcdb7d:

  1. Preserve null-containing tuples from static method data sources #7469 is merged and AOT run 41662664099 succeeded. Removed the obsolete analyzer suppression from Long_Reference_Tuple_Elements_Can_Be_Converted; that test now compiles and passes in generated and reflection modes.
  2. Kept implicit conversions: the merged runtime recursively reconstructs tuples and delegates element conversion to CastHelper (ReflectionTestDataCollector.CastInvocationArgument/ConstructInvocationTuple and generated invokers). CastHelper supports assignability/boxing, primitive conversions and user-defined operators; the existing Source-gen: static [MethodDataSource] row Tuple.Create<string?>(null) reaches a single Tuple<string?> parameter as null #7402 implicit-operator regression covers the fallback. Native operator discovery limitations are already tracked in Native AOT omits conversion operators used only by method data source return types #7511, with explicit AotConverterRegistry registration supported. Added analyzer cases for Tuple to Tuple and Tuple to Tuple; all 17 reference-tuple cases pass.
  3. Replaced repeated GetTypeByMetadataName/string construction with OriginalDefinition name/namespace checks, as suggested, including tuple element extraction.
    Formatting nit fixed. Please review the current head; the draft remains pending fresh Claude and Codex verdicts.

@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: cdcdb7d6c5

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

@github-actions

github-actions Bot commented Nov 3, 2026

Copy link
Copy Markdown
Contributor

Review of #7510

The analyzer change looks sound. CanConvertReferenceTuple validates the flattened logical elements, including the recursive Rest. It keeps exact-type binding and tuple spreading, and it rejects narrowing and mismatched arity. The new positive and negative analyzer cases cover those paths well. Replacing the per-element GetTypeByMetadataName lookups with the name-based IsReferenceTuple check is a good simplification.

Issue: pragma restore lines deleted in StaticTupleDataSourceTests.cs

The diff removes the 'pragma warning restore TUnit0001' lines after three 'pragma warning disable TUnit0001' directives, at lines 105, 115 and 142 in the PR head. Only the first removal (LongReferenceValue) was correct, because its matching disable was removed too. The other three disable directives remain, so TUnit0001 is now suppressed from line 105 to the end of the file. That hides real analyzer diagnostics for any test added to this file later, and weakens the file as a regression check for this change.

Suggested fix:

  • Restore the 'pragma warning restore TUnit0001' lines after the attributes at lines 105 and 115, which still need the suppression.
  • Check whether the line 142 suppression (reference-tuple widening analyzer limitation, TUnit0001 rejects numeric widening for a single reference-tuple parameter #7480) is still needed. That case is what this PR fixes, so the disable and restore pair should probably both be removed, as was done for LongReferenceValue. That also proves the fix against a real test.

Please build the test project after the fix to confirm there are no stray TUnit0001 warnings. Otherwise this is good to merge.

@thomhurst

Copy link
Copy Markdown
Owner Author

@codex review

Fixed the Claude, Codex and Greptile suppression findings in 2aa601f. Both intentional runtime-only suppressions now restore TUnit0001 immediately after their attributes; the supported reference-tuple case has no suppression. Build reports no TUnit0001 diagnostics; all 21 StaticTupleDataSourceTests pass in generated and reflection modes on net10.0 (SDK 10.0.403). No production code, generator output or public API changes.

The earlier blocker is cleared: #7469 merged and run 41662664099 completed successfully. This draft now awaits fresh review of the new head.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Chef's kiss.

Reviewed commit: 2aa601fc56

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

@github-actions

github-actions Bot commented Nov 3, 2026

Copy link
Copy Markdown
Contributor

Review: LGTM, with one minor point.

The change is small and well scoped. CanConvertReferenceTuple compares the flattened logical elements, recurses into nested tuples, and keeps the exact-type and spreading paths. IsReferenceTuple replaces the per-call GetTypeByMetadataName lookups with a symbol-name check, which is cheaper on a hot analyzer path. The tests cover both directions: widening is accepted, and narrowing, mismatched Rest elements and arity mismatches are rejected. Removing the two #pragma warning disable TUnit0001 suppressions from the runtime regression tests is a good sign the analyzer now matches the runtime.

Minor point: ClassifyConversion(...).IsImplicit also accepts user-defined implicit conversions and implicit nullable conversions. If the runtime converter in #7469 only handles built-in numeric, boxing and reference conversions, the analyzer could accept a row that fails at runtime. If the runtime uses Convert.ChangeType or an equivalent, it is worth adding a test with a user-defined implicit operator element to pin the intended behavior either way. Otherwise this is fine to merge.

I didn't build or run the tests myself.

@thomhurst

Copy link
Copy Markdown
Owner Author

Verified the earlier prerequisite is cleared: #7469 merged and AOT run 41662664099 succeeded. Current head 2aa601f has positive Claude and Codex verdicts and no unresolved threads.

Disposition of Claude’s remaining minor point: ReflectionTestDataCollector.CastInvocationArgument recursively reconstructs tuple elements and calls CastHelper.Cast; CastHelper unwraps nullable targets and supports cached user-defined operators, rather than relying only on Convert.ChangeType. The #7469 operator regressions and explicit native converter registration are documented above; automatic native operator discovery remains tracked in #7511. No further change is required for this analyzer PR. Marking ready for CI.

@thomhurst

Copy link
Copy Markdown
Owner Author

Promoted unchanged head 2aa601f to ready after both reviewers approved. Waiting for the promotion-triggered Claude review run 41707540291; read its new comment and re-check all review surfaces before arming auto-merge with scripts/Enable-AutoMerge.ps1. CI also started on promotion. The earlier #7469 prerequisite is cleared; no external blocker remains.

@github-actions

github-actions Bot commented Nov 3, 2026

Copy link
Copy Markdown
Contributor

Review

The change looks correct and well scoped.

  • CanConvertReferenceTuple is applied at the three places that matter: the enumerable/direct binding check, the single-parameter check, and UnwrapTypes. Exact-type binding and tuple spreading are unchanged.
  • IsReferenceTuple matches by name and namespace. It replaces the per-call GetTypeByMetadataName lookup and string formatting, so it is cheaper in an analyzer hot path.
  • The recursion handles nested tuples and the flattened Rest element. The negative tests cover narrowing, an incompatible Rest, and length mismatches.
  • Removing the #pragma warning disable TUnit0001 lines from the runtime regression tests confirms the analyzer and runtime now agree.

Minor point, non-blocking: ClassifyConversion(...).IsImplicit also accepts user-defined implicit conversions. That is only a problem if the runtime converter does not support them. If it doesn't, a case where Tuple<Foo> flows to Tuple<Bar> through an implicit operator would pass the analyzer and fail at runtime. A test for that case, or a restriction to numeric, boxing and reference conversions, would pin the behaviour down.

Approving.

@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: 2aa601fc56

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

Comment thread src/TUnit.Analyzers/TestDataAnalyzer.cs Resolved

@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: 049b61e99d

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

Comment thread src/TUnit.Analyzers/TestDataAnalyzer.cs Resolved
@github-actions

github-actions Bot commented Nov 3, 2026

Copy link
Copy Markdown
Contributor

Review

The change looks good. CanConvertReferenceTuple compares the logical elements, including the flattened Rest, with ClassifyConversion(...).IsImplicit. It is wired into all three binding paths (the argument conversion check, the single-parameter check and UnwrapTypes). Exact-type binding and tuple spreading are unchanged.

  • IsReferenceTuple matches by name and namespace, which removes the per-element GetTypeByMetadataName lookups. That is cheaper in an analyzer hot path and is correct.
  • The tests cover direct and enumerable rows, arities 1, 2, 8 and 15, boxing and reference conversions, narrowing, incompatible Rest elements, and length mismatches. The nested-Items negative case matches the runtime's actual behaviour.
  • Dropping the #pragma warning disable TUnit0001 suppressions in the runtime tests shows the analyzer fix works end to end.

Minor, non-blocking: IsImplicit also accepts user-defined implicit conversions. If the runtime's CastHelper doesn't honour those for tuple elements, the analyzer could accept something that fails at runtime. This is a rare edge case. A single test with a user-defined implicit operator would confirm which way it goes.

I haven't run the tests locally. The PR description reports the analyzer tests passing on net10.0.

Approve.

@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

Comment thread src/TUnit.Analyzers/TestDataAnalyzer.cs Resolved
This was referenced Nov 5, 2026

This branch was previously deployed

1 inactive deployment
Pull Requests — b73bcade Deployed Nov 3, 2026 by thomhurst via modularpipeline (ubuntu-latest) #21044
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.

TUnit0001 rejects numeric widening for a single reference-tuple parameter

1 participant