GetHashCode() issues when called from inside constructor of mocked class

[AP-Maptek]

I was upgrading Moq on some test fixtures and found an odd suddenly failing test which I have created a minimal reproducible scenario below.

The following works in Moq 4.10.0 and earlier, but is broken in Moq 4.11.0 and newer including latest at time of writing 4.16.1.

using Microsoft.VisualStudio.TestTools.UnitTesting;
using Moq;
using System.Collections.Generic;
using System.ComponentModel;

namespace MoqTest
{
  // Example class being mocked.
  public class Example : INotifyPropertyChanged
  {
    public event PropertyChangedEventHandler PropertyChanged;

    // Property being mocked (pretend it is worth mocking).
    public virtual string Title { get; set; }

    public Example(int foo)
    {
      // Inserting into a dictionary when constructor not finished is bad.
      var dictionary = new Dictionary<INotifyPropertyChanged, object>();
      dictionary.Add(this, "foo");
    }
  }

  [TestClass]
  public class UnitTest1
  {
    [TestMethod]
    public void TestMethod1()
    {
      var example = new Mock<Example>(42) { CallBase = true };
      example.SetupGet(x => x.Title).Returns("A");

      // If this is commented/deleted then issue in constructor mysteriously goes away.
      example.SetupSet(x => x.Title = It.IsAny<string>());

      // Inserting into a dictionary when constructor has finished is fine regardless of above.
      var dictionary = new Dictionary<INotifyPropertyChanged, object>();
      dictionary.Add(example.Object, "foo");
    }
  }
}

The error message given is:

Message: Test method MoqTest.UnitTest1.TestMethod1 threw exception:
System.Reflection.TargetInvocationException: Exception has been thrown by the target of an invocation. ---> System.NotSupportedException: The return type of the last member shown above is not mockable.

With stack trace:

Result StackTrace:	
at Moq.ActionObserver.Recorder.Intercept(Invocation invocation)
   at Moq.CastleProxyFactory.Interceptor.Intercept(IInvocation invocation)
   at Castle.DynamicProxy.AbstractInvocation.Proceed()
   at Castle.Proxies.ExampleProxy.GetHashCode()
   at System.Collections.Generic.ObjectEqualityComparer`1.GetHashCode(T obj)
   at System.Collections.Generic.Dictionary`2.Insert(TKey key, TValue value, Boolean add)
   at System.Collections.Generic.Dictionary`2.Add(TKey key, TValue value)
   at MoqTest.Example..ctor(Int32 foo) in C:\Users\alexp\source\repos\MoqTest2\MoqTest2\UnitTest1.cs:line 20
   at Castle.Proxies.ExampleProxy..ctor(IInterceptor[] , Int32 foo)
--- End of inner exception stack trace ---
    at System.RuntimeMethodHandle.InvokeMethod(Object target, Object[] arguments, Signature sig, Boolean constructor)
   at System.Reflection.RuntimeConstructorInfo.Invoke(BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
   at System.RuntimeType.CreateInstanceImpl(BindingFlags bindingAttr, Binder binder, Object[] args, CultureInfo culture, Object[] activationAttributes, StackCrawlMark& stackMark)
   at System.Activator.CreateInstance(Type type, BindingFlags bindingAttr, Binder binder, Object[] args, CultureInfo culture, Object[] activationAttributes)
   at System.Activator.CreateInstance(Type type, Object[] args)
   at Castle.DynamicProxy.ProxyGenerator.CreateClassProxyInstance(Type proxyType, List`1 proxyArguments, Type classToProxy, Object[] constructorArguments)
   at Castle.DynamicProxy.ProxyGenerator.CreateClassProxy(Type classToProxy, Type[] additionalInterfacesToProxy, ProxyGenerationOptions options, Object[] constructorArguments, IInterceptor[] interceptors)
   at Moq.CastleProxyFactory.CreateProxy(Type mockType, IInterceptor interceptor, Type[] interfaces, Object[] arguments)
   at Moq.ActionObserver.CreateProxy(Type type, Object[] ctorArgs, MatcherObserver matcherObserver, Recorder& recorder)
   at Moq.ActionObserver.ReconstructExpression[T](Action`1 action, Object[] ctorArgs)
   at Moq.Mock`1.SetupSet(Action`1 setterExpression)
   at MoqTest.UnitTest1.TestMethod1() in C:\Users\alexp\source\repos\MoqTest2\MoqTest2\UnitTest1.cs:line 34
Result Message:	
Test method MoqTest.UnitTest1.TestMethod1 threw exception: 
System.Reflection.TargetInvocationException: Exception has been thrown by the target of an invocation. ---> System.NotSupportedException: The return type of the last member shown above is not mockable.

Any thoughts on why this has changed? Is there a work around?

I suspect other System.Object methods could have issues as well.

[stakx]

Thanks for reporting. I'll look into it soon.

Any thoughts on why this has changed?

I suspect this is related to the way how Moq reconstructs a LINQ expression tree (Expression<Action<TMock>> ) from the delegate (Action<TMock>) passed to SetupSet. In earlier versions, Moq ran the delegate directly on the actual mock object. It would then record all calls happening on it as a trace used as the raw material from which to guess the expression tree. This reconstruction process could cause unwanted mutations on the mock object, which is why we switched to running the delegate on a separate, throw-away recording proxy instead.

I'm assuming that the newer reconstrucrion method (the one using a separate proxy) sees the recorded call to GetHashCode caused by dictionary.Add and can't determine how it should fit into the expression tree.

More analysis is needed but the problem will likely lie in that direction.

Is there a work around?

P.S. regardless of whether this can be fixed or not, your code is operating in the intersection of three areas that Moq 4 doesn't (sometimes cannot) support perfectly:

• mocking classes (can be problematic because not all members are overridable)
• constructors (can be problematic if they contain calls to virtual/overridable members)
• setup expressions containing an assignment (problematic because LINQ expression trees don't support assignment operators, thus the need to accept a delegate instead and try to reconstruct the expression tree from that)

I could go into each of these points further, let it suffice for the moment to say that if you can move your test code away from any of these points (mock interfaces instead; or don't trigger virtual calls from the ctor; or don't setup assignments if not needed) you'll generally be heading into better-supported territory.

[AP-Maptek]

Thanks for your quick response.

We have worked around the issue for now by removing the mock on the get/set of this property so we can upgrade to 4.16.1.

This did mean we had to replace VerifySet to check the value as well as how many times the setter was called with an Assert.AreEqual.

If this is solvable in the Moq library it would be good to revert this change in the future.

[stakx]

@AP-Maptek, I've taken a closer look, and like I suspected, the problem lies in the LINQ expression tree reconstruction machinery. Your particular issue could be fixed (details on that follow further down), but in the meantime, if you don't mind using an [Obsolete] API (but one that's still functioning and unlikely to go away in future Moq 4.x versions), you could also sidestep that machinery as follows:

-example.SetupSet(x => x.Title = It.IsAny<string>());
+example.SetupSet(x => x.Title);

This will work because that second SetupSet overload takes a LINQ expression tree to begin with (and therefore won't have to reconstruct it from a delegate), and internally adds an ... = It.IsAny<>() to it (see its implementation here). So it is actually equivalent to your current SetupSet call, minus the malfunction you've run into!

You can stop reading here, unless you're interested in some technical details about Moq internals.

---

The problem indeed lies in ActionObserver (which is the part of Moq that, given a delegate representing a setup expression, reconstructs a corresponding LINQ expression tree). ActionObserver makes the working assumption that all observed virtual method calls can be chained together to form a setup expression. The call to GetHashCode triggered inside the ctor doesn't fit in with this assumption; it has a non-mockable return type (int), so Moq concludes that there is no way any further virtual (interceptable) calls could be chained onto it, and that the reconstruction process cannot succeed.

https://github.com/moq/moq4/blob/a6fde8b6d79a7437bf642d115785b97f40779b6a/src/Moq/ActionObserver.cs#L290-L306

Your code could be got to work by special-casing object methods such that we'd produce dummy return values for them while otherwise ignoring the calls:

 else if (returnType.IsMockable())
 {
     this.returnValue = CreateProxy(returnType, null, this.matcherObserver, out _);
 }
+else if (invocation.Method.DeclaringType == typeof(object))
+{
+    this.returnValue = returnType.GetDefaultValue();
+}
 else
 {
     throw new NotSupportedException(Resources.LastMemberHasNonInterceptableReturnType);
 }

It's been a while since I implemented ActionObserver, so I'm not sure whether this might cause any collateral damage. My gut feeling is that it's probably not worth doing: ActionObserver's mode of operation is far from perfect to begin with: basically it just makes an educated guess about the LINQ expression tree, but there will probably always be edge cases where it guesses wrong (i.e. that it cannot support properly). It might be better to be honest about that instead of putting on bandaids for every such edge case we come across.

This maybe needs some more thought.

[AP-Maptek]

Ah yes you are right, using that obsolete SetupSet however in the original test I generated the simplified example from it then hits the same issue in a VerifySet which looks like there is no equivalent work around.

We have ended up changing the test to call the get/set directly instead of mocking and just checking the expected value rather than also the number of times the setter is expected to be called so we can upgrade to the latest released Moq version.

[stakx]

looks like there is no equivalent work around

(Update: The following probably won't work because there might not be any Verify overload accepting a Expression<Action<TMock>>.)

Well, starting from a setup/verification expression mock => X (a LINQ expression tree), you could derive a LINQ expression tree mock => X = It.IsAny<TX>() manually (it's just the compiler refusing to transform asssignment for us automatically, but it's supported just fine by the System.Linq.Expressions.Expression static factory methods; reuse the parameter, reuse the body node as the LHS of a new assignment node, which becomes the new body node, and put the correct generic instantiation of It.IsAny<> on the RHS; example) and hand that to plain Setup or Verify... but I understand that's not something that one wants to have to do. 😃

Given that you've got a workaround, I'm opting to close this issue. Like I wrote above, the current tree reconstruction logic will probably always have limitations like the one reported here, and it's perhaps more honest to just acknowledge the overall limitations than fixing certain special cases.