[thomhurst]

When MethodDataSource Arguments omit an optional parameter and the method name is overloaded, reflection discovery can throw AmbiguousMatchException even though source-generated discovery selects a valid overload.

Fall back to the existing overload scorer when the runtime binder finds no exact-arity match. This handles omitted optional and CancellationToken parameters while preserving the binder's existing successful selections. Retain the first equally scored overload unless a later one is more specific, matching the generator. Add regressions for the inherited override default and for tied overloads with one or two omitted CancellationToken parameters in both declaration orders, and update the parity test count.

Closes #7331

Validation: all 34 DataSourceOverloadParityTests passed on net10.0 in source-generated mode and with --reflection. Engine integration tests are left to CI per repository instructions. The separate analyzer optional-parameter limitation is tracked in #7418.

Native AOT validation: dotnet publish tests/TUnit.TestProject/TUnit.TestProject.csproj -c Release -f net10.0 -p:Aot=true --use-current-runtime reached native linking but failed because this machine lacks libssl (ld: library 'ssl' not found). AOT publishing remains for CI. No new reflection APIs or trimming annotations were introduced.

Summary by CodeRabbit

• Bug Fixes
  • Improved test data source overload selection when arguments are null, non-null, or omitted optional parameters.
  • Added coverage for a non-null argument in an overload scenario.

Review follow-up: 42492b8 fixes tied omitted-token overloads and updates the resolver comments. All 34 parity cases passed in both discovery modes. No generator implementation or public API changed; Engine integration and native AOT remain for CI. Existing analyzer build warnings are tracked in #7410.

Review follow-up: ddc3f41 aligns nullable numeric specificity with source generation, including lifted decimal conversions. Added null-argument parity rows for int?/long? in both declaration orders and int?/decimal?. All 37 cases passed in source-generated and reflection modes on net10.0. No generator implementation or public API changed. Engine integration remains for CI; the native AOT linker limitation documented above still applies.

[coderabbitai]

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 4 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 8 included reviews currently available.

Learn how review limits work.

Review configuration:

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: 2d0900c6-ca59-4c35-a9ef-3f744f81ef3e

Commits

Reviewing files that changed from the base of the PR and between 2ecfe92 and ddc3f41.

Files selected for processing: 3

• src/TUnit.Core/Attributes/TestData/MethodDataSourceAttribute.cs
• tests/TUnit.Engine.Tests/Issue7108Tests.cs
• tests/TUnit.TestProject/Bugs/_7108/DataSourceOverloadParityTests.cs

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: b2bca1ba-aa92-4be1-ab23-b925e3aa398c

Commits

Reviewing files that changed from the base of the PR and between ca4437c and 2ecfe92.

Files selected for processing: 3

• src/TUnit.Core/Attributes/TestData/MethodDataSourceAttribute.cs
• tests/TUnit.Engine.Tests/Issue7108Tests.cs
• tests/TUnit.TestProject/Bugs/_7108/DataSourceOverloadParityTests.cs

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

Walkthrough

MethodDataSource overload resolution now uses the shared scoring resolver for null-containing arguments and when the runtime binder selects no method. Tests add coverage for a non-null argument and update the expected test count.

Changes: MethodDataSource overload resolution

Layer / File(s)	Summary
Resolver fallback and parity coverage src/TUnit.Core/Attributes/TestData/MethodDataSourceAttribute.cs, tests/TUnit.TestProject/Bugs/_7108/DataSourceOverloadParityTests.cs, tests/TUnit.Engine.Tests/Issue7108Tests.cs	Both platform paths store the binder-selected method and use the scoring resolver when no method is selected. Null-containing arguments also use that resolver. The tests add a non-null argument case and increase the expected total and passed counts to 32.

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

Change: Bug fix · Severity of issue fixed: Low

Possibly related PRs

• thomhurst/TUnit#7336: Adds reflection helpers that use the same MethodDataSource overload resolver.

Merge Risk: Low · previous review 2ecfe

Reflection-mode discovery can fail for tied overloads with omitted tokens. The affected case is narrow, but it should be fixed or explicitly accepted before merging.

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 reflection data source overload resolution when optional arguments are omitted.
Linked Issues check		The changes satisfy #7331. ResolveDataSourceMethod keeps the runtime binder result when it succeeds and calls ResolveDataSourceMethodByScore when the binder returns no exact-arity match. The score…
Out of Scope Changes check		All changed files support #7331. The resolver change implements the fallback. The added parity row and the count update test the reported overload-selection behavior. No unrelated product behavior or …

Full details: Linked Issues check

Explanation

The changes satisfy #7331. ResolveDataSourceMethod keeps the runtime binder result when it succeeds and calls ResolveDataSourceMethodByScore when the binder returns no exact-arity match. The scorer covers omitted optional and CancellationToken parameters, including null arguments. The parity suite adds a non-null inherited override case and updates the expected count from 31 to 32. Existing Optional and Token rows also cover omitted trailing parameters.

Full details: Out of Scope Changes check

Explanation

All changed files support #7331. The resolver change implements the fallback. The added parity row and the count update test the reported overload-selection behavior. No unrelated product behavior or unrelated test scope is changed.

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 2 functions across 3 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

• X
• Mastodon
• Reddit
• LinkedIn

