Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix ProtectedMock fails when arguments are of type Expression #1188 #1189

Merged
merged 6 commits into from Aug 3, 2021

Conversation

tonyhallett
Copy link
Contributor

@tonyhallett tonyhallett commented Jul 22, 2021

fix #1188

@stakx
Copy link
Member

stakx commented Jul 22, 2021

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 tonyhallett changed the title fix #1188 fix ProtectedMock fails when arguments are of type Expression #1188 Jul 22, 2021
@tonyhallett
Copy link
Contributor Author

tonyhallett commented Jul 22, 2021

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

@tonyhallett tonyhallett force-pushed the fix-protectedmock-expression-type-args branch from 37636b7 to ae588a4 Compare Jul 22, 2021
@tonyhallett
Copy link
Contributor Author

tonyhallett commented Jul 22, 2021

@stakx Your thoughts please when you have the time.

@stakx
Copy link
Member

stakx commented Jul 23, 2021

@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
Copy link
Contributor Author

tonyhallett commented Jul 23, 2021

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
Copy link
Member

stakx commented Jul 23, 2021

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
Copy link
Contributor Author

tonyhallett commented Jul 23, 2021

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
Copy link
Member

stakx commented Jul 23, 2021

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
Copy link
Contributor Author

tonyhallett commented Jul 23, 2021

Great !

Copy link
Member

@stakx stakx left a comment

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

CHANGELOG.md Outdated
@@ -13,6 +13,7 @@ The format is loosely based on [Keep a Changelog](http://keepachangelog.com/en/1

#### Fixed

* mock.Protected().SetupSet fails when argument is of type Expression (@tonyhallett, #1189)
Copy link
Member

@stakx stakx Aug 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Contributor Author

@tonyhallett tonyhallett Aug 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@stakx stakx Aug 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor Author

@tonyhallett tonyhallett Aug 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

public ISetup<T> Setup(string methodName, Type[] genericTypeArguments, bool exactParameterMatch, params object[] args)

Link is from before #1186

Copy link
Contributor Author

@tonyhallett tonyhallett Aug 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@stakx stakx Aug 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

tests/Moq.Tests/ProtectedMockFixture.cs Show resolved Hide resolved
tests/Moq.Tests/ProtectedMockFixture.cs Outdated Show resolved Hide resolved
src/Moq/Protected/ProtectedMock.cs Outdated Show resolved Hide resolved
src/Moq/Protected/ProtectedMock.cs Outdated Show resolved Hide resolved
@stakx stakx added this to the 4.17.0 milestone Aug 1, 2021
src/Moq/Protected/ProtectedMock.cs Outdated Show resolved Hide resolved
stakx
stakx approved these changes Aug 2, 2021
@stakx
Copy link
Member

stakx commented Aug 2, 2021

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

@tonyhallett
Copy link
Contributor Author

tonyhallett commented Aug 2, 2021

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
Copy link
Member

stakx commented Aug 2, 2021

given that protected mock does not support ref or out.

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

@tonyhallett
Copy link
Contributor Author

tonyhallett commented Aug 2, 2021

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
Copy link
Member

stakx commented Aug 2, 2021

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
Copy link
Contributor Author

tonyhallett commented Aug 2, 2021

Great. Please merge.

@stakx stakx merged commit 6e2eba7 into moq:main Aug 3, 2021
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants