Parameter is invalid in Protected().SetupSet() method

[jerviscui]

Hello, I found a bug?

protected  virtual string PrivatePropForTest { get; set; }

public void PropValue_MockPrivateProperty_Setter()
{
    //moq protected virtual prop
    var mock = new Mock<PropValue>();

    var saved = "";
    mock.Protected().SetupSet<string>("PrivatePropForTest", "aaa").Callback(s => saved = "");//.Throws<ArgumentException>();
    mock.Protected().SetupSet<string>("PrivatePropForTest", "aa").Callback(s => saved = s);//.Throws<ArgumentException>();
    

    //todo: protected setter is wrong!!!
    mock.Object.SetTest("aa");
    mock.Object.SetTest("aaa");

    saved.ShouldBe("");// saved is "aaa"

    //assert
    mock.Protected().VerifySet<string>("PrivatePropForTest", Times.Exactly(2), ItExpr.IsAny<string>());
    mock.Protected().VerifySet<string>("PrivatePropForTest", Times.Once(), "aa");// throw Moq.MockException
    mock.Protected().VerifySet<string>("PrivatePropForTest", Times.Once(),
        ItExpr.Is<string>(s => s == "aa"));// throw Moq.MockException
}

In Protected().SetupSe() method, parameter "aaa" and "aa" is invalid, because at last the saved value is "aaa".
In Protected().VerifySet() method, ItExpr is invalid, will throw Moq.MockException:

Moq.MockException
Expected invocation on the mock once, but was 2 times: mock => mock.PrivatePropForTest = It.IsAny()
Performed invocations:
MockPropValue:1 (mock):
PropValue.PrivatePropForTest = "aaa"
PropValue.PrivatePropForTest = "aa"

Help me, pls.

[stakx]

Thanks for reporting these two issues, @jerviscui. I'll look into it.

[stakx]

Regarding the first issue:

In Protected().SetupSe() method, parameter "aaa" and "aa" is invalid, because at last the saved value is "aaa".

The cause for this appears to be that the second parameter to SetupSet<TProperty> (value) gets ignored – Moq always uses ItExpr.IsAny<TProperty>() instead:

https://github.com/moq/moq4/blob/a6fde8b6d79a7437bf642d115785b97f40779b6a/src/Moq/Protected/ProtectedMock.cs#L119-L132

Therefore, your second setup essentially shadows the former; it is matched for both calls.

This doesn't make sense to me and looks like a bug: why take a value at all when it doesn't get used?

[stakx]

Regarding the second issue:

In Protected().VerifySet() method, ItExpr is invalid, will throw Moq.MockException

Turns out we have the same problem here, the provided value argument gets ignored, Moq simply uses ItExpr.IsAny<TProperty>() instead.

[stakx]

@jerviscui:

• If you'd like to submit a PR to fix these issues, below you'll find a draft for the code changes that are probably needed. (A PR should also include unit tests and a changelog entry, see CONTRIBUTING.md for details.)

