'Returns' regression with null function callback

[Caraul]

Restore 4.7 behavior, add tests and update CHANGELOG.md

Closes #600.

[stakx]

This looks good to me, good work! 👍 If you don't mind, just two minor things that I'd like you to take a look at.

[stakx]

Please replace the issue number (#600) with the PR number (#602) to keep the changelog formatting consistent.

[stakx]

Let's rewrite these two comment lines so it becomes clear that we're not just being "overly cautious", but that this is in fact a required piece of code. For example:

// A `null` reference (instead of a valid delegate) is interpreted as the actual return value.
// This is necessary because the compiler might have picked the wrong overload for calls like
// `Returns(null)`, and instead of in `Returns(TResult)`, we ended up in `Returns(Delegate)` or
// `Returns(Func<TResult>)`, which likely isn't what the user intended. So here we do what
// we would've done in `Returns(TResult)`:

(We're essentially dealing with the same problem—C# picking the "wrong" overload—as already mentioned here:)

https://github.com/moq/moq4/blob/f86759225c46b072c4a721b200b31992d4c54b8b/Source/MethodCallReturn.cs#L92-L101

[Caraul]

Well, the comment is subject to discuss :)

I've already mentioned "Returns(null) doesn't compile due to ambiguous invocation" so there is no wrong pick - anyway the user should clearly choose one of Returns overloads with typecasting or "closing" generics etc. Rock on which we split is Returns<TResult>(null) which seems like Returns((TResult)null) but in fact it's IReturns.Returns<T>((Func<T, TResult>)null) - surprisingly, isn't it? So I'd propose sightly modified version:

// A `null` reference (instead of a valid delegate) is interpreted as the actual return value.
// This is necessary because the user might have picked an ambiguous overload like `Returns<TResult>(null)`,
// but instead of in `Returns(TResult)`, we ended up in `Returns<T>(Func<T, TResult>)`,
// which likely isn't what the user intended. So here we do what we would've done in `Returns(TResult)`

What do you think?

[stakx]

"Returns(null) doesn't compile due to ambiguous invocation" so there is no wrong pick

Returns(null) compiles just fine in some quick scenarios that I've tried, so I can't really confirm this.

anyway the user should clearly choose one of Returns overloads with typecasting or "closing" generics etc.

That may be so, but we have no control over whether they think so, too, nor whether users will actually do so. If everyone explicitly chose the right overload, then passing null instead of a delegate should trigger an ArgumentNullException. But let's stay on the safe side and assume that some people let the compiler pick the right overload. And because that can go wrong, we need to "correct" those situations.

Regarding your new comment, that's principally also fine with me. But let's be clear that to the compiler, the overload isn't ambiguous at all. It's simply that users might expect a different overload to be called than what the compiler will choose.

[Caraul]

Returns(null) compiles just fine in some quick scenarios that I've tried, so I can't really confirm this.

The truth appears to be in between - it's ambiguous for references but for value types compiler chooses Returns(Func<TResult>).

users might expect a different overload

My point exactly. The only note that reasons are "compiler chooses unexpected overload" and "user chooses an overload that behaves unexpectedly". I'd also try to avoid the word "wrong" as it may be incorrectly interpreted as "bug in compiler". So what about version:

// A `null` reference (instead of a valid delegate) is interpreted as the actual return value.
// This is necessary because the compiler might have picked the unexpected overload for calls like
// `Returns(null)`, or the user might have picked an overload like `Returns<TResult>(null)`,
// and instead of in `Returns(TResult)`, we ended up in `Returns(Delegate)` or
// `Returns(Func<TResult>)`, which likely isn't what the user intended. So here we do what
// we would've done in `Returns(TResult)`:

[stakx]

That's fine with me, too, except I think that when the user picks a specific overload e.g. by explicitly specifying the generic type argument, then AFAIK we cannot end up in any other overloads. So I'd remove the part that says or the user might .... (How can the chosen overload be unexpected if the choice was explicitly made by the user?)

I agree that avoiding the word wrong is a good idea.

I really don't mean to be too fussy about the comment, main thing is that we're more specific than my original overly cautious comment, which clearly no longer applies.

[stakx]

Thanks for the changes!

[stakx]

... and merged! Thank you for fixing this regression!

[stakx]

@Caraul - I just pushed Moq 4.8.3 to NuGet, which includes your changes. Sorry it's taken so long.

[Caraul]

Great job, thank you very much! 2018-06-09 22:59 GMT+03:00 stakx <[email protected]>:

…

@Caraul <https://github.com/Caraul> - I just pushed Moq 4.8.3 to NuGet, which includes your changes. Sorry it's taken so long. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#602 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AF6ZRLS3R282-C9TMju7ODuOG9269eMlks5t7CkRgaJpZM4Se-ro> .