Skip to content

refactor(mocks): remove generic/untyped overloads from public API#5313

Merged
thomhurst merged 3 commits intomainfrom
hide-untyped-callback-overload
Mar 30, 2026
Merged

refactor(mocks): remove generic/untyped overloads from public API#5313
thomhurst merged 3 commits intomainfrom
hide-untyped-callback-overload

Conversation

@thomhurst
Copy link
Copy Markdown
Owner

@thomhurst thomhurst commented Mar 30, 2026

Summary

Remove all generic object?[]-based overloads from the public mock API surface. The source generator produces typed alternatives for each mocked method, so these untyped methods are internal plumbing that shouldn't be visible to users.

Removed from interfaces, MockMethodCall/VoidMockMethodCall, and generated wrapper classes:

Removed generic method Typed alternative (source-generated)
Callback(Action<object?[]>) Callback((T1 a, T2 b) => ...)
Returns(Func<object?[], TReturn>) Returns((T1 a, T2 b) => ...)
Throws(Func<object?[], Exception>) Throws((T1 a, T2 b) => ...)
SetsOutParameter(int, object?) SetsOut{Name}(T) / SetsRef{Name}(T)
Raises(string, object?) Raises{EventName}(args)

Kept on concrete builders (MethodSetupBuilder, VoidMethodSetupBuilder) marked [EditorBrowsable(Never)] — needed internally by the generated typed wrappers which delegate to them.

Cleaned up:

  • Removed generic methods from PropertySetupBuilder / PropertySetterSetupBuilder
  • Removed unused using System.ComponentModel from interfaces
  • Slimmed ISetupChain / IVoidSetupChain interfaces
  • Converted all test callsites to typed alternatives

Closes #5287

Test plan

  • All 679 mock tests pass
  • All 22 source generator snapshot tests pass
  • Verified typed callbacks, typed returns, typed throws, typed out/ref setters all work end-to-end

… API

Remove the untyped Callback(Action<object?[]>) overload from interfaces
and generated wrapper classes so users are guided toward the typed
callbacks generated per-method. The untyped overload remains on concrete
builders (needed by generated typed callback wrappers) and on
MockMethodCall/VoidMockMethodCall as an escape hatch for methods
exceeding MaxTypedParams, all marked [EditorBrowsable(Never)].

Closes #5287
Copy link
Copy Markdown
Contributor

@claude claude bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review: Hide Untyped Callback(Action<object?[]>) from Public API

This is a clean, well-scoped refactor that improves the mock API's discoverability. Overall the design is sound.

What the PR does well

  • Correct abstraction level: Removing Callback(Action<object?[]>) from IMethodSetup<TReturn> and IVoidMethodSetup is the right call. The interface should express the ideal usage path, not the implementation detail.
  • Escape hatch preserved: Keeping the untyped overload on concrete builders (marked [EditorBrowsable(Never)]) for use by generated typed wrappers is the correct pattern — same as how Returns/Throws untyped overloads are handled.
  • Consistent treatment: Applying [EditorBrowsable(Never)] to PropertySetupBuilder and PropertySetterSetupBuilder untyped overloads maintains consistency across the API surface.
  • Tests converted cleanly: All test callsites now use typed callbacks, which also validates the typed callback feature end-to-end.
  • Snapshots updated correctly: All 13 affected .verified.txt snapshots are updated, and no .received.txt files are committed.

Minor observation: asymmetry between interfaces and concrete builders

The Returns(Func<object?[], TReturn>) overload already had [EditorBrowsable(Never)] on the interface before this PR, but Callback(Action<object?[]>) on the interface was only hidden (not removed) before this change. This PR removes the overload from the interface entirely, which is a breaking change for any external code that has a variable typed as IMethodSetup<TReturn> and calls .Callback((object?[] args) => ...).

If TUnit.Mocks is not yet at a stable API version, this is fine. If it is, consider whether a deprecation cycle is warranted. The PR description doesn't mention versioning implications — worth a note in the changelog or release notes.

No issues found with the implementation itself

The approach of removing from the interface while retaining on concrete types (hidden from IntelliSense) is a clean pattern used elsewhere in this codebase for the same purpose. The PR is consistent and complete.

Remove Returns(Func<object?[],...>), Throws(Func<object?[],...>),
Raises(string,...), and SetsOutParameter(int,...) from interfaces,
MockMethodCall/VoidMockMethodCall, and generated wrapper classes.

The source generator already produces typed alternatives for all of
these: typed Returns/Callback/Throws with method parameter types,
SetsOut{Name}/SetsRef{Name} for out/ref params, and
Raises{EventName} for events.

Generic overloads remain on concrete builders marked
[EditorBrowsable(Never)] for use by generated code only.
@thomhurst thomhurst changed the title refactor(mocks): hide untyped Callback(Action<object?[]>) from public API refactor(mocks): remove generic/untyped overloads from public API Mar 30, 2026
@thomhurst thomhurst enabled auto-merge (squash) March 30, 2026 17:59
Copy link
Copy Markdown
Contributor

@claude claude bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review: Remove All Generic/Untyped Overloads from Public API

This is a follow-up to the previous review. The scope has expanded from the first commit (hiding Callback(Action<object?[]>)) to the second commit (removing ALL untyped overloads — Returns, Throws, Raises, and SetsOutParameter as well). The overall approach remains sound and the execution is clean.

Previous concern addressed?

The previous review noted the asymmetry between Returns(Func<object?[], TReturn>) (previously [EditorBrowsable(Never)] on the interface) and Callback (removed entirely). This PR resolves that asymmetry by removing all the untyped overloads from the generated per-method wrapper classes consistently. ✓

What this PR does well

  • Consistent treatment across the board: All five untyped overload families (Callback, Returns, Throws, Raises, SetsOutParameter) are handled uniformly. No partial hiding, no inconsistent patterns.
  • Plumbing stays internal: Retaining the untyped methods on concrete builders (MethodSetupBuilder, VoidMethodSetupBuilder) marked [EditorBrowsable(Never)] is the right pattern — the source-generated typed wrappers delegate to them without exposing the indirection to users.
  • Raises deserves special mention: Removing the stringly-typed Raises(string eventName, object? args) in favour of source-generated Raises{EventName}(args) eliminates a whole class of runtime typos. This is a meaningful improvement beyond just API cleanliness.
  • Snapshot tests fully updated: All 13 snapshots updated correctly, no committed.

Remaining observation: this is a broader breaking change than the first commit

The previous review raised this for Callback specifically. Now that Returns, Throws, Raises, and SetsOutParameter are also removed from the generated per-method wrapper types, any external user code that:

  1. Holds a reference to a specific generated mock-call type (e.g. IGreeter_Greet_M0_MockCall), or
  2. Holds a variable typed as IMethodSetup<TReturn> / IVoidMethodSetup

...and calls these overloads will fail to compile after upgrading. This is still fine if TUnit.Mocks has not reached a stable API version, but is worth a prominent note in the changelog/release notes.

One specific edge case worth double-checking: Raises(string eventName, object?) — the typed alternatives (Raises{EventName}(args)) are generated only for events that the source generator knows about. If a user ever needs to raise an event that isn't declared on the mocked interface (e.g. inherited events surfaced indirectly), the typed route may not cover it. This is a niche scenario but worth confirming the generator handles it.

No implementation issues found

The source-generator change in MockMembersBuilder.cs is the minimal correct fix — removing the lines that emitted the untyped forwarding methods. The internal escape hatch on concrete builders is preserved. The approach is architecturally sound.

Copy link
Copy Markdown
Contributor

@claude claude bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review: Remove All Generic/Untyped Overloads from Public API

This is a follow-up to the previous review. The scope has expanded from the first commit (hiding Callback(Action<object?[]>)) to the second commit (removing ALL untyped overloads — Returns, Throws, Raises, and SetsOutParameter as well). The overall approach remains sound and the execution is clean.

Previous concern addressed?

The previous review noted the asymmetry between Returns(Func<object?[], TReturn>) (previously [EditorBrowsable(Never)] on the interface) and Callback (removed entirely). This PR resolves that asymmetry by removing all the untyped overloads from the generated per-method wrapper classes consistently. ✓

What this PR does well

  • Consistent treatment across the board: All five untyped overload families (Callback, Returns, Throws, Raises, SetsOutParameter) are handled uniformly. No partial hiding, no inconsistent patterns.
  • Plumbing stays internal: Retaining the untyped methods on concrete builders (MethodSetupBuilder, VoidMethodSetupBuilder) marked [EditorBrowsable(Never)] is the right pattern — the source-generated typed wrappers delegate to them without exposing the indirection to users.
  • Raises deserves special mention: Removing the stringly-typed Raises(string eventName, object? args) in favour of source-generated Raises{EventName}(args) eliminates a whole class of runtime typos. This is a meaningful improvement beyond just API cleanliness.
  • Snapshot tests fully updated: All 13 .verified.txt snapshots updated correctly, no .received.txt committed.

Remaining observation: this is a broader breaking change than the first commit

