"Different number of parameters" error when passing to Returns a dynamically compiled Expression

[MaMazav]

When trying to mock the following interface:

public interface IMyInterface
{
    void MyAction(int x);

    int MyFunc(int x);
}

By the following code:

        Mock<IMyInterface> mock = new Moq.Mock<IMyInterface>();
        var setupAction = mock.Setup(my => my.MyAction(It.IsAny<int>()));
        var setupFunc = mock.Setup(my => my.MyFunc(It.IsAny<int>()));

        int a = 5;
        Expression<Action<int>> callback = x => Console.WriteLine(a);
        Action<int> callbackCompiled = callback.Compile();

        int b = 4;
        Expression<Func<int, int>> funcResult = x => b;
        Func<int, int> funcResultCompiled = funcResult.Compile();

        setupAction.Callback(callbackCompiled); // OK
        setupFunc.Callback(callbackCompiled); // OK
        setupFunc.Returns(funcResultCompiled); // Fail

While setting callbacks using Callback method succeeds for method compiled on runtime, trying to mock using Returns method fail with "System.ArgumentException: 'Invalid callback. Setup on method with 1 parameter(s) cannot invoke callback with different number of parameters (2).'".

Preliminary investigation:
All compiled method has a "redundant" first argument of type System.Runtime.CompilerServices.Closure. In general, overcoming this redundant parameter is by calling to DynamicInvoke instead of simple Invoke, thus if I understand correctly it should not be a problem in a Moq (some answers on the web talk about that e.g. https://stackoverflow.com/questions/7935306/compiling-a-lambda-expression-results-in-delegate-with-closure-argument/7940617#7940617).
So seems that the problem is only the error method. And indeed, seems that the argument checks differ between the Callback and the Returns method internal implementations: While the former (in file src/Moq/MethodCall.cs, method SetCallbackWithArguments) calls extension method HasCompatibleParameterList to check correctness, the latter (in file src/Moq/MethodCallReturn.cs, method ValidateNumberOfCallbackParameters) have a different check of arguments which seems less stable.

(A side note, this problem was seen by another user: https://stackoverflow.com/questions/49641537/incorrect-number-of-arguments-supplied-for-call-to-method-expression-callmetho).

[stakx]

@MaMazav, thanks for reporting this, and for your detailed preliminary analysis. Weird, I have never encountered (or noticed) this additional Closure parameter before! I was initially somewhat baffled by the fact that a Func<int, int> delegate can reference a method with a int(Closure, int) signature until I remembered Jon Skeet's blog post "Making reflection fly and exploring delegates".

What appears to be missing in MethodCallReturn.ValidateNumberOfCallbackParameters is some logic that checks whether a delegate method's first parameter is bound to some target (which, if true, means that the first parameter should be excluded from the total count).

I'd like to look more closely at this. Also, it has always bothered me a bit that there appear to be so many different ways how Moq validates method signatures. But I'm not sure that can be simplified further.

Please let me know if you'd like to work on this and submit a patch. If not, I'll probably get around to fixing this during the next week or so. (Update: Found some spare time today, I hope you don't mind if I forge ahead and get this fixed.)

[idigra]

(Sorry in advance if I'm talking about already discussed issue).

I think that the problem is deeper than the Closure argument and touches the general strategy of argument check in Returns.

I tried to fix the issue by changing the MethodCallReturn path to use the HasCompatibleParameterList method, but then some tests failed. However the failing tests expose the fact that there was a difference between the Callback limitations and the Returns limitation.
For example, see test IssuesReportsFixture._267.HelperSetup. It finally calls the following line (line 3526 in IssuesReportsFixture.cs):

private void SetupOperationStub<T>(Func<T, string> valueFunction)
{
    m_OperationStub.Setup(m => m.Operation(It.IsAny<T>())).Returns<T>(valueFunction);
}

Although this line seems reasonable, because the It.Any causes the valueFunction to get a T argument for sure, it's only in mistake. For example, I tried to add the following line in the caller method (HelperSetupTest, line 3510):

m_OperationStub.Setup(m => m.Operation(It.IsAny<int>())).Returns<string>(value => "hello");

And it was not caught by the argument check (but crashed later when tried to call the method).

Moreover, when tried to replace the above method with Callback instead of Returns, the argument check caught the problem immediately:

private void SetupOperationStub<T>(Action<T> valueFunction)
{
    m_OperationStub.Setup(m => m.Operation(It.IsAny<T>())).Callback<T>(valueFunction);
}

So first we need to answer the question - is this inconsistency between Returns and Callback reasonable? And if so, why?
If the inconsistency is OK - we need to locally fix the Returns to consider also the Closure argument. Also we need to open a bug about my first trial to change the code (using It.IsAny with Returns which was only caught when method called).
If this is not OK, we need to consider backward compatibility and decide if fix Returns argument check (which may break legacy code) or to find some other backward compatible solution (e.g. deprecating Returns overloading which allows any Delegate and add another safe method which correctly check arguments).

[stakx]

the failing tests expose the fact that there was a difference between the Callback limitations and the Returns limitation.

Yes, I noticed that too when we implemented stronger callback checks going from 4.7.x to 4.8.0. I think there are actual use cases for the more lenient handling of Returns, cannot find the issue right now though.

is this inconsistency between Returns and Callback reasonable?

It doesn't matter a lot whether it's reasonable or not. This inconsistency has been in Moq for a long time; changing anything about it will invariably break things for users. This issue and the feedback we got for 4.8.0 (where we made Callback more strict) gave us a little taste of the consequences that such changes bring. Not too keen to repeat this TBH. :)

At this point, I think it would be wise not to do anything at all about it and just live with that inconsistency until Moq 5 is published. Being a new major version, it can afford breaking changes more easily.

[idigra]

OK. As I said there is the option to add another method and mark the old problematic Returns overloading as deprecated, but I see your point.
Anyway let me suggest a slightly another solution: In the argument check used in the Returns method, we can use same check as done in the Callback (using the HasCompatibleParameterList). Only if it fails we can go to the old backward compatible check.
That way we don't push any breaking change, but still catching the case in this issue in more correct and general way (we may catch also other issues which we don't currently consider, similar to that Closure one which was not considered at first).

EDIT: You can see the suggested solution here: idigra@dac25d6
(I based on your branch).

[stakx]

@idigra:

For example, I tried to add the following line in the caller method (HelperSetupTest, line 3510):

m_OperationStub.Setup(m => m.Operation(It.IsAny<int>())).Returns<string>(value => "hello");

And it was not caught by the argument check (but crashed later when tried to call the method). [...] Also we need to open a bug about my first trial to change the code (using It.IsAny with Returns which was only caught when method called).

I don't know whether this is even possible. If you wanted to catch this error at compile-time, you would need a Returns(TDelegate callback) overload where TDelegate is a delegate with a signature matching that of the method being set up in Setup. The problem is two-fold:

1. How can the compiler infer TDelegate from the Setup call? If you look at the Setup method group, there's basically (I'm simplifying a little but never mind) two overloads: one for non-void methods (Setup(Expression<Func<TMock, TResult>>)) and one for void methods (Setup(Expression<Action<TMock>>)). How do I derive the exact method signature from that at compile-time? If you come up with a workable solution, I'd love to hear about it.

2. How do you transport the static type information (TDelegate) from the Setup call to the Returns call? Assuming that there were a solution for (1), you'd now have to get the TDelegate parameter gathered at Setup to Returns (which is a separate method call). If you look at Moq's fluent API, you'll see that this would probably have to happen in the form of new interface types.

I suspect that (1) is an unsolvable problem given C#'s type system. And possibly as a result of that, we have Callback and Returns which are comparatively type-unsafe.

In the argument check used in the Returns method, we can use same check as done in the Callback (using the HasCompatibleParameterList). Only if it fails we can go to the old backward compatible check.

Moq knows too many ways of comparing method signatures already, so this sounds interesting. I've tried to merge these different strategies in the past, and have failed. There are some small differences between them and IIRC, there's a reason for that.

That being said, feel free to work on this and submit a PR. Just bear in mind two things: (1) No new API methods (differently-named variants on existing methods). The API should stay straight-forward, and not offer you a bunch of similarly-named methods that do slightly different things. (2) Breaking changes should be avoided.

[stakx]

@idigra: I'll be merging the PR that solves the immediate issue reported here, so this present issue will get closed. Please don't let this irritate you. We can keep on discussing the additional issues you've raised either here, or in a new dedicated issue if you prefer.

[idigra]

Regards your answer about early catch of signature inconsistency - I didn't talk about compile time check. I talked about checking when installing the moq method (on call to Returns) rather than deferring it to a strange exception when the actual method is called, as it happens today in the one liner code example I supplied and you quoted. This early check is possible for sure, as it happens in the Callback method.

Regards my suggestion - you probably missed my late edit from the previous post in which I added the code suggestion. It meets your two requirements: idigra@dac25d6

[stakx]

I talked about checking when installing the moq method (on call to Returns) rather than deferring it to a strange exception when the actual method is called [...]

I see; my bad. This reminds me of #445, #519, and #520. If you decide that you'd like to work on this, please study these first so we won't end up repeating history. :)

[stakx]

Regards my suggestion - you probably missed my late edit from the previous post in which I added the code suggestion. It meets your two requirements: idigra/moq4@dac25d6

@idigra - I'm sorry, I'm getting a little confused. I think we're discussing two things at the same time: (1) Catching errors early. (2) Using HasCompatibleParameterList. Am I right that we're now talking about (2)? To be honest, I don't quite see the point of that edit. It replaces a fairly lightweight check with a pretty expensive one...?

[idigra]

Yes, we are talking about (2). I left the idea of catching errors early when you raised the concern about breaking change and API changes. This change will make the checks more permissive rather than catching problems earlier.
The points of the edit are (a) be more consistent with Callback in Returns (at least permit anything that Callback permits if we can't disallow what Callback doesn't). (b) Permit some more cases which should be allowed but we don't consider now (cases similar to the Closure one).

Until now I didn't consider performance at all. I guessed that the fact that HasCompatibleParameterList used in Callback means that we are OK with its performance. Anyway, we should ask my previous question also about performance: Is there a performance requirements difference between Callback and Returns which allows us using the more stable and less performant HasCompatibleParameterList method only in Callback?

[stakx]

I still don't get it; sorry.

Your change, as you said, makes Returns more permissive because certain checks (which could lead to an early exception) aren't performed under certain conditions. So at first you said you'd like Returns to fail early, yet you suggest a change that goes in the opposite direction.

I totally agree that fail-early would be preferable if the setup is faulty and there's no chance for the actual mocked method calls to succeed.

Because of that I think returning early and skipping important checks in the ValidateReturnDelegate method is the wrong way to go about it.

Performance isn't super crucial in these methods, but if we degrade it (however slightly) only to skip cheap checks that shouldn't be skipped, then that's when I complain.

And finally, more to the point: Why is HasCompatibleParameterList so important? Why is it "more stable"? Callback and Returns simply & observably don't work in the same way, so they don't necessarily have to share implementation bits. They should only use the same validation logic if they have the same validation requirements (which isn't the case unfortunately).

[stakx]

Let me go and take a close look at the validation methods in question. I'm obviously missing something here. I'll be back.

[idigra]

As I said, I gave up about failing early when you wanted to avoid any breaking change.
The reason I think that HasCompatibleParameterList is more stable is that it implicitly caught the Closure case (which is the reason for opening this issue), while the parameter count check failed that. I'm afraid about more cases (permissive ones! not early-fail) that we are not aware of and also covered by HasCompatibleParameterList.

I'm not sure I understand why Callback and Returns doesn't share same validation requirements. E.g. the usecase of the It.Any with int callback (as in the failing test above) is reasonable also for Callback. But I guess your intuitions are more sharp than mine after seeing more Moq usecases than me.

[stakx]

I'm afraid about more cases (permissive ones! not early-fail) that we are not aware of and also covered by HasCompatibleParameterList.

What I don't understand is why you put so much trust in that one method. It may be just as faulty, or faulty ways of its own, as other validation logic. Who says it captures any more corner cases just because it might have caught this one here?

I'm not sure I understand why Callback and Returns doesn't share same validation requirements. [...]

Neither do I. But Moq's been like that for longer than I have been with Moq. :) I suspect this divergence started out as a simple accident / undiscovered bug, and now time has essentially frozen it into a de-facto specification because user code has likely come to rely on this behavior by now. That's probably all there is to it. This should be straightened out with Moq 5.

[idigra]

BTW if you are worried about

What I don't understand is why you put so much trust in that one method. It may be just as faulty, or faulty ways of its own, as other validation logic. Who says it captures any more corner cases just because it might have caught this one here?

Copied from my answer in #655: IMO Checking the standard .NET Invoke method is a more stable way to eliminate such manipulations than implementing a heuristics and extend it on each case we encounter (as was really proved in the Closure bug).

Neither do I. But Moq's been like that for longer than I have been with Moq. :) I suspect this divergence started out as a simple accident / undiscovered bug, and now time has essentially frozen it into a de-facto specification because user code has likely come to rely on this behavior by now. That's probably all there is to it. This should be straightened out with Moq 5.

If no real reason to have divergence except of historical ones and it's going to be merged in Moq 5, I don't see a reason why not doing another step to this direction. It will not catch false positive due to the avoidance of breaking change, but at least it will eliminate false negatives which the Closure bug might be only one example among others.

BTW, regards the performance concern - I can move the permissive check to after the existing logic instead of before. That way performance will not degrade for all existing working scenarios. However if that solution is going to be pushed into Moq 5 (is it? what did you think to be the correct merged strategy?), I think it will be better to have this performance degradation ASAP to get feedback if it's bad (as long as it's not a functional-breaking change, but only performance change).

[stakx]

regards the performance concern - I can move the permissive check to after the existing logic instead of before. That way performance will not degrade for all existing working scenarios.

That would invalidate the whole idea behind your PR, and be rather pointless, too:

If I understand your PR correctly, then its intent is to avoid false negatives triggered by the existing logic (which you suspect to be too strict in cases like the one reported at the beginning of this issue, as well as possibly other cases that we haven't discovered yet).

So if you move your proposed check to after the current logic, then it can no longer prevent false negatives.

Also, at the end of the current logic (i.e. at the end of the method), it's rather pointless to check whether we should return from the method, because that'll happen anyway. You'd only be making the method more costly.

You wrote in your PR:

The HasCompatibleParameterList method returns true only if it can be DynamicInvoked or Invoked [...]

I am still struggling to understand your faith in HasCompatibleParameterList. How do you know that your above statement is true? As far as I am aware, we don't even have a single unit test for that method.

What this boils down to is that you seem to trust HasCompatibleParameterList to do the right thing more than you trust the logic in MethodCallReturn.ValidateReturnDelegate. (Possibly because you ran a couple experiments comparing Callback with Return.) To me, this does not seem like a valid reason for changing the implementation of ValidateReturnDelegate just because it might trigger a false negative in some as-yet-unknown scenario.

At the same time as it doesn't have any known benefits, adding an additional HasCompatibleParameterList check has a definite cost (however small).

Adding that additional check just because it might prevent a problem in a hypothetical, unknown case doesn't feel right to me. If ValidateReturnDelegate is now longer buggy (to the best of our present knowledge), then I see no reason for changing it. If it ain't broken, don't fix it.

Show me a scenario where ValidateReturnsDelegate still triggers a false negative now, then instead of just adding HasCompatibleParameterList as a panacea, let's instead analyze why the current logic fails, write tests for it, and make a change only after all that has happened.

[idigra]

OK. I see your point. I hope that all cases are really covered. I'll abandon my PR.
Just re-

That would invalidate the whole idea behind your PR, and be rather pointless, too:
If I understand your PR correctly, then its intent is to avoid false negatives triggered by the existing logic (which you suspect to be too strict in cases like the one reported at the beginning of this issue, as well as possibly other cases that we haven't discovered yet).
So if you move your proposed check to after the current logic, then it can no longer prevent false negatives.
Also, at the end of the current logic (i.e. at the end of the method), it's rather pointless to check whether we should return from the method, because that'll happen anyway. You'd only be making the method more costly.

By saying "after the current logic" I didn't mean to cut-and-paste the code lines to the end of the method, but to do the new logic after we discovered that we are in one of the two problematic cases. Conceptually, think about that like I move the new code lines to before the two throw statements (not talking now about avoiding code duplication that could be solved e.g. by additional method).

I am still struggling to understand your faith in HasCompatibleParameterList. How do you know that your above statement is true? As far as I am aware, we don't even have a single unit test for that method.
What this boils down to is that you seem to trust HasCompatibleParameterList to do the right thing more than you trust the logic in MethodCallReturn.ValidateReturnDelegate. (Possibly because you ran a couple experiments comparing Callback with Return.) To me, this does not seem like a valid reason for changing the implementation of ValidateReturnDelegate just because it might trigger a false negative in some as-yet-unknown scenario.

My faith is based on the implementation of both ways. I just trust more on comparing the Invoke's argument which the CLR guarantees to be correct than manual way that heuristically tries to cover more and more cases.

[stakx]

I just trust more on comparing the Invoke's argument which the CLR guarantees to be correct than manual way that heuristically tries to cover more and more cases.

I sort of get your drift, but I'm afraid I still don't get it precisely. The CLR "guaranteeing" anything doesn't have anything to do with what happens inside HasCompatibleParameterList. True, it appears to make good sense to compare each parameter pair for assignment compatibility, and it is perhaps closer in spirit to the way ECMA-335 would specify signature matching. But HasCompatibleParameterList isn't derived from the CLI spec; it's based on known use cases and experience, just like the "heuristics" in ValidateReturnsDelegate. It's not better nor more foolproof--it's simply different due to a different origin and history.

Did you know, for example, that HasCompatibleParameterList doesn't compare parameter signatures as thoroughly as e.g. the C# compiler would? For instance, unlike C#, it completely ignores the difference between in and ref. This would be a historical oversight/bug if it weren't for the lucky fact that the CLR also ignores all custom modifiers in signatures in the case of delegate calls. But that wasn't known when the Moq method was written. It's pure coincidence!

[stakx]

@MaMazav, @idigra - I've just pushed Moq 4.10.0 (which includes a bugfix for this) to NuGet. It should be available shortly.

[idigra]

Thanks, I just verified and it works now.