feat(mocks): discriminate generic-method mocks by type argument

[thomhurst]

Problem

Discussion #4981 (latest comment) reported that TUnit.Mocks cannot distinguish generic-method setups by their type argument:

public interface IGreeter { string Greet<T>() where T : class; }

var mock = IGreeter.Mock();
mock.Greet<Class1>().Returns("Hello!");
mock.Greet<Class2>().Returns("Goodbye!");

greeter.Greet<Class1>(); // returned "Goodbye!" — both setups collided

Root cause: the whole match key was (memberId, argument-matchers). A generic method's type arguments (typeof(Class1) vs typeof(Class2)) were dropped at the runtime boundary — MethodSetup, CallRecord, and FindMatchingSetup had no notion of them. The existing Generic_Method_Different_Type_Arguments test only passed because it also varied a normal int arg, which masked the bug. The reported case has zero parameters, so the two setups were byte-identical.

Fix

Thread an optional Type[]? typeArguments (closed typeof(T) captured at the call site) through setup registration, call dispatch, and verification. null ⇒ non-generic ⇒ identical behaviour to before.

• Runtime: MethodSetup/CallRecord carry the type args; MockEngine gains object[]+Type[] HandleCall/HandleCallWithReturn overloads and a TypeArgumentsMatch gate in FindMatchingSetup; CallVerificationBuilder filters recorded calls by type argument.
• Generator: generic methods emit new Type[] { typeof(T), ... } and route through the fallback dispatch (typed dispatch can't carry type args), preserving any auto-mock factory.
• Wildcards: new AnyType / AnyValueType markers (TUnit.Mocks.Arguments) let a setup match any type argument, e.g. mock.Greet<AnyType>().Returns(...).

Scope / limitations

• Exact closed-type matching plus the Any wildcard.
• Type params constrained to a specific base class/interface support exact matching only (no marker can satisfy an arbitrary base).
• Partial/wrap virtual methods record no type args and are not discriminated — graceful, documented degradation (their abstract members, which route through HandleCallWithReturn, are discriminated).

Tests

New integration tests in GenericTests.cs:

• the zero-arg regression from the discussion;
• AnyType wildcard and exact-wins-over-wildcard;
• type-arg-aware verification;
• multiple type parameters — (T1, T2) order sensitivity, partial wildcards (one AnyType + one concrete), exact-wins-over-partial-wildcard, and multi-param verification with wildcard counts.

Plus updated generic-method snapshots.

Green: 1010 integration, 62 snapshot, 30 analyzer tests (net8.0 + net10.0); runtime builds across all TFMs + Roslyn variants.

Note for reviewers

FindMatchingSetup keeps a separate no-default (int, object?[]) overload. Collapsing it into a single defaulted-Type[]? overload makes 2-arg call sites silently bind to the generic FindMatchingSetup<T1> instead — which broke every wrap/partial mock during development. A comment marks the constraint.

[claude]

Review: feat(mocks) - discriminate generic-method mocks by type argument

This is a well-structured fix for a real bug. The root cause analysis is accurate and the approach is sound. A few observations:

What's done well

• TypeArgumentMatching is correctly extracted as a separate static class rather than being inlined into both MethodSetup and CallVerificationBuilder. This is good separation of concerns.
• null semantics are sane: null setup-args match any call (non-generic / old code), null call-args match any setup (partial/wrap virtual degradation). Both cases are documented.
• Test coverage is thorough: zero-arg regression, wildcards, exact-wins-over-wildcard, multi-param ordering, partial wildcards, and verification — all the important axes.
• Backward compatibility preserved: existing callers of HandleCall(int, string, object?[]) and HandleCallWithReturn without type arguments continue to work unchanged.

Concerns / suggestions

1. new Type[] { ... } allocations at every call site (minor perf concern)

Every generic-method dispatch now allocates a Type[] on each call, even for the common single-type-parameter case. The array contents are constants (typeof(T)) known at JIT time.

Consider caching these per generic instantiation using a generic static field:

// In generated or helper code:
private static class TypeArgCache<T>
{
    public static readonly Type[] Single = [typeof(T)];
}
// Emit: TypeArgCache<T>.Single instead of new Type[] { typeof(T) }

For two-param methods a similar TypeArgCache<T1, T2> would help. This is not a blocker for correctness but worth noting for hot-path mocks.

2. AnyValueType as a struct — usability gap

AnyValueType is declared as a struct. A generic method constrained where T : struct requires a value type, so AnyValueType satisfies the constraint — good. However, a method constrained where T : struct, IComparable or similar additional interface constraints will fail to compile with AnyValueType as the type argument. This is mentioned in the docs ("support exact-type matching only") but the AnyType docs don't make the symmetric limitation equally visible.

More importantly: since AnyValueType is a struct, typeof(AnyValueType) is always the same type regardless of which value-type constraint is involved, which is correct for the current IsAnyMarker check. No bug here, just noting the usability ceiling.

3. FindMatchingSetup overload comment is a helpful explanation but the design is fragile

The comment explaining why FindMatchingSetup(int, object?[]) cannot use a default parameter (it would silently bind to the FindMatchingSetup<T1>(int, T1) generic overload) is correct and important. However, this is a subtle C# overload resolution trap that future maintainers may not be aware of.

Consider adding a [Obsolete] or at minimum a // KEEP: do not add default parameters — see comment below guard on the two-arg signature to make the constraint self-documenting at the declaration site rather than only in a comment above the implementation.

4. Typed dispatch path (HandleCallWithReturn<TReturn, TArg1>) is completely bypassed for all generic methods

The generator now unconditionally routes all generic methods through the object?[] fallback, which is correct for capturing type args. But it means generic methods that previously used the typed fast path now box their value-type arguments. This is an existing limitation documented in the PR description ("graceful degradation"), but it should be noted that this trade-off is permanent for all generic methods, not just the zero-parameter ones that triggered the bug.

5. Minor: FormatExpectedCall does not include type arguments in its output

private string FormatExpectedCall()
{
    var argDescriptions = string.Join(", ", _matchers.Select(m => m.Describe()));
    return $"{_memberName}({argDescriptions})";
}

When a type-argument-filtered verification fails, the error message will show Greet() rather than something like Greet<Class1>(). This makes it harder to diagnose which setup was expected. Adding type argument names to the failure message would improve the developer experience.

---

Overall this is a solid, well-reasoned fix with good test coverage. The concerns above are suggestions for improvement rather than blockers. The perf note (#1) and error message quality (#5) are the most actionable ones.

[codacy-production]

Not up to standards ⛔

🔴 Issues 1 medium · 1 minor

Alerts:
⚠ 2 issues (≤ 0 issues of at least minor severity)

Results:
2 new issues

Category	Results
CodeStyle	1 minor
Performance	1 medium

View in Codacy

🟢 Metrics 76 complexity

Metric	Results
Complexity	76

View in Codacy

_{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.}

[claude]

Follow-up Review: feat(mocks) - discriminate generic-method mocks by type argument

Reviewed the 3 commits since the initial review. The last commit (refactor: dedup type-args literal + collapse redundant ctors) specifically addressed feedback. Here is an updated assessment of all five points raised previously.

Status of previous review points

#1 — new Type[] { ... } allocations at every call site

Not addressed in this PR, which is reasonable given it was flagged as a non-blocking note. The generated code still emits new global::System.Type[] { typeof(T) } at each call site. A TypeArgCache<T>.Single pattern or a collection expression [typeof(T)] (which the compiler can optimize) would reduce allocations, but this is a fine follow-up item.

#2 — AnyValueType as a struct, usability ceiling

No change needed; this was acknowledged as documented behaviour.

#3 — FindMatchingSetup overload comment

The comment explaining why a default parameter cannot be added is preserved and correct. The locked variant now uses Type[]? typeArguments = null (safe because it has no generic sibling), while the unlocked variant keeps two explicit overloads. This is the correct split.

#4 — All generic methods bypass typed fast path

No change (documented trade-off). Still correct, still permanent. Worth a changelog note when this feature ships.

#5 — FormatExpectedCall does not include type arguments in failure messages

Still not addressed. This is the most actionable open item — a verification failure for Greet<Class1>() will still show Greet() in the error output, making diagnosis harder. Since this is now more visible given the new multi-type-parameter tests that could produce confusing failures, I'd encourage addressing this before merge.

---

New observations from the refactor commit

VoidMockMethodCall constructor refactoring looks correct. The eagerRegister / typeArguments parameter order is consistent and the chain is preserved. No issues.

TypeArgumentsArrayLiteral is now internal static and shared between MockImplBuilder and MockMembersBuilder. This is a good deduplication — previously the literal was constructed inline in two places.

MatchesCall extraction in CallVerificationBuilder is clean. Centralises the combined argument + type-argument predicate, making future changes easier.

---

One new correctness observation

In VoidMockMethodCall, the public constructor now has signature:

public VoidMockMethodCall(IMockEngineAccess engine, int memberId, string memberName,
    IArgumentMatcher[] matchers, Type[]? typeArguments = null)

and the internal constructor:

internal VoidMockMethodCall(IMockEngineAccess engine, int memberId, string memberName,
    IArgumentMatcher[] matchers, bool eagerRegister, Type[]? typeArguments = null)

The public constructor chains to the internal one via : this(..., eagerRegister: true, typeArguments). This is correct. However, external callers constructing VoidMockMethodCall directly (rare, since it is [EditorBrowsable(Never)], but possible) who pass typeArguments positionally rather than by name will now have an implicit dependency on the new parameter position. Since the constructor is marked [EditorBrowsable(Never)] and the parameter is optional, this is a very low risk, but it is technically a binary-compatible-only change for code that named typeArguments in a previous version — if any such code existed. Not a blocker.

---

Summary

The implementation is correct and the tests are thorough. The remaining actionable item before merge is #5 (type arguments in failure messages). Everything else is either resolved, documented, or a follow-up candidate. The refactor commit is a net improvement in maintainability with no regressions observed.

[claude]

Code Review: feat(mocks): discriminate generic-method mocks by type argument

This is a solid, well-structured fix for the reported collision bug. The root cause diagnosis is correct, the implementation is clean, and the test coverage is comprehensive. Some observations below.

---

What's good

Design is layered correctly. TypeArgumentMatching as a dedicated static class centralises the matching semantics (null-means-non-generic, wildcard markers, arity check, position-wise equality). MethodSetup, CallRecord, and CallVerificationBuilder all delegate to it rather than duplicating logic.

The TypeArguments.Of<T>.Value caching is smart. Avoiding a new Type[] per-dispatch for the overwhelmingly common 1- and 2-type-parameter cases is the right call, and the static readonly pattern is safe here because the matching code never mutates the arrays.

The FindMatchingSetup overload note is invaluable. The comment explaining why the two-arg overload can't be collapsed into a single defaulted-parameter overload (it would silently bind to the generic FindMatchingSetup<T1>) is exactly the kind of non-obvious invariant that belongs in a code comment. Future maintainers would otherwise "clean up" the apparent duplication and reintroduce the bug.

Backwards compatibility is correctly maintained. null type-arguments on either the setup or the call record means "non-generic / not discriminated", preserving all existing behaviour for non-generic members.

The wildcard AnyType/AnyValueType approach is pragmatic and the documented limitations (constrained base class params, virtual method degradation) are clear.

---

Issues and suggestions

1. Shared Type[] instances are technically mutable

TypeArguments.Of<T>.Value returns a static readonly Type[]. Arrays in C# are always mutable, so any consumer that mistakenly called Array.Sort or wrote to an index would silently corrupt the shared cache. Right now the only consumer is TypeArgumentMatching.Matches, which reads only — so this isn't a current bug. But it's a latent risk.

Consider either:

• Exposing as ReadOnlySpan<Type> or IReadOnlyList<Type> in the public API, or
• Adding an AsSpan() call in TypeArgumentMatching.Matches to make the read-only intent explicit

At minimum a doc comment on Value stating "array is read-only; do not mutate" would make the constraint explicit.

2. Verification error message uses .Name — may be ambiguous across namespaces

// CallVerificationBuilder.cs  
var typeArgs = _typeArguments is { Length: > 0 } ta
    ? "<" + string.Join(", ", ta.Select(t => t.Name)) + ">"
    : "";

Type.Name for System.Collections.Generic.List<int> gives List\1, and for two types Foo.Bar.OrderandBaz.Orderboth giveOrder. In test failure messages this can be confusing. t.FullName ?? t.Name(ort.FriendlyName()` if such a helper exists in the codebase) would be more diagnostic for users.

That said, for the wildcard markers (AnyType, AnyValueType) .Name is exactly right — .FullName would include the namespace and make the message noisier. A targeted approach: use .Name but strip the arity suffix (`1) for closed generics, falling back to .FullName if disambiguation is needed.

3. Allocation gap for 3+ type parameters

TypeArgumentsArrayLiteral emits new global::System.Type[] { typeof(T1), typeof(T2), typeof(T3) } for 3+ params — one allocation per call. For a framework that cares about hot-path allocations, adding Of<T1, T2, T3> (and maybe Of<T1, T2, T3, T4>) is a natural follow-up. This is listed here as a forward-looking suggestion, not a blocker.

4. AnyValueType is not usable for constrained struct parameters today

The docs correctly note that AnyValueType can't satisfy where T : struct, IDisposable. Worth double-checking: does the where T : struct constraint on the generated mock extension method reject AnyValueType at compile time? If so, there's no way to wildcard a where T : struct parameter at all, which makes AnyValueType dead code for that constraint shape. Either a test demonstrating it works, or a XML-doc note that it only works on unconstrained / where T : struct (without further constraints), would help.

5. Minor: the MethodSetup.TypeArgumentsMatch method is [EditorBrowsable(Never)] but the guard helps

Appropriate since MethodSetup is public for generated-code access. The attribute correctly hides it from IDE autocomplete. No action needed, just confirming the choice is intentional.

---

Test coverage observation

The new tests in GenericTests.cs are thorough — regression case, wildcards, exact-beats-wildcard ordering, multi-param, partial wildcards, verification with filtering. One scenario not covered: a Callback() (or InvokeCallback) configured on a type-discriminated setup to verify the right callback fires per type argument. Low priority since the core matching logic is tested, but may be worth a follow-up.

---

Summary

The fix is correct, well-tested, and follows the project's patterns. The main actionable items are the error message ambiguity (#2) and the AnyValueType usability story (#4). The mutable shared array concern (#1) is low risk today but worth documenting.

[claude]

Code Review: feat(mocks): discriminate generic-method mocks by type argument

Overall this is a well-crafted fix for a real bug. The approach is architecturally sound: thread Type[]? through the call stack without touching non-generic paths, isolate matching logic in a dedicated TypeArgumentMatching utility, and cache the common 1–4 arity arrays to avoid per-call allocation. The test suite is exemplary — zero-arg regression, wildcard matching, ordering sensitivity, partial wildcards, and struct constraints all covered.

Some items worth addressing before merge:

---

Issue 1 — Breaking change on IMockEngineAccess (design concern)

Adding a new overload to a public interface:

// IMockEngineAccess.cs
ICallVerification CreateVerification(int memberId, string memberName, IArgumentMatcher[] matchers, Type[]? typeArguments);

breaks any downstream class that implements IMockEngineAccess directly (e.g. user-supplied test doubles or custom engine wrappers). Since TUnit already targets net8.0, this is a good candidate for a default interface method:

ICallVerification CreateVerification(int memberId, string memberName,
    IArgumentMatcher[] matchers, Type[]? typeArguments)
    => CreateVerification(memberId, memberName, matchers); // graceful degradation

This is strictly safer for library consumers without sacrificing the new behaviour for the real engine.

---

Issue 2 — Ordering dependency for exact-over-wildcard is undocumented and fragile

The test comment says "A later, more specific setup is matched first (last-wins iteration)" — the fix relies on last-registered-wins to give exact setups priority over wildcards. But this means the register order matters:

// This works (wildcard first, exact second):
mock.Greet<AnyType>().Returns("any");
mock.Greet<Class1>().Returns("specific"); // wins

// This breaks (exact first, wildcard second):
mock.Greet<Class1>().Returns("specific");
mock.Greet<AnyType>().Returns("any"); // shadows the exact setup silently!

Moq and NSubstitute solve this by scoring specificity (wildcard < concrete) instead of relying on registration order. The current approach is fragile and user-hostile — a silent wrong result, not a diagnostic failure.

Consider scoring each slot (wildcard = 0, concrete = 1) and selecting the highest-scoring match rather than first-from-end. This removes the ordering dependency entirely.

---

Issue 3 — TypeArguments.Of<>.Value arrays are mutable

public static readonly Type[] Value = { typeof(T) };

readonly prevents the field from being reassigned, but not the array contents from being mutated. Any code that receives this array and writes to an element corrupts the shared cache for the lifetime of the process. The comment says "must never be mutated" but that's a runtime invariant with no compile-time enforcement.

Safer alternatives:

• public static ReadOnlySpan<Type> Value => new[] { typeof(T) }; — stack-only, zero allocation, immutable view (though not usable everywhere)
• Change downstream consumers to accept ReadOnlySpan<Type> / IReadOnlyList<Type> where APIs allow
• At minimum, keep the Type[] but rename and note it's defensively safe because matching only reads it — and add a guard in TypeArgumentMatching.Matches

---

Minor Observations (no action required)

FindMatchingSetup two-overload pattern: The comment explaining why the two-arg overload cannot be collapsed into a single defaulted overload (competition with the generic FindMatchingSetup<T1>) is exactly the right thing to have written. Good defensive comment.

5+ type-param fallback allocates per-call: Documented correctly in the PR. Given the rarity of 5-arity generic methods this is fine; worth a one-liner in the TypeArguments XML doc so future maintainers know where to extend.

FriendlyTypeName for wildcards: When a wildcard (AnyType) appears in the failure message the output will be MethodName<AnyType>(...) — that's clear and helpful. No issue there.

Test infrastructure: The IValueDescriber interface and AnyValueType wildcard test close an easy-to-miss gap (struct-constrained type params). Appreciated.

---

The breaking-interface-extension (Issue 1) and silent ordering-dependency (Issue 2) are the two items I'd ask to address before merge. The mutability concern (Issue 3) is lower priority but worth a follow-up.

[claude]

Code Review: feat(mocks): discriminate generic-method mocks by type argument

Overall: Solid fix for a real, user-reported bug. The core design is sound, the test coverage is thorough, and the performance tradeoffs are well-considered. A few design concerns worth discussing before merge.

---

Bug Fix — Correctness ✅

The root cause diagnosis is correct: MethodSetup, CallRecord, and FindMatchingSetup had no concept of type arguments, so two setups for the same generic method with different type parameters were byte-identical keys. The fix correctly threads Type[]? typeArguments through all three layers.

The TypeArgumentMatching.Matches null-semantics (null on either side = match) is the right choice for the graceful-degradation path on virtual/partial methods.

---

Design Concern: Cast to Internal Interface in Public API Surface

Files: MockMethodCall.cs:116-119, VoidMockMethodCall.cs:125-128

private ICallVerification CreateVerification()
    => _typeArguments is null
        ? _engine.CreateVerification(_memberId, _memberName, _matchers)
        : ((ITypeArgumentVerificationFactory)_engine).CreateVerification(...);

This cast is fragile: _engine is typed as the public IMockEngineAccess, but we cast it to the internal ITypeArgumentVerificationFactory. Any external implementor of IMockEngineAccess (admitted via the public constructor) would get an InvalidCastException at runtime the moment a generic method setup is verified. Since ITypeArgumentVerificationFactory is internal, they can't opt in.

Why this matters more than it appears: The constructor MockMethodCall(IMockEngineAccess, ...) is [EditorBrowsable(Never)] but still public and binary-stable. Generated code from the new source generator will silently pass type arguments to it, then trigger the cast during WasCalled.

Suggested alternative: Add a default interface method to IMockEngineAccess:

// in IMockEngineAccess
ICallVerification CreateVerification(int memberId, string memberName, IArgumentMatcher[] matchers, Type[]? typeArguments)
    => CreateVerification(memberId, memberName, matchers); // default: ignore type args

This keeps the cast-free path for external engines while letting MockEngine<T> override it for the discriminating path. It avoids a hard runtime failure, at the cost of silent fallback (which is arguably better than a crash).

---

Minor: Mutable Shared Arrays in TypeArguments.Of<T>

File: TypeArguments.cs:15

public static readonly Type[] Value = { typeof(T) };

readonly prevents reassignment but not mutation (Value[0] = typeof(object) compiles and runs). Since the same array is handed to matching logic across many calls, a future change that accidentally mutates it would be a subtle cross-call corruption bug.

Wrapping as IReadOnlyList<Type> would make the intent enforceable:

public static readonly IReadOnlyList<Type> Value = Array.AsReadOnly(new[] { typeof(T) });

This is a low-risk change but makes the immutability contract explicit rather than relying on convention.

---

Minor: FriendlyTypeName Drops Namespace for Disambiguation

File: CallVerificationBuilder.cs:419-424

Type.Name gives the simple name without namespace. Two types with the same simple name in different namespaces (MyApp.Foo vs OtherLib.Foo) would both render as Foo in the error message — leaving the developer guessing which one was expected.

For a diagnostic-only path this is minor, but type.FullName ?? type.Name (with the same backtick strip) would make errors unambiguous in those cases.

---

Implementation Highlights Worth Calling Out ✅

• TypeArguments.Of<T>.Value caching (1–4 params): Avoids per-call Type[] allocation for the common cases. The fallback for 5+ parameters is correct and documented in the emitter.

• FindMatchingSetup two-arg overload: The comment explaining why this cannot use a default parameter (overload resolution with the generic FindMatchingSetup<T1> would silently bind) is exactly the kind of non-obvious trap that deserves explicit documentation. Well done.

• Test coverage: The regression test from the original discussion, wildcard semantics, order-sensitivity for multi-param type args, partial wildcards, and verification discrimination — this is unusually thorough test coverage for a mock internals change.

• Backward binary compatibility: Keeping the original constructors as delegation-only overloads (rather than adding optional parameters) is the correct choice for a library that generates code from a versioned source generator.

---

Summary

The fix is correct and well-structured. The main thing I'd flag before merging is the internal cast in the public wrappers — it introduces a silent runtime failure mode for the (admittedly unlikely) case of an external IMockEngineAccess implementor. The default interface method approach avoids this without breaking anything. The other items are minor improvements.