Argument Constraints should support derived types as Type Arguments

[oxfordemma]

Steps to reproduce:

1. Create class with a virtual method that takes in an object as a parameter
2. Create a fake of that class
3. Call the method on that fake with different object types (for example, a string and an int)
4. Verify one of the calls using '.That.Matches' syntax
5. You should get a InvalidCastException

Is this expected, or a bug?

The code I used to test it:

public class Foo{
  public virtual void SomeFunction(object arg) { }

  public void TestFunction(){
            var t = A.Fake<Foo>();
            t.SomeFunction("test");
            t.SomeFunction(123);
            A.CallTo(() => t.SomeFunction(A<string>.That.Matches(i=>i == "test"))).MustHaveHappened();
  }
}

And the output: System.InvalidCastException : Unable to cast object of type 'System.Int32' to type 'System.String'

[oxfordemma]

Potential fix seems pretty easy:

In the class MatchesConstraint (in DefaultAurgumentConstraintManager.cs),
method IArgumentConstraint.IsValid(object argument),

Add a type check (around line 74):

if (argument is T)
    return this.predicate.Invoke((T)argument);
return false;

[blairconrad]

Thanks for rasing this, @FUR10N.

(Note that I cleaned up the issue description using https://help.github.com/articles/github-flavored-markdown#fenced-code-blocks.)

It may be neither expected or a bug.
I mean, it's a fairly unusual operation, to attempt to use a predicate that insists on its argument being a string when the passed-in value may not be a string.

If anyone showed me this, I'd suggest they rewrite their predicate to take A<object>.That...` and check the type themselves, if there was a decent chance that the type was going to mismatch.

Your idea for a fix is a good one, but I have some concerns.
What if the argument is null?Then argument is T is false, but we may wanting to evaluate the predicate anyhow (maybe it takes nulls into account). Similar problems may arise if using as T.

But as you suggest, the error we get back:

System.InvalidCastException : Unable to cast object of type 'System.Int32' to type 'System.String'.
at FakeItEasy.Core.DefaultArgumentConstraintManager`1.MatchesConstraint.FakeItEasy.Core.IArgumentConstraint.IsValid(Object argument)
at System.Linq.Enumerable.All(IEnumerable`1 source, Func`2 predicate)
at System.Linq.Enumerable.Count(IEnumerable`1 source, Func`2 predicate)
at FakeItEasy.Core.FakeAsserter.AssertWasCalled(Func`2 callPredicate, String callDescription, Func`2 repeatPredicate, String repeatDescription)
at FakeItEasy.Configuration.RuleBuilder.MustHaveHappened(Repeated repeatConstraint)
at FakeItEasyQuestions.FUR10N.TestFunction() in FUR10N.cs: line 22 

leaves something to be desired. I like the idea of trapping the error and providing a more readable message for someone, but we may need to take a slightly different approach. I'm sure one could be devised, and it might even handle exceptions raised from within the predicate, for example.

[oxfordemma]

Using A<object> and then checking the type would work, but the syntax gets a little messier than I'd like.
The less contrived use case is, I have a messaging service that receives any number of message (and the messages can differ in type as long as they extend an interface). And in my test code, I want to make sure it received a specific message. Being able to pass in the actual type instead of A<object> looks so much clearer to me.

Also, in my sample code, if all of the calls were made with a parameter of the same type, then you'd be fine. Should it fail in that case too? It feels weird that it should only fail in some cases where multiple calls are made.

[oxfordemma]

Regarding it being an unusual operation, I think if you ignore the semantics, then yes it's unusual. But if you look at what you're trying to accomplish, then it makes sense to me to do something like that.

Essentially what I want to ask with the verify statement is: Verify that I called this function with an argument that matches this type. Not, verify that all calls made to this function match this argument type.

[blairconrad]

Ah, yes. I missed the bit where you had a passing call in the test, so even cleaning up the exception isn't quite what you need (still a good idea, I think, but that's another topic).

It still feels a little weird to me to have the derived type in the A<T> clause, but I can see how it would be useful for you. Of course, you could always write a helper method to clean up the syntax for you, especially as an interim step.

So, working from your original proposal, it would be possible to examine the argument, and if it's null OR is T, cast it and pass it to the predicate, but otherwise return false. I think that allays my concerns. Do you think it would satisfy your use case?

Hmm... if T is a value type, then casting a null value wouldn't work very well…

[oxfordemma]

I don't think it's that awfully weird of a thing to do.

So if the following method call is valid:

t.SomeFunction(new SomeDerivedClass());

Then I'd expect a similar verify call to work:

A.CallTo(() => t.SomeFunction(A<SomeDerivedClass>.That.Matches))

Rather than having to write a predicate that matches whatever base type the method might use.

A.CallTo(() => t.SomeFunction(A<BaseClass>.That.Matches(i=> if (i is DerivedClass) return checkValues((DerivedClass)i);))

But I do agree, I didn't take into account all inputs in the change I proposed. That does need some work. I had actually used argument as T at first, but ran into the value type issue you mentioned.
For getting around that, all I can think to do is reflect on T to check if it's a nullable type. But that gets pretty messy.

[oxfordemma]

Actually, I think this should 'just work' in all cases:

if (argument is T || argument == null)

If argument is T is true, then we're fine and everything is ok.
Else if it gets to argument == null and T is a value type, then that should always return false. If T is nullable, and argument is null, then we can just cast it. Thoughts?

Look at unbounded parameters section for null checking
http://msdn.microsoft.com/en-us/library/d5x73970.aspx

[blairconrad]

It looks promising. Are you interested in trying it out? The issue can be yours!

[oxfordemma]

Sure! It'll probably sometime Monday before I can look at this again, so don't wait up on this potentially crucial change.

[blairconrad]

Thanks!
We'll try to get by in the meantime.

[blairconrad]

Marking as taken by @FUR10N, putting in Working status, and slotting in Milestone 1.22.0.

[blairconrad]

Renamed from "InvalidCastException in A.CallTo(...).MustHaveHappened()" to "Argument Constraints should support derived types as Type Arguments"

[adamralph]

👍

[blairconrad]

@FUR10N, thanks very much for your work on this issue. Look for your name in the release notes. 🏆

This issue has been fixed in release 1.22.0.

https://www.nuget.org/packages/FakeItEasy/1.22.0
https://github.com/FakeItEasy/FakeItEasy/releases/tag/1.22.0