Moq does not mock 'Explicit Interface Implementation' and 'protected virtual' correctly

[oddbear]

If we use Explicit Interface Implementation, with same name as a protected virtual method, Moq will mock the wrong method.
Protected methods should not be possible to mock, due to their protection level.

We found this issue in one of our Custom HttpHandlers that inherits from HttpClientHandler.
The method we implement is a overridden protected virtual method SendAsync. To be able to mock this, we used a Interface method as a layer in between for calling base (do an actual httprequest instead for just calling the mock).

Steps to Reproduce

Code example of code with this behaviour:

void Main()
{
    var mock = new Moq.Mock<Contract>() { CallBase = true };
    mock.As<IContractOveride>().Setup(m => m.DoWork()).Returns(3);
    Console.WriteLine(mock.Object.PublicWork()); //Writes out "3". Should be "1". Wrong method is mocked.
}

public class Contract : IContractOveride
{
    public int PublicWork()
    {
        return this.DoWork();
    }
    
    protected virtual int DoWork()
    {
        //Should not be able to mock this method: '...' is inaccessible due to its protection level
        ((IContractOveride)this).DoWork(); //Does not return the value of interface method.
        return 1;
    }

    int IContractOveride.DoWork()
    {
        Console.WriteLine("IFace");
        return 2;
    }
}

public interface IContractOveride
{
    int DoWork();
}

Working code:

void Main()
{
    var mock = new Moq.Mock<Contract>() { CallBase = true };
    mock.As<IContractOveride>().Setup(m => m.DoWorkOverride()).Returns(3);
    Console.WriteLine(mock.Object.PublicWork()); //Writes out "1" as expected
}

public class Contract : IContractOveride
{
    public int PublicWork()
    {
        return this.DoWork();
    }
    
    protected virtual int DoWork()
    {
        //Should not be able to mock this method: '...' is inaccessible due to its protection level
        ((IContractOveride)this).DoWorkOverride(); //Does not return the value of interface method.
        return 1;
    }

    int IContractOveride.DoWorkOverride()
    {
        Console.WriteLine("IFace");
        return 2;
    }
}

public interface IContractOveride
{
    int DoWorkOverride(); //This name is the only change.
}

Expected Behavior

It should write out "1" as i am overriding the interfaced method and not the virtual one.
The virtual method should not be mocked in any circumstances, as this one is protected.

Actual Behavior

It writes out "3" since it mocks the protected virtual method, and not the interfaced method.

[stakx]

Hi @oddbear, thanks for reporting this. It's not the first time I've seen an issue about Moq picking the wrong method when .As<TInterface>() comes into play. Quite frankly, this is an area where Moq has grown rather... "organically". In other words, Moq's logic for identifying which method to mock is less than ideal. I'm not sure we can do anything about this without running the risk of breaking some other user's preexisting code, but I'll take a look and report my findings here later on.

Just as an aside:

Protected methods should not be possible to mock, due to their protection level.

You can mock protected methods with Moq, using mock.Protected().…. This works because the mock objects generated by Moq (actually, by DynamicProxy) are of a class type that's derived from the type-to-be-mocked, which has access to protected members.

I'll post back later about the actual issue here.

[oddbear]

Great thanks.
And totally forgot about '.Protected()'.

[stakx]

@oddbear, this is a fixable bug in MethodCall.IsEqualMethodOrOverride.

https://github.com/moq/moq4/blob/b8b61a4943f4ad297520f3bc93f38b7c3821055a/src/Moq/MethodCall.cs#L388-L416

In the above code, method refers to the method having been set up, while invocationMethod refers to the method being invoked. For your first example:

• method := IContractOveride.DoWork
• invocationMethod := Contract.DoWork

As you can see in that code, Moq performs a rather imprecise comparison to match the invoked method against a setup (but it's enough in all but a few corner cases such as yours). We'd need to add an additional check to prevent a match if the invoked method is not in fact the one that implements the set-up interface method.

The method shown above is one that gets called very frequently, so it's important to consider performance for any bug fix. I have a naïve bug fix that's probably way too expensive; I'll need to think some more about how to make the additional check cheaper.

[stakx]

The PR I've submitted contains the "naïve" bug fix mentioned above. While it's functional, it's also inefficent. A more efficient bug fix will become possible once we've dealt with #643, as that will enable us to map interface methods to class methods at setup time (vs. only being able to do this accurately at invocation time, as it is today), meaning that this mapping can happen only a single time per setup, instead of possibly for every invocation.

That's the reason why I've rescheduled this to the 4.10.0 milestone (which we likely won't reach before late autumn). @oddbear, I hope that's fine with you.

[oddbear]

That's great. Ty.

[stakx]

Note to self, now that #765 is merged, we might be able to proceed with this issue.