False positive "nested constraint" exception when using an argument constraint of the wrong type

[progala2]

Original title: Weird issue with nested constraints and simple types

public interface IEmailFactory
    {
        Task<IList<MailMessage>> GetEmailMessages(long idPairGame, EmailTemplateType emailTemplateType);
    }

_emailFactory = new Fake<IEmailFactory>().FakedObject;

A.CallTo(() => _emailFactory.GetEmailMessages(A<int>._, A<EmailTemplateType>._)).Returns(mailMessages);

EmailTemplateType is an enum. When I change it to int it is still wrong, so it is not an issue.

FakeIt version 4.1.

It throws the following exception

FakeItEasy.Expressions.ExpressionArgumentConstraintFactory.ArgumentConstraintExpressionVisitor.VisitMember(MemberExpression node)
at System.Linq.Expressions.MemberExpression.Accept(ExpressionVisitor visitor)
at System.Linq.Expressions.ExpressionVisitor.Visit(Expression node)
at System.Linq.Expressions.ExpressionVisitor.VisitUnary(UnaryExpression node)
at System.Linq.Expressions.UnaryExpression.Accept(ExpressionVisitor visitor)
at FakeItEasy.Expressions.ExpressionArgumentConstraintFactory.CheckArgumentExpressionIsValid(Expression expression)
at FakeItEasy.Expressions.ExpressionArgumentConstraintFactory.GetArgumentConstraintFromExpression(Expression expression, Object& value)
at FakeItEasy.Expressions.ExpressionArgumentConstraintFactory.GetArgumentConstraint(ParsedArgumentExpression argument)
at System.Linq.Utilities.<>c__DisplayClass2_03.<CombineSelectors>b__0(TSource x) at System.Linq.Enumerable.SelectEnumerableIterator2.ToArray()
at System.Linq.Enumerable.ToArray[TSource](IEnumerable1 source) at FakeItEasy.Expressions.ExpressionCallMatcher..ctor(ParsedCallExpression parsedExpression, ExpressionArgumentConstraintFactory constraintFactory, MethodInfoManager methodInfoManager, Factory outputWriterFactory) at FakeItEasy.RootModule.<>c__DisplayClass0_0.<RegisterDependencies>b__4(ParsedCallExpression callSpecification) at FakeItEasy.Configuration.FakeConfigurationManager.CallTo[T](Expression1 callSpecification)

[progala2]

It works with 4.0

[thomaslevesque]

Thanks for reporting this @progala2. Not sure what's going on. It's a very basic use case, which should be covered extensively by our tests, so I'm really surprised it fails. I'll look into it ASAP.

[progala2]

Nope. It works partially with 4.0 - it is registered without errors but doesn't return anything

mailsToSend.AddRange(await _emailFactory.GetEmailMessages(game.Id, (EmailTemplateType)mailReminder.EmailTemplate.Id));

Yes it is a very basic case. I tried to clean the solution, I restarted VS... nothing helps, so I post it here :/

[thomaslevesque]

There's a problem in your code: it should be A<long>._, not A<int>._. Since the parameter is of type long, the A<int>._ constraint will never match; in fact, we even added a diagnostic in the latest analyzer.

I think this is what causing the error, since I can't reproduce the problem when I change the constraint type. However, the exception is clearly incorrect, we definitely have a bug.

[progala2]

yes, it works now. Jeez, I hate this type of "typos" 😖
I definitely need to eat something 😂
Thanks 😄

[blairconrad]

Not saying this is it, but idPairGame is a long but the constraint is A<int>._. Consider rationalizing just to rule it out as a problem?

[blairconrad]

Bah! Github app didn't refresh the comments. I thought I was so smart.

Glad the problem is found. Definitely a big. And @progala2, do consider picking up the analyzer. It can help in surprising ways.

[progala2]

Installed 🗡
Thanks!

[blairconrad]

Ha, thank you. And I'm not just pushing it because the diagnostic in question the first one I put any significant effort into... 😉
I hope it helps you.

[thomaslevesque]

The problem appears to be here. In this case, there is also an implicit conversion, but it's not a boxing conversion, so we don't eliminate it, and the constraint is seen as nested in the conversion. The problem is that there's no way to distinguish between explicit and implicit conversions with the Expression API...

I think we should always eliminate the conversion, even if it's not a boxing conversion, but I'm a bit worried about unwanted side effects.

[blairconrad]

@thomaslevesque, this still may not be fixed:

[Scenario]
public static void ThatArgumentConstraintForDifferentValueTypeWithNonNullArgument(
    IHaveANullableParameter subject,
    int result)
{
    "Given a fake with a method that accepts a nullable value type parameter"
        .x(() => subject = A.Fake<IHaveANullableParameter>());

    "And a call configured for a non-nullable argument of a different type"
        .x(() => A.CallTo(() => subject.Bar(A<byte>._)).Returns(42));

    "When I make a call to this method with a non-null argument"
        .x(() => result = subject.Bar(7));

    "Then it matches the configured call"
        .x(() => result.Should().Be(42));
}

fails, with a complaint about a nested constraint. The nullable parameter is an int?, and I passed a byte constraint, resulting in 2 conversions!. I think we need to keep stripping conversions, instead of just taking the first.

[blairconrad]

Yeah, this seems to fix that portion, at least:

private static Expression GetExpressionWithoutConversion(Expression expression)
{
    while (expression is UnaryExpression conversion && conversion.NodeType == ExpressionType.Convert)
    {
        expression = conversion.Operand;
    }

    return expression;
}

Of course, this just means we consider the constraint not to be nested. It still does the wrong thing until #1237 is fixed.

[thomaslevesque]

OK, so it's easy enough to fix. Just change an if to a while ;)

Of course, this just means we consider the constraint not to be nested. It still does the wrong thing until #1237 is fixed.

True. I have started working on #1237, but it might be a few days before I can send a PR.

[blairconrad]

OK, so it's easy enough to fix. Just change an if to a while ;)

Pretty much. I can work on that later, if you like.

True. I have started working on #1237

Oh. So have I. It was unowned, so I self-assigned and started working on it last night.

[thomaslevesque]

This change has been released as part of FakeItEasy 4.1.1.

Thanks for the report @progala2! Look for your name in the release notes. 🏆