fix(mocks): disambiguate IEquatable<T>.Equals from object.Equals (#5675)

[thomhurst]

Summary

• Renames colliding setup-extension members (Equals, GetHashCode, ToString, GetType) to EqualsOf/GetHashCodeOf/ToStringOf/GetTypeOf so they are no longer shadowed by object's instance methods on Mock<T>.
• Restores the SKIPPED T8_Self_Referential_IEquatable_Mockable kitchen-sink test, covering setup, direct invocation, interface-cast invocation, and Was(Never)Called tracking.

Breaking change

Yes — minor. Prior to this PR, the four object-method names were already in MockMemberNames and emitted with a trailing underscore (Equals_, GetHashCode_, ToString_, GetType_). Those underscore-suffixed names are NOT shadowed by object (different identifier), so they were technically reachable at the call site. Anyone relying on mock.Equals_(...) / mock.GetHashCode_(...) / mock.ToString_(...) / mock.GetType_(...) will need to migrate to the new *Of names. The expected blast radius is small (the underscore form is undocumented and the only realistic use case — Equals — was the broken one users hit and reported as #5675), but flagging it explicitly here.

The Object -> Object_ rename (Mock.Object property collision) is unchanged.

Why

Mock<T> inherits object.Equals(object?) as an instance method. C# overload resolution always prefers an instance method over an extension method, so mock.Equals(other).Returns(...) bound to bool object.Equals(object?) and produced CS1061. Renaming at the emission site is the only fix that works uniformly for both interface and class mocks (class mocks go through BuildForPartialMock and return runtime Mock<T> from another assembly, so a typed-instance-method approach is not viable).

Test plan

• T8 (TUnit.Mocks.Tests/KitchenSinkEdgeCasesTests.cs) passes — now also covers GetHashCodeOf and ToStringOf (review feedback)
• TUnit.Mocks.Tests full suite (net10.0): 953/953
• TUnit.Mocks.SourceGenerator.Tests (net10.0): 45/45

Closes #5675

[claude]

Code Review

This PR correctly diagnoses and fixes a real C# overload resolution limitation: instance methods on object always beat extension methods, so the generator-emitted Equals(...) extension was unreachable on Mock<T>. The EqualsOf/GetHashCodeOf/etc. disambiguation approach is pragmatic and the explanatory comments are clear. The restored T8 test is well-structured, covering setup, direct call, interface-cast, and verification tracking.

A few things worth calling out:

---

1. Breaking change from _-suffix to Of-suffix (most significant concern)

The old MockMemberNames set would have generated Equals_, GetHashCode_, ToString_, GetType_. This PR renames those to EqualsOf, GetHashCodeOf, ToStringOf, GetTypeOf. Any existing user who was calling mock.Equals_(...) or mock.GetHashCode_() as a setup method will get a compile error after upgrading.

This is a breaking API change and should be explicitly noted in the changelog/release notes. Was the _-suffix approach actually usable before (i.e. did users ever reach those methods), or was it always broken due to the same overload-resolution problem? If the latter, documenting this as a bugfix rather than a breaking change would be accurate — but worth being explicit about.

---

2. Naming inconsistency: Object_ vs EqualsOf

MockMemberNames still uses the _-suffix convention for "Object" (→ Object_), while the four object method collisions now use the Of-suffix convention (→ EqualsOf etc.). These two strategies coexist in the same function without explanation.

The two cases are logically different (Object clashes with the Mock<T>.Object property, not an inherited object method), but a future maintainer reading GetSafeMemberName will see two divergent strategies without a clear reason why. A comment distinguishing the two collision kinds would help, or unifying on one strategy if feasible.

---

3. Only EqualsOf is tested — GetHashCodeOf, ToStringOf, GetTypeOf are not

Since GetHashCode, ToString, and GetType are also moved from MockMemberNames to ObjectMemberDisambiguations (changing their generated names), they should each have at least a minimal test. Right now there's no coverage that mock.GetHashCodeOf() is reachable and routes correctly, which matters if/when someone overrides GetHashCode in a mockable type.

---

4. Alternative design worth noting for the future

The core tension here (extension methods losing to instance methods on object) is a recurring C# limitation. A longer-term alternative worth considering is a mock.Setup(x => x.Equals(other)).Returns(true) expression-tree approach, which sidesteps overload resolution entirely because the lambda captures the method symbolically. This would be a larger architectural investment, but it would eliminate the entire class of EqualsOf/GetTypeOf-style API awkwardness. Not blocking for this PR, but worth a ticket.

---

Summary

The fix is correct and the test is well-written. The main actionable items are:

• Document the Equals_ → EqualsOf rename as a breaking change (or clarify that the old API was always broken/unreachable)
• Add a comment in GetSafeMemberName explaining why Object uses _-suffix while object methods use Of-suffix
• Add tests for GetHashCodeOf, ToStringOf, GetTypeOf to match the coverage for EqualsOf

[codacy-production]

Not up to standards ⛔

🔴 Issues 3 minor

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

Results:
3 new issues

Category	Results
BestPractice	1 minor
CodeStyle	2 minor

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.}

[thomhurst]

Review feedback addressed in 6e68366:

Applied

1. Naming-strategy comment (item 1) — expanded comments above MockMemberNames and ObjectMemberDisambiguations to explicitly contrast the two distinct shadowing problems: Object is a property/method member-kind collision (underscore suffices), whereas Equals/GetHashCode/ToString/GetType are inherited object instance-method collisions that overload resolution would still hide regardless of suffix style.
2. Test coverage (item 2) — extended T8 to also cover GetHashCodeOf and ToStringOf end-to-end (setup -> call -> verify). GetTypeOf is documented inline as not exercisable: object.GetType is non-virtual, so no derived type can override it and the helper is unreachable in practice. Added an override string ToString() to SelfEquatable so the typed ToStringOf setup is generated.
3. Breaking-change determination (item 3) — researched the prior emission via git show 932d2538c -- ...MockMembersBuilder.cs. Before this PR, MockMemberNames already contained Equals/GetHashCode/ToString/GetType and emitted them as Equals_/GetHashCode_/ToString_/GetType_. Underscore-suffixed names are NOT shadowed by object (different identifier), so they WERE reachable. The rename to *Of is therefore a (minor) breaking change for anyone using the undocumented underscore form. Noted explicitly in the updated PR body's new "Breaking change" section.

Deferred / not applicable

• Codacy issues (item 4) — ran dotnet format --verify-no-changes against both TUnit.Mocks.SourceGenerator and TUnit.Mocks.Tests. The whitespace/import warnings it surfaced are all in pre-existing files and pre-existing lines (none on the lines this PR touches in MockMembersBuilder.cs or in the T8 block). Per the review's explicit "changed files only" scoping I left those alone to avoid muddying the diff. The 3 Codacy findings (1 BestPractice + 2 CodeStyle) likely reflect Codacy's own analyzer ruleset rather than dotnet format rules and have no detail in the GitHub check — viewing the Codacy dashboard at https://app.codacy.com/gh/thomhurst/TUnit/pull-requests/5680/issues is required to see them.
• Suggestion And assertion conditions #4 (expression-tree Setup(x => x.Equals(...)) design) — out of scope per the reviewer's own framing; tracked for a future ticket.

Test status: T8 passes; full TUnit.Mocks.Tests net10.0 = 953/953; TUnit.Mocks.SourceGenerator.Tests net10.0 = 45/45.

[claude]

Follow-up Review

The second commit ("chore(mocks): address review feedback") directly addressed all three actionable items from the previous review. Here's the status:

---

Previous feedback — resolved

1. Breaking change documentation ✅
The PR description now explicitly calls out the Equals_ → EqualsOf rename, explains the blast radius (small — the _-suffix form was already functionally broken for the overload-resolution reason), and flags it as a breaking change.

2. Naming inconsistency (Object_ vs EqualsOf) ✅
The code now has clear two-block comments at lines 19–41 of MockMembersBuilder.cs distinguishing the two collision kinds (property collision vs. inherited instance method collision) and why they get different fixes. A future maintainer will understand GetSafeMemberName immediately.

3. Missing test coverage for GetHashCodeOf / ToStringOf ✅
T8 now tests setup, invocation, and WasCalled verification for both GetHashCodeOf and ToStringOf, alongside EqualsOf. The inline comment explaining why GetTypeOf is excluded (GetType is not virtual on object) is clear and sufficient.

---

New observation: no snapshot coverage for the renamed extension names

None of the 44 .verified.txt snapshot files reference EqualsOf, GetHashCodeOf, ToStringOf, or GetTypeOf — and none of the existing snapshot test types happen to override these object methods. That means the generated extension names for these members are only exercised at runtime (via T8), not at the source-generator snapshot layer.

If the generator produces a regression (e.g. falls back to Equals_ instead of EqualsOf due to a future refactor), the snapshot tests won't catch it — only T8 will. Since CLAUDE.md flags snapshot testing as required for source-generator output changes, a snapshot test for SelfEquatable (or a minimal IEquatable<T> interface) would provide earlier, cheaper detection at the code-gen layer. This isn't blocking but is worth a follow-up ticket or a snapshot added in this PR.

---

Minor: GetTypeOf dead API kept for symmetry

The GetTypeOf entry in ObjectMemberDisambiguations generates an extension method that can never be meaningfully used (since GetType is non-virtual and can't be overridden). The class-level comment documents this clearly, so it won't surprise a reader. This is fine as-is, but if the mocks team ever revisits the API surface, removing it would reduce noise in generated code and IntelliSense.

---

Summary

The fix is correct, well-explained, and the test is comprehensive. The only gap worth tracking is the missing snapshot coverage for the new *Of extension names, so regressions at the code-gen layer are detected without running the full runtime suite. Overall this is good work — the two-commit structure made it easy to track what changed in response to review.

[thomhurst]

Round-2 review feedback applied in f4a63be:

1. Snapshot coverage for *Of helpers — applied.
Added SelfEquatable_Generates_EqualsOf_GetHashCodeOf_ToStringOf snapshot test in TUnit.Mocks.SourceGenerator.Tests using a SelfEquatableSnapshot type that implements IEquatable<T> and overrides GetHashCode/ToString. The new .verified.txt records the generated EqualsOf (4 overloads — Arg+Func, both signatures), GetHashCodeOf, and ToStringOf extensions. The runtime T8 test no longer carries the burden alone; future regressions will surface as snapshot diffs.

2. Drop unreachable GetTypeOf — applied.
Removed the { "GetType", "GetTypeOf" } entry from ObjectMemberDisambiguations in TUnit.Mocks.SourceGenerator/Builders/MockMembersBuilder.cs. object.GetType is non-virtual so the generated helper could never be useful. Updated the class-level comment to drop the GetType reference and added a sentence explaining the omission. Also tightened the related comment block in KitchenSinkEdgeCasesTests.cs (T8) so it no longer claims GetTypeOf is generated.

Verification:

• TUnit.Mocks.SourceGenerator.Tests (net10.0): 46/46 pass — confirms no other snapshot referenced GetTypeOf.
• TUnit.Mocks.Tests (net10.0): 953/953 pass — including T8.
• No .received.txt files left behind.

[claude]

Follow-up Review (Round 3)

The third commit (f4a63be — "chore(mocks): add *Of snapshot test, drop unreachable GetTypeOf") addresses both remaining items from the previous round-2 review. All prior feedback is now resolved.

---

Previous feedback — fully resolved

1. Snapshot coverage for *Of extension names ✅
SelfEquatable_Generates_EqualsOf_GetHashCodeOf_ToStringOf.verified.txt is now committed and the corresponding MockGeneratorTests.SelfEquatable_Generates_EqualsOf_GetHashCodeOf_ToStringOf test exercises the snapshot path. If a future refactor accidentally reverts EqualsOf → Equals_, the source-gen snapshot catches it without needing to run the full runtime suite.

2. Dead GetTypeOf entry removed ✅
GetType → GetTypeOf has been dropped from ObjectMemberDisambiguations. Since GetType is non-virtual on object it cannot be overridden, so the generated extension helper would have been dead code and IntelliSense noise. The comment in MockMembersBuilder.cs documents the reasoning clearly.

---

Remaining observations (non-blocking)

These are minor enough that they do not warrant another revision — just noting them for the record.

Snapshot type name vs. runtime type name
The snapshot test uses a class called SelfEquatableSnapshot (to avoid a name collision in the test compilation context), while the runtime test uses SelfEquatable. They test the same code path and both are clearly named — just worth knowing if either name is ever searched for in the future.

object.Equals(object?) override intercept path
The generated implementation overrides both bool Equals(SelfEquatable? other) and bool Equals(object? obj), with the latter assigned member-id 1. There is currently no test or snapshot that exercises mock.EqualsOf(Arg.Any<object?>()) (the object-parameter overload). This is a fairly obscure path and the infrastructure is clearly correct given how member-id 0 works, so this is an optional follow-up rather than a blocking concern.

---

Summary

This PR is in good shape. All three rounds of review feedback have been addressed, the fix is correct, the test coverage is solid at both the source-gen (snapshot) and runtime layers, and the code comments explain the two distinct shadowing mechanisms clearly. Ready to merge.