fix ProtectedMock fails when arguments are of type Expression #1188

[tonyhallett]

fix #1188

[stakx]

Could you please give this PR a somewhat more meaningful title? If this gets merged, the PR title will end up in the commit history, where some random GitHub issue numbers won't be very helpful.

[tonyhallett]

This needs further thought due to ItExpr.IsAny<Expression>. Expression.IsMatch and specific check for It.Ref is probably the way to go.

[tonyhallett]

@stakx Your thoughts please when you have the time.

[stakx]

@tonyhallett, if I understand the problem correctly, a solution would perhaps need to check whether the provided Expression argument is assignment-compatible-to the target parameter; and if so, instead of returning it verbatim, wrap it in an additional ConstantExpression. (Actually, I suspect it would be more correct to wrap it in an Expression.Quote but that may not be understood by the existing code.)

I'm on the move right now and cannot verify whether this idea works, perhaps you can give it a try.

[tonyhallett]

We need to consider ItExpr.Is<Expression> as well and the only way to do that is to check if the expression is a matcher expression.

[stakx]

This (MethodCallExpression used as a parameter type of a protected method one wants to set up using mock.Protected()) is likely such an edge case that I'm honestly not sure whether it is worth fixing. I love fixing bugs and making everything work as it should, but here it's probably not going to pay off... because once we introduce a dedicated matcher expression backing ItExpr.IsAny, we potentially need to touch lots of other parts in the codebase. I for one don't really want to go there right now.

[tonyhallett]

I am not sure what you mean by

because once we introduce a dedicated matcher expression

I have not introduced anything new. On the rare occasion that an argument ( method, property setter, indexer ) is an Expression then we check if it is a matcher expression using the built in method of doing so.

[stakx]

I am not sure what you mean by

because once we introduce a dedicated matcher expression

I have not introduced anything new.

I know that you haven't. I was assuming that you might need to, but I see now that this assumption doesn't necessarily hold. My comment was in response to your following statement:

We need to consider ItExpr.Is<Expression> as well and the only way to do that is to check if the expression is a matcher expression.

ItExpr.IsAny is a factory method that ends up producing a MethodCallExpression for the It.IsAny<> method (IIRC). So given that you're looking at a MethodCallExpression-typed parameter, how do you distinguish between MethodCallExpressions that should be used verbatim (as a constant argument), and those that refer to a matcher and should be treated specially?

On the rare occasion that an argument ( method, property setter, indexer) is an Expression then we check if it is a matcher expression using the built in method of doing so.

I think you're right, and by "the built in method" I suppose you're referring to those extension methods in ExpressionExtensions (IIRC). So you'd need to run any Expression argument through that check to see whether it needs to be wrapped as a constant / quoted expression or not.

Yes, that sounds feasible!

[tonyhallett]

Great !

[stakx]

Thanks for the PR, @tonyhallett. Looks good 👍, just a few minor details.

[stakx]

Please remove. (We don't need a changelog entry because from a user's perspective, this issue never actually existed: It only surfaced after #1186, but wasn't yet observable before that, i.e. in the latest released version.)

[tonyhallett]

No it has always been an issue. You changed the issue title from "ProtectedMock fails when arguments are of type Expression" to mock.Protected().SetupSet fails when argument is of type Expression.
I have just put the issue title in to the changelog.

[stakx]

@tonyhallett, yes, the issue may always been present, but it never surfaced for Moq users because value was ignored up until recently. (Or am I missing something here?) So users don't need to know about this issue. The PR title is only relevant because it ends up in the commit history. The commit history is important to Moq developers, the changelog OTOH is important to Moq users. Please remove the changelog entry.

[tonyhallett]

The issue could have surfaced at any time over the last 12? years as ToExpressionArg was being called by ToExpressionArgs, called by the GetMethodCall methods ( args were not ignored ). GetMethodCall methods are called by numerous paths.

For instance
https://github.com/moq/moq4/blob/a6fde8b6d79a7437bf642d115785b97f40779b6a/src/Moq/Protected/ProtectedMock.cs#L47

Link is from before #1186

[tonyhallett]

As I said, perhaps your change to the issue title made you think it only applied to SetupSet which was ignoring the value.

[stakx]

@tonyhallet, I see. Thank you for the explanation. The changelog entry certainly made me think so, since it does mention SetupSet (which so far wouldn't have been affected, right? 😉). Let's keep the changelog entry then. Perhaps you can adjust it to include method(s) where this would have been observable even before now? And do feel free to give this PR a more accurate / correct title if you feel that the current title is inaccurate.

[stakx]

@tonyhallett, this should be ready to merge, right? Or was there anything else you need to finalize here before merging?

[tonyhallett]

Only thing was do you want to keep the
if (IsItRefAny(expression))
given that protected mock does not support ref or out.

I think I identified a way of adding ref / out to protected mock ( details above ) so I think that we should keep it. I will provide an issue / pull request in a little while.

[stakx]

given that protected mock does not support ref or out.

I'd say let's look at that in a separate PR.

[tonyhallett]

Ok so you want me to remove the check for IsItRefAny and the refactor of ToArgTypes for this common logic and add it to the pull request I prepared for ref / out support.

[stakx]

Why not keep things as they are now. I don't mind keeping IsItRefAny for now, even if you might only really need it in a next PR. (Moving those nested ifs into ItRefAnyField was a good refactoring either way, and IsItRefAny is such a small, slim method on top of it that there's no harm done keeping it for now.)

[tonyhallett]

Great. Please merge.