• If you do not want to submit a PR, I'll apply the changes myself in a few days' time. (I'll still credit you in the changelog for finding the bug.)

Suggested code changes (click to expand)

Change ProtectedMock.ToExpressionArg as follows: https://github.com/moq/moq4/blob/a6fde8b6d79a7437bf642d115785b97f40779b6a/src/Moq/Protected/ProtectedMock.cs#L513

-		private static Expression ToExpressionArg(ParameterInfo paramInfo, object arg)
+		private static Expression ToExpressionArg(object arg, Type paramType)
 		{
 			...
-			return Expression.Constant(arg, paramInfo.ParameterType);
+			return Expression.Constant(arg, paramType);
		}

Adjust ProtectedMock.ToExpressionArgs accordingly: https://github.com/moq/moq4/blob/a6fde8b6d79a7437bf642d115785b97f40779b6a/src/Moq/Protected/ProtectedMock.cs#L528

-				yield return ToExpressionArg(args[i], methodParams[i]);
+				yield return ToExpressionArg(methodParams[i], args[i]);

In order to fix the first issue, modify ProtectedMock.SetupSet<TProperty>: https://github.com/moq/moq4/blob/a6fde8b6d79a7437bf642d115785b97f40779b6a/src/Moq/Protected/ProtectedMock.cs#L119

-			var expression = GetSetterExpression(property, ItExpr.IsAny<TProperty>());
+			var expression = GetSetterExpression(property, ToExpressionArg(value, property.PropertyType));

In order to fix the second issue, modify ProtectedMock.VerifySet<Property>: https://github.com/moq/moq4/blob/a6fde8b6d79a7437bf642d115785b97f40779b6a/src/Moq/Protected/ProtectedMock.cs#L294

-			var expression = GetSetterExpression(property, ItExpr.IsAny<TProperty>());
+			var expression = GetSetterExpression(property, ToExpressionArg(value, property.PropertyType));

These changes will make two unit tests fail. To me they look nonsensical to begin with and should be changed, too:

https://github.com/moq/moq4/blob/a6fde8b6d79a7437bf642d115785b97f40779b6a/tests/Moq.Tests/ProtectedMockFixture.cs#L805-L812

(Why does this test expect the property to have been set to "bar", when it was set to "foo" a few lines above? 🤔)

https://github.com/moq/moq4/blob/a6fde8b6d79a7437bf642d115785b97f40779b6a/tests/Moq.Tests/ProtectedMockFixture.cs#L833-L841

(It.IsAny<int> for a string property? 🤔)

[jerviscui]

Therefore, your second setup essentially shadows the former; it is matched for both calls.

This doesn't make sense to me and looks like a bug: why take a value at all when it doesn't get used?

Yes, I'm so sorry. I have a mistake.
Now, I know the latest Setup() overwrite the previous one. I used to think they worked at the same time by different ItExpr.

[stakx]

I have a mistake.

No, your code is fine. Your second setup does not shadow the first, because it specifies a different argument. But because Moq currently ignores that argument, the second setup ends up shadowing the first setup anyway. This is an error inside Moq.

[jerviscui]

I am very grateful for your guidance and encouragement.

I'll try create a PR. I may not be as fast as you, you are the best.

[jerviscui]

Protected().SetupSet() ignores argument.
I think Protected().SetupGet() needs a argument, maybe.

Just like:

mock.Protected().SetupGet<string>("PrivatePropForTest", ItExpr.IsAny<TProperty>()).Returns("value");

and Protected().VerifyGet().

Just an idea of mine.

[stakx]

Nice to hear you're interested in contributing, @jerviscui. Take your time, we're not in a hurry.

Protected().SetupGet() needs a argument, maybe

No, I don't think so. Remember that properties are really just some syntactic sugar on top of a group of methods:

• a getter method, which does not have any parameters (and therefore SetupGet won't need one either); and
• a setter, which has one parameter for the new value (and therefore SetupSet has one, too).

(Things are a little different for indexers, which are essentially parameterized properties. ProtectedMock doesn't currently have full support for indexers, as witnessed by some TODO comments. But let's forget about indexers for the moment, let's focus on the reported bug. Best to look at indexer support separately.)

[jerviscui]

You're right. My mind was in turmoil yesterday. 😂

getter does not need any parameters.

[tonyhallett]

I have already raised this issue and provided a pull request.
The pull request is in draft status as I wanted to confirm if the current behaviour, "bug" , was ok to fix
#1167

@stakx Would you like me to complete this pull request ?
Also #1165 has had the changes that you suggested and i waiting for your approval.

@jerviscui If you use MoqProtectedAs as detailed in the quick start. It does not have the bug.

Moq 4.8 and later allows you to set up protected members through a completely unrelated type that has the same members and thus provides the type information necessary for IntelliSense to work

[stakx]

@tonyhallett, my apologies. I got stalled on your PRs because there were quite a few of them all at the same time, and I got a little confused what each of them is attempting to do. I was plannig to look at all of them in a single setting, but haven't yet found the time for that... resource-wise, it's far easier for me to deal with a single PR at a time.

If your PRs indeed cover the same bugs reported by @jerviscui, then I suggest we proceed with your (@tonyhallett) PRs — OK with you, @jerviscui?

[tonyhallett]

@jerviscui I'd appreciate it if we proceed with mine as it covers more than just the bug identified in this issue. The pull request took a bit of thought and time to craft.

[jerviscui]

Happy weekend!

I'm OK, listen to you @stakx .

I'll also try to do it, for study.

[jerviscui]

Hi, @stakx
I found the functions are Ok, the bug is just not create the setter MethodCallExpression.
How about use ToExpressionArg() method directly:

https://github.com/jerviscui/moq4/blob/0b702e9e6177e4989f9b147d1e974f5dfc316631/src/Moq/Protected/ProtectedMock.cs#L128

[stakx]

@jerviscui, yes. Using ToExpressionArg is what I would do, too; see my suggested changes in my post above. @tonyhallett has also chosen the same approach in his PR (#1186).

[jerviscui]

Sure, I see.

I mean is, why must change method define ToExpressionArg(ParameterInfo, object) to ToExpressionArg(object, Type),
How about use original ToExpressionArg(ParameterInfo, object) :

public ISetupSetter<T, TProperty> SetupSet<TProperty>(string propertyName, object value)
{
...	
	var expression = GetSetterExpression(property, ToExpressionArg(property.SetMethod.GetParameters()[0], value));
...
}

Are there hidden details that I don't know about?

[tonyhallett]

@jerviscui
ToExpressionArg in its original form only uses the Type ( ParameterType ) and so should be refactored accordingly. The method can then also be used for other use cases - such as supporting indexers.

[stakx]

Are there hidden details that I don't know about?

I'll try to keep it brief: Reflection is a complicated beast, and lots of things can go wrong. The less we have to rely on it, the better. In this case, ToExpressionArg doesn't really need a ParameterInfo, it just needs type information. So by providing just that instead of looking up a parameter, we provide all the info the method needs without running the risk that looking up the setter method will fail. We also end up doing less work overall, which means better performance (Reflection is relatively slow).

(You may think that looking up a property setter is trivial, but it turns out that it isn't always straightforward; check out e.g. CanWrite in src/Moq/Extensions.cs.)

[jerviscui]

OK, get.
Thankyou @tonyhallett @stakx

I have only considered doesn't throw exception, because method is checked by 'Guard.CanWrite()' in front.
Refactoring is good.