fix: resolve CS0121 IsEqualTo ambiguity on .NET 8 SDK (#6296)

[thomhurst]

Fixes #6296.

Problem

Consumers building with the .NET 8 SDK (pinned via global.json) get a compile error on a plain same-type equality assertion:

await Assert.That(actual).IsEqualTo(expected); // actual & expected are both Guid
// error CS0121: ambiguous between IsEqualTo<TValue, TOther>(...) and IsEqualTo<TValue>(...)

The cross-type IsEqualTo<TValue, TOther> / IsNotEqualTo<TValue, TOther> overloads (added in #5720 for value-object ergonomics) are equally applicable to the source-generated same-type overloads when the expected value is the same type as the source (Guid vs Guid). They were deprioritized only by [OverloadResolutionPriority].

Root cause (the key finding)

[OverloadResolutionPriority] honoring tracks the Roslyn compiler version that ships with the build SDK, NOT LangVersion. Reproduced with the exact SDK from the issue:

Build SDK	effective LangVersion	result
8.0.422	latest (props imported ✓)	CS0121
10.0.301	12.0	builds

Same project, same package (TUnit.Assertions 1.56.25). The .NET 8 SDK's Roslyn (4.11) simply ignores the attribute, so the LangVersion=latest props bump from #6282 is a no-op for this scenario — rollForward: latestFeature keeps the consumer on the 8.0.4xx SDK band, which never rolls to a 9/10 Roslyn. Any fix must therefore not depend on ORP.

Fix

Give the two cross-type overloads a trailing params object[] parameter, making them applicable only in expanded form. A same-type call then binds to the normal-form same-type overload via the C# "normal beats expanded" tie-break (§12.6.4.2), which every Roslyn version honors — including the .NET 8 SDK's. [CallerArgumentExpression] capture is preserved; ORP is kept as a redundant signal on modern compilers. Empty params compiles to Array.Empty<object>() (no allocation), and this is the value-object path, not a hot path.

Genuine cross-type calls (Assert.That(productCode).IsEqualTo("X")) are unaffected — only the cross-type overload is applicable there.

Validation

• End-to-end: packed the fix and consumed it from a project pinned to SDK 8.0.422. Both Assert.That(guid).IsEqualTo(guid) (was CS0121) and Assert.That(productCode).IsEqualTo("X") now compile and pass at runtime.
• Existing implicit-conversion / overload-resolution suites: 93/93 pass (Issue5720, ImplicitStringOperator, CollectionOverloadResolution).
• Added Issue6296Tests (5 tests; compilation is the assertion).
• PublicAPI snapshots regenerated for all 4 TFMs (the only change is the added params object[] _).

Known follow-up (not in this PR)

The broader Assert.That(...) typed surface (.All/.Count/.ContainsKey, #6276/#6280) is disambiguated the same way (ORP) and is therefore also broken for .NET 8 SDK-pinned consumers. The LangVersion props fix does not address it for the same reason. Worth a separate pass.

[claude]

Code Review

This is a well-analysed fix with excellent documentation. The root-cause diagnosis (ORP is honoured by the compiler version that ships with the build SDK, not by LangVersion) is accurate and the layered fix is the right call. A few observations:

---

The core fix is correct

Using params object[] _ to force the cross-type overload into expanded form is the canonical C# spec tie-break (§12.6.4.2). It is guaranteed to work on every Roslyn version, including 4.11 (SDK 8.x), without depending on [OverloadResolutionPriority]. Keeping ORP as a redundant signal on top is good defence-in-depth.

The fix correctly preserves:

• [CallerArgumentExpression] capture (no regression for error messages)
• Genuine cross-type calls (Assert.That(productCode).IsEqualTo("X")) — the cross-type overload is still the only applicable one there
• Zero-allocation behaviour — callers in the same-type path never reach this overload; callers in the cross-type path pass Array.Empty\<object\>()

---

Public API surface concern (minor, worth acknowledging)

public static EqualsAssertion<TOther> IsEqualTo<TValue, TOther>(
    this IAssertionSource<TValue> source,
    TOther? expected,
    [CallerArgumentExpression(nameof(expected))] string? expectedExpression = null,
    params object[] _)   // <-- now part of the public contract

params object[] _ is a public API contract change (visible in IntelliSense, documented in the snapshot). A caller could technically pass arbitrary extra arguments that are silently discarded. The underscore name is a strong enough signal that this is unintentional, and in practice no one will do it, but it's worth being aware that this leaks the implementation detail into the API surface permanently.

No better alternative exists without sacrificing ergonomics or backward compatibility, so this trade-off is justified — just worth noting for documentation purposes. The file-header comment is already an excellent record of this decision.

---

.Net4_7 snapshot: new overloads appear

The Net4_7 snapshot adds these two overloads for the first time:

+ public static .<TOther> IsEqualTo<TValue, TOther>(..., params object[] _)
+ public static .<TOther> IsNotEqualTo<TValue, TOther>(..., params object[] _)

They were previously absent from the .NET Framework target. Was this intentional (i.e. the cross-type overloads are now surfaced on .NET 4.7 for the first time), or is this an artefact of the [OverloadResolutionPriority] attribute not being available there causing the class to be excluded previously? Either way it should be correct, but worth confirming it's deliberate.

---

Known follow-up issue acknowledged

The PR body correctly identifies that .All/.Count/.ContainsKey (#6276/#6280) have the same ORP-only-disambiguation problem and are still broken for SDK 8.x consumers. Deferring it is a reasonable scope decision; just make sure the issue is tracked so it doesn't get lost.

---

Tests

The 5 regression tests in Issue6296Tests.cs cover the important cases: same-type equality, same-type inequality, different primitive types, and a negative assertion (different values should still fail). The class-level XML doc double-acts as a good regression specification — the kind of comment that actually earns its place.

---

Overall: Correct, well-documented, and appropriately scoped. The design trade-offs are understood and recorded. Approve with the minor notes above.

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 7 complexity

Metric	Results
Complexity	7

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 #6313: fix CS0121 IsEqualTo ambiguity on .NET 8 SDK

This PR fixes a genuine and well-diagnosed bug: [OverloadResolutionPriority] is silently ignored by the .NET 8 SDK's Roslyn compiler regardless of LangVersion, so same-type IsEqualTo calls were ambiguous for consumers pinned to that SDK. The chosen fix — adding params object[] _ to make cross-type overloads applicable only in expanded form — correctly exploits the C# spec's "normal form beats expanded form" tie-break, which every Roslyn version honors. The PR description, file header comments, and snapshot updates are thorough and accurate (the Array.Empty<object>() claim is correct — verified: Roslyn 4.8.x on .NET 8 SDK does emit Array.Empty for empty params T[] call sites).

One confirmed regression, one API design concern, and one observation follow.

---

1. 🔴 Confirmed regression: comparer argument silently swallowed by params object[] _

File: TUnit.Assertions/Extensions/ImplicitConversionEqualityExtensions.cs, line 47

Before this PR, calling the cross-type overload with an explicit third positional argument was a compile error (no matching overload). After it, any extra argument is silently accepted into params object[] _ and discarded. The most realistic way this bites a real user:

// ProductCode has implicit operator string(ProductCode)
await Assert.That(productCode).IsEqualTo("expected-value", StringComparer.OrdinalIgnoreCase);

Overload resolution picks the cross-type overload (it's the only applicable candidate — the same-type-with-comparer overload requires expected to be TValue, not TOther), and StringComparer.OrdinalIgnoreCase lands in _, silently discarded. The test runs without the intended comparer, potentially producing a false positive or false negative with no compile-time warning.

Suggested fix: add an IsEqualTo<TValue, TOther> overload that accepts a comparer explicitly, or add a [Obsolete(error: false)\]"… use the typed overload …" guard. At minimum, the PR should document this footgun in the XML doc — right now the signature appears to accept an arbitrary trailing argument with no indication it's ignored.

---

2. 🟡 params object[] _ is now permanently in the public API

Files: all four Tests.Assertions_Library_Has_No_API_Changes.*.verified.txt snapshots

The snapshots confirm params object[] _ is part of the committed public surface:

public static EqualsAssertion<TOther> IsEqualTo<TValue, TOther>(..., params object[] _)

Once any consumer compiles against this signature — including those who accidentally pass an extra arg (as in finding #1) — removing _ in a future release is a binary-breaking change regardless of whether it's the intent. The PR description calls this a "tie-break", but it's now load-bearing ABI. This is worth an explicit note in the PR or a comment in the code acknowledging that _ cannot be removed without a major version bump. It also means the same approach will have to be applied to the acknowledged follow-up (All/Count/ContainsKey overloads from #6276/#6280), locking in the same pattern across the public API.

---

3. 🟢 Net4_7 snapshot: cross-type overloads are genuinely new on .NET Framework 4.7

File: TUnit.PublicAPI/Tests.Assertions_Library_Has_No_API_Changes.Net4_7.verified.txt

The Net4_7 snapshot adds these overloads from scratch (no prior entries were removed). This is an additive API surface change for any .NET Framework 4.7 consumers, and [OverloadResolutionPriority] is absent from the Net4_7 snapshot (the attribute is stripped for netfx targets). The params tie-break still works there per spec, so this is not a correctness issue — but it's worth calling out that IsEqualTo\<TValue, TOther\> is being newly exposed to .NET Framework consumers as a side-effect of this fix.

---

Overall the root-cause diagnosis, fix mechanism, and documentation are excellent. The comparer-swallowing regression (#1) is the only thing worth addressing before merging — everything else is informational.

[thomhurst]

Addressed the 🔴 finding #1 (comparer silently swallowed by params object[] _) in 01da30a.

Fix: the trailing params element type is now a dedicated CrossTypeOverloadMarker with no accessible constructor instead of object. The empty-expanded-form tie-break that fixes #6296 is unchanged (callers still pass zero trailing args), but because no caller can ever construct a CrossTypeOverloadMarker, a stray trailing argument no longer converts into the params array — it's a compile error again, exactly as before #6296. So:

Assert.That(productCode).IsEqualTo("x", StringComparer.OrdinalIgnoreCase); // CS1503 again, not swallowed

Verified on the .NET 8 SDK's Roslyn (8.0.4xx): same-type tie-break still binds the same-type overload, genuine cross-type calls still compile, and the footgun call is now CS1503: cannot convert from 'System.StringComparer' to '...CrossTypeOverloadMarker'.

Added two regression tests confirming the same-type with-comparer overload still binds and the comparer is actually applied (the case that would have been stolen by object[]). PublicAPI snapshots regenerated for all 4 TFMs.

Re finding #2 (the param is permanent ABI): still true, and now the marker type is too — but it's [EditorBrowsable(Never)] and the trade-off (compile-time safety vs. a hidden public type) seems worth it for a test framework where a silently-ignored comparer = false pass. Re finding #3 (cross-type overloads newly visible on net4.7): unchanged and confirmed intentional.

[claude]

Code Review — Follow-up pass (commit 01da30a)

This is a follow-up review of the final commit, specifically checking whether the two findings from the previous review (on c135ff4) were addressed.

---

Previous findings — status

🔴 Finding #1 (comparer silently swallowed): ✅ Fully addressed.

Replacing params object[] _ with params CrossTypeOverloadMarker[] _ (where CrossTypeOverloadMarker has a private constructor) is the right fix. A stray trailing argument — including an IEqualityComparer — is now a compile error again (CS1503: cannot convert from 'StringComparer' to 'CrossTypeOverloadMarker'), exactly matching pre-#6296 behaviour. The fix also adds two regression tests (String_IsEqualTo_SameType_WithComparer_BindsComparerOverload and the failing-comparer variant) confirming the same-type-with-comparer overload still binds and actually applies the comparer. This is precisely the right level of test coverage for this edge case.

🟡 Finding #2 (params marker is permanent ABI): Acknowledged, accepted trade-off.

The author confirmed in the PR comment: the marker type and the parameter are now permanent ABI, same as params object[] would have been. The CrossTypeOverloadMarker approach is strictly better than object[] for exactly this reason — it makes any stray argument a compile error rather than silently discarding it. The [EditorBrowsable(EditorBrowsableState.Never)] hiding is the right move for a type that must be public but is an implementation detail.

🟢 Finding #3 (Net4_7 snapshot: cross-type overloads newly visible): Still informational. No further action needed.

---

Observations on the final commit

Design of CrossTypeOverloadMarker: Sound. A sealed class with a private constructor and [EditorBrowsable(Never)] is the correct combination — the tie-break works (zero trailing args in expanded form is still valid), no caller can accidentally supply an element, and IntelliSense hides the type from everyday use. The name is clear about intent.

One minor note on test XML doc length: The 27-line XML doc on Issue6296Tests is verbose given that the test names are already self-documenting. But for a subtle compiler-version-dependent regression, having the mechanism spelled out in one place (rather than spread across commit messages) is genuinely useful. This is a judgement call — it earns its length here.

Net4_7 snapshot — still appears intentional: The Net4_7 snapshot now shows CrossTypeOverloadMarker and the new overloads without [OverloadResolutionPriority]. This is correct; the params tie-break works on .NET Framework too (it's a C# spec rule, not an SDK-version feature), so .NET Framework consumers get the same fix as modern targets. No issue.

---

Overall

The previous critical finding is properly resolved. The approach — layering a spec-guaranteed tie-break (params expanded form) on top of ORP as redundant defence — is the correct permanent fix for this class of overload resolution problem. The CrossTypeOverloadMarker refinement avoids the footgun from params object[] while preserving the tie-break semantics. The tests are solid and the public API snapshots are up to date for all four TFMs.

Approve.

[claude]

Code Review — Final Pass (commit 9da71be)

This is a follow-up review of the current PR state, checking prior review findings and the overall design.

---

Prior review findings — all resolved

Three prior rounds of review (commits 8b151c5, c135ff4, 01da30a) raised the following:

🔴 Comparer silently swallowed by params object[] _: ✅ Fully resolved. Replacing params object[] _ with params CrossTypeOverloadMarker[] _ (sealed class, private constructor) is the right approach. A stray trailing argument — including an IEqualityComparer — is now a compile error (CS1503) rather than silently discarded. Two regression tests (String_IsEqualTo_SameType_WithComparer_BindsComparerOverload and String_IsEqualTo_SameType_WithComparer_Fails_When_ComparerRejects) confirm the same-type-with-comparer overload binds and applies the comparer correctly.

🟡 params marker is permanent ABI: Acknowledged and accepted. CrossTypeOverloadMarker is a strictly better choice than params object[] here because callers can never accidentally supply an element — stray trailing args are a compile error instead of being silently discarded and ignored.

🟢 Net4_7 cross-type overloads newly visible: Still informational. The params tie-break is a C# spec rule and works on .NET Framework too. The snapshot is correct.

---

Design of CrossTypeOverloadMarker

The implementation is sound:

• sealed + private constructor: callers can never instantiate it; the only valid call site is params expanded form with zero elements
• [EditorBrowsable(EditorBrowsableState.Never): hides the type from everyday IntelliSense while keeping it public (required because it appears in a public method signature)
• AOT compatibility: no reflection involved; a zero-length array of a sealed type with no members is trivially AOT-safe
• Zero-allocation path: modern Roslyn emits Array.Empty<CrossTypeOverloadMarker>() for empty params T[] call sites — same guarantee holds with the marker type as it did with object[]

The layered fix (spec-guaranteed tie-break via expanded form + redundant [OverloadResolutionPriority(-1)] for modern compilers) is the correct permanent solution. The file header comment thoroughly documents the why behind each layer.

---

Overall

All prior findings are addressed. The fix is correct, compiler-version-independent, and the CrossTypeOverloadMarker refinement is a better design than the original params object[] approach. Public API snapshots are up to date for all four TFMs. Test coverage is appropriate.

Approve.