Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
1 of 2 tasks
IanKemp opened this issue Mar 23, 2022 · 3 comments · Fixed by #1251
Assignees
Labels
Milestone

Comments

@IanKemp
Copy link

IanKemp commented Mar 23, 2022

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 IanKemp changed the title Moq is unable to invoke indirectly mocked methods that use in parameters Moq is unable to invoke indirectly mocked methods that use object in parameters Mar 24, 2022
@IanKemp
Copy link
Author

IanKemp commented Mar 24, 2022

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
Copy link
Contributor

stakx commented Mar 24, 2022

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

@stakx
Copy link
Contributor

stakx commented Apr 2, 2022

@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.)

@stakx stakx added the bug label Apr 2, 2022
@stakx stakx added this to the 4.17.3 milestone Apr 2, 2022
@stakx stakx self-assigned this Apr 17, 2022
@stakx stakx changed the title Moq is unable to invoke indirectly mocked methods that use object in parameters Failure when invoking a method with by-ref parameter & mockable return type on a mock with CallBase and DefaultValue.Mock configured Apr 17, 2022
@stakx stakx modified the milestones: 4.17.3, 4.17.2, 4.18.0 May 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants