Regression in 4.13.0

[BrunoJuchli]

After updating from 4.12.0 one of our tests fails, consistently every time I run it.
I've got a really hard time reproducing it, though, because call sequence, type definitions, inheritance and maybe even thread switches may play a role.

Before reading through all the complicated blabla below jump right to this comment

I've got a class:

internal class ViewModelCache
{
    private readonly List<object> viewModels = new List<object>();

    public void Register(object viewModel)
    {
        this.viewModels.Add(viewModel);
    }

    public virtual IReadOnlyList<TViewModel> Retrieve<TViewModel>()
    {
        return this.viewModels.OfType<TViewModel>().ToList();
    }
}

A test which sets up the mock like:

ViewModelCache = new Mock<ViewModelCache> { DefaultValue = DefaultValue.Mock };

No further setups.
Tthe test performs quite a lot of steps (it's not a class-level unit test), and at one point it fails here:

Inspecting the moq invocations I can see the reason:
Moq has actually created a mock of IReadOnlyList<IHaveCellComponet> instead of IReadOnlyList<StorageViewModel>:

Trying to Reproduce

Calling .Retrieve<StorageViewModel> immediately after the creation of the moq succeeds.

Also, the exception that originally occurred now doesn't happen anymore. All further calls to .Retrieve<StorageViewModel> succeed.

The test still fails, further down, when retrieving an IReadOnlyList of another type, MachineBaseViewModel:

"further down" is proven by there being more invocations than the first time:

When I add another .Retrieve<MachineBaseViewModel>() right after creation of the moq, we get even further.
This means, in the time between creation of the Moq and calling Resolve something is happening which get's Moq to behave erratic. But I don't have a clue to what that is. I can't find any other access to Mock<ViewModelCache>, only to Mock<ViewModelCache>.Object.
Within our solution this reproduces reliably, but I haven't found the key to creating a repro yet.

[BrunoJuchli]

@stakx
I know more now. There's a benefit in writing down what you know ;-)

This call sequence throws:

This does not, however:

Type definition:

However, I still haven't found a simple repro (creating a moq of a list of the interface before the type doesn't trigger the problem).
We will try to come up with a repro.

[stakx]

Thanks @BrunoJuchli for reporting. I'm not sure why you're saying that you haven't found a repro yet. Based on the information you've posted, I've hacked together the following, which produces the exception you've described:

[Fact]
public void Test()
{
    var mock = new Mock<X> { DefaultValue = DefaultValue.Mock };
    mock.Object.Method<I>();
    mock.Object.Method<C>();
}

public interface X
{
    IReadOnlyList<T> Method<T>();
}

public interface I { }

public class C : I { }

System.InvalidCastException:
Unable to cast object of type Castle.Proxies.IReadOnlyList`1Proxy to type System.Collections.Generic.IReadOnlyList`1[Moq.Tests.Regressions.IssueReportsFixture+Issue932+C].

That repro is good enough for me to start digging. Of course, as always, you're more than welcome to help digging, too!

[stakx]

While debugging the above repro, I noticed that the MockDefaultValueProvider only gets invoked once. That was a pretty strong clue for what's going on: Moq is caching the return value of the first mock invocation and reusing it for the second invocation!

TL;DR: This is a bug, and I can probably draft a fix during the next week or so. Read on if you're interested in the technical background.

When your mock uses DefaultValue.Mock, and it is used to create a return value of a mockable type, then Moq will cache that return value. (It does that for later mock.Verify[All]() calls which are recursive. Moq establishes an ownership relationship between mock and any auto-mocked return values; the "owned" sub-mocks are automatically included in verification. That ownership relationship between mocks has recently been discussed and disputed in #892, btw.)

The InvalidCastException appears to happen when Moq caches the auto-mocked Mock<IReadOnlyList<I>> during the first mock method invocation. The second invocation will reuse the cached value, which is obviously an error because I cannot be cast to C.

1. The caching behavior described above was introduced in Moq 4.11.0—or rather, the existing caching behavior was made much, much more consistent and correct than it was before.

2. Things would probably have continued working fine, but in Moq 4.12 I rectified the logic that matches invoked methods against setups—again, a very beneficial change that makes Moq much more consistent and correct.

What seems to have gone wrong in 4.13.0 is that the caching logic (1) wasn't updated to reflect the changes in (2).

The "cache" entry is created here:

https://github.com/moq/moq4/blob/a12fe5b1fc2eb7857799d5a7bb5cc9e18f61aace/src/Moq/Interception/InterceptionAspects.cs#L280-L285

When I say "caching", what really happens behind the scenes is that Moq creates an internal setup for m => m.Method<IReadOnlyList<I>>().

Due to change (2) mentioned above, a call to mock.Object.Method<IReadOnlyList<C>>() will match that setup (due to generic type variance).

I might be able to draft a bugfix during the next week or so. What I'm thinking of right now is that these kinds of internal "cached return value" setups should somehow be made to disregard type variance when matching calls against the set up method, and only match the precise generic arguments as were used when the cache entry was created.

[BrunoJuchli]

@stakx
Thanks a lot. I thought I tried that (though interface X was class X instead) and it didn't fail. That's why I said I didn't have a repro. My bad!

[BrunoJuchli]

@stakx
We've updated this morning. The original issue is fixed and we have not found any new issues introduced by 4.13.1.
Thanks a lot!

[stakx]

@BrunoJuchli, great to hear! Thanks for letting me know.