Setups for properties with multiple arguments override each other even with differing values

[MatKubicki]

This seems to relate to #252.

Consider the following test (VB specific due to the ability to have named properties with arguments):

    <Fact()>
    Public Sub SetupsForPropertiesWithMultipleArgsDoNotOverwriteEachOther()
        Dim mock As New Mock(Of ISimpleInterface)()

        mock.Setup(Function(m) m.PropertyWithMultipleArgs(1, 1)).Returns(1)
        mock.Setup(Function(m) m.PropertyWithMultipleArgs(1, 2)).Returns(2)

        Assert.Equal(1, mock.Object.PropertyWithMultipleArgs(1, 1))
        Assert.Equal(2, mock.Object.PropertyWithMultipleArgs(1, 2))

    End Sub

   Public Interface ISimpleInterface

        ReadOnly Property PropertyWithMultipleArgs(setting As Integer, setting2 As Integer) As Integer

    End Interface

This should pass, but instead it fails. The problem is that the second setup is overwriting the first, even though the arguments are different 1, 2 instead of 1, 1.

The reasons are many and wonderful it seems!
The crux is all in Interceptor.AddCall
-The keyText created for this setup does not include the arguments, this is because the code sees it as a property getter, rather than a property with arguments.
-This means that the KeyText for both setups is the same, we have to rely on the arguments being different
-The ExpressionKey GetHashCode function (updated in #252) returns the same hash for both 1, 1 and 1, 2.
-Then The ExpressionKey equals method as a bug where it comparesthe arguments, it combines all the checks with an OR rather than an AND.

As this can only be tested in VB i've created a VB test and added it to the solution.
I've quickly fixed the OR that should be an AND, this fixes the problem.
But the GetHashCode implmentation needs a rethink as well I would suggest.

I'll push my simple changes and new test ASAP.

[MatKubicki]

Ok so i've added my commit, not making a PR yet as I'd like to see the GetHashCode fixed as well. Any thoughts from anyone on how to do this?

[kzu]

IIRC, it's doing a logical OR, not a boolean OR to build the hash, so that should be the correct way

[MatKubicki]

It does indeed use an XOR during the building of the hash, but the change i've made is in the Equals implmentation, that should be using an AND but was using an OR.

There appears to be a problem with the GetHashCode implementation as well, but i've haven't had a good look at that.

[cyanite]

This "fix" causes a serious regression in how multiple setups with the same values are handled. Consider

someMock.Setup(m => m.Foo(1, 2)).Verifiable();
...
someMock.Setup(m => m.Foo(1, 2)).Verifiable();

Since 1 is not equal to 1 by reference (since both are boxed in separate boxes) and similar with 2, these two setups are now not considered the same. The result is a setup that can never be successfully verified. I'll create an issue for this.

I must say I am a bit surprised that a pull request which changes an Equals without changing the GetHashCode is taken in, since it's well known that this code uses dictionaries. I'm also surprised there is no test coverage of this.

[MatKubicki]

I have to agree with cyanite, despite it being my change!

I originally didn't create a pull request as it seemed like the real flaw was in the GetHashCode and that my change was just a sticking plaster, the issue unfortunately got no interest until I created the PR a month or so later.

I too would have assumed an existing test would have covered the bug you spotted. At the time the issue with the values being boxed hadn't occoured to me.