Fix Arg.IsNull<T> and Arg.IsNotNull<T> to support nullable value types

[thomhurst]

Summary

• Removed the where T : class? constraint from Arg.IsNull<T>() and Arg.IsNotNull<T>()
• This fixes CS8620/CS8634 warnings when using nullable reference types (e.g. string?)
• This also adds support for nullable value types (e.g. int?) — previously a gap where users couldn't use these matchers for methods with int?, bool?, etc. parameters
• The underlying matchers (NullMatcher<T>, NotNullMatcher<T>) already handled nullable types correctly via is null / is not null

Fixes #5360

Test plan

• Arg.IsNull<string?>() compiles without warnings
• Arg.IsNotNull<string?>() compiles without warnings
• Arg.IsNull<string>() existing usage still works
• Arg.IsNull<int?>() now compiles and matches null correctly
• Arg.IsNotNull<int?>() now compiles and matches non-null correctly
• TUnit.Mocks builds cleanly across all target frameworks

[claude]

Code Review

Summary

This is a clean, minimal, and correct fix. The change from where T : class to where T : class? is exactly the right approach to eliminate CS8620/CS8634 warnings when using nullable reference types with Arg.IsNull<T>() and Arg.IsNotNull<T>().

Analysis

The fix is correct. In C# nullable-aware contexts:

• where T : class — constrains to non-nullable reference types only, causing the compiler to warn when string? is passed as the type argument
• where T : class? — constrains to reference types (both nullable and non-nullable), which is what these methods actually need

The underlying matchers (NullMatcher<T> and NotNullMatcher<T>) already use T? in their Matches signatures, confirming they were designed with nullable intent. The constraint was simply lagging behind.

One Thing to Consider (not a blocker)

Currently neither IsNull nor IsNotNull supports Nullable<T> value types (e.g. int?). With where T : class?, Arg.IsNull<int?>() still won't compile because int? (Nullable<int>) is a struct. This is a pre-existing gap and outside the scope of this PR, but worth noting as a potential follow-up issue — users with methods like void Foo(int? x) can't use these matchers today.

If support for nullable value types is desirable in the future, a separate overload or unconstrained generic with runtime null check would be the path forward.

Verdict

The change is backward-compatible, fixes a real developer-experience issue with no side effects, and the PR description is clear and accurate. Approved.

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 23 complexity

Metric	Results
Complexity	23

View in Codacy

_{TIP This summary will be updated as you push new changes. Give us feedback}

[claude]

Updated Code Review (Follow-up)

This is an update to my earlier review. The PR has evolved significantly from what I initially analyzed — the constraint change is more substantial than where T : class → where T : class?.

What Changed

The final implementation completely removes the generic constraint from IsNull<T> and IsNotNull<T>:

// Before
public static Arg<T> IsNull<T>() where T : class => new(new NullMatcher<T>());
public static Arg<T> IsNotNull<T>() where T : class => new(new NotNullMatcher<T>());

// After (this PR)
public static Arg<T> IsNull<T>() => new(new NullMatcher<T>());
public static Arg<T> IsNotNull<T>() => new(new NotNullMatcher<T>());

This is a broader change than described in the PR summary (which only mentions relaxing to where T : class?). Removing the constraint entirely also allows non-nullable value types like int, bool, Guid, etc.

Correctness Analysis

For Nullable<T> (e.g. int?): Works correctly. The NullMatcher<T>.Matches(T? value) => value is null and the object? overload both handle this correctly. The new tests confirm this path. ✓

For non-nullable value types (e.g. int, bool): Arg.IsNull<int>() will now compile but will always return false — a value type can never be null. Similarly Arg.IsNotNull<int>() will always return true. These aren't runtime errors, but they are semantically vacuous/misleading usages.

This isn't necessarily a blocking concern (users shouldn't be calling IsNull<int>()), but it's worth deciding whether to:

1. Accept this as a "you get what you ask for" situation (the current implicit choice), or
2. Add a where T : class? constraint instead of no constraint, combined with a separate overload for nullable value types

Option 2 — More Precise API (optional follow-up)

If you want to prevent the vacuous IsNull<int>() usage from compiling, a two-overload approach would be more precise:

// Overload for reference types (including nullable reference types)
public static Arg<T> IsNull<T>() where T : class? => new(new NullMatcher<T>());

// Overload for nullable value types  
public static Arg<T> IsNull<T>() where T : struct => new(new NullMatcher<T?>());  // returns Arg<T?> implicitly

However, this adds complexity and is arguably over-engineering for a mocking DSL. The current unconstrained approach is pragmatic and correct for all realistic use cases.

Tests

The two new tests (Arg_IsNull_Matches_Null_Nullable_Value_Type and Arg_IsNotNull_Matches_NonNull_Nullable_Value_Type) correctly exercise the new int? path and cover both the null-matches and non-null-does-not-match cases. The INullableValueConsumer interface is correctly placed in BasicMockTests.cs alongside the other shared test interfaces.

