fix(assertions): prefer string overload of Member() over IEnumerable<char> (#5702)

[thomhurst]

Summary

• Adds three string-specific Member() overloads with [OverloadResolutionPriority(3)] that forward to the existing Member<TObject, string> generic overloads.
• Fixes [Bug]: Overload resolution on string members prefers equality of IEnumerable<char> over string #5702: .Member(x => x.StringProperty, p => p.IsEqualTo("...")) resolved to the collection path (TItem=char) because the collection overload carries [OverloadResolutionPriority(2)] and string implements IEnumerable<char>. The resulting IAssertionSource<IEnumerable<char>> triggered TUnitAssertions0016.
• With the new overloads, the lambda's key parameter is now IAssertionSource<string>, so IsEqualTo binds to the string-typed overload and the analyzer warning is silenced.

Test plan

• Added TUnit.Assertions.Tests/Bugs/Issue5702Tests.cs with two regression tests (direct KeyValuePair<string, object?> repro from the issue, plus a nested-property case). Uses a NonInterned helper to defeat string interning so the test would genuinely fail under reference-equality semantics.
• Verified repro: elevating TUnitAssertions0016 to error on main fires on both tests; with the fix it fires on neither.
• dotnet run -- --treenode-filter "/*/*/Member*/*" — 82 passed.
• Full TUnit.Assertions.Tests suite — 2019 passed.

[claude]

Code Review

Summary

The fix correctly identifies and resolves the root cause: implements , so the collection overload at was winning over the general member overload at no/lower priority when the member selector returned . Adding string-specific overloads is a valid, idiomatic C# solution to this problem.

Overall the approach is sound and the tests are well-designed. A few things worth discussing:

---

Issue 1: Redundant second overload

** with is a subset of with .**

When a caller writes:

.Member(x => x.Name, n => n.IsEqualTo("expected"))

returns , so in the first overload is inferred as — both overloads match at priority 3. C# will prefer the more specific non-generic overload (second), so there's no runtime issue, but it adds confusion about when each is used.

Suggested approach: Remove the second overload entirely. The overload covers this case already, and the "legacy" case is already covered by the third overload. If inference fails in a specific scenario, let it surface as a compile error with a good message rather than silently routing to a duplicate overload.

---

Issue 2: Proliferating type-specific overloads — a pattern to watch

This PR correctly fixes . But the same problem will occur for any type that implements a collection interface — e.g., (implements and has an indexer), , custom types with . Each time, three more overloads will need to be added.

Alternative worth considering: Instead of adding type-specific escape hatches, flip the priority scheme so the general member overload wins by default and collection/dictionary overloads require opting in explicitly:

• General : priority 2 (currently no priority / 0 or 1)
• Collection with selector: priority 1

This would make (and similar types) default to the scalar path, with the collection path only winning when the caller explicitly wants it via a collection-typed overload. That removes the need for any type-specific overloads and handles the entire class of problems.

The tradeoff is a potential breaking change for callers who currently get the collection overload implicitly for properties. But given that this implicit selection already caused bugs (#5702), a more explicit opt-in might be the healthier long-term design.

---

Issue 3: on the forwarding string overload

The third overload:

[RequiresDynamicCode("...")]
public static MemberAssertionResult<TObject> Member<TObject>(
    this IAssertionSource<TObject> source,
    Expression<Func<TObject, string?>> memberSelector,
    Func<IAssertionSource<string>, object> assertions)
    => Member<TObject, string>(source, memberSelector, assertions);

This correctly mirrors the annotation on the equivalent collection and dictionary overloads. The forwarding call resolves to the overload at line ~545 which also has . The attribute propagation is correct, no action needed here — just confirming consistency.

---

Tests: Well-designed

The helper is an excellent touch:

private static string NonInterned(string s) => new(s.AsSpan());

This is precisely the right way to guarantee the test catches reference-equality bugs rather than coincidentally passing due to string interning. Both test cases (direct repro and nested-property case) cover the meaningful scenarios from the issue.

---

Minor

The XML doc comments on the new overloads are more verbose than needed (they explain the overload resolution logic in detail). The existing file style is inconsistent on this — some overloads are well-documented, others are brief. Not a blocker, just noting the new comments lean longer than the file's average.

---

Verdict

Fix is correct and targeted. The approach works. The main architectural question is whether adding type-specific overloads is the right long-term strategy vs. revising the priority scheme. For the immediate bug fix, this PR is mergeable.

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 5 complexity

Metric	Results
Complexity	5

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

Summary

The fix correctly identifies and resolves the root cause: string implements IEnumerable<char>, so the collection overload at [OverloadResolutionPriority(2)] was winning over the general member overload when the member selector returned string. Adding string-specific overloads at priority 3 is a valid, idiomatic C# solution to this problem.

Overall the approach is sound and the tests are well-designed. A few things worth discussing:

---

Issue 1: Redundant second overload

The Member<TObject> overload with Func<IAssertionSource<string>, Assertion<string>> is a subset of Member<TObject, TTransformed> with Func<IAssertionSource<string>, Assertion<TTransformed>>.

When a caller writes .Member(x => x.Name, n => n.IsEqualTo("expected")), IsEqualTo returns Assertion<string>, so TTransformed in the first overload is inferred as string — both overloads match at priority 3. C# will prefer the more specific non-generic overload (the second one), so there is no runtime issue, but having both adds confusion about when each is used.

Suggested approach: Remove the second overload entirely. The TTransformed overload covers this case, and the "legacy" untyped case is covered by the third Func<..., object> overload.

---

Issue 2: Proliferating type-specific overloads — a pattern to watch

This PR correctly fixes string. But the same problem will occur for any type that implements a collection interface — ImmutableArray<T>, ReadOnlyMemory<T>, or any custom type that implements IEnumerable<T>. Each time, three more overloads would need to be added.

Alternative worth considering: Flip the priority scheme so the general member overload wins by default:

• General Member<TObject, TMember>: priority 2 (currently effectively 0)
• Collection Member<TObject, TItem> with IEnumerable<TItem> selector: priority 1

This would make string (and all similar types) default to the scalar path, with the collection path only winning when the assertion lambda explicitly expects a collection adapter. That removes the need for any type-specific overloads and handles the entire class of problems in one change.

The tradeoff is a potential breaking change for callers who currently get the collection overload implicitly. But given that this implicit selection already caused bugs (#5702), a more explicit opt-in might be the healthier long-term design.

---

Issue 3: [RequiresDynamicCode] annotation correctness

The third new overload correctly mirrors the annotation on the equivalent collection and dictionary overloads. The forwarding call resolves to the Member<TObject, TMember> overload at ~line 545 which also carries [RequiresDynamicCode]. Attribute propagation is consistent — no action needed.

---

Tests: Well-designed

The NonInterned helper is an excellent touch:

private static string NonInterned(string s) => new(s.AsSpan());

This is precisely the right way to guarantee the test catches reference-equality bugs rather than coincidentally passing due to string interning. Both test cases (direct KeyValuePair repro and nested-property case) cover the meaningful scenarios from the issue.

---

Verdict

Fix is correct and targeted. The approach works. The main architectural question is whether adding type-specific overloads is the right long-term strategy vs. revising the priority scheme across the board. For the immediate bug fix, this PR is mergeable.

[claude]

Code Review

Summary

This PR fixes a real and annoying overload resolution pitfall: implements , so the collection overload (priority 2) was winning over the generic scalar overload (priority 1). The fix — adding three -specific overloads at priority 3 — is correct and well-targeted.

---

Concerns

1. The three new overloads are redundant with each other

The second overload:

[OverloadResolutionPriority(3)]
public static MemberAssertionResult<TObject> Member<TObject>(
    this IAssertionSource<TObject> source,
    Expression<Func<TObject, string?>> memberSelector,
    Func<IAssertionSource<string>, Assertion<string>> assertions)
    => Member<TObject, string>(source, memberSelector, assertions);

is a strict specialisation of the first (TTransformed = string). The C# compiler will pick the first overload for any call site that satisfies both signatures, so the second can never be selected. It's dead code today — and it creates a maintenance trap, because a future refactor of the TTransformed overload might silently leave the Assertion<string> overload stale.

The same pattern already exists for the collection overloads (lines ~374/417), so this is consistent, but still worth questioning. If there is a reason the Assertion<string> overload is needed (e.g. a distinct runtime code path), that reason should be documented.

2. The -returning overload inherits a questionable annotation

The third overload:

[RequiresDynamicCode(...)]
public static MemberAssertionResult<TObject> Member<TObject>(
    ...
    Func<IAssertionSource<string>, object> assertions)
    => Member<TObject, string>(source, memberSelector, assertions);

simply delegates to the existing overload, which already carries . The attribute is therefore redundant here (it's already present on the target), but harmless. More importantly: should this overload be [Obsolete] like its dictionary counterpart seems intended to be? If the goal is to steer users toward the strongly-typed path, an explicit deprecation notice would help IDEs surface the better alternative.

3. Minor: XML doc comments are verbose for thin wrappers

Each new overload has a multi-sentence <summary> that describes what the overload does rather than why it exists. For thin forwarding methods, a single sentence referencing the root cause (// Disambiguates string from IEnumerable<char> — see #5702) is both more honest and easier to keep accurate. Long docs on trivial wrappers tend to drift.

---

Tests

The regression tests are well-designed. The NonInterned helper correctly defeats the interning optimisation that would have masked the bug under reference equality, and the second test covering a nested-property case adds useful coverage. The ToCharArray() approach used for net472 compat is a fine workaround.

---

Verdict

The fix is correct and the tests validate it properly. The structural concerns above are minor — the redundant overload and verbose docs won't cause bugs, but trimming them would reduce the surface area callers need to reason about and keep the file consistent with its best existing patterns.

[claude]

Code Review

Summary

This PR fixes a real and annoying overload resolution pitfall: string implements IEnumerable<char>, so the collection Member overload (priority 2) was winning over the generic scalar overload (priority 1). The fix — adding three string-specific overloads at priority 3 — is correct and well-targeted.

---

Concerns

1. The three new overloads are redundant with each other

The second overload (Func<IAssertionSource<string>, Assertion<string>>) is a strict specialisation of the first (TTransformed = string). The C# compiler will pick the first overload for any call site that satisfies both signatures, so the second can never be selected. It's dead code today — and it creates a maintenance trap, because a future refactor of the TTransformed overload might silently leave the Assertion<string> overload stale.

The same pattern already exists for the collection overloads, so this is at least consistent. But if there is a reason the Assertion<string> overload is independently needed, that reason should be documented.

2. The object-returning overload inherits a questionable annotation

The third overload delegates to the existing Member<TObject, string>(..., Func<..., object>) which already carries [RequiresDynamicCode]. The attribute is therefore redundant here (it's already present on the target). More importantly: should this overload be [Obsolete] to steer users toward the strongly-typed path? If the goal is to push callers to the TTransformed overload, an explicit deprecation notice would help IDEs surface the better alternative — the [RequiresDynamicCode] alone only fires in AOT scenarios.

3. Minor: XML doc comments are verbose for thin wrappers

Each new overload has a multi-sentence <summary> that describes what the overload does rather than why it exists. For thin forwarding methods a single inline comment referencing the root cause is both more honest and easier to keep accurate. Long docs on trivial wrappers tend to drift as the surrounding code evolves.

---

Tests

The regression tests are well-designed. The NonInterned helper correctly defeats the string interning optimisation that would have masked the bug under reference equality, and the second test covering a nested-property case adds useful coverage. The ToCharArray() approach for net472 compat is a sensible workaround.

---

Verdict

The fix is correct and the tests validate it properly. The structural concerns above are minor — the redundant overload and verbose docs won't cause bugs — but trimming them would reduce the surface area callers need to reason about and keep the file consistent with its best existing patterns.

[claude]

Code Review: fix(assertions): prefer string overload of Member() over IEnumerable (#5702)

Summary

This PR fixes a real bug: the IEnumerable<TItem> collection overload on Member() carried [OverloadResolutionPriority(2)], making the compiler prefer TItem=char when the selector pointed at a string property. At runtime this used IEnumerable<char> equality semantics and fired the TUnitAssertions0016 analyzer warning. The fix — adding three string-specific overloads with [OverloadResolutionPriority(3)] — correctly steers the compiler to the scalar path.

The fix is correct and the regression tests are well-written. The points below are design-level suggestions rather than blockers.

---

Issue 1: Second overload may be redundant or ambiguous

The second new overload (Member<TObject> returning Assertion<string>) is a concrete specialisation of the first new overload with TTransformed=string:

// First overload — TTransformed is inferred
public static MemberAssertionResult<TObject> Member<TObject, TTransformed>(
    this IAssertionSource<TObject> source,
    Expression<Func<TObject, string?>> memberSelector,
    Func<IAssertionSource<string>, Assertion<TTransformed>> assertions)

// Second overload — TTransformed fixed to string
public static MemberAssertionResult<TObject> Member<TObject>(
    this IAssertionSource<TObject> source,
    Expression<Func<TObject, string?>> memberSelector,
    Func<IAssertionSource<string>, Assertion<string>> assertions)

When the lambda returns Assertion<string>, the compiler will infer TTransformed=string for the first overload, making both candidates equally applicable — risking CS0121 ambiguity. If it doesn't cause ambiguity, the second overload is simply unreachable dead code. Worth verifying: does the first overload alone handle all cases, or is the second genuinely needed?

---

Issue 2: Task<string> and ValueTask<string> member selectors are still affected

The PR adds string-specific overloads for the synchronous case only. The same IEnumerable<char> preference bug exists for async selectors:

// These still route through the IEnumerable<char> path:
await Assert.That(obj).Member(x => x.GetNameAsync(), n => n.IsEqualTo("hello"));
await Assert.That(obj).Member(x => x.GetNameValueTask(), n => n.IsEqualTo("hello"));

The Task<TMember> and ValueTask<TMember> overloads also carry elevated [OverloadResolutionPriority] values and would have the same problem. Consider whether parallel Task<string> / ValueTask<string> overloads are needed here, or add test coverage to confirm these async paths are unaffected.

---

Issue 3: The approach doesn't scale to other IEnumerable<T> types

string is the most common culprit, but byte[] (which implements IEnumerable<byte>) has the exact same problem today:

// Still routes to CollectionMemberAssertionAdapter, not scalar
await Assert.That(obj).Member(x => x.Blob, b => b.IsEqualTo(expected));

The current approach adds a concrete string-specific escape hatch. A more scalable alternative would be to lower the [OverloadResolutionPriority] on the collection overloads back to 0 and rely on standard generic resolution, or introduce an opt-in marker (e.g., an attribute or wrapper type) to explicitly request collection semantics. The concrete-type overload pattern will need to be repeated for every IEnumerable<T>-implementing type that should be treated as scalar (potentially Memory<T>, ArraySegment<T>, etc.), creating long-term maintenance burden.

If concrete-type overloads remain the chosen pattern, tracking follow-up work for other types like byte[] would help.

---

Issue 4: Test coverage gap — chaining after string member assertion

The new tests in Issue5702Tests.cs cover the runtime behaviour nicely (the NonInterned helper is a particularly good touch for catching reference-equality mistakes). However, a test covering And/Or chaining after a string Member() call would confirm the new overloads' return type still composes correctly:

await Assert.That(obj)
    .Member(m => m.Name, n => n.IsEqualTo("hello"))
    .And.Member(m => m.Value, v => v.IsEqualTo(42));

This is likely fine given the delegation to Member<TObject, string, TTransformed>, but an explicit test would cement the chaining contract.

---

Minor: XML doc comment duplication

All three new overloads repeat the same "This overload takes priority over the collection overload so that string is treated as a scalar value" explanation. A shared <remarks> section or a cross-reference would reduce drift if the explanation needs updating in the future.

---

Verdict

The fix correctly resolves the reported bug and the regression tests are well-constructed. The main open questions worth addressing (either in this PR or as tracked follow-ups) are:

1. Whether the second Member<TObject> overload is truly needed or creates a latent ambiguity.
2. Whether Task<string> / ValueTask<string> selector paths also need patching.
3. Whether the concrete-type disambiguation pattern should be generalised or tracked for other IEnumerable<T>-implementing scalar types.

Good fix overall — the NonInterned test helper in particular ensures real runtime behaviour is tested, not just compilation.

[claude]

Code Review

This is a clean, targeted fix for the string/IEnumerable overload resolution issue. The root cause diagnosis is correct and the approach is consistent with the existing codebase patterns.

What works well

• Correct diagnosis: Using [OverloadResolutionPriority(3)] to outrank the collection overload (priority 2) is exactly the right lever to pull.
• NonInterned helper: The test helper new string(s.ToCharArray()) is a smart way to defeat interning and ensure the test would genuinely fail under reference equality — without it, interned strings would pass even with the broken overload.
• Snapshot coverage: Public API snapshots are updated for all four targets (net8/9/10 and net4.7). The expected absence of [OverloadResolutionPriority] on net8 (where it isn't in the BCL) is handled correctly.
• And-chaining regression test: Member_On_String_Property_Composes_With_And_Chaining is a good addition covering a secondary failure mode.

---

Architectural concern: this fix creates a precedent for type-specific proliferation

string is today's problem because it's the canonical type that implements IEnumerable<T> but is almost always treated as a scalar. But the root cause is structural: the overload resolution priority scheme uses collection-ness as a proxy for "how to treat this type," and that heuristic breaks for any type that wears two hats.

If a user later writes Member(x => x.ArraySegment, s => s.IsEqualTo(expected)) and ArraySegment<T> implements IEnumerable<T>, they hit the same issue. Or a user with a custom type. Each new case would need another set of three hardcoded overloads.

A more future-proof API direction would be to give callers explicit control rather than relying on overload resolution priority ordering — for example:

// explicit intent, no priority games needed
Member(x => x.Tags, c => c.AsCollection().Count().IsEqualTo(3))
Member(x => x.Name, s => s.AsScalar().IsEqualTo("expected"))

// or distinct method names
ScalarMember(x => x.Name, n => n.IsEqualTo("expected"))
CollectionMember(x => x.Tags, tags => tags.Contains("value"))

I understand that's a larger design change and not appropriate for a bug fix. But the comment in the docstring explaining why priority 3 is needed (so that string is treated as a scalar value rather than IEnumerable<char>) is effectively documenting a design debt — worth a // TODO or a tracking issue.

---

Minor: middle overload may be redundant

The second overload:

[OverloadResolutionPriority(3)]
public static MemberAssertionResult<TObject> Member<TObject>(
    this IAssertionSource<TObject> source,
    Expression<Func<TObject, string?>> memberSelector,
    Func<IAssertionSource<string>, Assertion<string>> assertions)
    => Member<TObject, string>(source, memberSelector, assertions);

...is a special case of the first (TTransformed = string). Both have priority 3, so when the assertion lambda returns Assertion<string>, the compiler prefers the non-generic overload as more specific — which means it does affect resolution in practice. However, it's worth noting this exists for disambiguation only, not for functionality. It's consistent with the existing collection overload pattern, so this is a comment not a blocker.

---

Missing test case

There's no test that verifies a genuine IEnumerable<char> (not a string) member still routes to the collection overload after the fix — i.e., that the fix doesn't over-apply. A simple regression:

[Test]
public async Task Member_On_IEnumerableChar_Property_Uses_Collection_Overload()
{
    var model = new WithChars { Chars = new[] { 'h', 'i' } };
    await Assert.That(model).Member(m => m.Chars, c => c.Contains('h'));
}

private sealed class WithChars
{
    public IEnumerable<char> Chars { get; set; } = [];
}

Not a blocker, but adds confidence that priority 3 doesn't shadow the intended collection path for explicit IEnumerable<char> members.

---

Overall: The fix is correct, well-tested, and consistent with the existing codebase conventions. The architectural concern about type-specific proliferation is worth a follow-up tracking issue, but is out of scope for this bug fix PR. Approving.

[claude]

Code Review (Follow-up)

This is a follow-up to the previous reviews. The code has not changed since the last review, so this focuses on whether prior concerns remain outstanding and a final assessment.

---

Prior Concerns — Status

1. Second overload is potentially redundant or unreachable — ⚠️ Still unresolved

The second new overload:

[OverloadResolutionPriority(3)]
public static MemberAssertionResult<TObject> Member<TObject>(
    this IAssertionSource<TObject> source,
    Expression<Func<TObject, string?>> memberSelector,
    Func<IAssertionSource<string>, Assertion<string>> assertions)
    => Member<TObject, string>(source, memberSelector, assertions);

...is a strict specialisation of the first (TTransformed = string). When a caller writes .Member(x => x.Name, n => n.IsEqualTo("expected")), both overloads resolve at priority 3. C# prefers the more specific non-generic candidate, so this overload would win — but it simply delegates to the same underlying call that the first overload would. It does not add a distinct code path.

The existing collection overloads follow this same pattern (there's precedent in the codebase), so this is at least consistent. If the intent is only consistency with the existing parallel collection overloads, that's a valid reason to keep it — but it should be explicit. If no such reason exists, the overload adds surface area with no benefit and is a future maintenance trap.

2. [RequiresDynamicCode] is redundant on the third overload — Minor, still present

The third overload delegates to Member<TObject, string>(..., Func<..., object>) which already carries [RequiresDynamicCode]. Re-annotating the wrapper is harmless but unnecessary. Not a blocker.

3. Scalability / proliferation concern — Valid, but out of scope for this PR

Any other scalar type that also implements IEnumerable<T> (e.g. ArraySegment<T>, custom collections) will hit the same overload resolution problem in the future. The previous reviews suggested either flipping priority ordering or adding explicit AsScalar()/AsCollection() disambiguation. This is a genuine architectural observation, but fixing it is clearly beyond the scope of this targeted bugfix PR.

---

What's Good

• Correct fix: [OverloadResolutionPriority(3)] correctly outranks the collection overload (priority 2) — this is exactly the right lever.
• NonInterned test helper: Using new string(s.ToCharArray()) to defeat interning is clever; without it the tests could pass under the broken behaviour due to reference equality on interned constants.
• Test coverage: Four test cases covering direct, nested, and-chained, and non-string IEnumerable<char> scenarios give good confidence.
• net472 compat fix: The ToCharArray() change in the second commit addresses a real compatibility gap.
• Snapshot updates: All four TFM snapshots are updated consistently.

---

Recommendation

The fix is correct and the regression tests are solid. The one remaining unresolved concern (redundant second overload) has been flagged across multiple reviews. If the intent is to mirror the existing collection overload pattern for consistency, please add a brief comment saying so — that would close out this question. Otherwise, consider removing the second overload.

No blockers — the PR is functionally correct as written.