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): Is<T>(predicate) passes T, not T?, for non-nullable T - #7405
Is<T>(Func<T?, bool>) forced a null guard (CS8602) in predicates that dereference a non-nullable reference argument. It now accepts Func<T, bool>, so Is<User>(u => u.Name == "Alice") compiles without CS8602. Use Is<User?>(...) for nullable arguments.
Nullable annotations do not distinguish the two forms at runtime: both pass null to the predicate. All predicate exceptions continue to propagate, as on main. An explicit null guard makes null a non-match. This preserves existing runtime behavior and avoids swallowing unrelated predicate bugs.
The signature change is annotation-only and binary compatible for unconstrained T. The implicit predicate conversion drops its unnecessary null suppression. The matcher guide and API remarks explain null handling. A file-specific .editorconfig rule enforces CS8602 in the regression tests.
…7304)
A null argument no longer reaches a dereferencing predicate as an NRE that
fails the mocked call; it simply does not match. Is<T?>(...) still lets a
predicate match null.
No actionable comments were generated in the recent review.
Walkthrough
Arg.Is<T> now accepts a predicate with parameter type T. PredicateMatcher<T> treats a NullReferenceException from evaluating a null argument as a non-match. Documentation and regression tests describe and verify nullable and non-nullable predicate behavior.
Changes: Predicate nullability
Layer / File(s)
Summary
Predicate contract and matching src/TUnit.Mocks/Arguments/Arg.cs, src/TUnit.Mocks/Arguments/ArgOfT.cs, src/TUnit.Mocks/Matchers/PredicateMatcher.cs, docs/docs/writing-tests/mocking/argument-matchers.md
Arg.Is<T> and PredicateMatcher<T> use Func<T, bool>. The matcher returns false when predicate evaluation for a null argument throws NullReferenceException. The documentation describes null handling and exception behavior.
The predicate nullability change has no identified merge-blocking risk.
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: non-nullable predicates now receive T instead of T?.
Linked Issues check
Issue [#7304] requires Arg.Is<T>(Func<T, bool>) to provide a non-nullable T for non-nullable reference types, while retaining Is<T?> for nullable matching and making null arguments non-matches f…
Out of Scope Changes check
The changed files are limited to the matcher API and implementation, targeted nullability documentation, a scoped CS8602 test configuration, and regression tests for issue [#7304]. Each change support…
Full details: Linked Issues check
Explanation
Issue [#7304] requires Arg.Is<T>(Func<T, bool>) to provide a non-nullable T for non-nullable reference types, while retaining Is<T?> for nullable matching and making null arguments non-matches for the non-nullable form. Arg.cs uses Func<T, bool>. PredicateMatcher<T> invokes the predicate for null and returns false when that path throws NullReferenceException. The four regression tests cover non-null dereferencing, null non-match, nullable null matching, and propagation for non-null exceptions.
Full details: Out of Scope Changes check
Explanation
The changed files are limited to the matcher API and implementation, targeted nullability documentation, a scoped CS8602 test configuration, and regression tests for issue [#7304]. Each change supports the requested predicate annotation or its documented runtime behavior. No unrelated product behavior is demonstrated.
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 2 functions across 4 files. (2 skipped: 2 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 rabbit checks the matcher’s way, “Nulls won’t match,” it hops to say. A nullable check can welcome null, While non-null names stay clear and full. The tests keep watch through every run, Then rabbit bounds beneath the sun.
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: 6590e681-7291-403a-b6af-3853141a0a96
Commits
Reviewing files that changed from the base of the PR and between 0db8edc and 68112ca.
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 0 remain after this review.
Looks good. The change is small, matches Moq's It.Is<T>, and fixes #7304. The new tests cover the main behaviours.
The annotation-only signature change is binary compatible, and Matches(object?) still routes null through Matches(T?), so the null handling applies on every path.
The tests cover the dereferencing predicate, a null argument, the Is<User?> form, and an exception for a non-null argument still propagating.
Non-blocking points:
Catching NullReferenceException can hide bugs. For a null argument it also swallows an NRE thrown from inside the predicate's own logic, for example a nested u.Address.City where Address is null. That is unlikely to matter because it only affects the null-argument path. Still, a null argument that silently "doesn't match" is harder to debug than a clear failure. The docs already say a null argument doesn't match, so this is acceptable.
Cost. The try/catch only runs for null arguments, so the hot path is unaffected. A throw/catch per null call is slow, but it's rare.
Possible alternative. Add a separate overload or marker for nullable predicates, so the matcher can skip null for Is<T> without exceptions. That is not possible here because T and T? are the same type at runtime, so the current approach is reasonable.
Documentation. The nullable-parameter note about Arg<User> not converting to Arg<User?> (CS8620) is useful. Consider adding it to the docs page too, since users will hit it.
The new head enforces CS8602 as an error for PredicateNullabilityTests.cs. Reverting the signature failed with three errors; the fixed signature passed all four targeted tests.
Claude’s non-blocking points are accepted: the null-only exception behavior and cost are documented and intentional. The nullable argument conversion note remains in the PR description; the existing matcher documentation already explains selecting T versus T?.
The change does what it says. Is<T>(Func<T, bool>) removes the CS8602 noise and stays binary compatible, since T? and T are the same type in IL for an unconstrained T. The tests cover the main cases and the docs note is useful. I found no blocking issues, but there are a few design points.
1. Catching NullReferenceException in PredicateMatcher.Matches
This is the part I'd reconsider.
It can hide real bugs. The catch applies to any NRE thrown while the argument is null, not only a dereference of the argument. A predicate like u => _lookup[u.Id].Name == "x" that fails on a null inside _lookup is silently treated as "no match". The user then sees a confusing "no setup matched" failure instead of a stack trace.
It is slow. A thrown exception costs microseconds, and a null argument can be hit on every call of a hot mock.
Behaviour depends on how the predicate is written. Is<User>(u => u.Name == ...) doesn't match null, but Is<User>(u => u is null) does, because the same runtime type serves both. The docs say "a null argument does not match it", which is only true for predicates that happen to throw.
A more predictable alternative is to decide from the declared nullability. Annotations aren't available at runtime for a generic T, but the source generator knows the parameter's annotation. It could emit a null short-circuit for non-nullable parameters, so null never matches Is<User> and always reaches Is<User?>, with no exceptions. If that is too big for this PR, at least narrow the catch, document the behaviour as best-effort, and soften the docs to "may not match".
2. .editorconfig entry for one test file
Escalating CS8602 to an error for a single file in the repo-root .editorconfig is clever but easy to miss. An .editorconfig under tests/TUnit.Mocks.Tests would keep the intent local. This is minor.
3. Minor
Null_Argument_Does_Not_Match_Dereferencing_Predicate asserts IsNotEqualTo("alice") on the default return. Asserting the actual default value would stop it passing for the wrong reason.
Matches(object?) delegates to the typed path, so behaviour is consistent. A short test or comment would confirm that.
Overall this is a good ergonomic improvement. I'd like the NRE-swallowing reconsidered, or at least its risk documented, before merge.
Addressed Claude's latest pre-merge request in 68112ca. The matcher guide, API remarks, and implementation comment now state that both forms pass null to the predicate, any NullReferenceException on that path is suppressed (including unrelated predicate failures), and the exception path has a cost. The guide recommends an explicit null guard and includes the nullable-parameter note. A generator-based null short-circuit would change matching semantics and does not distinguish the caller's generic annotations at this matcher boundary; this change takes the review's documentation option.
The remaining editorconfig placement and test-strengthening suggestions are non-blocking and left as follow-ups within this PR's review: the existing scoped rule enforces the regression, and the null-call test also verifies the predicate setup was never matched. The object overload visibly delegates to the same typed path.
Validation: git diff --check passed. This commit changes documentation and comments only; no runtime tests were rerun. The prior head's test validation remains in the PR description. Keeping the PR in draft for fresh reviews of this head.
The change does what it says. Is<T>(Func<T,bool>) removes the CS8602 forced null guards. The runtime behaviour is documented and tested, and the tests assert the compile-time behaviour through the .editorconfig severity override.
Points to consider:
Swallowing NullReferenceException is the weakest part of the design. Treating an NRE as a non-match when the argument is null hides real bugs in the predicate. For example, Is<User>(u => u.Address.City == "x") where Address is null will quietly never match. The cost and the hiding are both documented, but a cleaner option exists. When the argument is null and the matcher was created through the non-nullable overload, it could simply return false without calling the predicate. This needs the overload to record intent. That is not possible with a single runtime type, so it could be an explicit Is<T>/IsNullable<T> split, or leaving the current behaviour as is. As written, Is<User>(u => u is null) still matches null, which is inconsistent with swallowing the NRE. The behaviour is documented, so I would accept it, but please consider it.
Exception filter. If the catch stays, catch (NullReferenceException) when (value is null) is redundant because the code is already on the null path. A narrower alternative is to inspect ex.TargetSite or similar, but that is brittle. I would leave it as is.
Scope of the .editorconfig change. A per-file diagnostic override in the repo-wide .editorconfig works, but the file-scoped #pragma warning restore/<WarningsAsErrors> alternatives are more discoverable. This is minor.
Test gap. The tests do not cover the Matches(object?) path with a null argument or the implicit Func<T,bool> conversion with null. Both paths call the same code, so the risk is low.
No blocking issues. Binary compatibility reasoning (T? and T are the same type in IL for an unconstrained T) is sound.
Marked ready at unchanged head 68112ca after verifying positive Claude and Codex verdicts, every review surface, and resolved threads. The latest non-blocking suggestions retain the prior disposition: null-path behavior is documented and intentional, the scoped compiler regression rule is effective, and both matcher entry paths share the implementation.
Waiting for the ready-for-review Claude run 41551757718 and newly triggered CI. Auto-merge is not armed yet because the repository requires the fresh Claude run to pass and its comment to be read first.
The change does what it says: Is<T>(Func<T, bool>) now lets predicates dereference a non-nullable argument without a CS8602 guard. It is binary compatible, and the new tests cover the main cases. The .editorconfig rule that makes CS8602 an error in the test file is a good guard against the regression coming back.
Concern: swallowing NullReferenceException for null arguments
PredicateMatcher<T>.Matches catches any NRE when the argument is null and treats it as a non-match. It works, but:
It hides real bugs. An NRE from unrelated predicate logic, such as a closure over a null field, silently becomes "no match". The docs admit this.
It uses exceptions for control flow on a path that can be hot. A mock called with null many times pays the throw and catch cost each time.
It differs from Moq's It.Is<T>, which the PR cites as the model. Moq lets the exception propagate.
Alternatives:
Make null never match when the parameter is a non-nullable reference type. The source generator knows the parameter's nullability, so it could emit a nullability-aware matcher instead of catching at runtime.
Let the NRE propagate and document the Is<T?> opt-in. This is simpler and matches Moq. It is a behaviour change for null arguments, so it needs a release note.
Both are cleaner than a catch-all.
Minor
Add a test where the predicate throws a non-NRE exception for a null argument, to show that only NRE is swallowed.
AOT publish was not run. The change adds no reflection, so the risk is low.
Overall this is a good ergonomic fix. I'd reconsider the NRE catch before merge.
Addressed Claude's renewed concern from run 41551757718 in 9239682: removed the NRE catch rather than suppressing predicate bugs. This restores main's existing exception propagation; the only API change is the intended nullable annotation fix. Updated #7304's incompatible null-matching expectation under its own lock, since nullable generic annotations cannot encode runtime matching intent.
Docs and API remarks now explain propagation and explicit null guards. Added regression coverage for null dereference propagation, guarded null non-match, and an unrelated exception on a null argument. Full net10.0 mocks suite passed: 1456/1456. git diff --check passed. AOT publish was not run; no reflection changed. The unrelated ineffective warning pragmas are tracked in #7432.
Prior minor suggestions retain their disposition: the scoped editorconfig rule enforces CS8602; object matching delegates to the typed path. The explicit guard regression now asserts the actual empty-string default. Keeping this new head in draft for fresh Claude and Codex reviews. The earlier workflow wait is resolved; this unit pushed fixes.
The change is small and does what it says. Is<T>(Func<T, bool>) removes the forced CS8602 null guard from predicates over non-nullable reference types. The change only touches annotations, so there are no new allocations, no reflection and no AOT impact.
What I checked
PredicateMatcher<T>.Matches(T? value) => _predicate(value!) keeps the null-forgiving operator in one internal place. That is better than spreading suppressions across call sites.
Dropping the predicate! from the implicit operator in ArgOfT.cs is correct, because the types now line up.
Runtime behavior matches main: null still reaches the predicate and exceptions propagate. The tests pin both points: a null dereference gives NullReferenceException, and an unrelated predicate exception propagates.
Using .editorconfig to make CS8602 an error only in PredicateNullabilityTests.cs is a good way to keep the compile-time regression from slipping back in.
Suggestions (non-blocking)
Is<User>(u => u.Name == ...) against a mock parameter of User will throw NullReferenceException if a caller passes null. The signature now claims non-null while the runtime does not enforce it. The docs state this clearly, which is enough for me. A cleaner long-term design would be to skip the predicate for null when T is a non-nullable reference type, but that would change behavior and is rightly out of scope.
The source-compatibility claim is only partly tested. Callers that pass a Func<string?, bool> variable or a method group should still compile, with at most a nullability warning, and a test or build check for that would back up the "binary compatible" statement.
The 5th and 6th tests overlap. Both just show that predicate exceptions propagate.
I have no blocking concerns. The change looks good to merge once CI is green.
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
Summary
Is<T>(Func<T?, bool>)forced a null guard (CS8602) in predicates that dereference a non-nullable reference argument. It now acceptsFunc<T, bool>, soIs<User>(u => u.Name == "Alice")compiles without CS8602. UseIs<User?>(...)for nullable arguments.Nullable annotations do not distinguish the two forms at runtime: both pass null to the predicate. All predicate exceptions continue to propagate, as on main. An explicit null guard makes null a non-match. This preserves existing runtime behavior and avoids swallowing unrelated predicate bugs.
The signature change is annotation-only and binary compatible for unconstrained T. The implicit predicate conversion drops its unnecessary null suppression. The matcher guide and API remarks explain null handling. A file-specific .editorconfig rule enforces CS8602 in the regression tests.
Closes #7304
Validation