One minor note: consumer.Process(0) is a good explicit inclusion in the IsNotNull test — it verifies that 0 (falsy but not null) is matched, which is an easy edge case to miss.

Verdict

The change is correct and backward-compatible. The PR description understates what was done (the constraint is fully removed, not just relaxed to class?), but the implementation and tests are sound. The "always false/true for non-nullable value types" behavior is a theoretical concern but not a real-world problem for a mocking library.

My previous comment flagging nullable value type support as a future gap has been addressed by this PR. Approved.

[claude]

Code Review

This is a clean, well-thought-out fix. The approach of removing the constraint + adding a Roslyn analyzer to catch the now-possible-but-meaningless case is the right architectural pattern — better than keeping a compile-time constraint that over-restricts valid usage.

What's Good

• Constraint removal is correct: The underlying NullMatcher<T> and NotNullMatcher<T> already use is null / is not null pattern matching, which handles Nullable<T> correctly at runtime. No changes needed there.
• Analyzer correctness: The OriginalDefinition.SpecialType == SpecialType.System_Nullable_T check correctly distinguishes Nullable<T> (a value type that IS nullable) from plain structs. This is the right way to do this in Roslyn.
• Namespace hierarchy match in IsArgNullMethod is robust — it won't accidentally trigger on a user-defined Arg class that happens to have an IsNull method.
• Test coverage is solid across the expected happy/sad paths.

---

Issues to Address

1. TM005 is out of numerical order in Rules.cs

TM005 is declared before TM004, which breaks the numeric ordering convention the file otherwise follows. This is a minor thing but makes it harder to navigate as more rules accumulate.

2. Missing analyzer test: nullable reference type (string?)

The analyzer tests cover string (non-nullable reference) but not string? (nullable reference type). Reference types with the nullable annotation (?) are still reference types — IsValueType is false for them — so the analyzer correctly skips them. But since string? was specifically called out in the PR description as a scenario that previously caused CS8620/CS8634 warnings, it's worth adding an explicit "does not report" test to lock in this behavior:

3. INullableValueConsumer is defined in BasicMockTests.cs but used in ArgumentMatcherTests.cs

The file-level comment in BasicMockTests.cs says "Test interfaces used across mock tests", which is a reasonable home for shared interfaces. But this creates an implicit coupling — someone reading ArgumentMatcherTests.cs needs to hunt for where INullableValueConsumer comes from. Consider either:

• Moving it to a dedicated TestInterfaces.cs file (mirrors the existing comment intent), or
• Keeping it in BasicMockTests.cs is acceptable since the file already serves as a shared interface container.

Not a blocker, just worth being intentional about.

---

Nice-to-Have (Not Blocking)

A code fix provider paired with TM005 that auto-suggests appending ? (e.g., changing Arg.IsNull<int>() → Arg.IsNull<int?>()) would significantly improve DX. Not required for this PR, but worth a follow-up issue.

---

Overall this is a solid fix. The two things worth addressing before merge are the rule ordering in Rules.cs (trivial) and the nullable reference type analyzer test gap (important for regression protection).

[claude]

Code Review

This is a clean, well-thought-out fix. The approach of removing the constraint + adding a Roslyn analyzer to catch the now-possible-but-meaningless case is the right architectural pattern — better than keeping a compile-time constraint that over-restricts valid usage.

What's Good

• Constraint removal is correct: The underlying NullMatcher<T> and NotNullMatcher<T> already use is null / is not null pattern matching, which handles Nullable<T> correctly at runtime. No changes needed there.
• Analyzer correctness: The OriginalDefinition.SpecialType == SpecialType.System_Nullable_T check correctly distinguishes Nullable<T> (a value type that IS nullable) from plain structs. This is the right Roslyn idiom.
• Namespace hierarchy match in IsArgNullMethod is robust — it won't accidentally trigger on a user-defined Arg class that happens to have an IsNull method.
• Test coverage is solid across the expected happy/sad paths.

---

Issues to Address

1. TM005 is out of numerical order in Rules.cs

TM005 is declared before TM004, breaking the numeric ordering convention the file otherwise follows. As more rules accumulate this makes navigation harder.

Current order: TM001, TM002, TM003, TM005, TM004
Expected order: TM001, TM002, TM003, TM004, TM005

2. Missing analyzer test: nullable reference type (string?)

The analyzer tests cover string (non-nullable reference type) but not string? (nullable reference type). Reference types with the ? annotation still have IsValueType == false, so the analyzer correctly skips them — but since string? was specifically called out in the PR description as a previously-broken scenario (CS8620/CS8634), an explicit "does not report" test would lock in this behavior:

[Test]
public async Task IsNull_With_Nullable_Reference_Type_Does_Not_Report()
{
    await Verifier.VerifyAnalyzerAsync(
        ArgStub + """

        #nullable enable
        public class TestClass
        {
            public void Test()
            {
                TUnit.Mocks.Arguments.Arg.IsNull<string?>();
            }
        }
        """
    );
}

3. INullableValueConsumer is defined in BasicMockTests.cs but used in ArgumentMatcherTests.cs

