Mocks: structural fix for Mock<T> / mocked-member name collisions

[thomhurst]

Summary

• Move framework operations (Reset, VerifyAll, Invocations, …) off Mock<T> as instance members onto IMockControl<T> explicit interface implementations plus Mock.X(mock) static helpers. Mock<T> instance surface shrinks to Object + constructor + implicit conversion to T.
• Collision set is now structurally closed: { Object, Equals, GetHashCode, ToString }. Generator's existing Object_ / *Of renames keep working, with new iterative-suffix escalation when the user interface declares the renamed form (Object + Object_ → generated extension lands on Object__).
• On net9.0+ the generator emits low-priority extension polyfills inside the per-type *_MockMemberExtensions class so mock.Reset(), mock.VerifyAll(), mock.Invocations, mock.SetState("x"), mock.DefaultValueProvider = … remain ergonomic. [OverloadResolutionPriority(-1)] only ranks within one containing type, so the polyfills must live alongside the generated extensions; they skip emission when the mocked interface declares a member of the same name (no CS0111).

Background: GitHub Discussion #4981 raised the collision concern. Prior handling covered ~4 names by ad-hoc rename table and would have silently shadowed generator output for ~10 other public Mock<T> members. With this change, future framework additions go on IMockControl<T> / static Mock.* and cannot regress collision safety.

Breaking changes (TUnit.Mocks is in beta)

• Mock<T> no longer exposes Reset(), VerifyAll(), VerifyNoOtherCalls(), Invocations, Behavior, DefaultValueProvider, SetupAllProperties(), GetDiagnostics(), SetState(), InState() as instance members.
• net9.0+ consumers: code keeps compiling — generator emits ergonomic polyfills as low-priority extensions, so mock.Reset() etc. continue to work.
• net8.0 / netstandard2.0 consumers: call sites must move to the static helpers (Mock.Reset(mock), Mock.VerifyAll(mock), etc.). [OverloadResolutionPriority] isn't available on those targets so the polyfill path can't apply.
• Mock.DefaultValueProvider getter/setter overload split into Mock.GetDefaultValueProvider(mock) and Mock.SetDefaultValueProvider(mock, provider) per .NET naming conventions.

Test plan

• NameCollisionTests (new) exercises every former framework name plus Object_ / EqualsOf overflow paths.
• On net9.0+, polyfill tests verify mock.Reset() / mock.VerifyAll() / mock.Invocations / property setter mock.DefaultValueProvider = … route through and that polyfills are skipped when colliding.
• TUnit.Mocks.Tests full suite: 977 pass (net10.0), 967 pass (net8.0).
• Full solution dotnet build TUnit.slnx succeeds.
• CI: source-generator snapshot tests (TUnit.Mocks.SourceGenerator.Tests) — local runner blocked by an unrelated Verify.TUnit / TUnit.Core version mismatch; CI will validate.

[claude]

Code Review — Mocks: structural fix for Mock<T> / mocked-member name collisions

The architectural direction here is correct: moving framework operations off the Mock<T> public surface onto an explicit interface eliminates the entire class of name-collision bugs at the type level rather than playing whack-a-mole with a deny-list. The EscalateUntilUnique suffix chain and the net9.0+ polyfill approach are clever. Overall the implementation is solid. A few architectural concerns worth addressing:

---

1. Breaking change on net8.0 with no ergonomic fallback

The net9.0+ polyfill path restores mock.Reset() / mock.VerifyAll() etc. via [OverloadResolutionPriority(-1)] extensions. On net8.0 (a supported multi-target), users are left with only the verbose static form:

Mock.Reset(mock);         // net8.0 only option
Mock.VerifyAll(mock);     // same
Mock.Invocations(mock);   // same

There is no [OverloadResolutionPriority] on net8.0, so the polyfills would collide or simply can't be emitted. This is a significant ergonomic regression for net8.0 users who have existing code using mock.Reset(). The PR should either document a migration path, or investigate whether a conventional extension-method approach (without priority control) is viable for net8.0 — even with a narrower collision scope.

---

2. GetUserMemberNames called repeatedly — O(N²) allocation

In MockMembersBuilder.cs, GetSafeMemberName(name, model) is called once per member (methods, properties, etc.), and each call invokes EscalateUntilUnique which calls GetUserMemberNames(model):

