fix(mocks): skip interfaces with inaccessible abstract members - #6502
#6502fix(mocks): skip interfaces with inaccessible abstract members#6502thomhurst merged 5 commits intomainthomhurst/TUnit:mainfrom fix/6491-unimplementable-interfacethomhurst/TUnit:fix/6491-unimplementable-interfaceCopy head branch name to clipboard
Conversation
Since C# 8 an interface may declare non-public abstract members. RavenDB's ISessionBlittableJsonConverter.MissingProperties is `internal`, so no type outside its assembly can implement the interface. Auto-mock discovery walked into it anyway when it turned up as a member return type: mocking IDocumentStore produced CS0535/CS0548/CS0551/CS0122 on generated code for an interface the user never asked to mock, with no per-member opt-out. Drop such interfaces at discovery, and suppress the auto-mock factory reference on members that return them so the boundary type still compiles. Members with a default implementation are ignored — they are not the implementer's problem. Direct mock attempts now report TM007 naming the offending member, instead of the CS0535 cascade. Fixes #6491
Greptile SummaryThe PR prevents generation for interfaces whose required abstract members cannot be implemented and adds TM007 diagnostics for direct mock requests.
Confidence Score: 3/5The PR is not yet safe to merge because static-style mock requests can bypass TM007 when they bind to TUnit's generic fallback. The generator omits the per-type extension for an unimplementable interface, but the analyzer treats the remaining generic TUnit Mock extension as a non-entry-point call and returns before syntactic target recovery, leaving the request without the promised compile-time diagnostic. Files Needing Attention: src/TUnit.Mocks.Analyzers/InaccessibleInterfaceMemberMockAnalyzer.cs
|
| Filename | Overview |
|---|---|
| src/TUnit.Mocks.SourceGenerator/Discovery/InterfaceImplementability.cs | Centralizes checks that required interface members and accessors can be implemented by generated code. |
| src/TUnit.Mocks.SourceGenerator/Discovery/MockTypeDiscovery.cs | Drops unimplementable interfaces from direct, combined, attribute, and transitive generation paths. |
| src/TUnit.Mocks.SourceGenerator/Discovery/MemberDiscovery.cs | Prevents auto-mock factory references for interfaces deliberately excluded from generation. |
| src/TUnit.Mocks.Analyzers/InaccessibleInterfaceMemberMockAnalyzer.cs | Adds TM007 coverage and avoids unrelated bound Mock calls, but the semantic guard also suppresses the diagnostic when TUnit's generic static-style fallback binds. |
| tests/TUnit.Mocks.Analyzers.Tests/InaccessibleInterfaceMemberMockAnalyzerTests.cs | Covers inaccessible members, accessors, attributes, inheritance, and unrelated extensions, but not the real generic TUnit fallback binding. |
| tests/TUnit.Mocks.SourceGenerator.Tests/Issue6491Tests.cs | Exercises transitive discovery and verifies unimplementable interfaces are omitted without affecting implementable siblings. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Mock request discovered] --> B{All required interface members accessible?}
B -->|Yes| C[Generate mock model and factory]
B -->|No| D[Skip generated implementation]
D --> E{Direct request recognized by analyzer?}
E -->|Yes| F[Report TM007]
E -->|No| G[Generic Mock extension may run without a factory]
Reviews (4): Last reviewed commit: "test(mocks): make the snapshot harness's..." | Re-trigger Greptile
There was a problem hiding this comment.
Review
The fix for the RavenDB internal-member case (issue #6491) is well-targeted and the test coverage for that scenario is solid. However, I found a correctness bug in the accessibility check itself, plus a gap in coverage, both of which I verified experimentally rather than just from reading the diff.
1. protected interface members are incorrectly treated as unmockable (false positive / regression)
InterfaceImplementability.CanBeImplemented (and the mirrored logic in InaccessibleInterfaceMemberMockAnalyzer) uses:
compilation.IsSymbolAccessibleWithin(member, compilation.Assembly)IsSymbolAccessibleWithin evaluates accessibility relative to the within context. When within is an assembly (not a type), a protected member always evaluates to false — protected accessibility is defined relative to a derived type, not an assembly. I confirmed this directly:
// same compilation, no cross-assembly involved at all
protected string Hidden { get; set; } // DeclaredAccessibility=Protected, IsSymbolAccessibleWithin(assembly)=False
internal string Internal { get; set; } // DeclaredAccessibility=Internal, IsSymbolAccessibleWithin(assembly)=TrueBut a protected interface member is implementable by any class in any assembly via explicit interface implementation — I verified this compiles and runs across two separate assemblies (LibA declares protected string Hidden, AppB's Impl : IFoo provides string IFoo.Hidden { get; set; }, builds and runs clean). Checking accessibility against the implementing type instead of the assembly confirms this:
compilation.IsSymbolAccessibleWithin(member, mockType) // mockType : IFoo → True for the protected memberSo the test file's own framing is off — the comment in InaccessibleInterfaceMemberMockAnalyzerTests.cs says "protected members... are unimplementable by a class in any assembly," but that's not true; only internal/private protected members are assembly-gated. As written, this PR will make any interface with a protected abstract member silently drop out of mock generation (or hard-error with TM007 if mocked directly) even though it always worked and always could work.
Suggested fix: don't check accessibility against the bare assembly. Either check against the (hypothetical) implementing type, or special-case the two accessibility kinds that are actually assembly-scoped:
var accessibility = member.DeclaredAccessibility;
if (accessibility is Accessibility.Internal or Accessibility.ProtectedAndInternal
&& !SymbolEqualityComparer.Default.Equals(member.ContainingAssembly, compilation.Assembly))
{
return false; // or: return member;
}Protected and ProtectedOrInternal should always be treated as implementable — that's the entire point of C# 8's protected interface members.
2. Multi-interface Mock.Of<T1, T2>() doesn't get the same protection when the problem interface is T2/T3/T4
In MockTypeDiscovery.TransformToModels, the new CanBeImplemented guard is applied to the primary type (BuildSingleTypeModel(namedType, ...) at line ~146) and to transitive return-type discovery, but for additional interfaces in a multi-type mock:
var standalone = BuildSingleTypeModel(additionalType, isPartialMock: false, compilationAssembly, compilation);
if (standalone is null)
{
mapsBuilder.Add(EquatableArray<int>.Empty);
continue; // only skips the pair-model/map — additionalType is still in additionalInterfaceNames
}additionalInterfaceNames (used to build multiTypeModel.AdditionalInterfaceNames) is computed earlier, unconditionally, from all type arguments. MockImplBuilder then unconditionally lists every AdditionalInterfaceNames entry in the generated impl's base-type list (MockImplBuilder.cs:39-41 and :453-455). Since MemberDiscovery's existing external-accessibility filter still drops the inaccessible member from generation, the impl ends up declaring : T1, T2 without implementing T2's inaccessible member — reproducing the exact CS0535 cascade this PR fixes, just for Mock.Of<T1, T2>()/Of<T1, T2, T3>()/Of<T1, T2, T3, T4>() instead of the single-type entry point.
The new InaccessibleInterfaceMemberMockAnalyzer doesn't catch this either — ResolveMockTarget only handles methodSymbol.TypeArguments.Length == 1, so multi-interface calls get no TM007 and go straight to a raw compiler error, same bad UX as issue #6491 originally had.
Suggested fix: when standalone is null for an additional type, drop it from additionalInterfaceNames/AdditionalInterfaceNames (not just skip its pair model), and extend the analyzer's ResolveMockTarget to walk all of methodSymbol.TypeArguments, not just index 0.
Nit
Given finding #1, most of InaccessibleInterfaceMemberMockAnalyzerTests (which uses protected as the "unimplementable" stand-in) will need updating to use a genuinely assembly-gated accessibility (internal/private protected in a separate compilation, matching what Issue6491Tests already does for the generator side) once the accessibility check is corrected.
…pe combos Two review findings, both reproduced before fixing. IsSymbolAccessibleWithin(member, assembly) evaluates protected relative to an assembly, where it is always false — but a protected interface member is implementable from any assembly through explicit interface implementation. Verified with a two-assembly probe: the implementing class compiles clean while the check reports the member inaccessible. As written that would have dropped every interface with a protected abstract member out of generation. InterfaceImplementability now delegates to MemberDiscovery.IsMemberAccessible — the same rule that decides which members reach the model, so a required member discovery drops is exactly one the impl would fail to implement — and the analyzer mirrors it, gating only internal and private protected. Mock.Of<T1, T2>() listed every additional interface in the impl's base-type list without the implementability check, reproducing the same CS0535 cascade for T2/T3/T4. The combo is now skipped, and the analyzer walks all type arguments so TM007 still names the offending interface. Analyzer tests move to real cross-assembly compilations via a new VerifyAnalyzerWithLibraryAsync helper — same-assembly internal members are implementable, so the old single-compilation tests could not have exercised the reporting path.
|
Both findings confirmed and fixed in 4bb8a2b. 1. So Rather than special-casing the accessibility kinds inline, 2. Multi-type combos — confirmed. 3. Tests. You were right that the old On extracting the shared accessibility logic: done on the generator side (single call into Analyzer tests 41 passed, generator tests 84 passed, |
Accessors are no longer skipped. C# permits a per-accessor modifier on an
interface property (`string Value { get; internal set; }` compiles), so a
reachable property can still declare a slot no outside implementer can
provide — MemberDiscovery drops that accessor and the impl then fails the
interface contract. The diagnostic names the property, not the accessor.
[assembly: GenerateMock(typeof(T))] is a third entry point that produces no
invocation, so an unimplementable T was dropped by the generator with
nothing reported. The analyzer now inspects the attribute too.
The T.Mock() fallback no longer claims every zero-argument Type.Mock().
Probed the real symbol shape against Roslyn 4.14: the generated member's
containing type is the synthesised, unnamed extension type, whose own
containing type is the *_MockStaticExtension class — so the walk out to
that class is what distinguishes ours from another library's Mock()
extension, which now binds and is left alone.
ReviewRe-reviewed at HEAD ( Both previously-flagged correctness bugs are fixed and hold up under inspection:
I also traced the third generator entry point, Design notes (no action needed):
No further issues found. The end-to-end regression test for the original RavenDB shape ( |
…inistic The macOS CI job failed the two [assembly: GenerateMock]-driven tests (Kiota and GenerateMock_Attribute_With_Concrete_Class) while every other test passed, and both pass locally on Windows in Debug and Release. The reference set is the only platform-varying input. It is built from AppDomain.CurrentDomain.GetAssemblies(), which returns only assemblies the runtime has already faulted in — so whether TUnit.Mocks is referenced depends on which test ran first — plus a ref/TUnit.Mocks.dll copy whose <None Include> glob expands at evaluation time, so a clean build that evaluates this project before TUnit.Mocks/netstandard2.0 has produced output ships without it. With no TUnit.Mocks reference, GenerateMockAttribute doesn't bind, the generator emits nothing, and the failure surfaces as a snapshot mismatch rather than a missing reference. Fall back to the assembly this project references, and throw with the real reason if neither source yields one. Verified by deleting the ref copy: 84 passed with and without it.
|
CI note — the macOS failure on ad22d9b was a latent fragility in the snapshot harness, not the diagnostic logic. Fixed in 5e42fac. Only the two
With no
The 6504 windows failure was separate and unrelated to mocks — |
| // extension, or a static Mock() the user declared. Not a TUnit mock request. | ||
| if (!IsGeneratedStaticEntryPoint(methodSymbol)) | ||
| { | ||
| return Enumerable.Empty<INamedTypeSymbol>(); | ||
| } |
There was a problem hiding this comment.
Generic fallback suppresses TM007
When an unimplementable interface's T.Mock() call binds to the generic TUnit.Mocks.MockExtension.Mock<T>() fallback, this branch classifies the bound method as non-generated and returns before syntactic target recovery, so TM007 is omitted and the call instead reaches Mock.Of<T>() at runtime without a generated factory.
There was a problem hiding this comment.
Review
Re-reviewed after the latest commits. Both findings from the previous review round are now correctly fixed, and I verified the fix logic directly rather than just reading the diff:
1. protected/protected internal interface members — InterfaceImplementability.CanBeImplemented now delegates to MemberDiscovery.IsMemberAccessible, which only gates Internal/ProtectedAndInternal cross-assembly, leaving Protected/ProtectedOrInternal implementable. This is the same rule that decides which members reach the generated model, so there's no risk of the two definitions drifting for the generator side. Good call routing through the existing rule instead of re-deriving a second one.
2. Multi-interface Mock.Of<T1, T2, ...>() — MockTypeDiscovery.TransformToModels now checks CanBeImplemented on every type argument inside the same loop that validates they're interfaces (for (int i = 1; i < method.TypeArguments.Length; i++)), so this generically covers T2/T3/T4, not just the T2 case called out before. Confirmed the analyzer's ResolveMockTargets walks methodSymbol.TypeArguments for the same reason. This is a cleaner fix than the originally-flagged approach (which special-cased "drop the standalone model but leave the name in the base-type list") — aborting the whole combo before any names get collected removes the class of bug entirely instead of patching around it.
Other things checked:
- The analyzer/generator duplication (
FindInaccessibleAbstractMembervsInterfaceImplementability) is intentional and documented — no shared project between the two assemblies, so a second small copy is reasonable, and both are pinned to the same underlying accessor semantics. [assembly: GenerateMock(typeof(T))]and theT.Mock()static entry point are both covered by TM007 now, closing the silent-failure gaps called out in the last round.- New
VerifyAnalyzerWithLibraryAsynccorrectly exercises real cross-assemblyinternalaccessibility, which the prior single-compilation tests structurally could not do — this fixes the test/reality mismatch flagged before (the tests usingprotectedas an "unimplementable" stand-in). TM007(notTM006) avoids an ID collision with the in-flight #6501, which also adds a TUnit.Mocks diagnostic — good coordination.- Accessor-level checks (
get/setwith differing accessibility) are handled, with the diagnostic correctly naming the property rather than the synthetic accessor method.
No new issues found. Nice, thorough iteration — the commit messages documenting what was verified experimentally at each step (the two-assembly protected-member probe, the Roslyn 4.14 extension-type shape probe) made this much easier to re-verify than a typical PR.
Fixes #6491
Problem
Mock.Of<Raven.Client.Documents.IDocumentStore>()produced 9 errors on generated code:Confirmed against the shipped assembly —
MissingProperties's accessors carryAssemblyaccessibility:Since C# 8 an interface may declare non-public abstract members, and no type outside the declaring assembly can implement them.
MemberDiscoverycorrectly filtered the inaccessible member out of the model, so the generated impl never declared it — but the impl still had to satisfy the interface, hence CS0535.The user never asked to mock that interface: transitive auto-mock discovery reached it from
IDocumentStore, so there was no way to opt out from the call site.Fix
InterfaceImplementability.CanBeImplemented— an interface is mockable only when every abstract member it (or a base interface) requires an implementer to provide is accessible from the consuming compilation. Members with a default implementation are ignored; they are not the implementer's problem.BuildSingleTypeModelreturns null for those interfaces, so they are dropped from every discovery path — direct,[GenerateMock], and the transitive walk.GetAutoMockFactoryMethodapplies the same rule, so a member returning such an interface falls back to a plain default rather than referencing a factory that was deliberately not generated (which would be CS0400).TM007+InaccessibleInterfaceMemberMockAnalyzerreports at the call site when the interface is mocked directly, naming the member responsible — replacing the CS0535 cascade with one actionable error.Tests
tests/TUnit.Mocks.SourceGenerator.Tests/Issue6491Tests.cs— the reported shape end-to-end: an external assembly whoseIDocumentStoreexposes an interface with aninternalabstract member. Asserts no CS0535/CS0548/CS0551/CS0122 in the generated compilation, that the unimplementable interface gets no impl, and that a sibling public interface still gets its auto-mock factory.tests/TUnit.Mocks.Analyzers.Tests/InaccessibleInterfaceMemberMockAnalyzerTests.cs— 8 cases covering inaccessible property / method / inherited member, both entry points, and the non-reporting cases (all-public members, default-implemented non-public member, same-assemblyinternal, class targets).Analyzer tests 38 passed, generator snapshots 82 passed,
TUnit.Mocks.Tests1160 passed, Http 54, Logging 31.