fix(assertions): prevent implicit-to-string op from NREing on null (#5692)

[thomhurst]

Summary

• Closes NullReferenceException from source-gen when asserting a null value of a type with an implicit operator to string #5692. Assert.That(value) where value's static type declared implicit operator string routed through the Assert.That(string?) overload, invoking the user's op_Implicit on a null receiver at the call site — NRE inside user code before the assertion ran.
• Replace the overload's parameter with a StringValue wrapper struct that exposes a single implicit conversion from string?. C# allows only one user-defined conversion per chain, so Id → string → StringValue is no longer applicable and the generic Assert.That<TValue> overload wins by exact-match betterness.
• Add 31 regression tests pinning every Assert.That routing path so future overload-set changes cannot silently re-route (string / IEnumerable<char> cast / char[] / List<T> / IList<T> / IReadOnlyList<T> / HashSet<T> / IEnumerable<T> / IQueryable<T> / non-generic IEnumerable / Dictionary<K,V> / Memory<T> / ReadOnlyMemory<T> / Action / Func<Task> / Task / Task<T> / Func<T> / Func<IEnumerable<T>> / value types / ref types without implicit-to-string).

Why a wrapper struct (and not priority reshuffling)

The three required resolutions — "hello" → string overload, Id (implicit-to-string) → generic, IQueryable<T> → IEnumerable overload — impose a cycle in any priority ordering (string ≥ IEnumerable > generic ≥ string is unsatisfiable). A wrapper struct is the only fix that keeps existing overload semantics intact. No source-gen call rewriting involved.

Test plan

• TUnit.TestProject/Bugs/5692 engine-mode repro passes (previously failed with NRE in op_Implicit)
• New ImplicitStringOperatorOverloadTests — 31/31 green
• Full TUnit.Assertions.Tests suite — 2017/2017 green

[codacy-production]

Not up to standards ⛔

🔴 Issues 1 medium

Alerts:
⚠ 1 issue (≤ 0 issues of at least minor severity)

Results:
1 new issue

Category	Results
Performance	1 medium

View in Codacy

🟢 Metrics 37 complexity

Metric	Results
Complexity	37

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]

Code Review — PR #5696: StringValue wrapper to prevent NRE from implicit-to-string overload binding

Summary

This is a clever, minimal fix for a subtle and frustrating overload resolution trap. The approach is sound, the test coverage is comprehensive, and the documentation explains the rationale clearly. A few observations below.

---

What's Well Done

The StringValue trick is exactly right. Exploiting the C# single-user-defined-conversion rule to silently reroute non-string types away from the string overload is the canonical fix for this pattern. Priority reshuffling (via [OverloadResolutionPriority]) can't solve this because the three routing constraints — string → string overload, Id (implicit-to-string) → generic, IQueryable<T> → IEnumerable overload — form a cycle. StringValue is the only way to break it without source-gen rewriting.

readonly struct + private constructor + [AggressiveInlining] — All correct. Zero allocation overhead, only constructible via the implicit path, inlineable in JIT-hot paths.

Test breadth — 31 tests covering the full overload routing matrix is very thorough. Routing assertions (verifying the returned assertion type, not just absence of exceptions) are the right way to pin this.

[EditorBrowsable(EditorBrowsableState.Never)] — Good call for keeping this out of IntelliSense autocompletion.

---

Observations

1. Missing ToString() override

// StringValue.cs
public override string? ToString() => Value;

As-is, inspecting a StringValue in a debugger or error message will show TUnit.Assertions.StringValue rather than the underlying string. Since StringValue can appear in assertion failure messages or stack traces during development, adding ToString() would make debugging friction-free. (A [DebuggerDisplay("{Value}")] attribute would achieve the same for the debugger pane.)

2. Value property visibility

Value must be public because Assert.cs (also public) reads it. That's fine — just worth noting that this is intentional infrastructure, not a deliberate public API. The [EditorBrowsable(EditorBrowsableState.Never)] on the type covers discoverability, but Value itself has no such suppression. If IntelliSense ever surfaces someStringValue.Value, it would be confusing to users. Consider adding [EditorBrowsable(EditorBrowsableState.Never)] on the property too, or at minimum a <inheritdoc> pointing at the type-level XML doc.

3. Public API snapshot — worth checking

The CLAUDE.md notes that changes to public APIs require running snapshot tests. StringValue is a new public readonly struct with a public property and public static implicit operator. If this project uses API approval tests (e.g. PublicApiAnalyzer/.approved.txt files), they'll need updating. Quick check:

find . -name "*.approved.txt" | head -5

If those exist and haven't been updated, CI will fail.

4. Scope: only Assert.That is affected

The other string? parameters in Assert.cs (e.g., PropertyChanged(INotifyPropertyChanged, string, Action)) don't need the same treatment because they don't compete with a generic overload that could also accept types with implicit-to-string conversions. That's correct — no action needed.

---

Minor Nit

The // Cast to IEnumerable<object?> directly without calling Cast<>() comment in the non-generic IEnumerable overload is the one existing comment that's worth keeping — it explains a subtle correctness concern. No issues there.

---

Verdict

Approve — the design is solid and the fix is correct. The ToString() override is the one change I'd suggest before merging, since it directly improves the debugging experience for assertion failures involving strings. The API snapshot check is a build correctness concern to verify.

[claude]

Code Review — PR #5696

Overall Assessment

The core design is sound and the StringValue wrapper struct is the canonical solution for breaking the three-constraint cycle (string overload ≥ IEnumerable > generic ≥ string literal). The PR explanation and XML doc comments are unusually clear. The use of [MethodImpl(MethodImplOptions.AggressiveInlining)] on the implicit conversion is appropriate — the JIT should eliminate this pure data carrier entirely in release builds. Public API snapshot updates across all four target frameworks (net8.0, net9.0, net10.0, net4.7) are correct and consistent.

---

Previous Review Status

A prior Claude review noted three items:

1. Missing ToString() override on StringValue — the struct would display as TUnit.Assertions.StringValue in debugger output.
2. Value property visibility — suggested [EditorBrowsable(EditorBrowsableState.Never)] on the property itself.
3. Public API snapshot check — now addressed by the snapshot update commit.

Item 3 is addressed. Items 1 and 2 remain open (details below).

---

Remaining Concerns

1. Missing ToString() Override — Debugging Friction

StringValue has no ToString() override, so hovering over a variable in the debugger or watch window will show TUnit.Assertions.StringValue instead of the actual string content. If the framework's internal diagnostic paths ever call .ToString() on an unwrapped wrapper, failure messages will be opaque.

Suggested fix (1 line):

public override string? ToString() => Value;

Or, without touching ToString() semantics:

[System.Diagnostics.DebuggerDisplay("{Value}")]
public readonly struct StringValue { ... }

This is low-effort and eliminates a real friction point for anyone who steps through assertion code.

2. Value Property Missing [EditorBrowsable]

The [EditorBrowsable(EditorBrowsableState.Never)] attribute is correctly applied to the type, but not to the Value property. A user who obtains a StringValue reference (e.g., in a helper that captures an assertion chain) will see .Value in autocomplete. Since the type itself is hidden this is minor, but adding the attribute to the property is belt-and-suspenders:

[System.ComponentModel.EditorBrowsable(System.ComponentModel.EditorBrowsableState.Never)]
public string? Value { get; }

---

Minor Observations (Non-Blocking)

3. No test for a type implementing both implicit operator string AND IEquatable<string>

The new tests in Issue5692/ImplicitStringOperatorOverloadTests.cs cover the implicit operator path in isolation. A type that does both (e.g., a DDD value object like UserId) would route correctly through the fix, but there's no regression test pinning that combination. Worth adding to prevent future regressions.

4. Broad #pragma warning disable in the test file

The file-wide suppression of TUnitAssertions0002 is legitimate (the captured assertion is inspected for its runtime type, not awaited), but the comment only echoes the warning name. A brief note like // assertion deliberately captured for type inspection, not awaited would help future maintainers avoid accidentally removing the pragma.