private static HashSet<string> GetUserMemberNames(MockTypeModel model)
{
    var set = new HashSet<string>(System.StringComparer.Ordinal);
    foreach (var m in model.Methods)    set.Add(m.Name);
    foreach (var p in model.Properties) set.Add(p.Name);
    foreach (var e in model.Events)     set.Add(e.Name);
    return set;
}

This creates a new HashSet on every call. For an interface with N members, Build() performs O(N²) allocations and iterations. Since source generators run per-compilation, this matters for large interfaces. The fix is straightforward — pre-compute once in Build() and thread it through as a parameter:

var userNames = GetUserMemberNames(model); // once, at Build() entry
// pass to GetSafeMemberName(name, userNames) and GenerateMockControlPolyfills(writer, model, userNames)

---

3. DefaultValueProvider overload design is unconventional

The getter/setter property is now split into two overloaded static methods:

public static IDefaultValueProvider? DefaultValueProvider<T>(Mock<T> mock) where T : class  // get
public static void DefaultValueProvider<T>(Mock<T> mock, IDefaultValueProvider? provider)   // set

Using overloads to simulate get/set is unusual in C# and breaks the principle of least surprise. A reader seeing Mock.DefaultValueProvider(mock) vs. Mock.DefaultValueProvider(mock, provider) has to reason about what type-based dispatch means here. The recommended alternative is explicit naming:

public static IDefaultValueProvider? GetDefaultValueProvider<T>(Mock<T> mock) where T : class
public static void SetDefaultValueProvider<T>(Mock<T> mock, IDefaultValueProvider? provider) where T : class

This is consistent with how .NET itself handles similar cases (e.g., Attribute.GetCustomAttribute / Attribute.IsDefined).

---

4. Design inconsistency: Reset / VerifyAll / VerifyNoOtherCalls stay on IMock, not IMockControl<T>

The PR description says "framework operations live on IMockControl<T>", but Reset, VerifyAll, and VerifyNoOtherCalls remain on IMock as explicit implementations, while all other framework ops (Invocations, Behavior, DefaultValueProvider, SetState, InState, GetDiagnostics) move to IMockControl<T>. The static helpers reflect this split:

public static void Reset<T>(Mock<T> mock)             => ((IMock)mock).Reset();         // IMock
public static void VerifyAll<T>(Mock<T> mock)         => ((IMock)mock).VerifyAll();      // IMock
public static IReadOnlyList<...> Invocations<T>(...)  => ((IMockControl<T>)mock).Invocations; // IMockControl<T>

This split exists because IMock is a non-generic base that other things depend on, but from the user's perspective it's invisible. Worth documenting why the split exists, or consolidating Reset/VerifyAll/VerifyNoOtherCalls onto IMockControl<T> as well (which would be more consistent).

---

5. Possible blank-line emission when all polyfills are skipped

In MockMembersBuilder.Build():

if (!firstMember) writer.AppendLine();
GenerateMockControlPolyfills(writer, model);

If the mocked interface declares all polyfillable names (collisions on every method and property), GenerateMockControlPolyfills emits only #if NET9_0_OR_GREATER / #endif wrapping nothing. The preceding blank line would still appear. Minor cosmetic issue, but worth guarding with a check similar to what GenerateMockControlPolyfills does internally.

---

What's good

• The structural approach of moving framework operations to an explicit interface is the right long-term fix — it makes the collision set finite and immutable by design.
• The iterative suffix escalation (Object_, Object__, …) is a clean generalization of the previous ad-hoc rename table.
• The NameCollisionTests class with tests for every former framework member name is exactly the right regression coverage.
• The net9.0+ C# 14 extension block for property polyfills (Invocations, DefaultValueProvider) is a nice use of the new syntax.
• The [EditorBrowsable(EditorBrowsableState.Never)] on IMockControl<T> correctly keeps the internals out of IntelliSense.

The PR is mergeable as-is with the understanding that the net8.0 ergonomics gap exists. Items 2 (O(N²) allocation) and 3 (DefaultValueProvider naming) would be worth addressing before or shortly after merge.

[codacy-production]

Not up to standards ⛔

🔴 Issues 5 minor

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

Results:
5 new issues

Category	Results
CodeStyle	5 minor

View in Codacy

🟢 Metrics 34 complexity

Metric	Results
Complexity	34

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 — After Second Commit

What the latest commit addressed

Item 2 (O(N²) allocation) — Resolved. The ConditionalWeakTable<MockTypeModel, HashSet<string>> cache means each MockTypeModel builds its name-set once per generator pass, not once per member. Clean fix.

IMockControl<T> visibility — Changed from public [EditorBrowsable(Never)] to internal. This is strictly better: internal prevents the type from appearing in any cross-assembly context, not just IntelliSense. The previous concern about the IMock / IMockControl<T> split (item 4) is now moot — it's entirely an internal implementation detail.

---

Still open from previous review

Item 3 — DefaultValueProvider overload design

No change here. The static helper still splits property get/set into two overloads that look identical to the call-site reader:

Mock.DefaultValueProvider(mock);           // get — returns IDefaultValueProvider?
Mock.DefaultValueProvider(mock, provider); // set — returns void

This is still non-idiomatic. The recommended fix remains explicit verb prefixes:

Mock.GetDefaultValueProvider(mock)
Mock.SetDefaultValueProvider(mock, provider)

Item 5 — Empty #if block in generated code

When the mocked interface declares all polyfillable names (which IFrameworkNameMember in the new NameCollisionTests.cs actually does), GenerateMockControlPolyfills emits:

// (blank line from unconditional 'if (!firstMember) writer.AppendLine()' at call-site)
#if NET9_0_OR_GREATER
#endif

Valid C#, but it's noise in generated output, and the NameCollisionTests test suite will exercise this path. The fix is to move the blank-line guard inside GenerateMockControlPolyfills and only emit the #if / #endif wrapper when at least one polyfill will be written. A simple bool anyEmitted sentinel is enough, or pre-check all names against userNames before starting:

// In GenerateMockControlPolyfills, before the #if:
bool anyMethod = MethodPolyfills.Any(p => !userNames.Contains(p.Name));
bool anyInState = !userNames.Contains("InState");
bool anyProp = !userNames.Contains("Invocations") || !userNames.Contains("Behavior") || !userNames.Contains("DefaultValueProvider");
if (!anyMethod && !anyInState && !anyProp) return;

---

Net8.0 ergonomics gap (item 1)

Unchanged — still requires Mock.Reset(mock) etc. on net8.0. Noted in the previous review as acceptable; flagging again for completeness so it's tracked somewhere.

---

Overall

The structural redesign is solid and the second commit tightened the internals well. Items 3 and 5 above are small but concrete; item 3 in particular is a public API surface concern that'll be harder to change post-release. Worth a final pass before merge.

[thomhurst]

Thanks for the review. Status per item:

#2 (O(N²) HashSet allocation) — already addressed in commit 7e47b9ff3 (pushed before this review landed). GetUserMemberNames now memoizes per MockTypeModel via ConditionalWeakTable, so a single HashSet is built per type per generator pass. Re-confirmed.

#3 (DefaultValueProvider get/set overloads) — fixed in 0f5b969dc. Renamed the static helpers to Mock.GetDefaultValueProvider(mock) / Mock.SetDefaultValueProvider(mock, provider). The net9.0+ polyfill extension property keeps surfacing as mock.DefaultValueProvider (get/set) for the natural API at use sites.

#4 (IMock vs IMockControl split) — intentional, but now documented on IMockControl<T>. The split is driven by MockRepository, which iterates mocks as a heterogeneous IMock[] for batch operations and so needs Reset/VerifyAll/VerifyNoOtherCalls in non-generic form. Members that depend on the type parameter (Invocations, GetDiagnostics, typed Object, state-machine helpers) only make sense generic and live on IMockControl<T>. Added a <remarks> block explaining this.

#5 (blank line before empty #if/#endif) — fixed in 0f5b969dc. GenerateMockControlPolyfills now short-circuits when every polyfill name collides with a user member, so the #if NET9_0_OR_GREATER block isn't emitted at all in that (rare) case.

#1 (net8.0 ergonomic regression) — declined. The instance-style API on net8.0 was the source of the silent shadowing the PR exists to fix; restoring it on that TFM requires either polyfilling OverloadResolutionPriorityAttribute ourselves (consumer LangVersion ≥ 13 not guaranteed) or accepting collision risk to come back. TUnit.Mocks is beta and a single clean break across all TFMs is preferred over a partial fix. net8.0 users have Mock.X(mock) available; net9.0+ users get the polyfill ergonomics back.

Verified 977/977 pass on net10.0 and 967/967 on net8.0.

[claude]

Third-pass Review — After Review-Feedback Commit

Items resolved by the third commit

Item 3 (DefaultValueProvider naming) — Resolved. Mock.GetDefaultValueProvider / Mock.SetDefaultValueProvider follow the .NET naming convention and match what the polyfill extension property surfaces as on net9+. Clean.

Item 4 (IMock / IMockControl split documentation) — Resolved. IMockControl<T> now has a clear doc comment explaining why Reset/VerifyAll/VerifyNoOtherCalls stay on the non-generic IMock (needed for MockRepository batch iteration over IMock[]). The split is intentional and now visible.

Item 5 (empty #if/#endif) — Partially resolved. GenerateMockControlPolyfills now has an early-return guard when all polyfill names are blocked by user-declared members, which prevents the empty #if NET9_0_OR_GREATER / #endif wrapper from being emitted. However, the blank line at the call site is still unconditionally emitted before the guard fires:

// MockMembersBuilder.cs line 135
if (!firstMember) writer.AppendLine();          // ← always runs
GenerateMockControlPolyfills(writer, model);    // ← may return immediately

When GenerateMockControlPolyfills short-circuits, the blank line is already written. For a type like IFrameworkNameMember (which declares all polyfillable names), the generated class ends with a trailing blank line before the closing }. This is cosmetic, but the commit message implies it was fixed when it wasn't quite. The correct fix is to move the blank-line guard inside GenerateMockControlPolyfills after the early-return check, or extract a WillEmitPolyfills(model) predicate to gate both:

// Option A — guard inside GenerateMockControlPolyfills, after the early-return:
if (!anyMethod && !anyProperty) return;
if (!firstMember) writer.AppendLine();   // moved inside, only when something will be emitted
first = false;
// ...

// Option B — predicate at call site:
if (WillEmitPolyfills(model))
{
    if (!firstMember) writer.AppendLine();
    GenerateMockControlPolyfills(writer, model);
}

---

Item 1 — net8.0 ergonomics gap

Still present and unchanged. For users on net8.0 (a supported multi-target), the ergonomic mock.Reset() / mock.VerifyAll() form is gone and only Mock.Reset(mock) remains. This is understood and accepted, but worth a note in release notes or the PR description so users know to update call sites when targeting net8.0.

---

Overall

The structural redesign is sound and the third commit has addressed the substantive concerns. The remaining issues are:

• Trailing blank line in generated code (item 5, minor but incomplete fix) — the early-return guard in GenerateMockControlPolyfills doesn't prevent the blank line already written at the call site.
• net8.0 migration note — documentation gap, not a code defect.

The code is ready to merge. The blank-line issue is a low-priority follow-up.

[claude]

Fourth-pass Review

Summary of prior items

#	Issue	Status
1	net8.0 ergonomics gap	Declined by author — documented as accepted
2	O(N²) HashSet allocation	✅ Resolved via ConditionalWeakTable cache
3	DefaultValueProvider overload naming	✅ Resolved — GetDefaultValueProvider/SetDefaultValueProvider
4	IMock / IMockControl<T> split not documented	✅ Resolved — <remarks> on IMockControl<T> explains the MockRepository reason
5	Trailing blank line / empty #if block	⚠️ Partially resolved — see below

---

Item 5 — still present after the third commit

The early-return guard was added inside GenerateMockControlPolyfills, but the blank line is written before the call:

// MockMembersBuilder.cs — call site
if (!firstMember) writer.AppendLine();       // ← executes unconditionally
GenerateMockControlPolyfills(writer, model); // ← may return immediately

For an interface that collides with all polyfillable names (the IFrameworkNameMember test exercises this exact path), the generated class ends with a trailing blank line before its closing }. The comment inside GenerateMockControlPolyfills says "keeps callers from emitting a stray blank line", but the caller still emits it.

The fix is one of:

• Move the blank-line write inside GenerateMockControlPolyfills, between the early-return guard and the #if:

  if (emitMethods.Length == 0 && !emitInState && ...) return;
  if (!firstMember) writer.AppendLine(); // moved here
  writer.AppendLine("#if NET9_0_OR_GREATER");

• Or gate the whole block at the call site with a WillEmitPolyfills(model) predicate.

This is low-priority cosmetic noise in generated output and does not affect correctness.

---

Overall

The structural redesign is sound. Moving framework operations to IMockControl<T> explicit implementations permanently closes the collision surface, the iterative-suffix escalation handles the overflow case cleanly, and the net9.0+ polyfill approach recovers ergonomics where the language supports it. The test coverage in NameCollisionTests is thorough. The remaining issue (item 5) is a one-line fix and does not block merge.

[thomhurst]

Fixed in 02a0b3b — moved the separator blank-line emission inside GenerateMockControlPolyfills after the early-return guard, so nothing is written when every polyfill name collides with a user member. firstMember is now passed in as isFirstMember rather than acted on at the call site.

Re item 1: added to PR description as a release-notes follow-up.

[claude]

Fifth-pass Review

Previous items — all resolved

#	Issue	Status
1	net8.0 ergonomics gap	Declined — documented as accepted breaking change
2	O(N²) HashSet allocation	✅ Resolved via ConditionalWeakTable cache
3	DefaultValueProvider overload naming	✅ Resolved — GetDefaultValueProvider/SetDefaultValueProvider
4	IMock / IMockControl<T> split undocumented	✅ Resolved — <remarks> on IMockControl<T> explains the MockRepository reason
5	Trailing blank line before the polyfill block	✅ Resolved — blank line moved inside GenerateMockControlPolyfills after the early-return guard

---

New issue: C# 14 extension block gated on NET9_0_OR_GREATER instead of NET10_0_OR_GREATER

GenerateMockControlPolyfills emits property polyfills (Invocations, Behavior, DefaultValueProvider) using C# 14 extension block syntax:

// MockMembersBuilder.cs
using (writer.Block($"extension{typeParams}(global::TUnit.Mocks.Mock<{mockableType}> mock){constraints}"))
{
    // property polyfills…
}

This is all wrapped in #if NET9_0_OR_GREATER. The problem: extension blocks (new-style extensions with extension properties) are C# 14, which defaults to .NET 10. A user who targets net9.0 and uses the default <LangVersion>13 will get a compile error from the generated extension block syntax, because C# 13 does not support extension properties.

The method polyfills (using this Mock<T> mock parameter) are fine for C# 13/net9.0, but extension properties require C# 14.

Concrete options:

Option A — split the gate by feature:

#if NET9_0_OR_GREATER
// method polyfills (C# 13 compatible)
[OverloadResolutionPriority(-1)]
public static void Reset<T>(this Mock<T> mock) ...
// … other method polyfills …
#endif

#if NET10_0_OR_GREATER
// property polyfills (require C# 14 extension blocks)
extension<T>(Mock<T> mock) {
    public IReadOnlyList<CallRecord> Invocations => ...;
    // …
}
#endif

Option B — use method-based polyfills for the properties too, keeping a single NET9_0_OR_GREATER gate:

// Ergonomics: mock.GetInvocations() instead of mock.Invocations
[OverloadResolutionPriority(-1)]
public static IReadOnlyList<CallRecord> GetInvocations<T>(this Mock<T> mock) ...

This changes the polyfill surface to method-call syntax on net9.0, but keeps exact parity (mock.Invocations, mock.DefaultValueProvider = …) only on net10.0+.

Option C — require consumers to set <LangVersion>preview</LangVersion> and document it. Least desirable since it's a non-standard requirement for a library user.

The simplest safe fix is Option A: gate the extension block portion on NET10_0_OR_GREATER so net9.0 users still get the method polyfills (mock.Reset(), mock.VerifyAll()) while the property-style polyfills (mock.Invocations) only appear on net10.0+.

---

What's good

The structural redesign is solid. All previously raised concerns are now addressed. The NameCollisionTests coverage is thorough — the IFrameworkNameMember, IObjectOverflow, and IEqualsOfOverflow cases cover the full escalation logic. The IMockControl<T> / explicit-interface approach permanently closes the collision surface by construction.

[thomhurst]

Declining the C# 14 / net10 gate split. TUnit.Mocks already requires C# 14 across the library — the existing generator emits extension blocks unconditionally for mocked-interface properties, and consumers must already opt into a later LangVersion. Splitting the polyfill gate would add real complexity for an effectively non-existent compatibility envelope. Keeping the single #if NET9_0_OR_GREATER block.