Preserve specialized chaining after null assertions

[thomhurst]

Summary

• Add specialized IsNull() / IsNotNull() methods for collection, list, read-only list, dictionary, mutable dictionary, set, and async enumerable assertion bases.
• Preserve the domain-specific fluent surface after null checks, e.g. .IsNotNull().And.ContainsKey(...) and .IsNull().Or.ItemAt(...).
• Add focused coverage for collection and async enumerable chaining after null assertions.

Verification

• dotnet build TUnit.Assertions\TUnit.Assertions.csproj --no-restore -p:NoWarn=CS1591 -clp:ErrorsOnly
• dotnet test TUnit.Assertions.Tests\TUnit.Assertions.Tests.csproj --no-restore --treenode-filter "/*/*/AsyncEnumerableAssertionTests/*" --no-progress --output Normal
• dotnet test TUnit.Assertions.Tests\TUnit.Assertions.Tests.csproj --no-restore --treenode-filter "/*/*/CollectionAssertionTests/*" --no-progress --output Normal

Notes

• I attempted the public API snapshot test, but the local TUnit.PublicAPI build is currently blocked by unrelated assembly version conflicts between 99.99.99.0 and 1.45.46.0 project outputs, so no .received.txt files were generated.

[claude]

Code Review: Preserve specialized chaining after null assertions

The goal here is clear and the implementation works correctly. The static helper is a clean extraction. A few structural concerns worth discussing:

---

1. Class proliferation — 14 classes for 2 behaviours each (major)

Every specialized base now spawns two concrete null-assertion classes that differ only in which direction they check. For example:

// These two differ only in one method call and one string
internal class ListNullAssertion<TList, TItem>    : ListAssertionBase<TList, TItem> { ... CheckIsNull ... }
internal class ListNotNullAssertion<TList, TItem> : ListAssertionBase<TList, TItem> { ... CheckIsNotNull ... }

The same pattern repeats for 7 bases (Collection, List, ReadOnlyList, Dictionary, MutableDictionary, Set, AsyncEnumerable), yielding 14 classes that are structurally identical except for the bool-equivalent direction.

A simpler approach — collapse each pair into one class with a primary constructor bool:

internal class ListNullOrNotNullAssertion<TList, TItem>(
    AssertionContext<TList> context,
    bool expectNull)
    : ListAssertionBase<TList, TItem>(context)
    where TList : IList<TItem>
{
    protected override Task<AssertionResult> CheckAsync(EvaluationMetadata<TList> metadata)
        => expectNull ? NullCheck.CheckIsNull(metadata) : NullCheck.CheckIsNotNull(metadata);

    protected override string GetExpectation()
        => expectNull ? "to be null" : "to not be null";
}

This halves the class count (14 → 7), removes entirely duplicated structure, and keeps each specialisation in one place. If a third variant is ever needed (e.g., IsNullOrEmpty), there's a single class to change per type instead of two.

---

2. Dead field in SetNullAssertion / SetNotNullAssertion (minor)

Both set null assertion classes store _adapterFactory but NullCheck.CheckIsNull/NotNull never materialises the set, so CreateSetAdapter is never called in the null path:

internal class SetNullAssertion<TSet, TItem> : SetAssertionBase<TSet, TItem>
{
    private readonly Func<TSet, ISetAdapter<TItem>> _adapterFactory; // never called
    ...
    protected override ISetAdapter<TItem> CreateSetAdapter(TSet value) => _adapterFactory(value);
}

The field is forced on these classes by the abstract CreateSetAdapter contract — that constraint is unavoidable, but it's worth noting that this storage cost is pure overhead. The bool-collapse approach above doesn't change that, but it does contain the oddity in one place.

---

3. Test coverage only covers the happy path (minor)

The new tests verify that chaining is preserved when the assertion passes, but don't cover:

• Failure propagation: does .IsNotNull().And.Contains(3) on a null collection fail with a useful message?
• Compound failure: does .IsNull().Or.Contains(3) correctly propagate when both sides fail?

Adding one or two failure-path assertions would give confidence that the error message surfacing works end-to-end through the chain, not just the success case.

---

4. Public API snapshot not updated

The PR notes this couldn't be run due to a local build conflict. Before merging, the .verified.txt snapshot files should be updated — the new IsNull()/IsNotNull() overloads on all the base classes are public API surface.

---

What's good

• The static class cleanly consolidates the check logic, including the previously missing exception handling that the old lacked.
• The keyword shadowing is the correct C# idiom here for narrowing return types in a fluent chain.
• The fix to the old wrong error message ( → for ) is a good catch.
• Test structure follows existing conventions.

[claude]

Code Review: Preserve specialized chaining after null assertions

The goal here is clear and the implementation works correctly. The NullCheck static helper is a clean extraction. A few structural concerns worth discussing:

---

1. Class proliferation — 14 classes for 2 behaviours each (major)

Every specialized base now spawns two concrete null-assertion classes that differ only in which direction they check. For example:

// These two differ only in one method call and one string
internal class ListNullAssertion<TList, TItem>    : ListAssertionBase<TList, TItem> { ... CheckIsNull ... }
internal class ListNotNullAssertion<TList, TItem> : ListAssertionBase<TList, TItem> { ... CheckIsNotNull ... }

The same pattern repeats for 7 bases (Collection, List, ReadOnlyList, Dictionary, MutableDictionary, Set, AsyncEnumerable), yielding 14 classes that are structurally identical except for the bool-equivalent direction.

A simpler approach — collapse each pair into one class with a primary constructor bool:

internal class ListNullOrNotNullAssertion<TList, TItem>(
    AssertionContext<TList> context,
    bool expectNull)
    : ListAssertionBase<TList, TItem>(context)
    where TList : IList<TItem>
{
    protected override Task<AssertionResult> CheckAsync(EvaluationMetadata<TList> metadata)
        => expectNull ? NullCheck.CheckIsNull(metadata) : NullCheck.CheckIsNotNull(metadata);

    protected override string GetExpectation()
        => expectNull ? "to be null" : "to not be null";
}

This halves the class count (14 → 7), removes entirely duplicated structure, and keeps each specialisation in one place. If a third variant is ever needed (e.g., IsNullOrEmpty), there's a single class to change per type instead of two.

---

2. Dead field in SetNullAssertion / SetNotNullAssertion (minor)

Both set null assertion classes store _adapterFactory but NullCheck.CheckIsNull/NotNull never materialises the set, so CreateSetAdapter is never called in the null path:

internal class SetNullAssertion<TSet, TItem> : SetAssertionBase<TSet, TItem>
{
    private readonly Func<TSet, ISetAdapter<TItem>> _adapterFactory; // never called
    ...
    protected override ISetAdapter<TItem> CreateSetAdapter(TSet value) => _adapterFactory(value);
}

The field is forced on these classes by the abstract CreateSetAdapter contract, so it's unavoidable given the current design. The bool-collapse approach above doesn't change that, but it does contain the oddity in one place.

---

3. Test coverage only covers the happy path (minor)

The new tests verify that chaining is preserved when the assertion passes, but don't cover:

• Failure propagation: does .IsNotNull().And.Contains(3) on a null collection fail with a useful message?
• Compound failure: does .IsNull().Or.Contains(3) correctly propagate when both sides fail?

Adding one or two failure-path assertions would give confidence that the error message surfacing works end-to-end through the chain, not just the success case.

---

4. Public API snapshot not updated

The PR notes this couldn't be run due to a local build conflict. Before merging, the .verified.txt snapshot files should be updated — the new IsNull()/IsNotNull() overloads on all the base classes are public API surface.

---

What's good

• The NullCheck static class cleanly consolidates the check logic, including the previously missing exception handling that the old CollectionNotNullAssertion lacked.
• The new keyword shadowing is the correct C# idiom here for narrowing return types in a fluent chain.
• The fix to the old wrong error message ("value is null" → "value is not null" for CheckIsNull) is a good catch.
• Test structure follows existing conventions.

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 0 complexity

