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(mocks): match null literals in generated predicate overloads - #7415
Null and default literals passed to generated mock setup or verification members select predicate overloads. Those overloads currently warn with CS8625 and create an unusable predicate matcher.
Accept nullable predicate delegates and turn a null delegate into Arg.Is<T>(default(T)!); non-null delegates retain predicate matching. This also handles multi-argument calls combining null literals, exact values, and predicates without relying on compiler-specific overload priority.
Untyped default also matches zero for int parameters, including generic value-type instantiations. Null delegates consistently match the parameter type’s default value.
Regression tests treat CS8625 as an error and cover setup and verification, nullable reference and value parameters, void methods, default literals, and mixed predicate calls. Updated 39 generator snapshots; changes are limited to nullable predicate signatures and default-value matcher conversion.
Generated mock setup and matching extensions now accept null predicate delegates, matching only null arguments when passed null or an untyped default. Non-null predicates continue to match arguments as before.
Clarified that relevant nullability warnings are treated as errors.
Tests
Added coverage for null and default literals, nullable and reference arguments, and combinations of null matchers with predicates and exact values.
Generated mock setup and member-extension overloads now accept nullable predicate delegates. A null delegate becomes an exact-null Arg matcher, while non-null delegates retain the existing conversion. Regression tests cover null and default literals, and the test project enables relevant nullable warnings as errors.
Snapshots cover independent null matching for multiple predicate parameters and overloads that combine predicates with Arg<T> parameters.
Regression tests and warning settings tests/TUnit.Mocks.Tests/NullLiteralMatcherTests.cs, tests/TUnit.Mocks.Tests/Issue5425Tests.cs, tests/TUnit.Mocks.Tests/TUnit.Mocks.Tests.csproj
Tests cover null/default literals and combinations with predicates and exact values. The project treats selected nullability warnings as errors; file-level warning directives were removed from Issue5425Tests.cs.
Issue 7432 — The change removes file-level warning-error pragmas from Issue5425Tests.cs and relies on the test project’s WarningsAsErrors setting.
Suggested reviewers:quattordici
Merge Risk: Minimal · previous review c663d
Null and default literals for nullable parameters now match null instead of binding to a predicate overload and raising CS8625. No concrete regression introduced by this change remains. A pre-existing limitation for methods with more than four eligible parameters is unchanged and can be handled separately.
Pre-merge checks: 4 · 1
Failed checks: 1 warning
Check name
Status
Explanation
Resolution
Docstring Coverage
Docstring coverage is 0.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. Documentat…
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: generated predicate overloads now match null literals.
Linked Issues check
Issue [#7306] requires null and default literals for nullable parameters to match null without CS8625. MockMembersBuilder.cs now emits nullable predicate delegates and converts a null delegate to `A…
Out of Scope Changes check
The reviewed changes stay within the null-literal matcher objective in [#7306]. Generator logic, generated snapshots, warning enforcement, regression tests, and the Issue5425Tests.cs warning-directi…
Full details: Linked Issues check
Explanation
Issue [#7306] requires null and default literals for nullable parameters to match null without CS8625. MockMembersBuilder.cs now emits nullable predicate delegates and converts a null delegate to Arg.IsNull<T>(); non-null delegates retain predicate matching. The updated snapshots cover generated setup and call/verification surfaces, including mixed predicate and exact-value overloads. NullLiteralMatcherTests.cs covers nullable reference and value parameters, default literals, void members, and mixed calls. TUnit.Mocks.Tests.csproj treats CS8625 as an error.
Full details: Out of Scope Changes check
Explanation
The reviewed changes stay within the null-literal matcher objective in [#7306]. Generator logic, generated snapshots, warning enforcement, regression tests, and the Issue5425Tests.cs warning-directive cleanup support implementation or validation of the nullability behavior. No unrelated product behavior appears in the change summary.
Full details: Docstring Coverage
Explanation
Docstring coverage is 0.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 1 functions across 3 files. (39 skipped: 39 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 null slips in, the matcher knows, To find the place where nullness goes. Predicates still guide the rest, While tests put literal claims to test. A rabbit hops through snapshots bright, Then nibbles code and sleeps at night.
CodeRabbit Verify
Run CodeRabbit Verify to test the changes in this pull request.
The change is small and well scoped. Making the generated predicate parameters Func<T,bool>? removes the CS8625 warning on null and default literals. Converting a null delegate to Arg.IsNull<T>() gives the intended semantics. The generated code starts with #nullable enable, so the ? annotation is valid in consumers that have nullable disabled. The regression tests cover setup and verification, void methods, value and reference types, and mixed arguments, and the snapshots show only the intended signature and conversion changes.
Non-blocking suggestions:
The x is null ? Arg.IsNull<T>() : (Arg<T>)x expression is repeated in every generated overload. A single helper such as Arg.FromPredicate<T>(Func<T,bool>?) would shorten the generated code and the 38 snapshots. It would also keep the null-handling rule in one place rather than in generator output. Because the helper would be public API, you would need to update the PublicAPI snapshots.
For value-type T (for example int), IsNull<int>() can never match. Check that this is the behaviour you want for default passed to an int predicate overload. Normally the exact-value overload would win overload resolution for default. A test that pins down which overload default selects for int would document it.
Fixed the warning-enforcement finding in cc7ae89. Validation: all 1,457 mock tests passed on net10.0 with SDK 10.0.403. A temporary compilation probe confirmed CS8625 fails the build; the probe was removed.
Disposition of Claude’s non-blocking suggestions: retain the generated conversion to avoid expanding the shipped public API for this fix. The regression scope is nullable reference and nullable value arguments; non-nullable int default-overload behavior is outside this change and does not require changing the null predicate conversion.
The approach is sound. Making the generated predicate parameter Func<T, bool>? and mapping a null delegate to Arg.IsNull<T>() fixes the CS8625 warning at its source. It also avoids depending on overload-resolution priority between the Arg<T> and Func overloads. The change is small (one builder file), and the regression tests treat CS8625 as an error. They cover setup and verification, void methods, default literals, nullable value types, and mixed null/predicate/exact-value calls. I only read the diff and the generator code. I did not run the tests or the snapshot suite.
Observations (none blocking):
Non-nullable T. The predicate overload is emitted for every parameter type. The generated code now calls Arg.IsNull<T>() even for a non-nullable int or other value type. NullMatcher<T> is documented for reference types and nullable value types, so for non-nullable T it can never match. Passing a null delegate there would previously have thrown or warned. It now silently builds a matcher that never matches. That is an edge case and probably acceptable, but a one-line test or comment would pin the intended behaviour.
Conditional on every call. The generated x is null ? Arg.IsNull<T>() : (Arg<T>)x is emitted per predicate parameter on every setup and verification call. This is a small cost. A single Arg<T>.FromPredicate(Func<T,bool>?) helper in TUnit.Mocks would shrink the generated code, reduce snapshot churn (38 files here), and keep the null-handling rule in one place. It would also be easy to unit test. I suggest that as a follow-up refactor.
Mixed-argument tests.Pair(null, null) and Pair(default, default) resolve to the Arg<T> overload, not the predicate one. The setup is still correct, since both paths yield a null matcher. The tests only exercise the combination correctly because the assertions check runtime behaviour, which is good.
Overall this looks good to merge once CI is green.
Marked ready at unchanged head cc7ae89 after confirming positive current-head Claude and Codex verdicts and the resolved warning-enforcement thread. Claude’s non-blocking observations retain the existing disposition: keep this fix scoped to nullable argument matching without expanding the public API.
Waiting for the ready-for-review Claude run 41551908756 and CI. Read the new Claude verdict before arming auto-merge with scripts/Enable-AutoMerge.ps1.
The change is small and well targeted. The signature now takes Func<T,bool>?. A __fa_* local converts a null delegate to Arg.IsNull<T>() and otherwise uses the existing implicit conversion. That removes the CS8625 warning and the unusable predicate matcher. The generator change is one source file plus snapshot updates, and the PR description says the snapshots changed only in the nullable signatures and the null conversion. I did not build or run anything, and I only read the one generator file, not the snapshots or the new tests.
Points to consider
default for non-nullable value-type parameters. For an int id parameter, Find(default) most naturally means 0, an exact value. If overload resolution now picks the Func<int,bool>? overload, the call becomes Arg.IsNull<int>(), which can never match a non-nullable int. That would be a silent behaviour change. Please add an explicit test for default on non-nullable value-type parameters. It should assert either the previous behaviour or a deliberate one. If it is ambiguous or changes meaning, an analyzer diagnostic would be better than a never-matching matcher.
Resolution rests on subtle overload rules. The PR says it avoids relying on compiler-specific overload priority, but null still has to choose between Arg<T> and Func<T,bool>?. Since Arg<T> converts implicitly from both a value and a delegate, a test with mixed null, exact and predicate arguments across several parameters would show the choice is stable. The PR says it adds one, so this is only a request to check it covers the multi-parameter case.
Design. Emitting a per-call __fa_* local for every predicate parameter adds generated code, and it adds a null check on each call. This is minor. A single helper such as Arg.FromPredicate<T>(Func<T,bool>?) would keep the generated code shorter and the null handling in one place. It would also make the AOT and allocation behaviour easier to reason about than repeating the conditional in every generated member.
Overall this is a reasonable fix, and I'd approve it once the default value-type case has a test.
Addressed the ready-for-review Claude request in c663d1a. Added a runtime regression proving untyped default selects the nullable predicate overload for an int parameter and deliberately never matches; typed default(int) selects exact zero. Both setup and verification are covered. The existing two-parameter regression covers null combined with predicates and exact values. Retaining the generated conversion avoids expanding shipped public API for this fix.
Validation: all 3 NullLiteralMatcherTests passed on net10.0 (SDK 10.0.403). No generator output changed. The previous waiting run 41551908756 completed successfully; its new review request prompted this test. Keeping the PR in draft for fresh current-head Claude and Codex reviews.
I read the generator change in MockMembersBuilder.cs (around lines 1254 and 1303-1321) and the test and csproj changes. I did not run the tests. The 38 snapshot diffs I only skimmed, so I haven't confirmed they are limited to the nullable predicate signatures and the null-matcher conversion.
What looks good
The fix is at the right level. The predicate parameter becomes Func<T, bool>?, and a null delegate is converted to Arg.IsNull<T>(). That removes the CS8625 warning and the unusable matcher without depending on compiler overload priority.
Non-null predicates still go through the existing implicit-conversion path, so existing behaviour is unchanged.
The tests treat CS8625 as an error and cover setup and verification, void methods, nullable reference and value parameters, default literals and mixed null/value/predicate calls.
Setup and verification both use the shared builder path, so there is no duplicated implementation.
Minor suggestions (non-blocking)
The is null ? ... : ... conversion is emitted as a statement per predicate parameter in every generated member. A small helper such as Arg.FromPredicate<T>(Func<T,bool>?) would shrink the generated code and leave the null handling in one place. This would also shrink the snapshots.
__fa_{rawName} is a fixed prefix. It could collide with a user parameter that happens to have the same name. That is very unlikely, but it is worth confirming the existing naming scheme already accounts for it.
Confirm that an explicitly typed Arg<T> argument still binds unambiguously now that the Func overload is nullable.
I don't see any blocking issues. This looks good to merge once CI is green.
The old waiting run 41551908756 completed successfully. Current head c663d1a has positive Claude and Codex verdicts and no unresolved threads. The added regression dispositions the non-nullable default request.
Latest non-blocking suggestions: retain the generated conversion rather than add shipped public API. Nullable annotations do not change overload identity or conversions; explicitly typed Arg continues to select its identity-conversion overload. The _fa local naming predates this patch; tracked its collision gap separately in #7433.
Promoting this unchanged head to ready. The ready-for-review Claude workflow must finish and its verdict must be read before auto-merge can be armed.
Promoted unchanged approved head c663d1a to ready. Waiting for ready-for-review Claude run 41557232643, currently queued, plus full CI. Read the new Claude verdict and all review surfaces before running scripts/Enable-AutoMerge.ps1. The earlier blocker run 41551908756 is complete; no external blocker or on-hold label is needed.
1211-1211: Major · Stability & Availability · Quick win
Unverified finding
Verification ran but could not confirm this finding. It is shown for review, not as a verified issue.
Handle default literals when predicate overloads are skipped.
For a method with five eligible parameters, this guard emits no predicate overload. A call such as mock.F(default, 1, 2, 3, 4) then creates a default Arg<string?> with a null matcher. A matching invocation throws instead of matching null. Handle default Arg<T> values on the remaining setup path. (raw.githubusercontent.com)
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: 410afe3f-9d2c-4971-8fa9-ce024a82746d
Commits
Reviewing files that changed from the base of the PR and between ca4437c and c663d1a.
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 small and targeted. The generated predicate overloads now take Func<T, bool>?, and a null delegate becomes Arg.IsNull<T>(). That fixes the CS8625 warning for null and default literals, and it also works when a call mixes null literals, exact values and predicates. The generated files already emit #nullable enable, so the ? annotation shouldn't cause CS8632 in consumer projects. I did not build or run the tests.
Notes, none blocking:
Ambiguity:Mock.Foo(null) could still match both the Arg<T> overload and the Func<T,bool>? overload. The PR says the new tests cover this, and the 38 snapshot changes are limited to the signature and the null conversion. Please confirm the NullLiteralMatcherTests cases include a call with a single null argument on an overloaded method, because overload resolution there depends on the compiler.
Duplicated expression: the x is null ? Arg.IsNull<T>() : (Arg<T>)x text is repeated in every generated overload. A single helper such as Arg.FromPredicate<T>(Func<T,bool>?) would shrink the generated code and the snapshots, and it would give one place to change the null behaviour later. It is also AOT-neutral. This is optional and could be a follow-up.
Behaviour change: callers passing a null Func variable used to get an unusable matcher. They now get a null-matcher. That is the more sensible result, but a changelog line would be worth adding.
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
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Null and default literals passed to generated mock setup or verification members select predicate overloads. Those overloads currently warn with CS8625 and create an unusable predicate matcher.
Accept nullable predicate delegates and turn a null delegate into
Arg.Is<T>(default(T)!); non-null delegates retain predicate matching. This also handles multi-argument calls combining null literals, exact values, and predicates without relying on compiler-specific overload priority.Untyped
defaultalso matches zero forintparameters, including generic value-type instantiations. Null delegates consistently match the parameter type’s default value.Regression tests treat CS8625 as an error and cover setup and verification, nullable reference and value parameters, void methods, default literals, and mixed predicate calls. Updated 39 generator snapshots; changes are limited to nullable predicate signatures and default-value matcher conversion.
Validation: SDK 10.0.403, net10.0 on macOS arm64:
Closes #7306
Summary by CodeRabbit
nullor an untypeddefault. Non-null predicates continue to match arguments as before.