fix: handle explicit interface impl with different return types in mock generator

[thomhurst]

Summary

Fixes #5559 — mock generator failed to compile when mocking interfaces inheriting from IEnumerable<T> or having same-named members with different return types across multiple interfaces.

• Method/property dedup: Added two-level deduplication — signature keys (name+params) detect collisions, full keys (name+params+return type) distinguish genuinely different methods that need explicit interface implementation
• Explicit impl delegation: Added ExplicitInterfaceCanDelegate flag — interface return types (e.g. IEnumerator<T> → IEnumerator) delegate safely; incompatible types (int vs string) get their own engine dispatch
• Sealed override leak: Sealed overrides now register in seen sets to block base virtuals from generating invalid override

Test plan

• 3 snapshot tests for IEnumerable interface variants (generic, direct, nested generic)
• 8 behavioral tests for IEnumerable mocking (EnumerableInterfaceTests)
• 22 kitchen sink interface tests — void/return/async/overloaded/generic/ref/out/in/nullable/properties/events/explicit interface with colliding return types
• 29 kitchen sink abstract class tests — abstract/virtual/non-virtual/protected/override/constructor params
• 25 kitchen sink concrete class tests — override/new/sealed/virtual/non-virtual methods and properties
• 28 kitchen sink inheritance tests — 3-level hierarchy (L0→L1→L2), sealed overrides, new hiding, constructor forwarding
• All 888 behavioral tests pass
• All 164 snapshot tests pass across net8.0/net9.0/net10.0/net472
• All 41 HTTP mock tests pass (Azure SDK types exercise class hierarchy dedup)

[azure-pipelines]

Azure Pipelines:
1 pipeline(s) were filtered out due to trigger conditions.

[codacy-production]

Not up to standards ⛔

🔴 Issues 4 medium · 10 minor

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

Results:
14 new issues

Category	Results
BestPractice	4 medium 2 minor
CodeStyle	8 minor

View in Codacy

🟢 Metrics 155 complexity

Metric	Results
Complexity	155

View in Codacy

_{TIP This summary will be updated as you push new changes. Give us feedback}

[claude]

Code Review

The fix is well-reasoned and the test coverage is excellent — 888 behavioral tests across interfaces, abstract classes, concrete classes, and multi-level inheritance, plus 3 snapshot tests for the IEnumerable variants. The two-level deduplication design is sound for the target use case.

Issue 1: IsReturnTypeConvertible heuristic can produce broken generated code

MemberDiscovery.cs — IsReturnTypeConvertible:

private static bool IsReturnTypeConvertible(ITypeSymbol explicitReturnType)
{
    return explicitReturnType.TypeKind == TypeKind.Interface;
}

The comment says "likely implements it" but likely is doing a lot of work here. Consider:

interface IAlpha { IFoo Method(); }
interface IBeta  { IBar Method(); }  // IFoo and IBar are unrelated interfaces

interface IMyService : IAlpha, IBeta { }

When IBar.Method() (the explicit impl) is encountered, IsReturnTypeConvertible returns true because IBar is an interface. The generator emits:

IBar IBeta.Method() => Method();  // Method() returns IFoo — compile error!

The symptom is a C# compile error in generated code (CS0266: Cannot implicitly convert type 'IFoo' to 'IBar'), which is arguably worse than a runtime exception — it silently breaks mocking for any user who creates such an interface pair.

Better approach: At the point of collision detection, both ITypeSymbols are available. Use Roslyn's type hierarchy directly:

// In the collision branch, after retrieving existingIdx:
var existingMethodReturnType = /* ITypeSymbol of the already-seen method's return type */;
var canDelegate = IsConvertible(existingMethodReturnType, method.ReturnType);

private static bool IsConvertible(ITypeSymbol publicReturnType, ITypeSymbol explicitReturnType)
{
    // The public method returns publicReturnType; the explicit impl must return explicitReturnType.
    // Delegation is safe only if publicReturnType is assignable to explicitReturnType.
    if (explicitReturnType.TypeKind != TypeKind.Interface) return false;
    if (SymbolEqualityComparer.Default.Equals(publicReturnType, explicitReturnType)) return true;
    return publicReturnType.AllInterfaces.Any(i =>
        SymbolEqualityComparer.Default.Equals(i.OriginalDefinition, explicitReturnType.OriginalDefinition));
}

To make this practical you'd need to store the ITypeSymbol alongside existingIdx in the dictionary (e.g., Dictionary<string, (int index, ITypeSymbol returnType)>), or just keep existingIdx and look it up through a parallel List<ITypeSymbol>. This properly handles the IEnumerator/IEnumerator case while safely falling back to engine dispatch for unrelated interface types.

---

Issue 2: existingIdx is captured but never used in the interface loop

MemberDiscovery.cs lines ~68, ~201:

if (seenMethods.TryGetValue(key, out var existingIdx))
{
    // existingIdx is never referenced below
    var fullKey = GetFullMethodKey(method);
    ...
}

The variable exists to enable Issue 1's fix (accessing the existing method's return type symbol) but currently dead code. Either use it for the proper assignability check, or rename to _ to signal intentional discard. As-is it looks like unfinished work.

---

Issue 3: Misleading comment on explicit interface property generation

MockImplBuilder.cs — GenerateInterfaceProperty:

if (prop.ExplicitInterfaceName is not null)
{
    // Explicit interface implementation — delegates to the public property
    // with the same name (different return type from a different interface).
    writer.AppendLine($"{prop.ReturnType} {prop.ExplicitInterfaceName}.{prop.Name}");
    writer.OpenBrace();
    if (prop.HasGetter)
    {
        writer.AppendLine($"get => _engine.HandleCallWithReturn<{prop.ReturnType}>({prop.MemberId}, ...");

The comment says "delegates to the public property" but the code dispatches independently through the engine with a dedicated MemberId — it does not call the public property accessor. The behavior is actually correct (explicit interface properties with different return types must have independent setup/verify), but the comment will mislead future maintainers. Consider:

// Explicit interface property with different return type.
// Dispatches independently through the engine (not a delegation — return types are incompatible).

---

Issue 4: Duplicated two-level dedup logic between DiscoverMembers and DiscoverMembersFromMultipleTypes

The identical collision-detection block appears twice:

// In DiscoverMembers:
if (seenMethods.TryGetValue(key, out var existingIdx))
{
    var fullKey = GetFullMethodKey(method);
    if (!seenFullMethods.Add(fullKey)) continue;
    var canDelegate = IsReturnTypeConvertible(method.ReturnType);
    methods.Add(CreateMethodModel(method, ref memberIdCounter, interfaceFqn, interfaceFqn, canDelegate));
}

// Exact same block in DiscoverMembersFromMultipleTypes

Similarly for the property collision check. Each bugfix or enhancement (like fixing Issue 1) will need to be applied in two places. Extracting a shared helper — e.g., ProcessInterfaceMember(IMethodSymbol, ...) — would make both methods thin coordinators and keep the dedup policy in one place. This is the higher-priority refactor given the heuristic is likely to evolve.

---

Minor: CollectStaticAbstractMember doesn't receive seenFullMethods

If a static abstract method had the same name+params but a different return type across two interfaces (unusual but theoretically possible), the full-key dedup wouldn't run. Low-priority since static abstract methods with covariant return types across interfaces are unlikely in practice, but worth noting for completeness.

---

Overall

The core design — two-level deduplication with delegation vs. engine-dispatch fallback — is the right approach for this problem. The sealed-override fix is also correct and addresses a real gap. The main concern is that the IsReturnTypeConvertible heuristic is a correctness time bomb for users with unrelated interfaces that happen to share a method name: it will silently generate uncompilable mock code. The Roslyn APIs needed for a proper check are available at the point where the decision is made, so this seems worth the small additional complexity before merging.

[azure-pipelines]

Azure Pipelines:
1 pipeline(s) were filtered out due to trigger conditions.

[claude]

Follow-up Review

All four issues flagged in the previous review have been addressed in the second commit. Here's the audit:

Issues from Previous Review — All Resolved ✅

Issue 1 — IsReturnTypeConvertible heuristic: Fixed. Replaced with CanDelegateReturnType(ITypeSymbol publicReturnType, ITypeSymbol explicitReturnType) using proper Roslyn AllInterfaces traversal. The fix correctly handles the IAlpha/IBeta scenario (unrelated interfaces sharing a method name with different return types) that would have produced CS0266 compile errors in generated code.

Issue 2 — Dead existingIdx variable: Fixed. The seenMethods dictionary now stores (int Index, ITypeSymbol? ReturnType) and existing.ReturnType is actively passed to CanDelegateReturnType, making the delegation decision semantically correct.

Issue 3 — Misleading comment on GenerateInterfaceProperty: Fixed. The comment now reads "Dispatches independently through the engine with a dedicated MemberId", which accurately describes the behavior.

Sealed-override registration: The NonMockableEntry = (-1, null) sentinel for seenMethods is a clean way to block base virtuals without allocating a dedicated data structure. Good.

---

Remaining Minor Points (non-blocking)

Duplicated collision-detection logic — the same two-level dedup block (TryGetValue → seenFullMethods.Add → CanDelegateReturnType → methods.Add) appears in both DiscoverMembers and DiscoverMembersFromMultipleTypes, and the property collision check similarly. Any future evolution to CanDelegateReturnType would need to be applied in both places. An AddInterfaceMethod helper that owns the entire collision policy would prevent drift, though this is a refactor, not a correctness issue.

CollectStaticAbstractMember doesn't receive seenFullMethods — the overloaded-static-abstract edge case (same name+params, different return type across two static-abstract interfaces) would fall through to the old single-level dedup. Extremely unlikely in practice, but worth a // TODO: extend seenFullMethods to CollectStaticAbstractMember if static covariant return types become relevant comment if you want to leave a breadcrumb.

---

Overall

The fix is correct and safe. The critical correctness hazard (heuristic delegation emitting uncompilable code for unrelated interface pairs) has been eliminated. Test coverage is thorough — 8 IEnumerable behavioral tests, 3 snapshot tests, and 112 kitchen-sink tests across interfaces/abstract/concrete/inheritance. Good to merge.

[claude]

Follow-up Review

All four issues flagged in the previous review have been addressed in the second commit. Here is the audit:

Issues from Previous Review — All Resolved

Issue 1 — IsReturnTypeConvertible heuristic: Fixed. Replaced with CanDelegateReturnType(ITypeSymbol publicReturnType, ITypeSymbol explicitReturnType) using proper Roslyn AllInterfaces traversal. The fix correctly handles the IAlpha/IBeta scenario (unrelated interfaces sharing a method name with different return types) that would have produced CS0266 compile errors in generated code.

Issue 2 — Dead existingIdx variable: Fixed. The seenMethods dictionary now stores (int Index, ITypeSymbol? ReturnType) and existing.ReturnType is actively passed to CanDelegateReturnType, making the delegation decision semantically correct.

Issue 3 — Misleading comment on GenerateInterfaceProperty: Fixed. The comment now reads "Dispatches independently through the engine with a dedicated MemberId", which accurately describes the behavior.

Sealed-override registration: The NonMockableEntry = (-1, null) sentinel for seenMethods is a clean way to block base virtuals without allocating a dedicated data structure. Good call.

---

Remaining Minor Points (non-blocking)

Duplicated collision-detection logic — the same two-level dedup block (TryGetValue + seenFullMethods.Add + CanDelegateReturnType + methods.Add) appears in both DiscoverMembers and DiscoverMembersFromMultipleTypes, and the property collision check is similarly duplicated. Any future evolution to CanDelegateReturnType would need to be applied in both places. An AddInterfaceMethod helper that owns the entire collision policy would prevent drift. Non-blocking refactor.

CollectStaticAbstractMember does not receive seenFullMethods — the overloaded-static-abstract edge case (same name+params, different return type across two static-abstract interfaces) would fall through to single-level dedup. Extremely unlikely in practice, but worth a comment as a breadcrumb.

---

Overall

The fix is correct and safe. The critical correctness hazard — the heuristic delegation that would emit uncompilable code for unrelated interface pairs — has been eliminated. Test coverage is thorough: 8 IEnumerable behavioral tests, 3 snapshot tests, and 112 kitchen-sink tests across interfaces/abstract/concrete/inheritance hierarchies. Good to merge.