fix: resolve CS8920 when mocking interfaces whose members return static-abstract interfaces

[lucaxchaves]

Description

Continues the fix from #5070 for issue #5069 — CS8920 compiler errors when mocking AWS SDK interfaces (e.g. IAmazonDynamoDB) that inherit from IAmazonService, which declares:

static abstract IAmazonService CreateDefaultServiceClient(AWSCredentials awsCredentials, ClientConfig clientConfig);

Because IAmazonService itself has static abstract members, using it as a generic type argument triggers CS8920. The mock source generator was emitting MethodSetupBuilder<IAmazonService> and HandleCallWithReturn<IAmazonService>, causing every project that mocks such interfaces to fail to build.

Expected Behavior

Mocking an interface that contains methods returning an interface with static abstract members should compile without CS8920 errors. Setup and verification should work for those members via VoidMockMethodCall (the same pattern already used for ref struct return types).

Actual Behavior

error CS8920: The interface 'Amazon.Runtime.IAmazonService' cannot be used as type argument.
Static member 'IAmazonService.CreateDefaultServiceClient(...)' does not have a most specific implementation in the interface.

Steps to Reproduce

public interface IAmazonService
{
    static abstract IAmazonService CreateDefaultServiceClient(
        AWSCredentials awsCredentials, ClientConfig clientConfig);
}

public interface IAmazonDynamoDB : IAmazonService { /* ... */ }

[assembly: TUnit.Mocks.GenerateMock(typeof(IAmazonDynamoDB))]
// → build fails with CS8920

Root Cause & Fix

The generator tracked IsStaticAbstract on members but had no concept of whether a member's return type is itself an interface with static abstract members. All three builders emitted the return type directly as a generic type argument.

Added IsReturnTypeStaticAbstractInterface to MockMemberModel, populated during discovery. All three builders now fall back to the object?-cast pattern when this flag is set:

• MockMembersBuilder — emits VoidMockMethodCall instead of MockMethodCall<T>
• MockBridgeBuilder — uses HandleCallWithReturn<object?> + cast in DIM implementations
• MockImplBuilder — uses HandleCallWithReturn<object?> + cast across interface, wrap, and partial mock paths (sync and async)

Testing

• StaticAbstractMemberTests expanded with the real AWS SDK interface shape, including static abstract IAmazonService CreateDefaultServiceClient(...) — the CS8920 transitive scenario
• Snapshot test Interface_With_Static_Abstract_Transitive_Return_Type updated
• All 15 snapshot tests pass; no regression in existing static abstract member tests

Checklist

• I've searched existing issues to ensure this isn't a duplicate
• Fix applies to interface mocks, wrap mocks, and partial mocks
• Both sync and async return type paths handled
• Snapshot .verified.txt committed; no .received.txt committed
• No regression in existing static abstract member tests
• I've confirmed this issue occurs when running via dotnet test, not just in my IDE

[claude]

Code Review

This is a well-structured fix that correctly follows the existing IsRefStructReturn pattern to handle CS8920. The discovery logic, model updates, and all three builders are consistently updated. Good comprehensive test coverage. A few things worth flagging:

---

Primary concern: VoidMockMethodCall for non-void methods silently drops .Returns() capability

The biggest issue is a user-experience regression: methods that return an interface with static abstract members are treated as void for the setup API. Since VoidMockMethodCall has no .Returns() method, users cannot configure a return value:

var mock = Mock.Of<TUnit_Mocks_Tests_IAmazonService_Mockable>();

// This compiles, but silently ignores any return value setup — VoidMockMethodCall has no .Returns()
mock.CreateDefaultServiceClient(Arg.Any<AWSCredentials>(), Arg.Any<ClientConfig>())
    .Throws(new InvalidOperationException("unavailable")); // ✅ works
    
// There is no way to write:
//   mock.CreateDefaultServiceClient(...).Returns(someService); // ❌ not possible

The existing test suite confirms this — Static_Abstract_CreateDefaultServiceClient_Returns_Null_By_Default has no setup of a return value, and there's no positive test showing return value configuration.

This is a semantic mismatch: the method IS non-void (it returns IAmazonService), but we're treating it as void at the API level. The underlying engine already stores values as object? (as demonstrated by the HandleCallWithReturn<object?> cast), so a typed wrapper is feasible.

Suggested approach: Introduce a new ObjectReturnMockMethodCall (or similar) type that stores the return value as object? but exposes a typed Returns(object? value):

public sealed class ObjectReturnMockMethodCall : IObjectReturnMethodSetup, ICallVerification
{
    // Identical to MockMethodCall<TReturn> but with Returns(object? value)
    // so users can still configure return values
}

Then the generator emits ObjectReturnMockMethodCall instead of VoidMockMethodCall for SAI-returning methods. This preserves the .Returns() / .WasCalled() contract users expect.

If introducing a new type is out of scope for this PR, the limitation should at least be documented in the XML doc on IsReturnTypeStaticAbstractInterface and in comments on the generated setup method.

---

Minor: Redundant intermediate variable in non-SAI async path

In GenerateWrapMethodBody and GeneratePartialMethodBody, the non-SAI branch introduces an unnecessary rename:

// After the PR, non-SAI async path emits:
var __result = __rawResult;   // ← just a rename

This comes from the dual-branch structure:

if (method.IsReturnTypeStaticAbstractInterface)
    writer.AppendLine($"var __result = ({method.UnwrappedReturnType})__rawResult!;");
else
    writer.AppendLine($"var __result = __rawResult;");  // ← could just use __rawResult directly below

The else branch is harmless (compiler optimises it away) but slightly noisy in the generated code.

---

Minor: IsInterfaceWithStaticAbstractMembers — two loops can be unified

The helper currently iterates direct members first, then AllInterfaces members. Since AllInterfaces in Roslyn does NOT include the type itself (only inherited interfaces), the logic is correct, but a combined LINQ expression would be slightly cleaner and less error-prone:

private static bool IsInterfaceWithStaticAbstractMembers(ITypeSymbol type)
{
    if (type is not INamedTypeSymbol { TypeKind: TypeKind.Interface } namedType)
        return false;

    return namedType.GetMembers().Concat(namedType.AllInterfaces.SelectMany(i => i.GetMembers()))
        .Any(m => m.IsStatic && m.IsAbstract);
}

This is stylistic — the existing two-loop form is perfectly correct.

---

What's done well

• Consistently applies the fix across all three builders (MockImplBuilder, MockBridgeBuilder, MockMembersBuilder) and both sync and async code paths.
• IsInterfaceWithStaticAbstractMembers correctly uses AllInterfaces (transitive) to catch the exact AWS SDK pattern where the method return type inherits the static abstract member rather than defining it directly.
• Using GetAsyncInnerTypeSymbol() ?? returnType to handle Task<ISai> / ValueTask<ISai> is correct.
• Snapshot tests updated and committed, no .received.txt left in the tree.
• The null! → cast pattern ((T)engine.HandleCallWithReturn<object?>(...)!) is safe for reference-type interfaces and consistent with the ref struct fallback.

---

Summary: The CS8920 fix is solid. The main concern worth addressing before merge is that users lose the ability to configure return values for SAI-returning methods when using VoidMockMethodCall — this is a non-obvious capability regression that could be avoided with a new untyped return call type (or at minimum should be documented).

[thomhurst]

Thanks! 😄 Claude has identified some potential bugs

[lucaxchaves]

@thomhurst done!

[claude]

Code Review

This is a follow-up to my previous review. Let me first check which concerns have been addressed, then flag any remaining issues.

---

Previous concerns — status

VoidMockMethodCall drops .Returns() capability ✅ Fixed

The previous review noted that SAI-returning methods were treated as void at the setup API, silently dropping .Returns(). This has been correctly addressed:

• GenerateUnifiedSealedClass now calls GenerateReturnUnifiedClass(writer, ..., "object?", ...) instead of GenerateVoidUnifiedClass, so the generated class exposes .Returns().
• The extension method builder now emits MockMethodCall<object?> instead of VoidMockMethodCall.
• New test Static_Abstract_CreateDefaultServiceClient_Returns_Configured_Value confirms that .Returns() works end-to-end.

The snapshot also confirms: the extension method now returns MockMethodCall<object?> and the impl body casts back to IConfigProvider.

---

Remaining concerns

1. Type-safety loss at the setup API is undocumented

The setup extension method now returns MockMethodCall<object?> instead of MockMethodCall<IConfigProvider>. This means:

// Compiles fine, but crashes at the call site with InvalidCastException at runtime
mock.GetConfigProvider().Returns(new NotAConfigProvider());  // ← object? accepts anything

