Returns(null) requires explicit typecast in Strict mocks

[Caraul]

Hi,

Returns(null) doesn't compile due to ambiguous invocation.
Returns((SomeType)null) is "inferred" to Returns(SomeType value) which has no problems.
Returns<SomeType>(null) is "inferred" to Returns(Func<SomeType> valueFunction) which is simply set null delegate with ReturnValueKind.Explicit. For MockBehavior.Strict in MethodCallReturn.Execute it throws with ExceptionReason.ReturnValueRequired (despite of set ReturnValueKind.Explicit). Is it correct? I'd suppose (this.valueDel != null) should be replaced with (this.returnValueKind == ReturnValueKind.Explicit) and either calls delegate or invocation.Return(default(TResult)) for explicit null values. Or maybe should Returns(Func<SomeType>) behave the same as Returns(Delegate valueFunction) which has special treatment for nulls "because Moq has been very forgiving with incorrect Returns in the past."?

[stakx]

@Caraul: I've noticed before that Returns has a few inconsistencies. The way how it deals with null might be one of them. That being said, I'm not sure whether I understand what your issue is about.

Is it about this:

Returns(null) doesn't compile due to ambiguous invocation.

There's probably very little that we can do about this. Changing this would likely require changing the method overloads' definitions, which would be likely to cause breaking changes.

Or is the issue about this:

For MockBehavior.Strict in MethodCallReturn.Execute it throws with ExceptionReason.ReturnValueRequired [...].

If so, then please confirm and I'll take a closer look at it.

[stakx]

Please forget the above question, I misunderstood.

This appears to be a regression introduced in Moq 4.8.0. Before that, it appears to have been legal to call Returns(Func<TResult>) with null. I'll try to find out what went wrong and get back to you.

[stakx]

Looks like the problem is here in 4.8.0:

https://github.com/moq/moq4/blob/f6a8be6f7fd0baf6f57eba714e591d6274ae0f67/Source/MethodCallReturn.cs#L154-L163

which used to be (in 4.7.145):

https://github.com/moq/moq4/blob/2d06029206558f8fc1571da52a062e192ac83ff7/Source/MethodCallReturn.cs#L124-L136

Note that the null special case that used to be there in 4.7.145 has been removed in 4.8.0. This bug probably crept in while we improved Moq's support for by-ref parameters.

We should probably restore it, something along these lines:

if  (value == null)
{
    this.valueDel = (Func<TResult>)(() => default(TResult));
    // though I wonder why Moq used `default(TResult)` instead of `null` previously...?
}
else
{
    ValidateReturnDelegate(value);
    this.valueDel = value;
}
...

Would you like to work on this?

[Caraul]

Of course yes, I'll fix it.