.Callback(…) after .Returns(…) / .CallBase() causes a variety of issues

[stakx]

Moq has allowed a .Callback(…) to occur after .Returns(…) or .CallBase() for a while now. This was originally discussed in this discussion on Google Groups and then formally requested in Google Code issue 19.

Unfortunately, the way after-return callbacks are currently implemented in the fluent API, they cause a variety of issues:

• Fluent API allows setting the return value and throwing an exception at the same time #665: Fluent API allows setting the return value and throwing an exception at the same time
• Fluent API allows setting the return value twice #666: Fluent API allows setting the return value twice
• Fluent API allows setting up a second callback that won't get involved if .Throws() is specified too #667: Fluent API allows setting up a second callback that won't get involved if .Throws() is specified too

These issues result largely from the fluent API interface ICallback being reused inappropriately, instead of introducing a dedicated and more limited ICallbackAfterReturn.

It can be argued that the ability of configuring a callback after .Returns(…) or .CallBase() is fundamentally wrong, since in regular methods everything after a return statement would be unreachable. The order of the Callback, CallBase, Raises, Returns, Throws verbs in a Moq setup generally represents the order in which these steps will be executed at runtime, so in analogy to regular methods, one could expect that .Returns(…) should be the final verb.

Here are few options (there may be more) how to fix this:

1. Rename the after-return Callback verb to Finally:

   mock.Setup(m => m.Message).Callback(…).Returns(…).Finally(…);
                                                     ^^^^^^^^^^

   This would be a breaking change for two reasons: (1) Different method name. (2) Different semantics: A finally gets invoked even in the presence of an exception. An after-return callback currently won't get invoked if an exception happens beforehand.

2. Fix the fluent API so .Returns(…) and .CallBase() return an interface that doesn't inherit ICallback, but a new ICallbackAfterReturns interface instead which has more limited "flow" options. This could fix the issues linked above (at the cost of quite a bit of additional internal code duplication), but it doesn't even address the more fundamental issue.

3. Don't fix the problems linked above. Instead, deprecate after-return Callback and possibly remove this facility in Moq v5 or v6.

[stakx]

@kzu: I would recommend that we deprecate after-returns callbacks, i.e. the following setup pattern:

mock.Setup(m => m.GetMessage()).Returns("Blah.").Callback(…);
//                                              ^^^^^^^^^^^^
//           Let's make `Callback` illegal after `Returns` (or `CallBase`).

This pattern doesn't make sense due to the reason I gave in the previous post. TL;DR: In a regular method, anything after return is unreachable, and Moq's fluent API should reflect that.

What's your opinion on this? Would this be an option for Moq v5 or v6?

[kzu]

I do think it's useful to be able to do a callback after a returns or a throws. This will be an option in v5.
The reasoning is that in an interception scenario, you can always run code before/after the method
execution, and hiding/preventing that feature is an unnecessary restriction. I agree that it's not obvious
what you'd use that for, I'm having trouble making a concrete and obvious example :)

That said, introducing a new Finally verb sounds good to me. Sounds like a useful thing, with the
behavior of running whether you have a Returns, CallBase or Throws.

Further restricting the ICallback by introducing a, say ICallbackAfter (wouldn't call it AfterReturns
because I think it also applies to CallBase, no?) would be a good thing. That interface could have the
same "shape" as ICallback to avoid breaking the build for existing customers, but with the obsoleted
members (Returns and Throws) marked as EditorBrowsable.Never and with an [Obsolete] attribute explaining a bit.

Alternatively, a Roslyn analyzer that flags the pattern as an error could do too ;).

[stakx]

Thanks for the feedback! 👍 Since Moq 5 will allow such callbacks we should then keep them in Moq 4. ICallbackAfter with some members marked [Obsolete] sounds good. (Though it's technically a breaking change anyway, since we're changing the return type of the .Returns and .CallBase verbs.) I'll prepare this for the next release.

[stakx]

Closing this for now. Might reconsider this for the next release; if we have other binary breaking changes in there, we could include the closed PR (linked above) there.