5. Issue2993 test gap (pre-existing)

ImplicitConversionTests.cs in Issue2993 tests StringRecord in a collection context but never via Assert.That(singleRecord). This pre-dates this PR, but a direct single-value test there would confirm the new overload routing is consistent across both issue scenarios.

---

Conclusion

Neither item 1 nor item 2 is blocking — the fix is architecturally correct and the bug scenario is thoroughly tested. The PR is ready to merge once the maintainer decides whether to add the ToString() override and/or the [EditorBrowsable] annotation on the Value property.

[claude]

Code Review — PR #5696: fix(assertions): prevent implicit-to-string op from NREing on null

Third pass. Two items from the previous review are still open; adding a couple of new observations. Overall this is a clean and well-reasoned fix.

---

What's good

The StringValue wrapper struct approach is the right call. The PR description correctly identifies why this is the only viable fix — any priority reordering among the existing overloads creates an unsatisfiable cycle (string ≥ IEnumerable > generic ≥ string). Using C#'s single-user-defined-conversion-per-chain rule as the discriminator is precise and doesn't disturb any other routing path.

The implementation details are solid:

• readonly struct + private constructor — good; prevents misuse, eliminates defensive copies.
• [MethodImpl(AggressiveInlining)] on op_Implicit — correct; zero-overhead wrapping.
• [EditorBrowsable(EditorBrowsableState.Never)] on the struct — correct infrastructure pattern.
• XML docs clearly explain the why, not just the what — exactly what future maintainers need.

The regression test suite is exemplary. 31 tests pinning every overload routing path is exactly the kind of regression guard that prevents silent re-routing in the future. Engine-mode repro in TestProject adds an extra layer of confidence. The #pragma warning disable TUnitAssertions0002 on line 1 with its inline comment is clear enough.

Public API snapshots updated correctly across all four TFMs — the API change is properly surfaced and reviewable.

---

Still open from previous reviews

1. Missing ToString() override on StringValue

// Current — default struct ToString() returns "TUnit.Assertions.StringValue"
public readonly struct StringValue { ... }

// Suggested
public override string? ToString() => Value;

This is a debugger ergonomics issue. When an assertion fails and the failure reporter calls ToString() somewhere in the chain (e.g. in an error message or watch window), the default struct representation leaks the infrastructure type into user-visible output. Adding => Value costs nothing and eliminates that friction.

2. Value property lacks [EditorBrowsable(EditorBrowsableState.Never)]

// Current
public string? Value { get; }

// Suggested
[EditorBrowsable(EditorBrowsableState.Never)]
public string? Value { get; }

The struct-level attribute suppresses the type from top-level completion, but if a user navigates to its definition (e.g. "Go to definition" on StringValue), Value appears in completion without suppression. Annotating the property makes the "do not use this directly" intent consistent at every access point.

---

New observation

StringValue is public in the public API surface without an [Obsolete] or [EditorBrowsable] on Value

The API snapshots correctly reflect public string? Value { get; }. If users discover and consume Value directly (unlikely but possible), the fix above makes the intent explicit in tooling without requiring a breaking API change.

No test for a type with both implicit operator string AND implicit operator StringValue

If a user type declared both conversions, C# would have an ambiguous conversion to StringValue and a compiler error would occur at the call site — which is arguably acceptable. But it might be worth a quick sanity check or a comment in the docs that this edge case is a compile error, not an NRE.

---

Summary

Item	Status
Core StringValue fix	✅ Correct and minimal
Overload routing tests (31 cases)	✅ Comprehensive
Engine-mode repro test	✅ Good
API snapshots updated	✅ All 4 TFMs
ToString() override	⚠️ Still missing
[EditorBrowsable] on Value	⚠️ Still missing
Pragma comment clarity	✅ Adequate

The two open items are polish, not correctness. The fix itself is sound. Happy to approve once the maintainer makes a deliberate call on ToString() and [EditorBrowsable] on Value — even a "won't fix, reason X" is a valid resolution.