The existing comment at the top of BasicMockTests.cs describes it as a home for "Test interfaces used across mock tests", which is reasonable. But this creates an implicit dependency — readers of ArgumentMatcherTests.cs need to hunt for where INullableValueConsumer is declared. A dedicated TestInterfaces.cs would make this explicit. Not a blocker — just worth being intentional about.

---

Nice-to-Have (Not Blocking)

A code fix provider paired with TM005 that auto-suggests appending ? (e.g., Arg.IsNull<int>() → Arg.IsNull<int?>()) would improve DX. Worth a follow-up issue.

---

Overall this is a solid fix. The two things most worth addressing before merge are the rule ordering in Rules.cs (trivial) and the missing nullable reference type analyzer test (important for regression protection given the PR's stated motivation).

[claude]

Code Review

This is a well-executed fix. The approach is correct: remove the overly-restrictive where T : class constraint (the runtime matchers already worked via is null/is not null), then compensate with a Roslyn analyzer to catch the genuinely broken case.

What's Good

Clean runtime fix — NullMatcher<T> and NotNullMatcher<T> already used value is null/value is not null pattern matching which handles both reference and nullable value types correctly. The constraint was purely a compile-time restriction that didn't reflect reality.

Analyzer as compensation — Adding TM005 to warn on Arg.IsNull<int>() (non-nullable struct, can never be null) is exactly the right trade-off. Users get the runtime flexibility they need, but still get guidance when they've made a logical error.

Namespace chain check in IsArgNullMethod — The full namespace walk is the correct Roslyn approach for identifying a specific well-known method without taking a package reference.

Test coverage — Both the integration tests and analyzer tests cover the full matrix: non-nullable struct (warns), nullable value type (no warn), reference type (no warn), nullable reference type (no warn).

Minor Issues

INullableValueConsumer placed in the wrong file — The interface is defined in BasicMockTests.cs but is only used in ArgumentMatcherTests.cs. This creates a subtle coupling between unrelated test files. Consider moving it to ArgumentMatcherTests.cs (where it's consumed) or a shared TestInterfaces.cs file.

TM005 missing description parameter — The other rules (TM001–TM004) don't have it either, but it's worth noting that a description string gives IDE tooling richer hover text and is surfaced in dotnet_diagnostic suppression docs. Not a blocker, just a gap consistent with the rest of the file.

Behavior note (not a bug): When T = int (non-nullable struct, triggering TM005), NullMatcher<int>.Matches(int? value) will be called and will always return false. The diagnostic makes this a warning, not an error. If you want to prevent the "silent never-matches" case more strictly, TM005 could be DiagnosticSeverity.Error. But Warning is defensible if you want to avoid breaking existing code during upgrade.

Summary

Solid implementation. The single actionable item is relocating INullableValueConsumer to ArgumentMatcherTests.cs. Everything else is well-reasoned.

[claude]

Updated Code Review

This is a follow-up to my earlier reviews. The PR now fully addresses the concern I raised — let me document the final state.

Previous Concern: Addressed

My prior review flagged that removing where T : class would allow semantically vacuous usages like Arg.IsNull<int>() (which can never match since int is never null). The new ArgIsNullNonNullableAnalyzer (TM005) directly closes that gap with a compile-time error.

What's New In This Version

TM005 Analyzer (ArgIsNullNonNullableAnalyzer.cs) — correct and well-structured:

• Fires on SyntaxKind.InvocationExpression — efficient, avoids broad symbol walks
• IsArgNullMethod uses full namespace qualification (TUnit.Mocks.Arguments.Arg) — avoids false positives from other Arg classes
• Correctly excludes Nullable<T> via OriginalDefinition.SpecialType == SpecialType.System_Nullable_T
• DiagnosticSeverity.Error is appropriate — calling Arg.IsNull<int>() is always wrong

Analyzer Tests (ArgIsNullNonNullableAnalyzerTests.cs) — comprehensive coverage:

• Reports for non-nullable struct (int, bool)
• Does not report for int?, string, string?
• The ArgStub correctly reflects the unconstrained signature — good test hygiene

TM001–TM004 descriptions added — small but welcome improvement for IDE tooltips and docs.

One Minor Observation

In the ArgStub used by analyzer tests:

public struct Arg<T> { }

The real Arg<T> is presumably a struct or class — this is fine for analyzer testing since the analyzer only inspects the method symbol, not the return type. Just worth noting that the stub doesn't need to exactly match the real type; it only needs to satisfy the namespace/method/generic-arity check in IsArgNullMethod.

Verdict

The implementation is correct, complete, and well-tested. The three-part solution (constraint removal + runtime matchers + TM005 analyzer) is the right architecture:

1. Constraint removal — enables int? usage that was previously blocked
2. Runtime matchers — already handled null checks correctly via is null / is not null
3. TM005 analyzer — prevents the nonsensical Arg.IsNull<int>() usage from silently compiling

All concerns from previous reviews have been addressed. Approved.