Support static abstract interface members in mock generation

[thomhurst]

Summary

Fixes #5069

• Generate explicit interface implementation stubs (throwing NotSupportedException) for static abstract members so generated mock classes compile when the mocked interface has static abstract members (e.g. AWS SDK's IAmazonService)
• Skip transitive mock factory generation for interfaces that contain static abstract members, avoiding CS8920 when they would be used as type arguments in Mock<T>/MockEngine<T>
• Add new MockStaticAbstractMemberModel to capture static abstract method/property signatures for code generation

Test plan

• Added snapshot test Interface_With_Static_Abstract_Transitive_Return_Type simulating the AWS SDK scenario
• All 12 source generator snapshot tests pass
• All 659 TUnit.Mocks tests pass
• All Roslyn variant builds (414, 44, 47) succeed

[claude]

Code Review: Support static abstract interface members in mock generation

Good fix for a real-world compilation blocker. The two-pronged approach is correct and pragmatic.

What is well done:

• Generating NotSupportedException stubs for directly-mocked interfaces and skipping transitive factory generation for interfaces with static abstract members is exactly the right split. Attempting to use a static-abstract interface as a type argument in Mock/MockEngine always hits CS8920.
• seenStaticAbstract deduplication keys on the declaring-interface FQN, correctly handling cases where multiple base interfaces contribute static abstract members.
• The SnapshotTestBase.cs change is a good correctness fix. Defaulting to LanguageVersion.Preview prevents confusing parse failures for future test scenarios using modern C# features.
• The equatability pattern in MockStaticAbstractMemberModel is consistent with the rest of the codebase (MockMemberModel also uses a partial-field GetHashCode).

Issue 1: Hardcoded indentation in GenerateStaticAbstractStub

In MockImplBuilder.cs around line 352, the method expression body uses a hardcoded 4-space indent:

writer.AppendLine($"static {returnType} {fqn}.{name}...");
writer.AppendLine($"    => {throwExpr};");  // hardcoded 4 spaces

The rest of the builder uses writer.Block(...) to manage indentation level. If CodeWriter's indentation model ever changes (tabs vs. spaces), this prefix will drift. Consider using a block body or a single expression-body line without additional indentation to stay consistent with the rest of the builder.

Issue 2: Missing snapshot test for the direct mock case

The added snapshot test covers the transitive case: IMyService (mockable) returns IConfigProvider (has static abstract) and the transitive mock is skipped. There is no test for the direct case: Mock.Of() where IConfigProvider has both instance members AND static abstract members.

The generated impl should include regular method stubs plus the NotSupportedException stub. This is the actual code path through GenerateStaticAbstractStub -- the current test only exercises the skip-transitive path, not the stub-generation path itself. A direct test would provide a snapshot of the full output including the static stub, making regressions immediately visible.

Issue 3: Unused using statements in MockStaticAbstractMemberModel.cs

System.Collections.Immutable and System.Linq are imported but not referenced directly in this file. Minor cleanup opportunity.

Minor / future work: No compile-time diagnostic for unmockable members

When a user mocks an interface with static abstract members, the mock silently generates a stub that throws NotSupportedException at runtime. There is no compile-time signal that those members cannot be set up or verified. A separate analyzer diagnostic would make the limitation discoverable. Not blocking this PR, but worth tracking as a follow-up issue.

Summary

The core fix is correct and production-ready. Main actionable items:

1. Align indentation in GenerateStaticAbstractStub with the rest of the builder
2. Add a snapshot test for directly mocking an interface with static abstract members
3. Remove unused imports from MockStaticAbstractMemberModel.cs

The decision to skip transitive generation rather than attempting to generate and fail is the right architectural call for CS8920. Overall a solid, well-scoped fix.

[dealloc]

anything I can do to get this merged? This is currently blocking tests for me

[claude]

Follow-up Code Review

Thanks for the additional commits addressing the previous review. Here's a status on the outstanding items and one new finding.

Previous Review: Status Check

✅ Fixed: Hardcoded indentation in GenerateStaticAbstractStub
The stub generator now properly uses writer.OpenBrace() / writer.CloseBrace() and writer.AppendLine() throughout — consistent with the rest of the builder.

✅ Fixed: Unused using System; in MockStaticAbstractMemberModel.cs
The unnecessary imports are gone.

❌ Still missing: Snapshot test for direct mock of static-abstract interface
The only added test (Interface_With_Static_Abstract_Transitive_Return_Type) covers the indirect path — where another interface's method returns a static-abstract interface. There is still no test for the direct path:

var mock = Mock.Of<IConfigProvider>(); // IConfigProvider has static abstract members

The stub-generation path in GenerateStaticAbstractStub is exercised only when StaticAbstractMembers is non-empty on the mocked type. The current test only confirms that IConfigProvider_MockFactory is not generated; it does not snapshot what the IConfigProvider_MockImpl looks like with stubs. A separate test such as Interface_With_Static_Abstract_Direct_Mock would close this gap and provide regression coverage for the actual code-generation path.

---

New Finding: GetHashCode is incomplete in MockStaticAbstractMemberModel

Equals correctly compares all nine fields including IsVoid, IsProperty, HasGetter, and HasSetter, but GetHashCode only hashes five of them:

// Equals includes all of these — GetHashCode misses IsVoid, IsProperty, HasGetter, HasSetter
hash = hash * 31 + InterfaceFullyQualifiedName.GetHashCode();
hash = hash * 31 + Name.GetHashCode();
hash = hash * 31 + ReturnType.GetHashCode();
hash = hash * 31 + Parameters.GetHashCode();
hash = hash * 31 + TypeParameters.GetHashCode();

This violates the GetHashCode contract: two objects where only IsProperty differs will have the same hash but not be equal, causing incorrect deduplication in any HashSet<MockStaticAbstractMemberModel> or Dictionary key. Since this is a record, the simplest correct fix is to remove the explicit Equals/GetHashCode overrides entirely and let the record compiler generate them (the EquatableArray<T> fields already implement structural equality). If the existing models in the codebase require explicit implementations for a specific reason, at minimum add the missing four booleans to GetHashCode:

hash = hash * 31 + IsVoid.GetHashCode();
hash = hash * 31 + IsProperty.GetHashCode();
hash = hash * 31 + HasGetter.GetHashCode();
hash = hash * 31 + HasSetter.GetHashCode();

---

Summary

Item	Status
Hardcoded indentation	✅ Fixed
Unused imports	✅ Fixed
Direct-mock snapshot test	❌ Still missing
GetHashCode incomplete	🆕 New issue

The core fix is solid and the transitive-skip approach remains the right architectural call. Resolving the two remaining items above — particularly the GetHashCode bug which could cause silent correctness issues — would make this production-ready.

[claude]

Code Review: Support Static Abstract Interface Members in Mock Generation

This PR tackles a genuinely hard problem — CS8920 prevents interfaces with unresolved static abstract members from being used as generic type arguments, which breaks the core Mock<T> / MockEngine<T> architecture. The bridge interface + DIM + AsyncLocal approach is clever and shows deep understanding of the constraints involved. Good comprehensive test coverage with snapshot tests and integration tests.

Architecture Overview

The solution introduces three interacting pieces:

1. _Mockable bridge interface with DIM implementations routing calls through the static engine
2. _StaticEngine static class using AsyncLocal<IMockEngineAccess?> to hold the per-test engine
3. [assembly: GenerateMock(typeof(T))] attribute as the discovery mechanism (avoiding CS8920 via typeof)

This is architecturally sound for the problem space. The AsyncLocal choice is correct for test isolation in TUnit's async-per-test model.

Concern: Multiple Mock Instances of Same Type in One Test

The most significant correctness issue: _StaticEngine.Engine is set in the constructor on every mock creation:

// MockImplBuilder.cs — constructor body
if (model.HasStaticAbstractMembers)
    writer.AppendLine($"{safeName}_StaticEngine.Engine = engine;");

If a test creates two mocks of the same bridge type, the second mock's engine silently overwrites the first. Static member calls on the first mock will route to the second mock's engine:

var mock1 = Mock.Of<TUnit_Mocks_Tests_IAmazonService_Mockable>();
mock1.CreateDefaultConfig().Returns(config1);

var mock2 = Mock.Of<TUnit_Mocks_Tests_IAmazonService_Mockable>();
// Now _StaticEngine.Engine points to mock2's engine
// CallStaticAbstract<...>() will consult mock2, not mock1

This is a fundamental constraint of the static-dispatch model, but it should be documented prominently. A diagnostic or warning when a second mock of a static-abstract interface is created in the same context would prevent silent test breakage.

UX Friction: Verbose Generated Type Names

The bridge type name (TUnit_Mocks_Tests_IAmazonService_Mockable) is derived by replacing dots in the FQN with underscores. This creates real usability pain:

• Users must know the mangled name before the generator has run
• IDEs won't autocomplete it initially
• The documentation example in GenerateMockAttribute shows this verbose name

A using alias or a helper method (e.g., Mock.Of(typeof(IAmazonService)) returning a non-generic Mock) could improve ergonomics. Even just a // ReSharper disable once or IDE annotation to help discoverability would help. The [EditorBrowsable(Never)] on the bridge interface also means it won't appear in completion lists, which is correct but adds to the friction.

_StaticEngine Not Reset After Test

After a test completes, _StaticEngine.Engine retains the engine reference for the lifetime of the async execution context. Since TUnit gives each test its own async context, this should be fine — the value flows with the context and is GC'd appropriately. Worth explicitly documenting that this relies on TUnit's per-test async context isolation.

Minor: HasStaticAbstractMembers Computed Each Time

// MockTypeModel.cs
public bool HasStaticAbstractMembers =>
    Methods.Any(m => m.IsStaticAbstract) || Properties.Any(p => p.IsStaticAbstract);

This is called multiple times during code generation (at least 3 times per model in the constructor paths alone). Since EquatableArray is essentially immutable, this could be cached. Not a hot-path concern for a source generator, but noting it for completeness.

CI Fix Commits

The two CI fix commits (PackTUnitFilesModule rebuild-before-pack, TestNugetPackageModule [NotInParallel]) are clearly explained and address real CI race conditions. They're unrelated to the feature but are reasonable inclusions given the PR context.

What's Done Well

• HasStaticAbstractMembers transitive-type skip in MockTypeDiscovery correctly prevents CS8920 when auto-mocking interfaces that return static-abstract-bearing types
• Fallback to CandidateSymbols for OverloadResolutionFailure in TransformToModels is the right Roslyn pattern for this scenario
• IsStaticAbstract flag on MockMemberModel cleanly integrates static abstract members into the existing model rather than a parallel hierarchy (the earlier MockStaticAbstractMemberModel approach was correctly abandoned)
• Snapshot tests cover the three key cases: direct static abstract members, inherited static abstract members, and transitive return types
• The #if NET7_0_OR_GREATER guard in the integration test is correct since static abstract interface members are a C# 11 / .NET 7 feature

Summary

The core design is solid. The primary actionable items are:

1. Document (or ideally detect) the multiple-mock-same-type footgun
2. Consider a more ergonomic discovery path for the generated bridge type (even just a better naming convention or a generated const string MockableTypeName somewhere)

The CS8920 constraint means there's no perfect solution here — the bridge pattern is probably the best available approach given the language limitations.

[claude]

Follow-up Code Review

Solid rework. The shift from NotSupportedException stubs + factory-skip to the bridge interface with DIM implementations is a significantly better architecture — static abstract members are now actually mockable and verifiable, not just silently failing at runtime. Previous review items are addressed; two items remain.

---

Previous review: status check

Item	Status
Hardcoded indentation in stub generator	✅ Resolved (whole approach replaced)
Unused imports in MockStaticAbstractMemberModel	✅ Resolved (model removed entirely)
Direct-mock snapshot test	✅ Interface_With_Static_Abstract_Members added
GetHashCode incomplete in MockStaticAbstractMemberModel	✅ Resolved (model replaced by IsStaticAbstract flag on MockMemberModel with correct hash)

---

Issue 1 (Critical): HasStaticAbstractMembers missing from MockTypeModel.Equals and GetHashCode

MockTypeModel.cs adds the field but neither Equals nor GetHashCode reference it:

// Added to the record:
public bool HasStaticAbstractMembers { get; init; }

// Equals — missing this check:
// && HasStaticAbstractMembers == other.HasStaticAbstractMembers

// GetHashCode — missing this hash:
// hash = hash * 31 + HasStaticAbstractMembers.GetHashCode();

The incremental source generator compares MockTypeModel values between compilations to decide whether to re-run the output step. If a user adds a static abstract member to an interface they are mocking (so HasStaticAbstractMembers flips from false to true), Roslyn's incremental cache will see two "equal" models and skip regeneration — the bridge file will never be produced. The result is a silent CS8920 or missing bridge type until a full rebuild.

Fix: add both to Equals and GetHashCode, consistent with how IsStaticAbstract was handled in MockMemberModel.

---

Issue 2 (UX / discoverability): Mock.Of<T>() silently produces nothing for static-abstract interfaces

When a user writes:

var mock = Mock.Of<IServiceWithStaticAbstracts>(); // ← triggers CS8920 at call site

the compiler emits CS8920, but there is no diagnostic or note from the source generator explaining that [assembly: GenerateMock(typeof(T))] + Mock.Of<T_Mockable>() is the intended workaround. The user sees only a cryptic compiler error with no guidance.

A Roslyn Diagnostic emitted by the generator when it detects a Mock.Of<T>() call site where T has static abstract members would make this significantly more discoverable:

TUnit.Mocks warning TUNIT_MOCK001:
  'IServiceWithStaticAbstracts' has static abstract members and cannot be used
  as a type argument. Use [assembly: GenerateMock(typeof(IServiceWithStaticAbstracts))]
  and Mock.Of<TUnit_Mocks_IServiceWithStaticAbstracts_Mockable>() instead.

Not a blocker, but without this, real-world users will be confused by a CS8920 error with no path to resolution.

---

Architecture observation: AsyncLocal engine lifetime

The per-type _StaticEngine stores the engine in an AsyncLocal<T>, which correctly isolates concurrent tests running on separate Task chains. One scenario worth documenting: if a test that mocks IAmazonService calls Initialize() (an instance method) and CreateDefaultConfig() (a static abstract) on separate awaited boundaries, the AsyncLocal value is captured at the point the async context branches. Since the engine is set in the constructor (before any await), this is fine — but it is implicit behavior. A comment in BuildStaticEngineClass explaining the AsyncLocal choice and its isolation guarantees would make this easier to maintain.

---

Summary

Item	Severity	Status
HasStaticAbstractMembers missing from Equals/GetHashCode	🔴 Critical	New issue
No diagnostic for Mock.Of<T>() with static-abstract T	🟡 UX / follow-up	New finding
AsyncLocal lifetime undocumented	⚪ Minor	Suggestion

The bridge interface approach is the right long-term design. Fixing the MockTypeModel equality bug is the only blocker before this is production-ready.

[claude]

Code Review: Support Static Abstract Interface Members in Mock Generation (Follow-up)

Previous review concerns have been well addressed in the latest commits:

• ✅ Multiple-mock detection: constructor now throws InvalidOperationException with a clear message
• ✅ HasStaticAbstractMembers is now a stored property on MockTypeModel, not recomputed on every access
• ✅ Hardcoded indentation replaced with writer.OpenBrace/CloseBrace
• ✅ Unused imports removed; dead code deleted

The overall architecture (bridge interface + DIMs + AsyncLocal dispatch) remains solid and is the right approach given the CS8920 constraint.

Issue: IMockEngineAccess is a Breaking Change

Two new methods were added to a public interface:

// IMockEngineAccess.cs
void HandleCall(int memberId, string memberName, object?[] args);
TReturn HandleCallWithReturn<TReturn>(int memberId, string memberName, object?[] args, TReturn defaultValue);

Any code outside TUnit implementing IMockEngineAccess (e.g., for testing, custom engines, or stubs) will now fail to compile. These methods appear to have already existed on MockEngine<T> as internal/concrete members — consider whether they need to be on the interface at all, or whether the bridge DIM code can be refactored to call through MockEngine<T> directly (since the static engine _engine field already has that concrete type at generation time).

If they must be on the interface, this is a breaking change that warrants a note in the changelog / semver bump.

Subtle Gap: HasStaticAbstractMembers Misses Static Abstract Events

The model-level flag is populated from:

HasStaticAbstractMembers = methods.Any(m => m.IsStaticAbstract) || properties.Any(p => p.IsStaticAbstract)

However, the transitive-skip guard in MockTypeDiscovery.HasStaticAbstractMembers(INamedTypeSymbol) correctly checks all members:

if (member.IsStatic && member.IsAbstract)  // catches events too
    return true;

If an interface has only static abstract events (rare but valid C# 11), the MockTypeModel.HasStaticAbstractMembers flag would be false, no bridge would be generated, and CS8920 would reappear. Aligning both checks would close this gap.

Minor: AsyncLocal Guard and [Before(Test)] Interaction

The guard is correct and the reasoning is sound — AsyncLocal provides per-test isolation via TUnit's execution context model. One edge case worth documenting: if a user creates the mock in a [Before(Test)] hook and also attempts to create one in the test body itself, the guard will correctly throw. This is expected behavior but could surprise users who don't realize the engine flows into the test context. A brief note in the GenerateMockAttribute XML doc or README would help.

Acknowledged But Not Changed: Verbose Bridge Type Name

The doc example still shows:

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

This was flagged previously and is acknowledged as a language-imposed constraint. A possible mitigation would be generating a file-scoped or internal type alias in the same namespace as the original interface (e.g., IAmazonService_Mockable without the namespace prefix), but this may conflict with multiple assemblies. Not blocking for this PR — worth a follow-up issue.

What's Done Well

The approach of integrating static abstract members into the existing MockMemberModel via the IsStaticAbstract flag (rather than a parallel model hierarchy) is clean and avoids duplication. The CandidateSymbols fallback for OverloadResolutionFailure is the correct Roslyn pattern. The three snapshot tests covering direct, inherited, and transitive cases give good confidence in the output. The [NotInParallel] CI fix and the pack-version-rebuild fix are sensible and well-explained.

Overall this is a significant improvement over the initial submission and addresses the hardest parts of the problem correctly. The IMockEngineAccess interface change is the most actionable item before merging.

[claude]

Code Review: Support Static Abstract Interface Members in Mock Generation (Follow-up)

Previous review concerns have been well addressed in the latest commits:

• ✅ Multiple-mock detection: constructor now throws InvalidOperationException with a clear message
• ✅ HasStaticAbstractMembers is now a stored property on MockTypeModel, not recomputed on every access
• ✅ Hardcoded indentation replaced with writer.OpenBrace/CloseBrace
• ✅ Unused imports removed; dead code deleted

The overall architecture (bridge interface + DIMs + AsyncLocal dispatch) remains solid and is the right approach given the CS8920 constraint.

---

Issue: IMockEngineAccess Interface Expansion (Potential Breaking Change)

Two new methods were added to a public interface:

// IMockEngineAccess.cs
void HandleCall(int memberId, string memberName, object?[] args);
TReturn HandleCallWithReturn<TReturn>(int memberId, string memberName, object?[] args, TReturn defaultValue);

Any code implementing IMockEngineAccess outside TUnit (e.g., test doubles, custom engines) will now fail to compile. These methods already exist as concrete members on MockEngine<T> — it is worth considering whether the bridge DIM dispatch code could call through MockEngine<T> directly instead of widening the interface. If the interface expansion is intentional, this is a semver-significant change.

---

Subtle Gap: HasStaticAbstractMembers Does Not Cover Static Abstract Events

The model-level flag is populated as:

HasStaticAbstractMembers = methods.Any(m => m.IsStaticAbstract) || properties.Any(p => p.IsStaticAbstract)

But the transitive-skip guard in MockTypeDiscovery correctly covers all members:

if (member.IsStatic && member.IsAbstract)  // catches events too
    return true;

If an interface declares only static abstract events (valid C# 11), the model flag would remain false, no bridge would be generated, and CS8920 would reappear. Aligning these two checks would close the gap.

---

Minor: AsyncLocal Guard and [Before(Test)] Interaction

The guard logic is correct — AsyncLocal provides per-test-context isolation. One pattern worth documenting: a mock created in a [Before(Test)] hook flows into the test body via AsyncLocal, so creating another mock of the same bridge type in the test body will correctly throw. This is expected and safe, but could surprise users. A sentence in the GenerateMockAttribute XML doc about "create once per test context" would prevent confusion.

---

Acknowledged: Verbose Bridge Type Name

The doc example still shows:

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

This is a language-imposed constraint and was flagged in the prior review. Not blocking, but a follow-up issue to explore shorter naming conventions (e.g., dropping namespace segments, or a generated type alias) would improve DX.

---

What's Done Well

• Integrating static abstract members into MockMemberModel via IsStaticAbstract rather than a parallel model class is the right design — clean and non-duplicative.
• The CandidateSymbols fallback for OverloadResolutionFailure is the correct Roslyn pattern for this scenario.
• Three snapshot tests covering direct, inherited, and transitive cases give solid confidence in generator output.
• The [NotInParallel] CI fix and pack-version-rebuild fix are well-explained and address real race conditions.

Overall this is in good shape. The IMockEngineAccess interface change is the primary item to decide on before merging.

[claude]

Code Review: Support Static Abstract Interface Members in Mock Generation (Follow-up #5)

Previous Review: Status Check

The two most recent commits are merge conflict resolutions for MockGeneratorTests.cs (keeping both the keyword-parameter-names test from main and the static abstract tests from this branch). No functional changes to the mock generation code. All prior review items from Follow-up #4 carry over unchanged:

✅ Fully resolved from earlier reviews:

• Multiple-mock guard (throws InvalidOperationException via EmitStaticEngineAssignment)
• HasStaticAbstractMembers covers events, methods, and properties
• HasStaticAbstractMembers is a stored init property, not a LINQ scan
• Dead code removed, indentation fixed, unused imports removed
• MockTypeModel.Equals / GetHashCode includes HasStaticAbstractMembers

---

Still Unresolved: IMockEngineAccess Public Interface Expansion

This item has been flagged in every prior review and remains the one blocker before merging:

// IMockEngineAccess.cs — two new public API members
void HandleCall(int memberId, string memberName, object?[] args);
TReturn HandleCallWithReturn<TReturn>(int memberId, string memberName, object?[] args, TReturn defaultValue);

IMockEngineAccess is public. [EditorBrowsable(Never)] affects IDE autocomplete only — it provides zero protection against compile-time breakage. Any downstream code implementing this interface will fail to compile on upgrade.

Recommended fix: Since the static engine field is always a MockEngine<T> at generation time, the generated DIM code could cast directly rather than going through the interface:

// In GenerateStaticEngineDispatchBody — instead of:
__engine.HandleCall(...)
// Use a cast that avoids the interface:
((global::TUnit.Mocks.MockEngine<{mockableType}>)__engine).HandleCall(...)

Or, keep the methods on the interface but add default interface implementations that throw NotSupportedException, giving existing implementors a compile-time-safe migration path without removing the new functionality.

---

Still Minor: Two Documentation Gaps

These are non-blocking but would prevent user confusion:

1. AsyncLocal + [Before(Class)] footgun — GenerateMockAttribute's XML doc should note that mocks must be created per-test, not per-class. The existing guard (throws InvalidOperationException when engine is already set) means a class-scoped mock will make every test in the class throw on its own mock creation, which is surprising behavior with no obvious cause.

2. Static abstract events are no-op — GenerateStaticEventDim emits empty add { } remove { } with no engine dispatch, no setup support, and no verification. A comment in the builder and/or a note in the attribute doc would prevent future bug reports from users who expect the same configurability as instance events.

---

What's Done Well

The overall solution is solid and well-implemented. The bridge interface + DIM + AsyncLocal approach correctly handles the CS8920 constraint, inherited static abstracts are resolved via CandidateSymbols fallback, the IsStaticAbstract flag unifies the model hierarchy cleanly, and the three snapshot tests plus StaticAbstractMemberTests.cs provide good coverage. The iterative refinement across commits is exemplary.

The IMockEngineAccess expansion is the last substantive decision before this can merge.

[thomhurst]

@dealloc New version should be out in the next hour

[dealloc]

@thomhurst appreciate the fast resolution!