Failure when invoking a method with by-ref parameter & mockable return type on a mock with CallBase and DefaultValue.Mock configured

[IanKemp]

NOTE: this is not a dupe of #1148 - my problem code is not using generics.

Apologies in advance if this is unclear - the tests in the repro solution I've attached should be more eloquent than my writing.

The repro uses AutoMoq because I haven't been able to trigger this issue in Moq directly, but the stack trace generated points directly to Moq as the culprit.

Describe the Bug

Given an object <OuterService>
That has a constructor dependency on an interface <InnerDependency>
And <InnerDependency> defines a method that accepts a parameter decorated with the in modifier
And <OuterService> defines a method that calls the previously-mentioned method on <InnerDependency>

When attempting to AutoMoq <OuterService> using the following:

var sut = new Fixture().Customize(new AutoMoqCustomization()).Create<<OuterService>>();

Moq 4.17.2 fails:

  Message: 
System.ArgumentException : Type must not be ByRef (Parameter 'type')

  Stack Trace: 
System.Dynamic.Utils.TypeUtils.ValidateType(Type type, String paramName, Boolean allowByRef, Boolean allowPointer)
System.Linq.Expressions.Expression.Constant(Object value, Type type)
Moq.MethodExpectation.CreateFrom(Invocation invocation)
Moq.Behaviors.ReturnBaseOrDefaultValue.Execute(Invocation invocation)
Moq.Return.Handle(Invocation invocation, Mock mock)
Moq.Mock.Moq.IInterceptor.Intercept(Invocation invocation)
Moq.CastleProxyFactory.Interceptor.Intercept(IInvocation underlying)
Castle.DynamicProxy.AbstractInvocation.Proceed()
<IInnerDependency>Proxy.DoSomething(Poco& poco)
<OuterService>.DoSomething(String input) line 16
<TestMethod>() line 26

but only if the in parameter on <IInnerDependency> is a non-primitive object type - if you use string, there is no exception (i.e. it works as expected)!

Based on the above stack trace it looks like this line is the problem. I think it needs to be changed to something like:

while (parameterTypes[i].HasElementType)
{
    parameterTypes[i] = parameterTypes[i].GetElementType();
}

arguments[i] = E.Constant(invocation.Arguments[i], parameterTypes[i]);

I haven't pulled this repo's source and tried the above, but a quick console app I whipped up (included in the repro) uses this approach and seems to work.

Scenario

Attached. There are 2 tests:

• TestPoco: using a POCO as the in parameter (fails)
• TestString: using a string as the in parameter (passes)

Expected Behavior

The first scenario (dependency with POCO as in parameter) works.

[IanKemp]

And yes, I'm aware that using in on an object parameter is only useful to force the compiler to disallow reassignment of said parameter in said method. But that's precisely what I'm using it for: enforcing correctness.

[stakx]

Thanks for reporting, @IanKemp. I'll look into it as soon as I find a spare moment.

[stakx]

@IanKemp, based on your repro code, I've managed to derive a failing unit test that uses Moq exclusively:

public class Mockable { }

public interface IFoo
{
    Mockable DoSomething(in int arg);
}

[Fact]
public void Test()
{
    var mock = new Mock<IFoo>() { CallBase = true, DefaultValue = DefaultValue.Mock };
    _ = mock.Object.DoSomething(1);
}

The problem appears to surface if all of the following conditions hold:

1. The mock is configured with CallBase = true.
2. The mock is configured with DefaultValue = DefaultValue.Mock.
3. The invoked method's return type can be mocked.
4. The invoked method has an in parameter of any type.

(1) and (2) are necessary to get to this line of code. (3) is necessary to get past the ifs at that location in order to arrive at this line of code. And (4) is necessary to trigger the type mismatch there that you've already correctly identified.

(I used a few more test cases to track down this issue, click here to see them.)

		public class Issue1249
		{
			public sealed class Sealed { }

			public class NonSealed { }

			public interface IUseSealed
			{
				Sealed DoSomething(int arg);
			}

			public interface IUseInSealed
			{
				Sealed DoSomething(in int arg);
			}

			public interface IUseNonSealed
			{
				NonSealed DoSomething(int arg);
			}

			public interface IUseInNonSealed
			{
				NonSealed DoSomething(in int arg);
			}

			[Fact]
			public void TestSealed()
			{
				var mock = new Mock<IUseSealed>() { CallBase = true, DefaultValue = DefaultValue.Mock };
				_ = mock.Object.DoSomething(1);
			}

			[Fact]
			public void TestInSealed()
			{
				var mock = new Mock<IUseInSealed>() { CallBase = true, DefaultValue = DefaultValue.Mock };
				_ = mock.Object.DoSomething(1);
			}

			[Fact]
			public void TestNonSealed()
			{
				var mock = new Mock<IUseNonSealed>() { CallBase = true, DefaultValue = DefaultValue.Mock };
				_ = mock.Object.DoSomething(1);
			}

			[Fact]
			public void TestInNonSealed()
			{
				var mock = new Mock<IUseInNonSealed>() { CallBase = true, DefaultValue = DefaultValue.Mock };
				_ = mock.Object.DoSomething(1);
			}
		}

Your suggested fix is almost correct. I suspect that the only thing still wrong with it is that you're checking for .HasElementType, which will also yield true for array and pointer types. We don't need to care about pointer types much (since Moq has never claimed to support unsafe code), but we shouldn't turn array types E[], E[][], etc. into E. In order to only target by-ref types in/ref/out E, the check should be .IsByRef instead. Also, no while loop is needed to unpack the non-by-ref type, since the usual .NET languages don't produce nor support types such as ref ref E.

I'd therefore suggest the following change here:

https://github.com/moq/moq4/blob/b50a8a1003fa1cb4aabf4d359c2f1045e4866106/src/Moq/MethodExpectation.cs#L38-L41

 for (int i = 0; i < n; ++i)
 {
+    var parameterType = parameterTypes[i];
+    if (parameterType.IsByRef)
+    {
+        parameterType = parameterType.GetElementType();
+    }
-    arguments[i] = E.Constant(invocation.Arguments[i], parameterTypes[i]);
+    arguments[i] = E.Constant(invocation.Arguments[i], parameterType);
 }

Are you interested in submitting a PR for that? (If so, please include the unit test(s) and an entry in the changelog.)