+semver:minor - feat(mocks): runtime auto-stubs for interfaces the source generator cannot see - #6519
Conversation
…urce generator cannot see A loose-mode mock member that must produce an interface with no source-generated factory — most notably a generic method like IFeatures.Get<T>() invoked inside a third-party SDK with a T that is internal to that SDK, unnameable by the test assembly — fell through to null, surfacing as an NRE deep in SDK code (#6514). Runtime-proxy libraries handle this via Castle auto-substitution, and its absence was a structural migration blocker. The engine's auto-mock fallback now emits a functional stub at runtime when the registry has no factory: a Reflection.Emit type in a dynamic assembly named DynamicProxyGenAssembly2 carrying Castle DynamicProxy's well-known public key, so the InternalsVisibleTo grants SDKs already extend for NSubstitute/Moq compatibility apply to TUnit.Mocks too. Dynamic assemblies are never strong-name verified, so the public key alone satisfies the grant. Stubs are recursive with NSubstitute-style defaults — empty strings, completed tasks, empty collections, zeroed value types, further stubs (or a real configurable Mock<T> when a generated factory exists) for interface members — properties round-trip set values, and member results are cached for stable identity. Generic interface methods are supported, including constraint re-emission. Not configurable or verifiable: their types cannot be named by the test. Guards: loose mode only; RuntimeFeature.IsDynamicCodeSupported (inert on Native AOT, retaining the previous defaults); opt-out via TUnitMocksSettings.RuntimeAutoStubs; emission failures (no IVT grant, static abstracts, by-ref returns) are cached misses that fall back to the previous behavior. netstandard2.0 keeps a no-op implementation. Fixes #6514
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Greptile SummaryRuntime auto-stubs now provide loose-mode defaults for interfaces unavailable to source generation.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| src/TUnit.Mocks/RuntimeStubs/RuntimeStub.cs | Stores property and indexer state and caches defaults by member slot and closed return-type identity, resolving the previously reported generic-instantiation collision. |
| src/TUnit.Mocks/RuntimeStubs/RuntimeStubGenerator.cs | Emits cached runtime implementations for supported closed interfaces, including generic method signatures, constraints, accessors, events, and out-parameter initialization. |
| src/TUnit.Mocks/RuntimeStubs/RuntimeStubDefaults.cs | Produces recursive mocks or stubs and conventional defaults for strings, tasks, collections, arrays, nullable types, and value types. |
| src/TUnit.Mocks/MockEngine.cs | Adds runtime stubs to loose auto-mock fallback and keys the engine cache by member name and exact return-type identity. |
| tests/TUnit.Mocks.Tests/Issue6514Tests.cs | Covers inaccessible interfaces, recursive defaults, settings and platform guards, generic-instantiation isolation, identity caching, and stateful properties and indexers. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Loose mock member call] --> B{Generated mock factory exists?}
B -->|Yes| C[Return configurable generated mock]
B -->|No| D{Runtime auto-stubs enabled and dynamic code supported?}
D -->|No| E[Return previous default value]
D -->|Yes| F[Emit or reuse interface stub type]
F --> G[Create and cache stub per member and return type]
G --> H[Return recursive default or stored property/indexer value]
Reviews (9): Last reviewed commit: "fix(mocks): preserve custom modifiers on..." | Re-trigger Greptile
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 722f6f2571
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| protected object? GetReturnValue(int slot, Type returnType) | ||
| => _returnCache.GetOrAdd(slot, static (_, t) => RuntimeStubDefaults.GetDefault(t), returnType); |
There was a problem hiding this comment.
When a runtime stub member returns an ordinary class, delegate, nullable value, or another unsupported reference type, RuntimeStubDefaults.GetDefault returns null, but ConcurrentDictionary.GetOrAdd rejects null values and throws ArgumentNullException. Thus loose stubs throw instead of returning the documented default; use a sentinel or bypass caching when the computed value is null.
Useful? React with 👍 / 👎.
| protected object? GetReturnValue(int slot, Type returnType) | ||
| => _returnCache.GetOrAdd(slot, static (_, t) => RuntimeStubDefaults.GetDefault(t), returnType); |
There was a problem hiding this comment.
For an emitted generic member such as T Get<T>(), every constructed invocation shares the same numeric slot. After Get<int>() caches a boxed integer, a later Get<string>() retrieves that value and the emitted unbox.any/cast fails with InvalidCastException. Key the cache by both slot and the runtime return type so different generic instantiations retain independent defaults.
Useful? React with 👍 / 👎.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review
Clever fix for #6514 — reusing Castle DynamicProxy's well-known public key to piggyback on SDKs' existing InternalsVisibleTo grants is a neat way to reach unnameable internal types without needing per-SDK configuration. AOT/trimming guards, the opt-out flag, and the docs are all solid.
Two correctness issues, one already partially flagged by Greptile:
1. Generic-method return cache collides across type arguments (confirmed, same as Greptile's finding)
RuntimeStub.GetReturnValue(int slot, Type returnType) (RuntimeStub.cs:99-100) keys the cache by slot alone:
protected object? GetReturnValue(int slot, Type returnType)
=> _returnCache.GetOrAdd(slot, static (_, t) => RuntimeStubDefaults.GetDefault(t), returnType);RuntimeStubGenerator explicitly supports generic methods on the stubbed interface (constraint re-emission, DefineGenericParameters, etc. in RuntimeStubGenerator.cs:490-536), and each generic method gets one fixed slot. If a single stub instance invokes that method twice with different type arguments (e.g. Convert<int>() then Convert<string>()), the second call reuses the first call's cached boxed value and Unbox_Any throws InvalidCastException — the generic-method support is untested for this exact case (Issue6514Tests only exercises Get<T>() on the source-generated mock of IStubFeatures, never a generic method declared on a runtime-stubbed interface).
Suggested fix: key the cache by (int Slot, Type ReturnType) instead of int alone — e.g. ConcurrentDictionary<(int, Type), object?>. That preserves per-instantiation identity stability (the stated goal) while eliminating the collision, and is a minimal change to the existing structure. Please also add a test that invokes a stubbed generic method with two different type arguments on the same stub instance.
2. Unsynchronized ModuleBuilder/TypeBuilder access under TUnit's parallel-by-default execution
EmitStubType (RuntimeStubGenerator.cs:349-425) calls module.DefineType(...) / tb.CreateType() with no synchronization beyond the ConcurrentDictionary.GetOrAdd on _stubTypes. GetOrAdd's factory can run concurrently for different keys — so two tests running in parallel that each first-touch a distinct never-before-stubbed interface will call into EmitStubType on the same shared ModuleBuilder from different threads simultaneously.
ModuleBuilder.DefineType/TypeBuilder.CreateType are documented as not thread-safe on coreclr without external synchronization (see dotnet/runtime#64094, "Generation of dynamic types with multiple threads ... causes runtime corruption"). Notably, Castle DynamicProxy — whose identity this code deliberately impersonates — wraps its own ModuleScope.DefineType in a lock for exactly this reason (castleproject/Core#399); this PR borrows Castle's assembly identity but not its concurrency safeguard. Given TUnit tests run in parallel by default, this looks reachable in normal use, not just a theoretical edge case.
Suggested fix: wrap the type-definition/creation section of EmitStubType (or at minimum module.DefineType(...) through tb.CreateType()) in a static lock, matching Castle's approach.
Everything else — the defaults table, out-parameter zeroing, recursive stub/mock preference order in RuntimeStubDefaults.GetDefault, and the opt-out test — looks correct and well covered.
Review finding (#6519): a generic member like T Get<T>() on a runtime-stubbed interface shares one member slot across every instantiation, so the per-slot default cache handed Get<string>() the boxed value cached by Get<int>() and the emitted unbox/cast threw InvalidCastException. The cache key now pairs the slot with the closed return type observed at the call; property slots are unaffected (their type is constant) and same-instantiation identity stays stable.
|
Both P1s reviewed: Generic return cache conflation — valid, fixed in 9bba13e. "GetOrAdd rejects null values" — not correct, no change. |
There was a problem hiding this comment.
Review
Overall this is a well-scoped, carefully-guarded feature: gated behind RuntimeFeature.IsDynamicCodeSupported (inert on AOT), a documented opt-out, cached emission with a sane try/catch-to-null-on-miss fallback, and solid generic-constraint re-emission for the Get<T>() case the issue is actually about. The two P1s from the earlier review round (generic-instantiation cache conflation, GetOrAdd-null claim) both look correctly resolved in the current diff — the return cache is now keyed by (slot, closedReturnType) and there's a regression test for it (Stub_Generic_Method_Keeps_Instantiations_Separate).
One correctness issue I think is worth fixing before merge:
Indexer properties ignore their index argument and share a single cached value.
RuntimeStubGenerator.EmitAccessor (src/TUnit.Mocks/RuntimeStubs/RuntimeStubGenerator.cs:314-341) and EmitReturn (:362-394) key a property's cached value purely by slot — the emitted IL for a getter never loads the index parameter(s) onto the stack, and the setter only ever stores the last argument (the value) via SetPropertyValueMethod at that same fixed slot (:324-333). IsSupported (:189-207) only rejects static-abstract members and by-ref returns — it doesn't flag indexers, so an interface with an indexer is still considered "supported" and gets a stub silently emitted.
Concretely, for an interface with TValue this[TKey key] { get; set; } (e.g. IDictionary<,>-shaped interfaces, or something like ASP.NET's IHeaderDictionary — exactly the kind of HTTP "features" interface this PR's own motivating issue is about):
stub["A"] = valueA;
stub["B"] = valueB;
stub["A"]; // returns valueB, not valueA — same slot, index ignoredand for an unset indexer, stub[key1] and stub[key2] return the same cached default instance regardless of key. This is silent — it doesn't throw or fall back to the documented null/previous-default behavior, so it's a plausible source of confusing test failures for exactly the SDK-interface scenarios this feature targets.
Given the existing pattern in the file (reject unsupported shapes in IsSupported and let them fall back to the previous null-returning behavior, as already done for static-abstract members and by-ref returns), I'd suggest adding a check there for indexers (property.GetIndexParameters().Length > 0) rather than trying to key the cache by argument values — that keeps the fix small and consistent with how every other unsupported shape in this emitter is already handled, and it's a safe fallback since loose-mode null is already an accepted (if less convenient) outcome elsewhere in this design.
Everything else — the module/type caching, the DynamicProxyGenAssembly2 identity trick, the generic constraint re-emission, the AOT/netstandard2.0 fallback split — looks solid and well-tested. Nice work on the fast turnaround on the earlier review round.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9bba13e66d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (definition == typeof(ValueTask<>)) | ||
| { | ||
| return Activator.CreateInstance(type, GetDefault(typeArgs[0])); |
There was a problem hiding this comment.
When T is an ordinary reference type for which GetDefault(T) returns null, such as a sealed DTO, this reflective construction must choose between ValueTask<T>(T) and ValueTask<T>(Task<T>). Since null matches both unrelated parameter types, the default binder throws AmbiguousMatchException, so invoking a runtime-stub member returning ValueTask<Dto> fails instead of returning a completed task containing null. Select the constructor whose parameter is exactly T, or use a closed generic helper.
Useful? React with 👍 / 👎.
…alueTask default construction Review findings (#6519): Indexers: the emitted accessors forwarded only the member slot, so stub[1] = x made stub[2] return x and every index observed the last write. The emitted IL now boxes the index arguments into an object?[] and state lives in a per-(slot, structural index key) map; defaults stay index-independent for stable identity. Indexers with by-ref / ref-struct / pointer index parameters (which cannot be boxed) keep the previous slot-level behavior. ValueTask<T> defaults: ValueTask<T> has both (T) and (Task<T>) constructors, and a null inner default (reference-typed T) matches either, making Activator.CreateInstance ambiguous. The constructor is now selected explicitly by parameter type.
|
Second review round — both P2s verified and fixed in 5cae648: Indexer state keyed by index arguments — valid. Emitted accessors now box the index arguments into an
Suites: 1195/1195 net10.0, 1172/1172 net8.0. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5cae64836e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Two types with identical namespace-qualified names from different assemblies collided in the auto-mock/stub cache (memberName + FullName string), so the second invocation retrieved the first type's stub and the TReturn cast threw InvalidCastException. Cache is now keyed by (memberName, Type). TryGetAutoMock (no callers) updated to match.
|
Third round — Codex P2 (stub cache keyed by Valid: Regression test emits two dynamic assemblies each declaring Suites: 1196/1196 net10.0, 1173/1173 net8.0. |
ReviewChecked this round's changes against the three prior review rounds — the generic-instantiation cache fix ( One issue from round 1 appears to have been dropped rather than fixed:
I flagged this in the first review round; the reply addressing "Both P1s" fixed the generic-cache collision and a separate Suggested fix, consistent with the private static readonly Lock _emitLock = new();
...
lock (_emitLock)
{
var module = EnsureModule();
...
return tb.CreateType();
}This keeps the existing Everything else in this round — Type-identity cache keys across all call sites in |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 89c6d6bf54
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| var cacheKey = (memberName, typeof(TReturn)); | ||
| var autoMock = AutoMockCache.GetOrAdd(cacheKey, _ => | ||
| { | ||
| MockRegistry.TryCreateAutoMock(typeof(TReturn), Behavior, out var m); |
There was a problem hiding this comment.
When a generated non-generic member has 1–8 arguments and returns an interface without a generated factory—for example, IServiceProvider Resolve(int id), since MemberDiscovery.GetAutoMockFactoryMethod excludes System.* interfaces—the generator selects this typed handler. Its cache factory only queries MockRegistry, so a miss still returns null and never reaches the new RuntimeStubGenerator fallback used by the object-array handler. Apply the runtime-stub fallback to every typed arity as well, preferably through the shared helper.
Useful? React with 👍 / 👎.
…ugh stubs Review round 4: - EmitStubType now runs under a static lock: ConcurrentDictionary.GetOrAdd only de-duplicates per key, so distinct interfaces first-touched in parallel reached ModuleBuilder.DefineType/TypeBuilder.CreateType on the shared module concurrently (unsafe on coreclr, dotnet/runtime#64094). Cache hits stay lock-free. - The eight typed cache-miss handlers (1-8 args) in MockEngine.Typed.cs now call TryGetLooseAutoMockResult instead of an inlined registry-only lookup, so members returning interfaces without a generated factory (e.g. System.*) reach the runtime-stub fallback like the object-array handler.
1 participant
Summary
Fixes #6514 — feature parity with runtime-proxy libraries' auto-substitution. When a loose-mode mock member must produce an interface the source generator produced no mock for — most notably a generic method like
IInvocationFeatures.Get<T>()invoked inside a third-party SDK with aTthat isinternalto that SDK and structurally unnameable by the test assembly — the call fell through tonull, surfacing as an NRE deep in SDK code. This was a hard migration blocker from NSubstitute (reported against the Azure Functions Worker SDK).How it works
The key insight: the generated
Get<T>()override is itself generic, so at runtime the engine sees the realSystem.Typeeven when the test assembly can't name it. The engine's existing auto-mock fallback (TryGetLooseAutoMockResult→MockRegistry.TryCreateAutoMock) now falls back toRuntimeStubGeneratoron a registry miss:Reflection.Emittype per interface (cached), in a dynamic assembly namedDynamicProxyGenAssembly2carrying Castle DynamicProxy's well-known public key — the exact identity SDKs across the ecosystem already grantInternalsVisibleTofor NSubstitute/Moq compatibility, so every internal interface reachable by those proxies is reachable by these stubs. Dynamic assemblies are never strong-name verified; the public key alone satisfies the grant.""for strings, completed tasks (Task<T>/ValueTask<T>included), empty collections, zeroed value types, recursive stubs for interface members (or a real configurableMock<T>when a source-generated factory exists for that nested type). Properties round-trip set values. Generic interface methods are supported, including constraint re-emission and out-parameter zeroing.RuntimeFeature.IsDynamicCodeSupported— inert on Native AOT, which keeps the previous default-value behavior; opt-out viasettings.Mocks.RuntimeAutoStubs = false; any emission failure (interface with no matching IVT grant, static abstract members, by-ref returns) is a cached miss falling back to the previous behavior. netstandard2.0 builds carry a no-op implementation.Behavior change
In loose mode, unconfigured interface-returning calls that previously returned
null(no generated factory) now return a functional stub on JIT runtimes — hence+semver:minor. Opt-out restores the old behavior globally.Tests
Issue6514Tests(skipped on Native AOT via the existingSkipIfNotDynamicCodeSupportedattribute): the issue'sGet<T>-with-internal-type shape (the test project now grants IVT toDynamicProxyGenAssembly2exactly like real SDKs do), unregistered public interfaces, recursion, task/collection/string defaults, identity caching, registry-preferred real mocks viaMock.Get, value-type defaults unchanged, strict mode unchanged, and the opt-out. Full suites: 1192/1192 (net10.0), 1191/1191 (net9.0), 1169/1169 (net8.0); generator snapshots 90/90; Http 54/54, Logging 31/31.Docs: new Runtime Auto-Stubs section under Recursive / Auto-Mocking in
docs/writing-tests/mocking/advanced.md.