fix(mocks): implement new-hidden base interface members in wrapper (#6252)

[thomhurst]

Fixes #6252

Problem

Mocking an interface that hides a base member with new fails to compile:

public interface IBase { void SomeMethod(); }
public interface IDerived : IBase { new void SomeMethod(); }

var mock = IDerived.Mock(); // error CS0535: 'IDerivedMock' does not implement 'IBase.SomeMethod()'

Root cause

IDerived.SomeMethod() and IBase.SomeMethod() are two distinct interface slots.

• The backing impl IDerivedMockImpl implements them implicitly with one public void SomeMethod() — a single implicit method satisfies both slots, so it compiled fine.
• The generated wrapper IDerivedMock : Mock<IDerived>, IDerived forwards every member as an explicit interface impl (void IDerived.SomeMethod() => Object.SomeMethod();). An explicit impl satisfies only the slot it names. Member discovery deduped the base slot away, so the wrapper never emitted void IBase.SomeMethod() → CS0535.

The same flaw affected properties, indexers, and events.

Fix

When discovery drops an identically-signed member from a different interface (a new-hidden base member, or one inherited from multiple interfaces) in the single-interface path, record that interface on the surviving member via a new AdditionalExplicitInterfaceNames field. The wrapper then emits one extra explicit forward per recorded slot, cast to that interface (((IBase)Object).SomeMethod()) so the correct slot is hit — which also keeps Mock.Wrap of a real object with differing explicit impls correct.

The comprehensive tests surfaced a second defect in the diamond case (IC : IA, IB where both declare void Go()): the primary forward Object.Go() is ambiguous (CS0121) because Object's static type inherits Go() from two interfaces. So the primary forward is now also cast to its own slot whenever the member spans multiple slots.

Only the wrapper consumes the new field (single-interface mocks); the impl class and setup extensions are unchanged, so the dense member-ID invariant holds and snapshot output is byte-identical for the common single-slot case.

Testing

• Issue6252Tests (runtime, 10 tests): void method, method with args+return, get-only property, read/write property, indexer, event (subscribe + raise through both slots), three-level hiding, wrapper assignability, and the diamond case — each verifying dispatch through both interface slots.
• 2 generator-level regression tests in MockGeneratorTests: lock the generated wrapper shape (both forwards + cast targets) and assert CS0535/CS0121 are gone.
• Full TUnit.Mocks.Tests: 1116 passed (net10.0); Issue6252 subset 10 passed on net8.0 (cross-TFM).
• Full TUnit.Mocks.SourceGenerator.Tests: 71 passed (69 existing snapshots unchanged + 2 new).

Known remaining gap (pre-existing, not a regression)

Asymmetric-accessor new hiding (base {get;set;}, derived new {get;}) still fails to build (CS0550) — it was already broken before this PR and needs per-slot accessor tracking to fix. Out of scope here.

[codacy-production]

Not up to standards ⛔

🔴 Issues 1 high · 5 minor

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

Results:
6 new issues

Category	Results
ErrorProne	1 high
CodeStyle	5 minor

View in Codacy

🟢 Metrics 29 complexity

Metric	Results
Complexity	29

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 — fix/6252-new-hidden-interface-members

Good fix. The root cause analysis in the PR description is accurate and precise, the approach is well-scoped, and the changes are minimal for the correctness impact they deliver.

What's good

• Correct mental model. The PR correctly identifies that an explicit impl satisfies exactly one interface slot, and that deduping on signature alone misses distinct slots created by new-hiding. The cast approach (((IBase)Object).SomeMethod()) is the right tool to dispatch to the correct slot on the underlying object.
• Additive data model. AdditionalExplicitInterfaceNames is a pure extension: empty by default, consumed only by the wrapper builder, invisible to the impl and setup generators. The "snapshot output is byte-identical for the non-shadowing case" claim in the PR description is the right invariant to hold.
• Dedup guards. Both RecordAdditionalWrapperInterface and its event counterpart guard against recording the member's own slot or a slot already registered — correct.
• Refactoring. Extracting EmitMethodForward, EmitPropertyForward, EmitIndexerForward, and EmitEventForward removes the previous copy-and-modify pattern cleanly.

---

Issues

Missing test for event forwarding through both slots

Issue6252Tests.cs defines IShadowDerived.Tick (which hides IShadowBase.Tick with new) but never asserts that dispatching through ((IShadowBase)mock) reaches the mocked event. Methods and properties each have a dedicated test exercising both slots; events are only checked at compile time (the first test). A runtime test would look like:

[Test]
public async Task Wrapper_Forwards_Hidden_Event_Through_Both_Interface_Slots()
{
    var mock = IShadowDerived.Mock();
    int calls = 0;
    mock.Tick.Invocations.Add(() => calls++);  // or however event mocking works

    ((IShadowDerived)mock).Tick += () => { };
    ((IShadowBase)mock).Tick += () => { };

    // Verify both slots subscribed without throwing
    await Task.CompletedTask;
}

(Exact API may differ — but the coverage gap is real.)

Asymmetric event lookup is a pre-existing debt surfaced by this PR

RecordAdditionalWrapperInterface for methods and properties takes a direct index (fast, O(1)). The event counterpart RecordAdditionalWrapperInterfaceForEvent does a linear scan by name because the event seen-set (SeenEvents) is a HashSet<string> that throws away the index on add.

This is pre-existing — the PR correctly works around it — but since you're already touching this code it may be worth noting as tech debt. Storing `(index, ...)" in the event seen-set (matching the method pattern) would make future changes to event handling symmetric and O(1). Not a blocker, but worth a TODO comment or an issue.

Property dedup unconditionally calls RecordAdditionalWrapperInterface

At MemberDiscovery.cs:271 (inside the else if (primaryClassSymbol is null) branch), RecordAdditionalWrapperInterface is called without checking whether the existing property is genuinely a distinct interface slot vs. a same-slot dedup. For methods, the guard is existing.Index >= 0 (filters out NonMockableEntry). Properties don't have that check because the non-mockable sentinel pattern doesn't apply to properties in the same way, but it's worth confirming the call is safe for all paths that reach line 271. Looking at the surrounding code it appears correct (you only reach it via existingIndex.HasValue), but an explicit comment would help future readers.

---

Minor

The comment on the CastTarget helper (line 191) is good — keep it. The multi-paragraph XML doc on AdditionalExplicitInterfaceNames in both models is unusually long for an internal field, but it documents a subtle invariant correctly, so no objection.

---

Summary: Correct fix, well-tested for methods and properties. The main gap is an event regression test. The event lookup asymmetry is pre-existing debt worth tracking.

[claude]

Review

Previous Review Follow-up

All three items from the prior review have been addressed:

1. Event runtime test — Wrapper_Forwards_Hidden_Event_Through_Both_Slots is now present in Issue6252Tests.cs and explicitly asserts derivedFired == 1 and baseFired == 1 after RaiseTick(). Concern closed.
2. Property dedup comment — The MergePropertyAccessors + RecordAdditionalWrapperInterface pairing in MemberDiscovery.cs now has a comment explaining the "distinct slot" rationale. Concern closed.
3. O(n) event scan — Still O(n); acknowledged as pre-existing debt and out of scope. Acceptable.

---

New Observations

Three-level hiding is handled correctly. Walking ILevel3 → ILevel2 → ILevel1 visits Run() three times; the first adds it, the second and third each call RecordAdditionalWrapperInterface, so the builder ends up emitting three explicit forwards. The runtime test confirms the dispatch count.

Incremental generator cache correctness. AdditionalExplicitInterfaceNames is included in both Equals and GetHashCode on MockMemberModel and MockEventModel. This is important: the incremental pipeline will correctly invalidate cached output when slot lists change. Good.

Event primary-target asymmetry is intentional and preserved. Methods/properties cast on ExplicitInterfaceName is not null, events have always used raw Object. The new code keeps events on Object unless AdditionalExplicitInterfaceNames.Length > 0, matching the stated intention in the inline comment. The known asymmetry (events not casting on ExplicitInterfaceName) is pre-existing and not introduced here.

One minor observation: RecordAdditionalWrapperInterface calls AsImmutableArray() then .Add() per recorded slot, which copies the array each time. For the typical 2–3 slot case this is inconsequential, but it's worth noting that repeated calls (e.g. three-level hiding) do O(n²) copies. Not a problem in practice given the scale, but a future ImmutableArray.CreateBuilder or batch-record path could eliminate this if it ever shows up in profiling.

The AssertNoGeneratedError helper iterates diagnostics and throws on the first matching error ID. This is correct, but it's worth noting that if multiple distinct error IDs should be absent, callers must call it multiple times. Not a bug — just a design note if the helper is extended later.

---

Summary

This is a solid, well-scoped fix. Root cause analysis is accurate, the approach (recording additional slots rather than changing the dedup structure) is minimal-change and correct, test coverage is comprehensive across all member kinds and multiple hiding depths, and the previous review concerns have all been addressed. The known gap (asymmetric-accessor new hiding) is clearly documented. Approved.