The previous review raised this for Callback specifically. Now that Returns, Throws, Raises, and SetsOutParameter are also removed from the generated per-method wrapper types, any external user code that holds a reference to a specific generated mock-call type (e.g. IGreeter_Greet_M0_MockCall) and calls these overloads will fail to compile after upgrading. This is still fine if TUnit.Mocks has not reached a stable API version, but is worth a prominent note in the changelog/release notes.

One edge case to consider

Raises(string eventName, object?) — the typed alternatives (Raises{EventName}(args)) are generated only for events the source generator knows about. If a user ever needs to raise an event that is not declared on the mocked interface (e.g. inherited events surfaced indirectly), the typed route may not cover it. Worth confirming the generator handles inherited event detection correctly.

No implementation issues found

The source-generator change in MockMembersBuilder.cs is the minimal correct fix — removing the lines that emitted the untyped forwarding methods. The internal escape hatch on concrete builders is preserved. The approach is architecturally sound.

@thomhurst thomhurst disabled auto-merge March 30, 2026 18:31
The CallbackBenchmarks still used the removed untyped
Action<object?[]> overload, causing CS1593 build failures.
Copy link
Copy Markdown
Contributor

@claude claude bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review: Remove Generic/Untyped Overloads from Public API (Follow-up)

This is a third review pass. Previous reviews have been tracked carefully — here is the status of each prior concern and any new observations.

Prior concerns: addressed?

1. Asymmetry between Callback and Returns/Throws/Raises/SetsOutParameter (Review 1)
Fully resolved. All five untyped overload families are now treated uniformly — removed from generated per-method wrappers and from MockMethodCall/VoidMockMethodCall, and hidden with [EditorBrowsable(Never)] on the concrete builders. ✓

2. Breaking change versioning note (Reviews 1 and 2)
Not addressed in the PR (no changelog/release note added), but this is an editorial concern rather than a code correctness issue. Still worth a note in the release, but not a blocker.

3. Inherited events coverage for Raises{EventName} (Review 2)
Still not addressed. Since this is a niche edge case and there is no test demonstrating the failure path, it remains an open question rather than a regression. No action required from this PR.

New observation: PropertySetupBuilder / PropertySetterSetupBuilder diverge from the concrete-builder pattern

MethodSetupBuilder and VoidMethodSetupBuilder keep the untyped overloads but annotate them [EditorBrowsable(Never)]. This is correct because the source-generated typed wrappers delegate to them.

PropertySetupBuilder and PropertySetterSetupBuilder remove the untyped overloads entirely rather than hiding them:

// PropertySetupBuilder.cs — methods simply deleted:
-    public ISetupChain<TProperty> Callback(Action<object?[]> callback) => _inner.Callback(callback);
-    public ISetupChain<TProperty> Returns(Func<object?[], TProperty> factory) => _inner.Returns(factory);
-    public ISetupChain<TProperty> Throws(Func<object?[], Exception> exceptionFactory) => _inner.Throws(exceptionFactory);
-    public ISetupChain<TProperty> Raises(string eventName, object? args = null) => _inner.Raises(eventName, args);
-    public ISetupChain<TProperty> SetsOutParameter(int paramIndex, object? value) => _inner.SetsOutParameter(paramIndex, value);

This is actually correct behaviour for property builders, because:

  • Properties have no parameters, so Callback(Action<object?[]>) and Returns(Func<object?[], T>) are meaningless (the array would always be empty).
  • Properties have no out/ref parameters, so SetsOutParameter is meaningless.
  • Source-generated typed wrappers for properties don't generate Callback(Action<T1, T2, ...>) overloads at all, so there is nothing internal that needs to delegate through PropertySetupBuilder.

So the apparent inconsistency is actually intentional and correct. This is worth a short inline comment in PropertySetupBuilder explaining why the delegation methods are absent (not just because the API is being cleaned up, but because they are semantically irrelevant to properties). Without such a comment, a future contributor might add them back for "symmetry" with MethodSetupBuilder.

Suggestion (non-blocking): Add a brief XML doc or code comment to PropertySetupBuilder:

// Properties have no parameters, so untyped Callback(Action<object?[]>)/Returns(Func<object?[], T>)
// overloads are not forwarded here — they are meaningless for property getters.

Summary

The implementation is correct and complete. The only actionable suggestion is a non-blocking documentation improvement to explain why PropertySetupBuilder / PropertySetterSetupBuilder intentionally omit the delegation methods that MethodSetupBuilder keeps (hidden). All previous substantive concerns have been resolved.

This was referenced Mar 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

TUnit.Mocks: Add typed callback overloads

1 participant