NullReferenceException on subsequent setup if expression contains null reference

[IFYates]

Hi everyone.

I've recently jumped from Moq 4.12.0 to 4.14.3 and a few of our tests broke.

It took a while, but I eventually tracked it down to the use of expression references to objects that are null at the setup time, but only after the first Setup call.

I wouldn't say that the tests failing like this are well written, but I doubt this is an intentional change to Moq behaviour. I haven't looked at the internal cause yet, as I need to fix our failing tests first.

Reproduction example:

public class StubDataStore
{
    public string Value { get; set; }
}

public interface IDataStore
{
    bool IsStored(string arg);
}

[TestMethod]
public void MultiSetupNRE()
{
    // Arrange
    var exampleMock = new Mock<IDataStore>(MockBehavior.Strict);

    StubDataStore obj = null;
    exampleMock.Setup(m => m.IsStored(It.Is<string>(s => obj.Value == s))) // Binds correctly
        .Returns(true).Verifiable();
    exampleMock.Setup(m => m.IsStored(It.Is<string>(s => obj.Value != s))) // Null Reference Exception
        .Returns(false).Verifiable();

    // Act
    obj = new StubDataStore { Value = "a" };
    var resHit = exampleMock.Object.IsStored("a");
    var resMiss = exampleMock.Object.IsStored("b");

    // Assert
    exampleMock.Verify();
    Assert.IsTrue(resHit);
    Assert.IsFalse(resMiss);
}

Actual behavior

The call to the second Setup throws:

Message: 
    Test method Tests.MultiSetupNRE threw exception: 
    System.NullReferenceException: Object reference not set to an instance of an object.
  Stack Trace: 
    lambda_method(Closure )
    ExpressionExtensions.IsMatch(Expression expression, Match& match)
    ExpressionExtensions.PartialMatcherAwareEval_ShouldEvaluate(Expression expression)
    Nominator.Visit(Expression expression)
    ExpressionVisitor.VisitBinary(BinaryExpression node)
    BinaryExpression.Accept(ExpressionVisitor visitor)
    ExpressionVisitor.Visit(Expression node)
    Nominator.Visit(Expression expression)
    ExpressionVisitor.VisitLambda[T](Expression`1 node)
    Expression`1.Accept(ExpressionVisitor visitor)
    ExpressionVisitor.Visit(Expression node)
    Nominator.Visit(Expression expression)
    ExpressionVisitor.VisitUnary(UnaryExpression node)
    UnaryExpression.Accept(ExpressionVisitor visitor)
    ExpressionVisitor.Visit(Expression node)
    Nominator.Visit(Expression expression)
    ExpressionVisitorUtils.VisitArguments(ExpressionVisitor visitor, IArgumentProvider nodes)
    ExpressionVisitor.VisitMethodCall(MethodCallExpression node)
    MethodCallExpression.Accept(ExpressionVisitor visitor)
    ExpressionVisitor.Visit(Expression node)
    Nominator.Visit(Expression expression)
    Nominator.Nominate(Expression expression)
    Evaluator.PartialEval(Expression expression, Func`2 fnCanBeEvaluated)
    ExpressionExtensions.PartialMatcherAwareEval(Expression expression)
    InvocationShape.PartiallyEvaluateArguments(IReadOnlyList`1 arguments)
    InvocationShape.Equals(InvocationShape other)
    HashSet`1.AddIfNotPresent(T value)
    SetupCollection.MarkOverriddenSetups()
    SetupCollection.Add(Setup setup)
    <>c__DisplayClass63_0.<Setup>b__0(Mock targetMock, Expression originalExpression, InvocationShape part)
    Mock.SetupRecursive[TSetup](Mock mock, LambdaExpression originalExpression, Stack`1 parts, Func`4 setupLast)
    Mock.SetupRecursive[TSetup](Mock mock, LambdaExpression expression, Func`4 setupLast)
    Mock.Setup(Mock mock, LambdaExpression expression, Condition condition)
    Mock`1.Setup[TResult](Expression`1 expression)
    Tests.MultiSetupNRE() line 36

Expected behavior

I would see two options:

1. (Preferred) Expression trees don't fail on bad references at Setup
2. Expression trees always fail on bad references at Setup, but more gracefully

Moq version used

4.14.3 (works in 4.12.0)

[stakx]

Thanks for reporting this @IanYates83. Sounds like a regression, that second Setup call shouldn't throw. A fix for this should definitely go in your preferred direction (i.e. restore previous behavior).

I'll take a look soon.

[stakx]

Preliminary analysis: it seems this change in behavior was unintentionally introduced in d8f877e. Marking setups as overridden/shadowed entails comparing them to newer setups (which is why this repro required at least two setups), and comparing setups involves simplifying (partially evaluating) their expressions, which requires figuring out which parts may be evaluated and which ones need to be preserved as is—e.g. calls to matchers. To recognize matchers we (unfortunately) need to sometimes compile and run expressions, which is what calls the null reference expression here.

I'm not quite certain yet how to fix this cleanly, but a bug fix should become available soon.

[stakx]

@IanYates83, a new bug fix version 4.14.4 should shortly become available on NuGet. It isn't the most thorough bug fix possible—the ideal solution would introduce a breaking change for everyone defining custom matchers, which I'm trying to avoid for now—so you might run into this or similar problems again; if so, please report them, too.