Detect method signature mismatch in Setup / Returns

[ohadschn]

Consider the following code:

public interface IFoo
{
    string Bar(string str, int i);
}

var mock = new Mock<IFoo>();

mock
  .Setup(x => x.Bar(It.IsAny<string>(), It.IsAny<int>()))
  .Returns((string s) => s.ToLower());

Console.WriteLine(mock.Object.Bar("HELLO", 42));

On runtime, the Bar() call will fail with System.Reflection.TargetParameterCountException: Parameter count mismatch. It would be better if it could fail faster, ideally as early as the Returns call (at that point it can already check that the signatures don't match). A method like Mock<T>.ValidateSetup() could work too, though you'd have to remember to call it so not as good IMHO.

The same goes for SetupSequence and possibly others (I'm not well versed in the Moq API).

[stakx]

Hi @ohadschn, thanks for reporting this. I think we should go even further than checking parameter count only; we should also verify parameter and return type compatibility, where applicable. (This is already done for .Callback, but not for e.g. .Returns nor .Raises.)

Do you agree?

[ohadschn]

@stakx Oh, definitely. That's what I meant when I said "at that point it can already check that the signatures don't match". My bad with the issue's title, I fixed it.

[stakx]

I've implemented the necessary changes (easy enough), but was surprised to learn that several unit tests fail now. Apparently, for .Returns, it should be permissible to pass in a callback whose parameters don't match the setup method with respect to their ref-ness. This is unlike present-day .Callback, which insists that ref-ness of parameters must match.

So we have an inconsistency in Moq's API that needs to be dealt with, and this means that we should include #105 while we're at it.

TL;DR: Fixing this issue is going to take a little longer than expected, because we need to take some other things into consideration. Might have to remove this from the v4.7.next milestone.

[stakx]

@ohadschn - It's taken me two attempts to arrive at a solution that I think strikes a sensible balance between being doing what's technically correct (validate as much as possible) and being forgiving about certain grey-area real-world use cases.

My first attempt (#519) would've introduced strong validation of all delegates passed to Returns, at the cost of possibly breaking user code. That's why it included a back-compatibility / feature switch that you'd have to explicitly enable to have the validation kick in. That PR felt wrong from the moment I submitted it: Some validation checks establish very basic correctness, and there shouldn't need to be an opt-in feature switch for those.

The second attempt (#520) therefore introduces validation that is weaker overall, but with the benefit of better compatibility with existing code and no feature switch. In particular, parameter types do not get validated at all. This could be slightly improved upon in the future, but I feel it probably isn't worth the trouble at present. (See additional notes in the PR.)