-
-
Notifications
You must be signed in to change notification settings - Fork 802
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
implicit guard too strict; prevents valid user wildcard _
implementations
#1199
Comments
Hi @adamfk, nice idea! I agree that Unfortunately, faking a discard operator That aside, your example above shows that users would also have to do some configuration to get this to work for own types: one would have to derive a For those two reasons ( With regard to enabling
Just for the sake of completeness, this is what you can currently do. It's not super-succinct, but a little bit shorter than public abstract class TestFixtureBase
{
public static AnyBool => Match.Create(_ => true, () => AnyBool);
public static AnyInt => Match.Create(_ => true, () => AnyInt);
public static AnyGear => Match.Create(_ => true, () => AnyGear);
...
}
public class TestClass : TestFixtureBase
{
...
[Fact]
public void OldWay2()
{
...
mockCar.Setup(car => car.SetGear(AnyGear, AnyInt)).Returns(42);
...
}
} |
Excellent points! I hadn't thought of implicit conversions with interfaces. It might be able to work with interfaces, but it starts to get even more janky. For interests sake, my original code above does get past the guard mentioned above because The guard does throw however if I make the changes you suggested in the diff above. I'll poke around a bit more and see what I can figure out. Thanks for the fast and helpful response! Feel free to close. I'll report back here if I make any substantial progress. |
OK, I'll close this for the time being. We can always reopen if and when you find a viable solution. Good luck! :-) |
@stakx it wasn't as hard as I thought to find reasonable work arounds. I've opened a PR more for discussion than expecting a merge. |
Here's a cleaner solution: adamfk@c4dca50#diff-e19e65740ba7805f6c3c79daa3472dc0e01fe06285f3cdf5831fb8286501852c I think this coupled with a 3rd party Roslyn analyzer/code fix lib could work really well. The only thing that it would require from moq is adding the Thoughts? |
I think Roslyn is great! Take a look at the upcoming Moq v5 and its foundation library Avatar regarding source generators. For Moq v4, we'd probably profit most from analyzers for detecting common usage errors. My primary issue with your PR is the introduction of If possible, we should reuse the existing mechanism, which is to go through |
I can definitely understand wanting to keep the public API slim. Do you think that Moq v5 will support wildcard matchers like Because C# doesn't allow implicit conversion to interfaces (as you mentioned above), I don't see a road forward using the existing public API. These are some options I see:
Of those options, I only like the second :) Coming from using gmock a lot, I see a lot of value in its wildcard matcher. I realize moq has different design goals. Do you see any/much value in a wildcard matcher? Thanks for your time :) |
It's the latter ( It doesn't really matter who is trying to write the source code, you or a Roslyn generator. The fact of the matter is that C#'s static type system simply doesn't make it easy to create a construct like your
gMock is a C++ library, right? C++ is more flexible in that regard; I'm not sure you'll find something comparable in mainstream .NET languages. For that reason, I suspect you'll need to get used with the reality of your option (1). (2) is an option but like I stated before, if we find a way where it isn't needed, that would be preferable. See my earlier post for the likely direction we'd need to investigate. (3) would be an option, however strong static typing support is one of Moq's explicitly stated design goals, so it's unlikely we'll be adding something like |
Totally agree. I want a solution that supports refactoring and other IDE/compiler goodness. 🤖 Without the implicit guard ( public abstract class AutoIsAny
{
public static AnyValue _
{
get
{
It.IsAny<object>();
return default;
}
}
} This obviously isn't a solution. Just showing one of the many things that I've tried. Even if I find a way around the implicit guard without removing it, I don't like that solution because it is likely that another fix to moq will break whatever workaround I find. It's like relying on undefined behavior. Without adding some way for moq to realize that an implicit conversion is OK, or that something should be treated as I'm happy to investigate more if you can provide some direction. Sorry if I'm missing something totally obvious. |
But it wouldn't be undefined behavior: once you've demonstrated that the current implicit guard is too strict, and you've found a way to change it such that it allows your scenario while still working for the original use case, then we add a test with your use case to the test suite. Any future changes to Moq will from then on have to take your legitimate use case into consideration.
Let me start by making a correction to my first post above. I really should have suggested the following: -public static T _ => It.IsAny<T>();
+public static T _ => (T)It.IsAny<object>();
-public static T any => It.IsAny<T>();
+public static T any => (T)It.IsAny<object>(); Because otherwise, the implicit guard is correct and nothing would ever match (Btw. that's the only place where matchers are needed. Your If we then temporarily comment out the implicit guard, your If you're still worried about this approach relying on undefined behavior, let's look at it in a different way. We're actually trying to fix a bug here, because the test we just ran has proven that the current implicit guard is faulty: it claims something to be unmatchable that wouldn't be, and it thus blocks a legitimate use case. Let's briefly review what led to the guard being added. interface IX
{
void Method(DateTimeOffset arg);
}
var mock = new Mock<IX>();
mock.Setup(x => x.Method(It.IsAny<DateTime>())).Verifiable();
mock.Object.Method(DateTime.Now);
mock.Verfify(); The programmer is making a mistake here, because I suspect the issue with the implicit guard is that it compares the wrong thing against the parameter type: (For the above example, The thing is, this code only looks at the static type of the matcher expression, and not at the matcher's type argument. In the above example, those two types are identical; but in your case, they are not: public static T _ => (T)It.IsAny<object>(); The implicit guard checks the assignment-compatibility of The So I think that might be one way how to solve this puzzle. 😉 |
Thanks for the help! I'll take another crack at it this weekend :) If I can get this working normally, maybe a source generator could do all the grunt work of making the compiler happy. That would make for an excellent user experience 🚀 |
Agreed. It might also make a nice contrib package! |
It.IsAny<T>()
_
implementations
Edit: if you want to see where the bug is discussed, skip to #1199 (comment)
The fix for the issue seems counterintuitive at first, but was thoughtfully considered here: #1202 (comment)
The rest that follows is a bunch of discussion/attempts to enable an alternative user-code shorthand
_
forIt.IsAny<>()
.Hello! Awesome project.
I have a suggestion for how to support an alternate shorthand
_
instead ofIt.IsAny<T>()
.This is inspired by gmock's
_
wildcard matcher.When mocking a function that takes more than a few arguments, the
It.IsAny<T>()
noise really distracts and is unpleasant to write.versus
The code below compiles fine and triggers the correct
It.IsAny<T>()
calls, but it doesn't result in a proper moq match. I traced it through the moq codebase for a bit and then figured I should ask here first.Even if you aren't interested in adding the
AnyHelper
classes to Moq, would you be receptive to a PR that would allow this type of functionality to work? I believe a change is required in theExpression
handling.The text was updated successfully, but these errors were encountered: