refactor(mocks): unify Mock.Of<T>() and Mock.OfPartial<T>() into single API

[thomhurst]

Summary

• Unifies Mock.Of<T>() and Mock.OfPartial<T>() into a single Mock.Of<T>() API. The source generator determines whether to generate a full mock (interfaces) or partial mock (classes) based on the type kind — users no longer need to choose.
• Fixes a bug where [assembly: GenerateMock(typeof(ConcreteClass))] always generated interface-style mocks for concrete classes instead of partial mocks.
• Removes Mock.OfPartial<T>() and MockRepository.OfPartial<T>() — Mock.Of<T>(constructorArgs) now handles constructor arguments for concrete classes.

Changes

Runtime (TUnit.Mocks):

• Merged _factories and _partialFactories into a single unified factory registry
• Removed RegisterPartialFactory<T>(), OfPartial<T>() from Mock and MockRepository
• Added Of<T>(params object[] constructorArgs) and Of<T>(MockBehavior, params object[]) overloads

Source Generator (TUnit.Mocks.SourceGenerator):

• IsPartialMock is now derived from TypeKind.Class instead of method.Name == "OfPartial"
• All factories use unified RegisterFactory<T> with (MockBehavior, object[]) signature
• Fixed TransformGenerateMockAttribute to generate partial mocks for concrete classes
• Simplified tautological IsPartialMock && !IsInterface guards to just IsPartialMock

Analyzers, Tests, Docs: Updated to reflect the unified API.

Test plan

• All 14 partial mock tests pass with Mock.Of<T>()
• All 17 MockRepository tests pass (including former OfPartial tests)
• All 22 analyzer tests pass
• All 21 source generator snapshot tests pass (updated verified files)
• Basic mock, async, auto-mock, protected member, and delegate tests all pass

[claude]

Code Review

This is a clean, well-motivated refactor. Aligning with the convention of Moq/NSubstitute (single factory method that works for both interfaces and classes) makes the API more discoverable, and the bug fix for [GenerateMock(typeof(ConcreteClass))] was a genuine correctness issue. The implementation is consistent across runtime, source generator, analyzers, and tests. The snapshot updates are correctly committed as .verified.txt files.

A few things worth considering before merging:

---

Issue 1: constructorArgs is silently discarded for interface mocks

All generated interface factory methods now accept object[] constructorArgs but silently ignore it:

// Generated code for IGreeter:
private static Mock<global::IGreeter> Create(MockBehavior behavior, object[] constructorArgs)
{
    var engine = new MockEngine<global::IGreeter>(behavior);
    var impl = new IGreeter_MockImpl(engine);  // constructorArgs never used
    ...
}

If a user accidentally writes Mock.Of<IMyService>("some", "args"), the args are silently thrown away with no diagnostic feedback. The generated interface factory could guard against this:

private static Mock<global::IGreeter> Create(MockBehavior behavior, object[] constructorArgs)
{
    if (constructorArgs.Length > 0)
        throw new ArgumentException($"Interface mock 'IGreeter' does not support constructor arguments, but {constructorArgs.Length} were provided.");
    ...
}

This is especially valuable because the old OfPartial separation made the distinction explicit. Now that both use Of<T>(), users have no compile-time signal that one path ignores args.

---

Issue 2: MockBehavior constructor arg ambiguity (edge case breaking change)

With the new overloads:

public static Mock<T> Of<T>(MockBehavior behavior) where T : class
public static Mock<T> Of<T>(params object[] constructorArgs) where T : class

If a class under test takes a MockBehavior value in its constructor (unlikely but valid), Mock.Of<MyClass>(MockBehavior.Strict) will now silently set the mock behavior rather than pass the value as a constructor arg. Previously with Mock.OfPartial<MyClass>(MockBehavior.Strict) this would pass the value to the constructor correctly.

The workaround is to cast: Mock.Of<MyClass>((object)MockBehavior.Strict). This is a narrow edge case, but it might be worth documenting in the PR description as a migration caveat.

---

Minor: unused parameter name in interface snapshots

All interface factory snapshots gained a constructorArgs parameter (13+ files touched). Since interfaces never use it, you could instead keep the cleaner interface signature as Create(MockBehavior behavior) and only add object[] constructorArgs to the partial/class factory — unifying the runtime registry signature while keeping the generated code clean. This would mean two Func types in the registry, but the existing split between _factories and other registries already handles this. The current approach is simpler though and consistency is a fair trade-off.

---

Summary

The core logic is correct and the changes are coherent throughout all layers. The main actionable item is Issue 1 — the silent discard of constructorArgs for interface mocks should at minimum throw, since the unified API removes the visual cue that helped users know which path they were on.

[claude]

Code Review

This is a clean, well-motivated refactor. Unifying the two APIs eliminates a conceptual burden for users ("which Of do I call?") and makes the type-kind determination an implementation detail rather than user responsibility. The bugfix to TransformGenerateMockAttribute is a welcome side-effect.

---

What works well

• Correct pivot point: Deriving isPartialMock from namedType.TypeKind == TypeKind.Class in MockTypeDiscovery is exactly the right place to centralize this decision — one clear rule, applied consistently for both Of<T>() calls and [assembly: GenerateMock].
• Tautology cleanup: Removing the redundant && !model.IsInterface guard in MockFactoryBuilder and MockImplBuilder is a correctness improvement — IsPartialMock being true for classes only means the condition was always equivalent.
• Snapshot tests updated correctly: The .verified.txt changes correctly reflect the unified RegisterFactory<T> (no more RegisterPartialFactory<T>) and the consistently-shaped factory delegate signature.

---

Concerns

1. Constructor args silently dropped for interface mocks

The unified factory signature Create(MockBehavior behavior, object[] constructorArgs) is now used for both interfaces and classes. For interfaces, constructorArgs is accepted by the Mock.Of<T>(params object[] constructorArgs) overload at the call site, but the generated factory ignores it entirely.

A user writing Mock.Of<IFoo>("unexpected arg") today would get no error, no warning, and a silently discarded argument. This is a latent bug/DX trap.

Suggested approach: Add a Roslyn analyzer diagnostic (or extend the existing SealedClassMockAnalyzer/StructMockAnalyzer pattern) that fires when constructorArgs are passed to Mock.Of<T>() where T is an interface. This turns a silent no-op into a compile-time warning.

2. Overload ambiguity risk: MockBehavior vs params object[]

The public API now has these overloads:

Mock.Of<T>()
Mock.Of<T>(MockBehavior)
Mock.Of<T>(params object[] constructorArgs)
Mock.Of<T>(MockBehavior, params object[] constructorArgs)

If MockBehavior is an enum, a caller writing Mock.Of<ConcreteService>(MockBehavior.Strict) could inadvertently hit Of<T>(params object[]) instead of Of<T>(MockBehavior) — enum values are implicitly convertible to object. C# overload resolution will prefer the non-params overload here, but this is subtle and the behavior could surprise callers who accidentally pass behavior as a constructor arg.

Suggested approach: Consider making the params object[] overload require at least one explicit non-MockBehavior argument via a required leading parameter (e.g., Of<T>(object firstArg, params object[] rest)), or document explicitly that the behavior overload must be used when behavior is the sole argument.

3. Missing test for the [assembly: GenerateMock] bugfix

The PR description calls out a concrete bug: [assembly: GenerateMock(typeof(ConcreteClass))] always generated interface-style mocks for concrete classes. The fix in TransformGenerateMockAttribute (isPartialMock: namedType.TypeKind == TypeKind.Class) is correct — but there's no snapshot test exercising this code path. If someone later changes TransformGenerateMockAttribute, the regression won't be caught automatically.

Suggested approach: Add a MockGeneratorTests case with [assembly: GenerateMock(typeof(SomeConcreteClass))] and a corresponding .verified.txt snapshot that asserts partial-mock code generation (i.e., RegisterFactory<ConcreteClass> with new ConcreteClass_MockImpl(engine, constructorArgs) pattern).

---

Minor

• The BOM removal (-// <auto-generated/> → +// <auto-generated/>) in several snapshots is fine but worth verifying it's intentional (likely a side-effect of the generator output changing slightly).

---

Overall this is a solid improvement. The three concerns above are worth addressing before merge — particularly #1 (silent arg drop) and #3 (missing regression test for the bug that motivated the fix).

[claude]

Code Review

This is a well-motivated refactor that simplifies the public API surface. The core direction — eliminating OfPartial<T>() and letting the source generator infer mock strategy from type kind — is sound. Below are my findings, from higher to lower priority.

---

1. Overload Ambiguity: Of<T>(MockBehavior) vs Of<T>(params object[])

File: TUnit.Mocks/Mock.cs, lines 109–114

public static Mock<T> Of<T>(MockBehavior behavior) where T : class
    => Of<T>(behavior, []);

public static Mock<T> Of<T>(params object[] constructorArgs) where T : class
    => Of<T>(MockBehavior.Loose, constructorArgs);

MockBehavior is an enum, and enums are implicitly convertible to their underlying integral type (int). If a concrete class has a constructor taking a single int, then calling Mock.Of<MyService>(42) is ambiguous — the compiler must choose between Of<T>(MockBehavior) and Of<T>(params object[]). Because MockBehavior is an enum with value 42 not defined, the params object[] overload wins, but the intent is unclear and fragile.

More concretely: if a user calls Mock.Of<ConcreteLogger>(MockBehavior.Strict), the compiler resolves to Of<T>(MockBehavior) today, but a future overload or change to how the enum is defined could silently shift resolution. Users reading the call can't immediately tell which path is taken.

Suggested fix: Make the behavior+args overload the canonical one and keep Of<T>(MockBehavior) as => Of<T>(behavior, []), which is already the case. The risk is in the standalone Of<T>(params object[]) overload conflicting with Of<T>(MockBehavior) for enum-as-int arguments. A conservative fix is to document this clearly and add an analyzer, or consider a named-parameter pattern:

// Alternative: avoid the overload entirely by using a factory-style API
public static Mock<T> Of<T>(MockBehavior behavior = MockBehavior.Loose, params object[] constructorArgs)

This requires C# 12's params + optional parameter combination, which is available given LangVersion: preview.

---

2. Runtime Validation is the Wrong Layer for Interface Constructor-Arg Guard

Files: All updated *.verified.txt snapshots, MockFactoryBuilder.cs line 65

if (constructorArgs.Length > 0) throw new global::System.ArgumentException(
    "Interface mock 'global::IGreeter' does not support constructor arguments, but "
    + constructorArgs.Length + " were provided.");

This check is generated into every single interface mock factory. That means the guard code exists in N generated classes (one per mocked interface), and the error is thrown at runtime rather than at compile time or source-gen time.

Since the source generator already knows at codegen time whether the type is an interface, this validation could instead be surfaced as:

• A Roslyn diagnostic (analyzer warning/error) when constructorArgs are passed to Mock.Of<IInterface>(args), giving immediate feedback in the IDE and at compile time.
• Or at minimum, handled in the shared Mock.Of<T>(MockBehavior, params object[]) method using reflection (typeof(T).IsInterface), rather than emitting the same string-concatenation check into every generated factory.

The per-factory emission bloats the generated code unnecessarily and makes the error message fragile (the fully-qualified name is hardcoded as a string literal in the generated output, which is fine for accuracy but adds size for zero benefit over a centralized check).

---

3. IsPartialMock Semantics Now Overlap with IsInterface

File: TUnit.Mocks.SourceGenerator/Models/MockTypeModel.cs

After this PR, IsInterface == true implies IsPartialMock == false by construction (interfaces get false via TypeKind == TypeKind.Class). The field pair is no longer orthogonal — one is fully derivable from the other in current usage. The tautological guard removal (IsPartialMock && !IsInterface → IsPartialMock) is correct, but the IsInterface field is now only used in factory/impl routing, not for the partial-vs-full distinction.

This isn't a bug, but it signals an opportunity to simplify the model further. A discriminated union or a MockKind enum (Interface, AbstractClass, ConcreteClass, Delegate, Wrap, Multi) would make the routing logic in MockFactoryBuilder.Build() and MockImplBuilder.Build() more explicit and extensible:

// Instead of: IsInterface, IsPartialMock, IsDelegateType, IsWrapMock
internal enum MockKind { Interface, Class, Delegate, Wrap, Multi }
internal sealed record MockTypeModel { public MockKind Kind { get; init; } ... }

This is a bigger refactor than this PR targets, but worth noting as a follow-up to avoid the model growing more boolean flags.

---

4. Constructor Dispatch: No Validation for Abstract Classes

File: TUnit.Mocks.SourceGenerator/Builders/MockFactoryBuilder.cs, GenerateConstructorDispatch

For abstract classes, model.Constructors is populated (since isPartialMock && namedType.TypeKind == TypeKind.Class is satisfied for abstract classes too). However, abstract classes can have constructors that are protected but not public. The generated dispatch uses new {safeName}_MockImpl(engine, args...) which calls the base constructor. If the user passes zero args to Mock.Of<AbstractService>() but the abstract class only has a protected (string) constructor, the dispatch falls to the else branch and throws at runtime.

This scenario is already handled for concrete classes, but the error message could be more actionable. More importantly, the DiscoverConstructors method should ensure it's only surfacing accessible constructors, and the dispatch else branch message could hint at which constructors are available:

throw new ArgumentException(
    $"No matching constructor for '{model.FullyQualifiedName}' with {constructorArgs.Length} arg(s). " +
    $"Available arities: {string.Join(", ", model.Constructors.Select(c => c.Parameters.Length))}.");

---

5. _multiFactories Still Uses object[] Signature but Always Passes []

File: TUnit.Mocks/Mock.cs, lines 179–226

The multi-interface overloads (Of<T1,T2>, Of<T1,T2,T3>, etc.) now pass [] to the factory but the factory signature accepts object[]. The generated multi-interface factories also throw on constructorArgs.Length > 0. Since multi-interface mocks are always interface-only combinations, the object[] parameter in _multiFactories serves no purpose. This adds unnecessary noise to the delegate signature. A simple static helper Mock.Of<T>(MockBehavior behavior, object[] constructorArgs) could be the single implementation target, but the multi registry could keep the simpler Func<MockBehavior, object> (now removed) rather than the array variant.

This is a minor cleanliness issue, but it means the unified signature introduced extra complexity in the multi-factory path rather than simplifying it.

---

6. Snapshot File: BOM Removal Side-Effect

Several snapshot files have a diff entry changing // <auto-generated/> (with BOM) to // <auto-generated/> (without BOM). This is an unrelated change mixed into the PR. While removing BOMs from generated files is generally good practice, it may cause unexpected diff noise in future PRs if some files are regenerated with BOM and others without. It's worth confirming whether the snapshot verification tool (Verify) changed its output format, or if this is a one-time normalization.

---

Summary

The unification of Of<T>() and OfPartial<T>() into a single entry point is a good ergonomic improvement and the bug fix for [assembly: GenerateMock] with concrete classes is correct. The main concerns to address before merging:

1. Overload ambiguity between Of<T>(MockBehavior) and Of<T>(params object[]) — at minimum add a comment or test showing the intended precedence for enum values passed as constructor args.
2. Move the interface+constructor-arg guard out of generated code and into a centralized place (either an analyzer or Mock.Of<T>(MockBehavior, params object[]) via typeof(T).IsInterface).
3. Improve the constructor dispatch error message to enumerate available arities.

[claude]

Code Review (updated — all 4 commits)

Previous reviews flagged two concrete issues. Both have been addressed:

• Interface mock constructor args guard — the generated interface factory now throws ArgumentException with a clear message. The switch to string interpolation in commit 4 is a nice cleanup.
• Missing regression test for [GenerateMock(typeof(ConcreteClass))] — the new GenerateMock_Attribute_With_Concrete_Class snapshot test covers the bugfix path. Good addition.

The overall direction (unify OfPartial into Of, derive mock kind from TypeKind, fix the GenerateMock bug) is well-executed. A couple of remaining items worth noting:

---

Residual concern: snapshot covers only default-constructor class

The new snapshot test uses a MyService with no explicit constructor (so base() is correct in the generated MockImpl). But the motivating bugfix is that [assembly: GenerateMock(typeof(ConcreteClass))] was generating interface-style code — meaning constructor arg dispatch was also broken for that path.

A class with a non-default constructor in the snapshot (e.g., MyService(string name)) would assert that constructorArgs actually get forwarded to the base call, not just that the right factory type is generated. Without it, a regression in the constructor-dispatch path for attribute-triggered mocks won't be caught automatically.

Minor: empty array allocation on every Of<T>(MockBehavior) call

public static Mock<T> Of<T>(MockBehavior behavior) where T : class
    => Of<T>(behavior, []);

[] allocates a new object[0] on every call. In test contexts this is negligible, but since the pattern appears in four call sites (OfDelegate, multi-interface overloads, etc.), using Array.Empty<object>() is a zero-cost improvement that's consistent with the project's allocation-awareness ethos.

---

Summary

The three concerns from earlier reviews that were actionable have been addressed. The residual items above are low severity — the constructor-dispatch snapshot gap is worth a follow-up but doesn't block correctness (the underlying BuildPartialFactory already handles the args; it's a test coverage gap, not a logic gap). This is ready to merge with those notes in mind.

[claude]

Code Review (final pass — commit 5/de303ab9)

This is the fifth review pass. All three previously-flagged actionable concerns have been addressed across commits 3–5:

• Interface constructor-arg guard — generated factories now throw ArgumentException with an interpolated message (commit 4 cleanup).
• GenerateMock concrete-class regression test — GenerateMock_Attribute_With_Concrete_Class snapshot added (commit 3).
• Array allocation in Of<T>(MockBehavior) — [] replaced with Array.Empty<object>() (commit 5).

The PR is in good shape. The remaining notes below are low-severity and informational rather than blockers.

---

Residual gap: concrete-class snapshot uses only default constructor

The new snapshot test (GenerateMock_Attribute_With_Concrete_Class) declares a MyService with no explicit constructor. The generated MockImpl therefore uses new MyService_MockImpl(engine) (the model.Constructors.Length == 0 path in GenerateConstructorDispatch).

The bug this test was added to cover — attribute-triggered mocks generating interface-style code for concrete classes — is correctly caught. However, the constructor-arg forwarding path (GenerateConstructorDispatch branches for arity > 0) is not exercised by any attribute-triggered test. A regression there would still go undetected. This is a test-coverage gap, not a logic gap; the underlying BuildPartialFactory already forwards args correctly via Mock.Of<T>()-triggered tests.

Suggested follow-up (non-blocking): Add a second snapshot with a concrete class that has a single non-default constructor, e.g.:

[assembly: GenerateMock(typeof(MyService))]

public class MyService(string name, int retries) 
{
    public virtual string GetValue() => name;
}

This would assert that the generated factory dispatches on constructorArgs.Length == 2 and casts correctly, closing the coverage gap for the attribute code path.

---

Minor: _multiFactories still passes Array.Empty<object>() but accepts object[]

Multi-interface overloads (Of<T1,T2>(), Of<T1,T2,T3>(), etc.) call factory(behavior, Array.Empty<object>()) and the corresponding generated factories immediately throw if constructorArgs.Length > 0. The registry signature Func<MockBehavior, object[], object> carries a parameter that is structurally never used for multi-interface mocks.

This is a cosmetic inconsistency left over from the unification. It causes no correctness issue. A follow-up could use Func<MockBehavior, object> for _multiFactories to match the actual semantics, but that's a larger signature change and the current approach is consistent (uniform delegate type across all registries).

---

No new concerns

The overload-ambiguity question (previous reviews: Of<T>(MockBehavior) vs Of<T>(params object[]) when an enum value is passed) is inherent to C# overload resolution and is now a documented trade-off. MockBehavior being an enum means C# will prefer the non-params overload, which is the correct behavior. The edge case (a class whose constructor takes a MockBehavior-typed argument as its sole argument) is narrow enough that the current design is acceptable without further mitigation.

---

Summary

All previously-raised actionable concerns are resolved. The only remaining item is a test-coverage gap for the attribute-triggered constructor-args path, which is low priority. This PR is ready to merge.