InvalidOperationException when specifiying setup on mock with mock containing property of type Nullable<T>

[dav1dev]

Description

Value of nullable object gets lost while it is used to setup a method call on another mock. This results in an InvalidOperationException.
Example: myMock.Setup(x => x.Do(dataMock.Id.Value));

Expected Behavior

Value of nullable type remains on mock as setup.

Actual Behavior

Mock instance/context gets lost in scope of setup call on another mock in case the referenced property is of nullable type.

Reproduction

To reproduce this issue I created a repository: https://github.com/dav1dev/moq-NullableIssue.
The issue is reliably reproduced in failing tests in class ProcessorTest.

Versions

The issue can be reproduced in moq version 4.10.0 on .NET framework 4.6.1. Castle.Core is used in version 4.3.1.

[stakx]

@dav1dev - Thank you for reporting this. Will look into this ASAP and report findings back here.

[stakx]

@dav1dev - Nice find! 👍

TL;DR: If you're just interested in fixing this problem, then swap out data with something that is not a Mock<>. For example, manually create a sub-class that returns an appropriate value for the Id propery. If you want to understand what's going on, read on.

I'll start with a short recap of what your repro code boils down to, then let's look at what goes wrong:

public interface IHandler
{
    void Do(Guid id);
    void DoNullable(Guid? id);
}

public class DataStructureGuid
{
    public virtual Guid? Id { get; }
}

[Fact]
public void Setup_Do_GetUnexpectedException()
{
    var id = Guid.NewGuid();
    var data = Mock.Of<DataStructureGuid>(x => x.Id == id);
    new Mock<IHandler>().Setup(x => x.Do(data.Id.Value));
}

[Fact]
public void Setup_DoNullable_NoException()
{
    var id = Guid.NewGuid();
    var data = Mock.Of<DataStructureGuid>(x => x.Id == id);
    new Mock<IHandler>().Setup(x => x.DoNullable(data.Id));
}

The arguments inside the setup expressions (data.Id and data.Id.Value) are of special interest here. The latter argument will cause an exception because data.Id.Value will trigger a null dereference exception, even though data.Id.Value has been set to a valid GUID just prior. How is this possible?

For each argument in a setup expression, Moq needs to find out whether it is a matcher (such as It.IsAny<T>()) or not, as matchers must be treated differently from e.g. constant argument values.

In order to discover argument matchers, Moq makes use of an internal component called the "ambient observer". This is a special mode of operation that, when activated on the current thread, will take note of all mock invocations and matcher invocations. Moq will activate the ambient observer, then compile and execute the argument expression, and check with the observer whether a matcher was invoked. (See these lines of code.)

One side effect of the ambient observer is that setups of invoked mocks will be ignored (see these lines of code). This is very much necessary for other use cases of the ambient observer. Being an "observer", it shouldn't cause state changes of its own, thus setups which can have Callbacks and whatnot must be disabled.

Now, in all of your repro unit tests, data are mocks, and the data.Id property's value has been set up.

So when Moq tries to discover whether data.Id and data.Id.Value are argument matchers, it will switch into its ambient observer mode, thereby disabling all setups, which makes data.Id return, not the GUID you set it up to return, but default(Guid) (in the case of the working unit test), or default(Guid?) (in the case of the failing unit test), respectively. So when the expression default(Guid?).Value gets executed, you get a null dereference and thus an exception.

A fix for this problem will be tricky, but would seem feasible.

[dav1dev]

@stakx Thank you for this insight. A fix for this case did not pop into my mind just yet.

Meanwhile I worked around the issue by specifying the setups with an extra variable as demonstrated below:

var id = Guid.NewGuid();
var data = Mock.Of<DataStructureGuid>(x => x.Id == id);
_handler.Setup(x => x.Do(id)).Returns(Mock.Of<IResult>());

_testee.Process(data);

[stakx]

@dav1dev - Glad you found a workaround for the time being!

Regarding a bug fix, there are several options. My personal favourite would be to bring back the [Matcher] attribute, which has been deprecated a long time ago. However, Moq's XML documentation comments still (erroneously) state that all custom matcher methods must be decorated with a [Matcher] attribute. I'd like to reinstate this requirement (which would be a small breaking change). This would then allow Moq to discover matchers statically (just by looking at the expression, without compilation & execution) and make things much more efficient at the same time.

There are other options (making the matcher recognition algorithm more intelligent by inspecting the argument expressions more closely before executing them) but that gets complicated very quickly.

[dav1dev]

@stakx Thank you for closing this issue!