Make guard against unmatchable matchers less strict to enable user-based wildcard matching #1202
Conversation
There was a problem hiding this comment.
Great to see you got this working! 👍
In addition to the change requests below, please also add an entry to the changelog, e.g.:
#### Fixed
...
+* The guard against unmatchable matchers (added in #900) was too strict; relaxed it to enable an alternative user-code shorthand `_` for `It.IsAny<>()` (@adamfk, #1199)
## 4.16.1 (2021-02-23)
src/Moq/MatcherFactory.cs
Outdated
| else | ||
| { | ||
| foundType = convertExpression.Operand.Type; | ||
| } |
There was a problem hiding this comment.
(Ideally, we wouldn't have any conditionals here. We ought to be able to simply ask the Match object directly what type of values it targets.
I also suspect that we currently don't have any tests covering this else block.
For both reasons, it'd be nice to simplify this whole if–else block to something non-conditional, but this is more of a bonus—you can leave this code as is for now.)
There was a problem hiding this comment.
I wouldn't mind making a new ticket to clean it up. I'd like to get this in and working first.
There was a problem hiding this comment.
If you are willing to clean this up, then I say, let's do it now. (It should be a fairly trivial change, see my earlier comment over in the issue about adding some Type property to Match.)
stakx
changed the title
stakx
changed the title
There was a problem hiding this comment.
Please roll back your last commit that extracts the assignment compatibility check into a method and adds tests for it.
I'm generally all for more tests, but I think it really isn't necessary to test Type.IsAssignableFrom... too low-level. Also, there is already UnmatchableMatchersFixture that targets this particular bit of Moq.
This reverts commit f315474.
@stakx Done. I just added it so that I could show that the original I've used bitbucket a lot at work, but this is my first github PR. Do you get notified to my conversation replies like this one #1202 (comment), or do I need to explicitly tag you? |
GitHub notifies me whenever there is any change to the PR, be it a new commit or a new comment somewhere... it's then up to me to locate the change(s). Not the best possible notification system, really, but it's what we have to work with. 🙂 |
| public static implicit operator Animal(AnyValue _) => It.IsAny<Animal>(); | ||
| public static implicit operator Dolphin(AnyValue _) => It.IsAny<Dolphin>(); | ||
| public static implicit operator int(AnyValue _) => default; | ||
| public static implicit operator byte(AnyValue _) => default; |
There was a problem hiding this comment.
@stakx should I switch these back to using It.IsAny<T>()? Or should I rely on implicit conversion functions like this never being called? Just thinking about making a source generator for people to use and if possible, I don't want to rely on behavior that may be changed.
There was a problem hiding this comment.
No, keep these as default. You should only have one call to a matcher per parameter. If you have more than that, you may confuse the SetupSet / VerifySet delegate reconstruction machinery (ActionObserver), which attempts to distribute recorded matchers over the available parameters.
Note also this comment in ActionObserver, which states Moq's general assumption of matchers returning default(T):
Meaning, if you want SetupSet and VerifySet to interact correctly with your _ shorthand, it should probably return default / null instead of new AnyValue().
|
Thanks for contributing, @adamfk! |
All tests are passing. No changes to moq API. I think we might have it!
Resolves #1199.