Metric	Results
Complexity	0

View in Codacy

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.}

[claude]

Follow-up review — previous findings status

Checking against the prior review:

Addressed

Public API snapshots — the second commit (c08259e9) added the .verified.txt updates for all four target frameworks. This is now complete.

---

Still open

1. Class proliferation — 14 concrete classes for 2 behaviours (major)

This was not addressed. The current state in CollectionNullAssertion.cs still has 14 classes (Null + NotNull for each of Collection, List, ReadOnlyList, Dictionary, MutableDictionary, Set, AsyncEnumerable) that differ only in which NullCheck helper they call and which string they return.

The concrete cost is maintenance: if the base hierarchy ever needs a constructor parameter change, or if a third null variant is added (e.g., IsNullOrEmpty), there are 14 classes to update instead of 7. Each pair is structurally identical boilerplate.

The suggested consolidation from last time still stands — using a primary-constructor bool expectNull to collapse each pair:

// Before: two classes, each ~10 lines, identical except for CheckIsNull vs CheckIsNotNull
internal class ListNullAssertion<TList, TItem>    : ListAssertionBase<TList, TItem> { ... }
internal class ListNotNullAssertion<TList, TItem> : ListAssertionBase<TList, TItem> { ... }

// After: one class
internal class ListNullOrNotNullAssertion<TList, TItem>(
    AssertionContext<TList> context,
    bool expectNull)
    : ListAssertionBase<TList, TItem>(context)
    where TList : IList<TItem>
{
    protected override Task<AssertionResult> CheckAsync(EvaluationMetadata<TList> metadata)
        => expectNull ? NullCheck.CheckIsNull(metadata) : NullCheck.CheckIsNotNull(metadata);

    protected override string GetExpectation()
        => expectNull ? "to be null" : "to not be null";
}

The public IsNull() / IsNotNull() methods on each base stay unchanged from the consumer's perspective; only the internal implementation class count halves.

---

2. Dead field in SetNullAssertion / SetNotNullAssertion (minor)

Still present. Both set null assertion classes store _adapterFactory via constructor injection, but NullCheck.CheckIsNull / CheckIsNotNull never materialise the set, so CreateSetAdapter is never called in the null-check path. The field is dead weight in those classes.

This is forced by the abstract CreateSetAdapter contract on SetAssertionBase. The best mitigation given the current design is a code comment explaining why the field is required but unused in the null path, to avoid future confusion. The bool-collapse approach from point 1 consolidates this into one place rather than two.

---

3. Test coverage only covers the happy path (minor)

Still only success-path assertions. Two targeted failure tests would close this gap:

[Test]
public async Task NullAssertions_IsNotNull_FailsWithUsefulMessage_WhenNull()
{
    List<int>? nullItems = null;
    await Assert.That(async () =>
        await Assert.That(nullItems).IsNotNull().And.Contains(1))
        .Throws<TUnitAssertionException>()
        .WithMessage("*to not be null*");
}

[Test]
public async Task NullAssertions_IsNull_FailsWithUsefulMessage_WhenNotNull()
{
    var items = new List<int> { 1 };
    await Assert.That(async () =>
        await Assert.That(items).IsNull().Or.Contains(99))
        .Throws<TUnitAssertionException>();
}

These verify that error messages propagate correctly through the chain, not just that the chaining API compiles and passes.

---

Summary

One of the four prior findings is resolved. The three remaining ones are still worth addressing before merge, particularly the class proliferation (major) which sets a maintenance pattern that will compound as the assertion hierarchy grows.

[claude]

Code Review — PR #6008: Preserve specialized chaining after null assertions

The goal of this PR is solid and the coverage of all collection-family base classes is thorough. Public API snapshots are included. A few issues worth addressing before merge:

---

🔴 Issue 1 — Existing extension method is now dead code; return type regressed

File: TUnit.Assertions/Extensions/AssertionExtensions.cs lines 54-63 + TUnit.Assertions/Sources/CollectionAssertionBase.cs

The PR adds a public instance method IsNotNull() to CollectionAssertionBase, but does not remove the pre-existing extension method in AssertionExtensions.cs:

// Still exists — now unreachable for CollectionAssertionBase receivers:
public static CollectionNotNullAssertion<TCollection, TItem> IsNotNull<TCollection, TItem>(
    this CollectionAssertionBase<TCollection, TItem> source)
    where TCollection : class, IEnumerable<TItem>

In C#, instance methods always take priority over extension methods of the same name. Any call to .IsNotNull() on a CollectionAssertionBase-typed variable now silently resolves to the instance method, making the extension unreachable dead code.

Two concrete consequences:

1. Return type narrowed: The extension returned CollectionNotNullAssertion<TCollection, TItem>; the new instance method is declared as returning CollectionAssertionBase<TCollection, TItem>. Code that stored the result as the more specific type gets a compile error.
2. Context.Map dropped: The extension did Context.Map((TCollection? v) => v!) to narrow nullability in the assertion context. The instance method passes Context directly. This is only a nullability-flow concern (no runtime difference), but it's a silent regression in what the extension was designed to do.

Fix: Remove the now-dead extension method from AssertionExtensions.cs. If the Context.Map narrowing is important for downstream nullability, carry it into the instance method.

---

🟡 Issue 2 — new keyword hiding means base-typed references get the wrong implementation

File: TUnit.Assertions/Sources/DictionaryAssertionBase.cs, ListAssertionBase.cs, ReadOnlyListAssertionBase.cs, MutableDictionaryAssertionBase.cs, SetAssertionBase.cs

The subclass overloads correctly use public new ... IsNull() to shadow CollectionAssertionBase.IsNull(). But because this is method hiding, not polymorphic override, a caller holding a CollectionAssertionBase<TDictionary, KeyValuePair<TKey, TValue>> reference gets CollectionNullAssertion (no dictionary methods) instead of DictionaryNullAssertion:

// Concrete type gets the right implementation:
DictionaryAssertionBase<...> dict = Assert.That(myDict);
dict.IsNull().And.ContainsKey("k"); // ✅ DictionaryNullAssertion

// Base-typed reference gets the wrong one:
CollectionAssertionBase<IDictionary<string,int>, KVP<string,int>> col = Assert.That(myDict);
col.IsNull().And.ContainsKey("k"); // ❌ CollectionNullAssertion — no ContainsKey

This is unlikely to bite users in day-to-day chaining (since Assert.That(dict) returns the concrete type), but it will break any generic helper that returns or accepts CollectionAssertionBase. The root cause is the design choice to use new instead of a virtual+override design.

Better approach: Declare IsNull() and IsNotNull() as virtual on CollectionAssertionBase and use override in subclasses. This correctly propagates the specific null assertion type regardless of how the reference is typed.

---

🟡 Issue 3 — NullCheck.Check exception handling differs from both existing null assertion paths

File: TUnit.Assertions/Conditions/CollectionNullAssertion.cs lines 107-110

The old CollectionNotNullAssertion.CheckAsync did not inspect metadata.Exception — it went straight to checking metadata.Value. The new NullCheck.Check adds an exception-first branch:

if (metadata.Exception != null)
    return Task.FromResult(AssertionResult.Failed($"threw {metadata.Exception.GetType().Name}", metadata.Exception));

This creates three different failure messages for conceptually equivalent null-check failures depending on which path is taken:

• NotNullAssertion (scalar): "received null"
• Old CollectionNotNullAssertion: "value is null"
• New NullCheck.Check (collection): "threw ExceptionTypeName"

When a delegate/Func-based collection source throws during evaluation, metadata.Exception is set and metadata.Value is null. Previously the collection path failed with "value is null"; now it fails with "threw InvalidOperationException". This changes the user-visible failure message for existing tests that check exception message text.

Suggestion: Align the exception-branch message with NotNullAssertion ("received null") or create a shared constant/helper that all null assertions use for consistency.

---

🟡 Issue 4 — CollectionNullAssertion is internal while CollectionNotNullAssertion is public

File: TUnit.Assertions/Conditions/CollectionNullAssertion.cs lines 12 and 30

CollectionNullAssertion<TCollection, TItem> (returned by IsNull()) is internal, while CollectionNotNullAssertion<TCollection, TItem> (returned by IsNotNull()) is public. Both are returned through the same public API surface — the asymmetry has no apparent justification. All other new null assertion classes in this PR (ListNullAssertion, DictionaryNullAssertion, etc.) are also internal, while CollectionNotNullAssertion is the odd one out by being public.

Either make CollectionNullAssertion public for symmetry, or make CollectionNotNullAssertion internal to match the rest.

---

✅ What works well

• The NullCheck static helper is a clean factoring — one implementation for all the CheckAsync/GetExpectation logic rather than per-class duplication.
• SetNullAssertion correctly threads CreateSetAdapter through even though it's never called on the null path — the null guards in each set-specific assertion (IsSubsetOf, DoesNotOverlap, etc.) prevent NRE before the factory is invoked.
• Test coverage is appropriately scoped: happy-path chaining and both failure-message checks for IsNull/IsNotNull.
• Public API snapshot files are updated for all four target frameworks.

[thomhurst]

@
Addressed the latest review:

• 🔴 Dead extension method / return regression — removed the shadowed IsNotNull extension in AssertionExtensions.cs. The base IsNull()/IsNotNull() instance methods now return the concrete CollectionNullAssertion/CollectionNotNullAssertion types, restoring the specific return type the extension gave.
• 🟡 Visibility asymmetry — CollectionNullAssertion is now public, matching CollectionNotNullAssertion.
• Failure-path tests — already present (NullAssertions_IsNotNull_Fails_When_Null_Through_Chain, NullAssertions_IsNull_Fails_When_NotNull_Through_Chain).
• Class proliferation — already collapsed to a bool expectNull ctor for List/ReadOnlyList/Dictionary/MutableDictionary/Set in the current diff.

Pushing back on two:

• 🟡 new → virtual/override — keeping new shadowing. It is the same idiom already used for And/Or on CollectionAssertionBase, and the only way to make virtual/override preserve each subtype's specific return type is covariant returns, which are not honored at runtime on the netstandard2.0 target consumed by .NET Framework. Added a comment documenting this.
• 🟡 Exception-branch message — keeping "threw {ExceptionType}". This matches the convention of the sibling null-family assertions (IsDefault/IsNotDefault* all emit "threw {name}"); only the scalar NotNullAssertion swallows it into "received null", which is the less informative path. These collection null classes are new, so no existing test depends on the old message.
  @

[claude]

Code Review — PR #6008 (third pass)

Checking against the second-pass review and the third-pass review.

---

Addressed since the last review

Dead extension method removed — AssertionExtensions.cs no longer contains the IsNotNull<TCollection, TItem>(this CollectionAssertionBase<...> source) overload. The public API snapshot confirms the removal. This was the Issue 1 from the last pass.

Test failure paths are covered — CollectionAssertionTests.cs now includes NullAssertions_IsNotNull_Fails_When_Null_Through_Chain and NullAssertions_IsNull_Fails_When_NotNull_Through_Chain, both checking that AssertionException is thrown with the right message text. This closes Issue 3 from the first pass.

Public API snapshots — all four .verified.txt files are updated and committed.

---

Still open

1. Class proliferation — 7 pairs still unaddressed (major, raised twice)

CollectionNullAssertion.cs still contains 7 separate class pairs — CollectionNullAssertion / CollectionNotNullAssertion, plus six more single-bool classes (ListNullAssertion, ReadOnlyListNullAssertion, DictionaryNullAssertion, MutableDictionaryNullAssertion, SetNullAssertion, AsyncEnumerableNullAssertion).

Notably, the five internal classes already use the consolidated single-bool design (ListNullAssertion(context, bool expectNull)), but the two public collection classes (CollectionNullAssertion / CollectionNotNullAssertion) are still split into separate classes.

The asymmetry has grown: internal classes use one pattern; public classes use another. The public split is the source of the inconsistency.

2. CollectionNullAssertion is public while all other null assertion classes are internal (minor, raised last pass)

CollectionNullAssertion<TCollection, TItem> is public (it appears in the .verified.txt snapshot) but ListNullAssertion, ReadOnlyListNullAssertion, DictionaryNullAssertion, MutableDictionaryNullAssertion, SetNullAssertion, and AsyncEnumerableNullAssertion are all internal. CollectionNotNullAssertion is also public.

There is no stated reason for this asymmetry. Making the two Collection*NullAssertion classes internal would: halve the public API surface added by this PR, allow them to be collapsed into one class with a bool parameter (addressing point 1), and make the pattern consistent across all seven collection types.

If they must stay public for some reason (e.g. external subclassing), that reason should be documented.

3. NullCheck.Check exception-branch message is inconsistent with NotNullAssertion (minor, raised last pass)

NotNullAssertion.CheckAsync (in NullAssertion.cs) maps an exception to "received null":

if (metadata.Exception != null)
    return Task.FromResult(AssertionResult.Failed("received null"));

NullCheck.Check maps an exception to "threw ExceptionTypeName":

if (metadata.Exception != null)
    return Task.FromResult(AssertionResult.Failed($"threw {metadata.Exception.GetType().Name}", metadata.Exception));

A user who uses .IsNotNull() on a plain value gets "Expected ... to not be null but received null". A user who uses .IsNotNull() on a collection source that threw gets "Expected ... to not be null but threw InvalidOperationException". The second message is arguably more informative (it names the exception), but the inconsistency across the two paths will be confusing. A decision should be made: either align them (use "received null" everywhere) or document the deliberate divergence.

---

Summary

Three of the five findings from the two prior passes remain. The two confirmed-addressd items (dead extension method, failure-path tests) are good progress. The biggest outstanding item is still the CollectionNullAssertion / CollectionNotNullAssertion pair being public and split, while every other null assertion class in this PR is internal and consolidated. Making those two classes internal and merging them into a single bool-parameterised class would resolve both the consistency and the proliferation issues at once.

[claude]

Code Review — PR #6008 (fourth pass)

This is a follow-up check against the third-pass review and the author's response.

---

All outstanding findings are now resolved

Class proliferation — fully closed. CollectionNullAssertion and CollectionNotNullAssertion have been collapsed into a single internal class CollectionNullAssertion<TCollection, TItem>(AssertionContext<TCollection> context, bool expectNull), matching the pattern already used by the other six null assertion classes. The PR now has exactly 7 internal null assertion classes, one per collection type, each sharing the NullCheck helper.

Visibility asymmetry — closed. All null assertion classes (CollectionNullAssertion, ListNullAssertion, etc.) are internal. They do not appear in the .verified.txt snapshots, only the public IsNull()/IsNotNull() instance methods on each assertion base appear there.

Exception-branch message consistency — closed. NullAssertion.CheckAsync in NullAssertion.cs was also updated to use "threw {ExceptionTypeName}" (instead of the old "received null" for the exception path), so the scalar and collection null paths now emit the same message format for that case.

Dead extension method — closed in a prior pass.

Failure-path tests — closed in a prior pass.

Public API snapshots — all four .verified.txt files are up to date.

---

One minor observation (no blocking issue)

NullCheck is declared internal static but its two methods are public. Since the class itself is internal, the public on the methods has no practical effect — callers outside the assembly cannot reach them regardless. Declaring the methods internal would be more accurate and self-documenting. This is purely cosmetic and not a blocker.

---

Summary

The PR is in good shape. The three previously-raised structural concerns (class proliferation, visibility asymmetry, message consistency) have all been addressed. The implementation is clean, internally consistent, and well-tested. Ready to merge subject to any CI checks passing.