fix(mocks): skip MockBridge for class targets with static-abstract interfaces (#5677)#5681
fix(mocks): skip MockBridge for class targets with static-abstract interfaces (#5677)#5681
Conversation
…terface (#5677) When the mock target is a *class* that implements an interface containing static-abstract members, MockBridgeBuilder previously emitted a bridge interface that listed the class in its base list (CS0527) and added explicit interface impls without listing the interface (CS0540). The class already provides the concrete static implementation, so no bridge is needed: only the instance-virtual surface needs mock dispatch. Two-layer guard: - MemberDiscovery now skips collecting static-abstract interface members when the target's TypeKind is Class. - MockGenerator only invokes MockBridgeBuilder when the target is an interface (defensive). Existing static-abstract-on-interface mocks (StaticAbstractMemberTests) remain unchanged.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
Clean, focused fix. The root-cause analysis in the PR description is solid and the chosen approach is correct. A few observations:
Architecture: Duplicate logic in two loops
The flag is independently computed and applied in two separate loops within (~line 60 and ~line 211). Both are identical one-liners with the same rationale. If a third collection pass is ever added, it's easy to forget this guard.
A more resilient approach would be to pass this flag as a parameter to a shared helper, or compute it once at the top of the public method and thread it through:
// Compute once, pass to both collection phases
bool collectStaticAbstracts = typeSymbol.TypeKind != TypeKind.Class;
CollectInstanceMembers(..., collectStaticAbstracts);
CollectBridgeMembers(..., collectStaticAbstracts);This removes the need to remember to duplicate the guard everywhere that walks interfaces for static-abstract members.
Redundant guard in
After the fix, HasStaticAbstractMembers will never be true for a class target, so the new && model.IsInterface condition in MockGenerator.cs is defence-in-depth that can never be triggered. That's not necessarily wrong, but future maintainers may be confused by what looks like a required gate when it's actually unreachable.
If kept, a brief comment explaining why it can never be true for classes (pointing to MemberDiscovery) would clarify intent. If removed, the contract lives entirely in MemberDiscovery, which is arguably cleaner.
Issue-number references in source comments
The code comments cite (#5677) directly:
// For class targets, the class itself provides the concrete static impl that
// satisfies any static-abstract interface members (#5677).
Per the project's code-comment principle, issue numbers belong in commit/PR descriptions — not in source — because they become meaningless without GitHub context and rot as the codebase evolves. The why (CS0527/CS0540, class vs. interface semantics) is already captured in the comment body; dropping the issue number would make the comment self-contained and timeless.
Test coverage
The T15 test is well-constructed:
- Guards correctly with
- Verifies the mock can intercept the instance-virtual surface ()
- Confirms the class's concrete static impl is untouched by the mock engine
- The cast workaround for CS8920 is explained inline — good
No complaints here.
Summary
The fix is correct and the test plan is comprehensive. The main structural suggestion is consolidating the duplicated collectStaticAbstractFromInterfaces flag into a single shared parameter rather than re-deriving it in each loop — this makes the invariant harder to accidentally break in future maintenance.
There was a problem hiding this comment.
Code Review
Clean, focused fix. The root-cause analysis in the PR description is solid and the chosen approach is correct. A few observations:
Architecture: Duplicate logic in two MemberDiscovery loops
The collectStaticAbstractFromInterfaces flag is independently computed and applied in two separate loops within MemberDiscovery.cs (~line 60 and ~line 211). Both are identical one-liners with the same rationale. If a third collection pass is ever added, it is easy to forget this guard.
A more resilient approach would be to pass this flag as a parameter to a shared helper, or compute it once at the top of the public method and thread it through:
// Compute once, pass to both collection phases
bool collectStaticAbstracts = typeSymbol.TypeKind != TypeKind.Class;
CollectInstanceMembers(..., collectStaticAbstracts);
CollectBridgeMembers(..., collectStaticAbstracts);This removes the need to remember to duplicate the guard everywhere that walks interfaces for static-abstract members.
Redundant guard in MockGenerator.cs
After the MemberDiscovery fix, HasStaticAbstractMembers will never be true for a class target, so the new && model.IsInterface condition in MockGenerator.cs is defence-in-depth that can never be triggered. That is not necessarily wrong, but future maintainers may be confused by what looks like a required gate when it is actually unreachable.
If kept, a brief comment explaining why it can never be true for classes (pointing to MemberDiscovery) would clarify intent. If removed, the contract lives entirely in MemberDiscovery, which is arguably cleaner.
Issue-number references in source comments
The code comments cite (#5677) directly in the source. Issue numbers belong in commit/PR descriptions rather than source code, because they become meaningless without GitHub context and rot as the codebase evolves. The why (CS0527/CS0540, class vs. interface semantics) is already captured in the comment body; dropping the issue number would make the comment self-contained and timeless.
Test coverage
The T15 test is well-constructed:
- Guards correctly with
#if NET8_0_OR_GREATER - Verifies the mock can intercept the instance-virtual surface (
InstanceValue.Returns(42)) - Confirms the class concrete static impl is untouched by the mock engine
- The
object?cast workaround for CS8920 is explained inline
No complaints here.
Summary
The fix is correct and the test plan is comprehensive. The main structural suggestion is consolidating the duplicated collectStaticAbstractFromInterfaces flag into a single shared parameter rather than re-deriving it in each loop — this makes the invariant harder to accidentally break in future maintenance.
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| CodeStyle | 2 minor |
🟢 Metrics 0 complexity
Metric Results Complexity 0
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.
- Extract `ShouldCollectStaticAbstractFromInterfaces` helper so both `DiscoverMembers` and `DiscoverMembersFromMultipleTypes` (and any future collection loop) honor the same TypeKind guard. - Tighten `MockGenerator.GenerateSingleTypeMock` comment to call out the defence-in-depth role of the `model.IsInterface` check. - Strip issue-number references from source comments (kept the WHY: CS0527 / CS0540 / class vs. interface semantics).
|
Applied review feedback in 46f5894:
Tests (net10.0):
No snapshot diffs produced; nothing to commit beyond the source edits. |
There was a problem hiding this comment.
Code Review (follow-up)
Two previous reviews were already submitted by claude[bot] at the same timestamp — this appears to be a duplicate-submission artifact. Let me provide a clean assessment and correct one factual error from those reviews.
Previous review: correction on issue-number references
The earlier reviews flagged (#5677) appearing in source code comments. This is incorrect. Looking at the actual diff:
MockGenerator.cs:// Defence-in-depth: MemberDiscovery already filters static-abstract collection on TypeKind, so HasStaticAbstractMembers is always false here for class targets.— no issue referenceMemberDiscovery.csXML doc: references CS0527/CS0540 by name, no issue number- Test file comments: no issue numbers
The #5677 reference appears only in the PR title/description, which is exactly where it belongs. No action needed.
Open concern: duplicate flag computation (still valid)
The collectStaticAbstractFromInterfaces flag is independently computed in two separate loops inside MemberDiscovery.cs:
// ~line 44 (first collection phase)
var collectStaticAbstractFromInterfaces = ShouldCollectStaticAbstractFromInterfaces(typeSymbol);
// ~line 193 (second collection phase — separate scope)
var collectStaticAbstractFromInterfaces = ShouldCollectStaticAbstractFromInterfaces(typeSymbol);The XML doc on ShouldCollectStaticAbstractFromInterfaces mitigates this with a "MUST gate through this helper" directive — which is documentation-based enforcement. That is better than nothing, but still fragile. A third loop added under deadline pressure could easily miss it.
The better approach: if both loops are in the same public entry-point method, compute once and thread the value through. If they are in separate methods with the same typeSymbol parameter, make collectStaticAbstractFromInterfaces part of a small context struct or compute it once in the caller:
var ctx = new MemberCollectionContext(typeSymbol)
{
CollectStaticAbstracts = ShouldCollectStaticAbstractFromInterfaces(typeSymbol)
};
// pass ctx to both phasesThis makes it structurally impossible to forget the guard rather than relying on a doc comment.
Redundant guard in MockGenerator.cs — already well-handled
The && model.IsInterface defence-in-depth guard comes with a clear comment explaining it is unreachable for class targets. The intent is legible. This is acceptable as written.
Test coverage
T15 is well-structured: proper #if NET8_0_OR_GREATER guard, exercises mock interception on the instance surface, confirms the static implementation is not hijacked, and the object? cast for CS8920 is documented inline. No issues.
Overall
The root cause is correctly diagnosed and the fix is sound. The structural concern about the duplicate flag computation remains open from the previous reviews — the XML doc comment on ShouldCollectStaticAbstractFromInterfaces partially addresses it but structural enforcement via a shared parameter would be more robust. The rest of the implementation is clean.
… helper Both DiscoverMembers and DiscoverMembersFromMultipleTypes used to hoist ShouldCollectStaticAbstractFromInterfaces(typeSymbol) into a local boolean and then check it inline at each static-member branch. This relied on documentation/discipline -- a future third loop could silently skip the gate and reintroduce the #5677 regression. Funnel both call sites through TryCollectStaticAbstractFromInterface, which derives the flag from the typeSymbol it was handed. The only way to collect a static-abstract interface member is now through this helper, so the bypass is structurally impossible.
|
Round 2 review applied in 8efe4cf. Item 1 (structural enforcement): Funnelled both Chose this shape over a Item 2 (defence-in-depth Tests (net10.0):
|
There was a problem hiding this comment.
Code Review: fix(mocks): skip MockBridge for class targets with static-abstract interfaces
Overall: The fix is correct and the iterative refactoring toward structural enforcement via TryCollectStaticAbstractFromInterface is the right direction. The PR is in good shape. The observations below range from logic precision to missing test coverage.
Finding 1 — ShouldCollectStaticAbstractFromInterfaces guard is too permissive
File: TUnit.Mocks.SourceGenerator/Discovery/MemberDiscovery.cs
// Current:
private static bool ShouldCollectStaticAbstractFromInterfaces(ITypeSymbol typeSymbol)
=> typeSymbol.TypeKind != TypeKind.Class;The "collect unless class" guard would also pass TypeKind.Struct, TypeKind.Enum, TypeKind.Delegate, etc. There's no current execution path that triggers those, but if a third kind is ever accepted (e.g., a record struct mock) the guard silently allows collection for it. The adjacent branching logic in DiscoverMembers uses == TypeKind.Interface — the same pattern should apply here:
private static bool ShouldCollectStaticAbstractFromInterfaces(ITypeSymbol typeSymbol)
=> typeSymbol.TypeKind == TypeKind.Interface;Why this is better: It encodes the intended semantic (bridge only applies to interface targets), matches the existing pattern in the codebase, and is robust against future kinds being introduced upstream.
Finding 2 — Missing snapshot test for the new code path (most important gap)
File: TUnit.Mocks.SourceGenerator.Tests/MockGeneratorTests.cs
The PR adds a runtime test (T15_Class_Implementing_Static_Abstract_Interface_Mockable) that verifies the mock compiles and runs. However, there's no snapshot generator test for a class that implements a static-abstract interface.
The three existing Interface_With_Static_Abstract_Members snapshot tests only cover the interface-target path. The fixed class-target path has no snapshot to verify:
- That no
_MockBridge.g.csis emitted for the class target. - That the class's own instance-virtual surface is still correctly mocked.
Without this, a future generator refactor could re-enable bridge generation for class targets and only the runtime test (in a later CI stage) would catch it. A snapshot test would lock in the "no bridge" invariant at the generator level.
Suggested test: Class_Implementing_Static_Abstract_Interface — mirror the T15 types in a source-gen snapshot test, and assert the generated output does not include a MockBridge type.
Finding 3 — Cleanup commit missed a #5677 reference in the doc-comment
File: TUnit.Mocks.SourceGenerator/Discovery/MemberDiscovery.cs (the TryCollectStaticAbstractFromInterface XML doc)
The third commit message explicitly states "Strip issue-number references from source comments." However, the TryCollectStaticAbstractFromInterface doc-comment still contains #5677. The cleanup was incomplete — worth removing for consistency.
Finding 4 — Oversized inline comment in MockGenerator.cs
File: TUnit.Mocks.SourceGenerator/MockGenerator.cs:93
The comment explaining the defence-in-depth check is ~165 characters on a single line. The codebase convention is short single-line // comments. For an invariant explanation this important, a multi-line block comment above the if is more readable:
// Defence-in-depth: MemberDiscovery never sets HasStaticAbstractMembers for class targets
// (TypeKind guard in TryCollectStaticAbstractFromInterface), so this branch is only entered
// for interface targets. The IsInterface check makes the intent explicit.
if (model.HasStaticAbstractMembers && model.IsInterface)Finding 5 — DiscoverMembersFromMultipleTypes doc-comment misleads about valid input
File: TUnit.Mocks.SourceGenerator/Discovery/MemberDiscovery.cs (~line 166)
The XML doc says "Used for multi-interface mocks like Mock.Of<T1, T2>()" — implying T1 is always an interface. But MockTypeDiscovery.TransformToModels can pass a class as the first element (for isPartialMock == true). TryCollectStaticAbstractFromInterface handles this correctly, but a future reader might assume the TypeKind guard is redundant. A one-line doc addition clarifying that the first element can be a class prevents that misunderstanding.
Finding 6 — ShouldCollectStaticAbstractFromInterfaces as a single-use method may invite misuse (minor style)
File: TUnit.Mocks.SourceGenerator/Discovery/MemberDiscovery.cs
The helper has exactly one caller and its doc-comment explicitly warns it should only be consumed by TryCollectStaticAbstractFromInterface. A named single-use private helper that warns against other callers can paradoxically attract wrong usage later. Inlining it as a named condition with the same comment inside TryCollectStaticAbstractFromInterface removes the ambiguity. That said, if the team prefers the named-helper style for readability, it's fine to keep — this is a style observation, not a correctness issue.
Summary
| # | Severity | Description |
|---|---|---|
| 1 | Medium | Guard should be == TypeKind.Interface, not != TypeKind.Class |
| 2 | Medium | No snapshot generator test for the class-target (fixed) path |
| 3 | Low | Stray #5677 reference in doc-comment after the cleanup commit |
| 4 | Low | Oversized inline comment; prefer multi-line block above the if |
| 5 | Low | DiscoverMembersFromMultipleTypes doc misleads about valid input types |
| 6 | Nit | ShouldCollectStaticAbstractFromInterfaces could be inlined |
Findings 1 and 2 are the most actionable before merge. The rest are polish.
- ShouldCollectStaticAbstractFromInterfaces now uses TypeKind == Interface rather than != Class, so structs/enums/delegates can never satisfy the guard either. Robust against future TypeKinds. - Adds Class_Implementing_Static_Abstract_Interface snapshot mirroring T15: a class implementing an interface with a static-abstract member plus an instance virtual member. Verified output contains zero MockBridge types. - Removes the lingering #5677 reference from TryCollectStaticAbstractFromInterface (keeps the CS0527/CS0540 rationale). - Reformats the long single-line invariant comment in MockGenerator into a 3-line block above the IsInterface gate. - Clarifies DiscoverMembersFromMultipleTypes XML doc that the first element may be a class (via MockTypeDiscovery.TransformToModels with isPartialMock), so the TypeKind guard inside the static-collection helper is genuinely required and not redundant.
|
3rd-round review feedback applied in 5b24c60. Per item:
Test status (all net10.0):
No |
There was a problem hiding this comment.
Code Review: fix(mocks): skip MockBridge for class targets with static-abstract interfaces
The fix is correct, well-structured, and the iterative refinement across the commits has landed in a good state. The structural guard via TryCollectStaticAbstractFromInterface is the right approach — it makes it impossible to reintroduce the regression without going through the gated helper. Below are remaining observations, roughly ordered by actionability.
1. seenFullMethods is not threaded through CollectStaticAbstractMember (pre-existing gap, now visible)
DiscoverMembers maintains two deduplication sets: seenMethods (keyed on name + parameter types, ignoring return type) and seenFullMethods (keyed on name + params + return type). For ordinary instance members, both are used to detect the return-type-differing overload case (e.g. IEnumerable<T>.GetEnumerator vs IEnumerable.GetEnumerator) and emit the explicit interface impl variant.
CollectStaticAbstractMember — and therefore TryCollectStaticAbstractFromInterface — only uses seenMethods. If a static-abstract interface hierarchy ever has two methods with the same name/params but different return types, the second one is silently dropped. In practice this is exotic for static-abstract members, but it represents an inconsistency between the two deduplication paths. This PR didn't introduce it, but now that this code is being touched, cleaning it up would keep the two paths aligned.
2. #5677 reference remains in a test comment (stale after the cleanup commit)
The commit message says "Strip issue-number references from source comments." That was done in production source, but MockGeneratorTests.cs still contains a // ... see #5677 reference. The compiler-error codes (CS0527, CS0540) are self-contained and timeless; the issue reference is not. Worth removing for consistency.
3. The snapshot test implicitly encodes "no bridge" but doesn't assert it explicitly
Class_Implementing_Static_Abstract_Interface.verified.txt correctly omits any bridge interface type — confirming the fix. The assertion is implicit: if bridge generation is accidentally re-enabled, the snapshot diff will catch it via a new ===== FILE SEPARATOR ===== section. That is acceptable.
A brief comment in the test explaining why the output contains exactly N file sections (no bridge = one fewer than the interface-with-static-abstract snapshot) would make future debugging faster and turn the implicit assertion into explicit intent.
4. TryCollectStaticAbstractFromInterface has 11 parameters — symptom of a missing MemberCollectionContext
private static void TryCollectStaticAbstractFromInterface(
ISymbol member,
ITypeSymbol typeSymbol,
string interfaceFqn,
List<MockMemberModel> methods,
List<MockMemberModel> properties,
List<MockEventModel> events,
Dictionary<string, (int Index, ITypeSymbol? ReturnType)> seenMethods,
Dictionary<string, int?> seenProperties,
HashSet<string> seenEvents,
ref int memberIdCounter)The same 7-parameter "collection context" tuple (methods, properties, events, seenMethods, seenProperties, seenEvents, memberIdCounter) is threaded through CollectStaticAbstractMember, CollectInstanceMembers, ProcessClassMembers, and MergePropertyAccessors throughout the file. Introducing a MemberCollectionContext ref struct would reduce each call site from 7 positional arguments to one, eliminating the risk of accidentally swapping similarly-typed arguments (e.g. the two Dictionary<...> parameters).
This is clearly out of scope for a targeted bug fix — noting it as a future improvement rather than a blocker.
5. ShouldCollectStaticAbstractFromInterfaces is a one-line predicate with a single caller
The predicate centralises the TypeKind.Interface check with a named concept and a doc comment explaining the CS0527/CS0540 rationale. The doc comment on TryCollectStaticAbstractFromInterface already carries that full explanation. The extra indirection adds a little navigation overhead without adding information not already present at the call site.
Options:
- Inline it with the same comment, removing one level of indirection.
- Keep it as-is if the team prefers named predicates for readability (perfectly valid).
Either is fine. The implementation is correct either way.
Summary
| # | Severity | Actionable now? | Description |
|---|---|---|---|
| 1 | Low | Optional | seenFullMethods not threaded to CollectStaticAbstractMember — pre-existing inconsistency now visible |
| 2 | Low | Yes | Stale #5677 reference in test comment after cleanup commit |
| 3 | Low | Optional | Snapshot test comment could note expected file-section count and why |
| 4 | Nit | Future | 11-parameter helper is a symptom of missing MemberCollectionContext |
| 5 | Nit | Optional | ShouldCollectStaticAbstractFromInterfaces could be inlined |
Item 2 is the only one worth addressing before merge. Everything else is polish or future work. The core fix is sound and the gating via TryCollectStaticAbstractFromInterface is the right architectural decision.
Doc-only follow-up to review feedback on PR #5681: - Remove `#5677` from the `Class_Implementing_Static_Abstract_Interface` test comment; compiler-error codes (CS0527, CS0540) are timeless, the issue reference is not. - Make the snapshot's "no bridge" assertion explicit: add a one-line note that the verified output intentionally omits `_MockBridge.g.cs` for class targets, turning the implicit file-count assertion into stated intent for future debugging. No logic changes.
|
Final-round review feedback addressed in 8cdd519 (doc-only follow-up to 5b24c60): Applied (both items 2 and 3):
Deferred (per reviewer guidance):
Tests (net10.0, all green):
Codacy: ran |
There was a problem hiding this comment.
Code Review (follow-up): previous issues addressed
This is a status check against the concerns raised in the earlier claude[bot] reviews. The PR is now in good shape.
Previously raised — now resolved
1. Duplicate logic in two collection loops
Both DiscoverMembers (~line 56) and DiscoverMembersFromMultipleTypes (~line 202) now funnel through TryCollectStaticAbstractFromInterface, so the guard is enforced in exactly one place. No future loop can silently bypass it. ✅
2. Guard too permissive (!= TypeKind.Class vs == TypeKind.Interface)
ShouldCollectStaticAbstractFromInterfaces now uses:
=> typeSymbol.TypeKind == TypeKind.Interface;This correctly rejects structs, enums, delegates, etc. ✅
Still open (pre-existing gap, not introduced by this PR)
seenFullMethods is not passed to CollectStaticAbstractMember
The caller (TryCollectStaticAbstractFromInterface) receives seenMethods (name + param key) but not seenFullMethods (name + params + return type). This means if a static-abstract interface ever declares methods that differ only by return type, deduplication would miss the collision and silently emit both. In practice this is extremely unlikely for static-abstract members (they can't do return-type covariance the same way instance members can), but it is an inconsistency. No action needed for this PR — just worth tracking as a follow-up cleanup.
Observation: snapshot correctly omits bridge file
The verified snapshot for Class_Implementing_Static_Abstract_Interface contains only three sections (ImplFactory, Members, StaticExtension) and intentionally has no _MockBridge.g.cs section. The comment in the test method explaining this absence is clear and valuable for future readers. Good call.
Overall: The previous review concerns have been addressed. The fix is correct, the guard is tight, and the test coverage directly exercises the regression case. Ready to merge.
thomhurst
had a problem deploying
to
Pull Requests
GitHub Actions
Failure
…ract-5677 # Conflicts: # TUnit.Mocks.Tests/KitchenSinkEdgeCasesTests.cs
|
Resolved merge conflicts with Conflict scope: Two regions in
Resolution: kept all real types and tests from both sides (T14, T14b, T15) and dropped both "SKIPPED" placeholder comments since each side now has a working implementation. No type or method names collide. Verification (net10.0):
No |
There was a problem hiding this comment.
Code Review (Final)
All concerns raised in the previous reviews have been addressed across the iterative commits. Here is a clean summary of the current state.
Previous issues — all resolved
| Issue | Resolution |
|---|---|
| Duplicate guard in two collection loops | Both now funnel through — the guard is enforced in one place |
| Guard was (too permissive) | Now correctly , rejecting structs, enums, delegates, etc. |
| Redundant in MockGenerator | Kept as defence-in-depth with a clear comment explaining it is unreachable for class targets — intent is legible |
DiscoverMembersFromMultipleTypes doc misleading about class inputs |
Doc-comment updated to note the first element can be a class |
Stray #5677 reference in test comment |
Stripped in the doc-only cleanup commit |
| Snapshot test missing for class-target path | Class_Implementing_Static_Abstract_Interface.verified.txt added; absence of a bridge section is the assertion |
Merge-from-main commit (e39e5e03d5d0)
The only substantive change to the mocks source in the merge commit is in MemberDiscovery.cs:
- Direction = ParameterDirection.In
+ Direction = p.GetParameterDirection()This is unrelated to the static-abstract fix — it is the T14b in-parameter indexer correction from main. It is correct and does not interact with the TypeKind guard.
Remaining pre-existing gap (non-blocking)
seenFullMethods is not threaded through CollectStaticAbstractMember, meaning if a static-abstract interface ever declares two methods with the same name/params but differing return types, the second would be silently dropped. This is an extremely unlikely scenario for static-abstract members and was not introduced by this PR. Worth tracking as future cleanup.
Overall
The fix is correct, the guard is tight (== TypeKind.Interface), the test coverage directly exercises the regression case (T15 restored, snapshot locks the no-bridge invariant), and all previous review suggestions have been incorporated. The PR is ready to merge.
There was a problem hiding this comment.
Code Review (Final)
All concerns raised in the previous claude[bot] reviews have been addressed across the iterative commits. Here is a clean summary of the current state.
Previous issues — all resolved
| Issue | Resolution |
|---|---|
| Duplicate guard in two collection loops | Both now funnel through TryCollectStaticAbstractFromInterface — the guard is enforced in one place |
Guard was != TypeKind.Class (too permissive) |
Now correctly == TypeKind.Interface, rejecting structs, enums, delegates, etc. |
Redundant && model.IsInterface in MockGenerator |
Kept as defence-in-depth with a clear comment explaining it is unreachable for class targets — intent is legible |
DiscoverMembersFromMultipleTypes doc misleading about class inputs |
Doc-comment updated to note the first element can be a class |
Stray #5677 reference in test comment |
Stripped in the doc-only cleanup commit |
| Snapshot test missing for class-target path | Class_Implementing_Static_Abstract_Interface.verified.txt added; absence of a bridge section is the assertion |
Merge-from-main commit (e39e5e0)
The only substantive change to the mocks source in the merge commit is in MemberDiscovery.cs:
- Direction = ParameterDirection.In
+ Direction = p.GetParameterDirection()This is unrelated to the static-abstract fix — it is the T14b in-parameter indexer correction from main. It is correct and does not interact with the TypeKind guard.
Remaining pre-existing gap (non-blocking)
seenFullMethods is not threaded through CollectStaticAbstractMember, meaning if a static-abstract interface ever declares two methods with the same name/params but differing return types, the second would be silently dropped. This is an extremely unlikely scenario for static-abstract members and was not introduced by this PR. Worth tracking as future cleanup.
Overall
The fix is correct, the guard is tight (== TypeKind.Interface), the test coverage directly exercises the regression case (T15 restored, snapshot locks the no-bridge invariant), and all previous review suggestions have been incorporated. The PR is ready to merge.
1 participant
Summary
_MockBridge.g.cswhen the mock target is a class. Bridge shape is only required for interface targets that need a runtime dispatch hook for static-abstract members; a class already provides the concrete static implementation directly.MemberDiscoveryso static-abstract interface members are not collected for class targets, plus a defensivemodel.IsInterfacecheck inMockGenerator.GenerateSingleTypeMock.T15_Class_Implementing_Static_Abstract_Interface_Mockablekitchen-sink test.Why
Previously
MemberDiscoverywalked a class's implemented interfaces and pulled in their static-abstract members, which setHasStaticAbstractMembers = trueon the model.MockBridgeBuilderthen emitted a bridge that put the class in its own interface base list (CS0527) and produced explicit interface impls without listing the interface (CS0540), making any class that implements a static-abstract interface unmockable.Test plan
TUnit.Mocks.Tests/KitchenSinkEdgeCasesTests.cs) passesStaticAbstractMemberTests(interface-target path): 13/13TUnit.Mocks.Testsfull suite (net10.0): 953/953TUnit.Mocks.SourceGenerator.Tests(net10.0): 45/45Closes #5677