SetupAllProperties no longer overrides pre-existing property setups

[stakx]

After #826, mock.SetupAllProperties() no longer overrides pre-existing property setups, as it would have up until 4.11.0.

Steps to reproduce:

public interface IX
{
    string P { get; set; }
}

var mock = new Mock<IX>();
mock.SetReturnsDefault("default");

mock.SetupAllProperties();
Console.WriteLine(mock.Object.P);

mock.Object.P = "explicit";
Console.WriteLine(mock.Object.P);

mock.SetupAllProperties();
Console.WriteLine(mock.Object.P);

Actual behavior (output):

default
explicit
explicit

Expected behavior (output):

default
explicit
default

Notes:

According to the reasoning I used in #196 (comment):

The [...behavior...] fits well with Moq's "the last setup overrides previous setups" rule of thumb [...].

... this qualifies as a regression / bug, esp. for people who are using it to pave over existing setups.

It would be good to restore the previous behavior, for overall API consistency. However, that might be fairly non-trivial (one possible solution might require that all setups be timestamped).

That being said, fixing this regression is probably not super-important, since SetupAllProperties should typically happen as the first, or one of the first, setups performed on a fresh mock, and calling the method repeatedly is probably not a very common pattern.

/cc @vanashimko

[ishimko]

Ah, and still I broke something! Disappointed but not surprised 😄

How about removing all property accessors setups inside SetupAllProperties call? I've had a brief look at SetupCollection, at the moment it is not allowed to remove from there, but if you ok to allow it, Remove(Func<Setup, bool> predicate) can be added. This method will do lock, remove from list and update this overridden mask (I did not have a detailed look in there, but suspect it is possible to shift bits and fill the gap after setup removal), but only for non-empty setup list. IsPropertyAccessor will be a predicate for removal. Given that common pattern is calling SetupAllProperties on a fresh mock, most of the times it will remain efficient, since Remove will return immediately without any locks because of empty setups.

How do you think?

[stakx]

Ah, and still I broke something!

Don't worry, that's no fault of yours. And no harm has been done. :)

How about removing all property accessors setups inside SetupAllProperties call?

Good idea! I hadn't thought of that, but this should work.

at the moment it is not allowed to remove from [SetupCollection], but if you ok to allow it, [...]

In principle, that would be OK, although I had hoped to keep it strictly add-or-clear-only (as that would in principle allow concurrency optimizations similar to those found in InvocationCollection)... but that's not a big priority. Functional correctness is more important, so if we don't find another solution for the present issue, then going ahead with your suggestion will be fine with me.

[...] update this overridden mask (I did not have a detailed look in there, but suspect it is possible to shift bits and fill the gap after setup removal)

That'd be one way to do it, but perhaps it wouldn't even be necessary. overridden is only a cache to speed up some things, but unless I'm mistaken, it's a completely optional mechanism: it should be possible to simply reset overridden to 0 and it should get rebuilt eventually.

(We could go further and remove those setups known to be overridden, since we'd already be removing setups anyway. That should logically also result in an zero overridden mask.)

There's another, somewhat less elegant solution which I'm mentioning only for completeness' sake: Instead of removing existing property setups, we could keep them but somehow "reset" them to use different backing fields and default value providers. One could go through the list of all setups, and when encountering a setup of type AutoImplementedProperty[GS]etterSetup, update their callback delegates to new ones.

[ishimko]

it should be possible to simply reset overridden to 0 and it should get rebuilt eventually

Good news on overridden! Setting to zero is what I would definitely prefer to do instead of bit manipulations 😄

One could go through the list of all setups, and when encountering a setup of type AutoImplementedProperty[GS]etterSetup, update their callback delegates to new ones.

I'm afraid that looking for AutoImplementedProperty[GS]etterSetup won't be enough if the goal is to keep old behavior.

Earlier you could also do:

var mock = new Mock<IBar>();
mock.Setup(x => x.Value).Returns(1);

mock.SetupAllProperties(); // reset all the setups added previously

Assert.Equal(0, mock.Object.Value);

Even though we can filter against method.IsPropertyAccessor() to get all properties related setups, we'll probably had to remove non AutoImplementedProperty[GS]etterSetup anyway (like in this example), because for them replacing underlying delegate does not make sense and won't lead to desired result (at first glance).

[stakx]

I'm afraid that looking for AutoImplementedProperty[GS]etterSetup won't be enough [...]

... and you're right!

I don't think I can come up with an alternative solution that beats your proposal in terms of simplicity and correctness, so if you're interested to work on this, another PR would be welcome!