The trade-off is unavoidable given CS8920, but it should be called out in the generated XML doc comment (or at minimum in the IsReturnTypeStaticAbstractInterface doc comment already added to the model). Users need to know they've lost compile-time type safety for the return value. Suggestion:

/// <returns>
/// A <see cref="MockMethodCall{T}"/> typed as <c>object?</c> because the return type
/// (<typeparamref name="..."/>) contains static abstract members and cannot be used as
/// a generic type argument (CS8920). Pass a value that implements the return interface
/// to <c>.Returns()</c> — incorrect types will cause <see cref="InvalidCastException"/> at call time.
/// </returns>

2. Unrelated WaitsForAssertionTests.cs changes should be a separate PR

This PR significantly weakens the performance test assertions in WaitsFor_Performance_Many_Quick_Polls:

• Timeout: 5s → 30s
• Target count: IsGreaterThan(20) → IsGreaterThan(5)
• Assertion: IsGreaterThanOrEqualTo(21) → IsGreaterThanOrEqualTo(6)
• Upper bound: IsLessThan(TimeSpan.FromSeconds(5)) → IsLessThan(TimeSpan.FromSeconds(30))

And in WaitsFor_With_Custom_Polling_Interval: timeout 1s → 30s.

These are unrelated to CS8920 mocking fixes and make the tests nearly trivially easy to pass. If these are addressing legitimate CI flakiness, they deserve their own PR with justification (and ideally a proper fix to the flakiness root cause rather than 30x-larger timeouts and 4x-fewer assertions).

---

What's well done

• All three builders consistently updated across both sync and async paths, wrap/partial/impl.
• IsInterfaceWithStaticAbstractMembers now correctly handles the transitive case: a return type interface that inherits a static abstract member (the exact AWS SDK pattern) is detected via AllInterfaces.SelectMany(i => i.GetMembers()).
• Tests are comprehensive: null default, configured return value, verification, exception throwing — all four scenarios are covered.
• Snapshot is committed without .received.txt files.
• The null! → cast pattern (TReturn)engine.HandleCallWithReturn<object?>(...)! is safe for reference-type interfaces (null cast succeeds; null default is consistent with other unmocked methods).

---

Summary: The CS8920 fix is solid and the primary concern from the previous review (lost .Returns() capability) has been resolved. Two things worth addressing before merge: (1) document the type-safety trade-off in the generated setup API, and (2) move the unrelated WaitsForAssertionTests.cs weakening into a separate, justified PR.

[claude]

Code Review — Follow-up (Round 3)

This is a follow-up to the previous two reviews. All outstanding concerns have now been resolved.

---

Previous concerns — final status

VoidMockMethodCall drops .Returns() capability ✅ Fixed (Round 2)
Resolved by switching to MockMethodCall<object?> via GenerateReturnUnifiedClass.

Type-safety loss at the setup API is undocumented ✅ Fixed (Round 3)
MockMembersBuilder now emits XML doc comments on the generated extension methods when IsReturnTypeStaticAbstractInterface is set, explaining that the return type is object? due to CS8920 and that incorrect types will cause InvalidCastException at call time (line ~603 in the current builder).

Unrelated WaitsForAssertionTests.cs weakening ✅ Fixed (Round 3)
The final commit restores the stricter assertion values (IsGreaterThan(20), 5 s timeout) that had been loosened to 30 s / 5 polls in a previous commit. The test file is back to its original intent.

---

Overall assessment

The fix is correct, comprehensive, and well-structured:

• IsReturnTypeStaticAbstractInterface is detected transitively via AllInterfaces.SelectMany(...), which is exactly what is needed for the AWS SDK pattern where the return type inherits the static abstract member rather than declaring it directly.
• All three builders (MockImplBuilder, MockBridgeBuilder, MockMembersBuilder) are updated consistently across both sync and async code paths, as well as interface, wrap, and partial mock modes.
• The object? → cast pattern (T)engine.HandleCallWithReturn<object?>(...)! is safe for reference-type interfaces and is consistent with the existing IsRefStructReturn fallback.
• Snapshot tests are committed (.verified.txt), no .received.txt files remain.
• The generated XML doc comments adequately communicate the type-safety trade-off to users consuming the setup API.

The PR is ready to merge.

[thomhurst]

Thanks @lucaxchaves - looks ready to go - just a failing test

[thomhurst]

Ty!