_{A rabbit checks each overload row,
Where null or missing arguments go.
The binder tries, then scores the rest,
A parity case joins the test.
Thirty-two pass; the carrots cheer.}

CodeRabbit Verify

• Run CodeRabbit Verify to test the changes in this pull request.

[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-03T00:02:06.817420Z	ddc3f41	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]

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

Reviewed commit: 2ecfe92402

ℹ️ 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 previous overload-selection issue is fixed and no new blocking issues were found.

Summary

Reflection discovery now falls back to overload scoring when the binder finds no match with the supplied argument count.

• Reflection discovery now finds data-source methods when arguments are omitted.

No new actionable issues found. Tests were not run during this review.

_{Reviews (3) · Last reviewed commit: "Fix nullable numeric overload specificit..." · Reviewed by Greptile}

[github-actions]

Review: LGTM, with one minor nit.

The change is small and well-targeted. The runtime binder is tried first, so existing successful selections don't change. When it returns null (it needs exact arity), the code falls back to the existing scorer, which already mirrors the source generator's ScoreDataSourceOverload. That is why the AmbiguousMatchException from the caller's name-only lookup goes away. Reusing the scorer instead of adding a third resolution path keeps reflection and source-gen behaviour aligned. Renaming ResolveDataSourceMethodWithNullArgument to ResolveDataSourceMethodByScore fits what it now does. No new reflection APIs or trimming annotations are introduced, and the suppression and DynamicallyAccessedMembers attributes carry over.

• Stale comment (nit): the block comment above ResolveDataSourceMethod still says the scorer is used only 'when any argument is null'. Please update it to mention the fallback when the binder finds no exact-arity match.
• Test coverage: the new non-null OverriddenDefault row covers the inherited-override case. A row with an overloaded name where only optional or CancellationToken params are omitted and all arguments are non-null would pin the exact scenario from Reflection mode: MethodDataSource Arguments that omit an optional parameter throw AmbiguousMatchException when the method is overloaded #7331 more directly, if Token(...) doesn't already cover it.
• Validation: the Engine test count bump (31 to 32) matches the one added row. The AOT publish was not completed locally, so CI should confirm it.

Nothing blocking.

[thomhurst]

Review disposition for 2ecfe92: Claude and Codex report no blocking findings; there are no review threads. The new inline comment immediately above the fallback documents the binder's exact-arity limitation and scorer fallback. The earlier block describes the null-argument path correctly, although it could also summarize the new fallback; accepting that documentation nit without another code push per the loop's nonblocking-finding policy.

The existing non-null Optional row (line 275), Token row (line 276), and new non-null inherited override row (line 279) exercise omitted trailing arguments; the inherited override case supplies the overloaded lookup regression. No additional coverage is required for this review suggestion.

Promoting the reviewed head to ready. Promotion triggers fresh CI and Claude review; auto-merge will remain unarmed until that Claude run passes and its review has been read.

[coderabbitai]

• Fix comments: Fix CodeRabbit comments on this PR

[github-actions]

Review of #7421

I found no blocking issues. The change is a good simplification.

What I checked

• Fallback scope: ResolveDataSourceMethodByScore now runs when the binder returns no exact-arity match. It only picks a method when ScoreArguments is >= 0, so it can't select an overload that can't accept the arguments. Cases the binder already resolved are unchanged.
• Tie-breaking: IsMoreSpecific returns anyDifferent, so it is a strict comparison. Replacing the candidate only when it is strictly more specific keeps the first declaration on a true tie, which matches the generator. This replaces the old list-and-pairwise pass and is O(n) with no per-call list allocation.
• AOT/trimming: the DynamicallyAccessedMembers and UnconditionalSuppressMessage annotations carried over, and no new reflection APIs were added.
• Tests: the parity table now covers omitted tokens in both declaration orders, the inherited override default, and nullable numeric/decimal ties. The Issue7108Tests count of 37 matches the new rows.

Non-blocking suggestions

1. The lifted nullable/decimal logic in IsMoreSpecific is dense. Pulling the hasNumericConversion computation into a named helper, for example IsImplicitNumericConversion(Type from, Type to), would make it easier to compare with the generator's IsMoreSpecificDataSourceOverload. A shared test table already guards drift between the two, so this is optional.
2. The Type.GetTypeCode(...) is A or B ... chain depends on pattern precedence and spans several lines. Moving it into a static HashSet<TypeCode> or a small switch helper would be clearer.

Engine integration tests and native AOT are left to CI, as noted in the PR description.

[thomhurst]

The earlier wait on run 41547620666 no longer applies: that old-head run was cancelled and superseded by successful current-head Claude run 41549694535. Codex also reports a completed clean review of ddc3f41.

Reviewed every review surface. Both correctness findings are fixed in 42492b8 and ddc3f41 and their threads are resolved. Claude’s current-head helper-extraction and pattern-readability suggestions are optional; retaining the tested inline implementation avoids a further review cycle for stylistic changes. CodeRabbit’s older docstring-coverage warning is advisory rather than a repository requirement; the resolver’s behavior and tie handling are documented in the updated comments.

Every current CI check is successful, skipped, or neutral, including Engine integration and Native AOT on Linux, Windows, and macOS. This supersedes the local AOT limitation in the PR description. Proceeding through the repository merge gate with the reviewed head unchanged.