Ignored/That argument constraints do not Work for ByRef-Arguments in VisualBasic

[thunderbird55]

All Versions >= 2.00

See the following interface

Public Interface IFoo
    Sub Bar(b As Object)
    Sub BarByRef(ByRef b As Object)
End Interface

As soon as I want to use any argument constraint for an ByRef-Argument an exception is thrown. (System.InvalidOperationException: A.Ignored, A._, and A.That can only be used in the context of a call specification with A.CallTo)

<TestMethod>
Public Sub TestMethod1()
    Dim test = A.Dummy(Of IFoo)

    test.Bar(2)
    test.BarByRef(2)

    A.CallTo(Sub() test.Bar(A(Of Object).Ignored)).MustHaveHappened()      'OK

    A.CallTo(Sub() test.BarByRef(2)).MustHaveHappened()                    'OK

    A.CallTo(Sub() test.BarByRef(A(Of Object).Ignored)).MustHaveHappened() 'Error
End Sub

[thomaslevesque]

Hi @thunderbird55,

Interesting... we never noticed this issue before, because it can't occur in C#. In C#, only a local variable or a field can be passed by reference, not a value (like 2) or a property (like A(Of Object).Ignored), so there's no way to use an argument constraint on a by-ref parameter.

I'll look into it. In the meantime, for this specific case you can use this:

A.CallTo(Sub() test.BarByRef(Nothing).WithAnyArguments().MustHaveHappened()

If there are other arguments that need to be constrained, you can use WhenArgumentsMatch.

[thunderbird55]

Hi @thomaslevesque
thank you for your quick response. For my case WhenArgumentsMatch works.
I think this issue happens because of the way VB handles ByRef-Arguments. As you wrote c# does not allows properties to pass by reference ( for good reasons).
The VB-Compiler creates automatically a local variable that is passed by reference.
So

A.CallTo(Sub() test.BarByRef(2)).MustHaveHappened()

becomes

Dim tmp As Object = 2
A.CallTo(Sub() test.BarByRef(tmp)).MustHaveHappened()

[thomaslevesque]

Yes, but that doesn't really explain the error message... I'll need to debug step by step to see exactly why it's not working.

[blairconrad]

@thomaslevesque, I think @thunderbird55 has explained the error message. A corollary to yo's last statement is that

A.CallTo(Sub() test.BarByRef(A(Of Object).Ignored)).MustHaveHappened()

becomes

Dim tmp As Object = A(Of Object).Ignored
A.CallTo(Sub() test.BarByRef(tmp)).MustHaveHappened()

And the latter will throw on the first line.

[thomaslevesque]

Ah, yes, I missed that. Actually, I expected the code to become something like this:

A.CallTo(Sub()
    Dim tmp As Object = A(Of Object).Ignored
    test.BarByRef(tmp)
End Sub).MustHaveHappened()

(probably not valid syntax, but you get what I mean)

But it doesn't really change anything, since A(Of Object).Ignored is still not directly passed as a parameter.

So, in fact, I'm not sure if this should be considered a bug. We can't use argument constraint on by-ref parameters:

• in C#, because the language doesn't allow passing properties by reference
• in VB.NET, because of the way the compiler rewrites the code to allow it

What bothers me is that the error message is really confusing, and I'm not sure what we could do to improve it...

[thomaslevesque]

btw @thunderbird55, maybe this code didn't throw in FakeItEasy 1.x, but it didn't do what you would expect either: A<T>.Ignored returns null, so it was the same as doing A.CallTo(Sub() test.BarByRef(Nothing)).

Starting in 2.0.0, we made A<T>.Ignored and A<T>.Matches() throw when called outside a call specification, to prevent invalid usage that wouldn't give the expected result.

[thunderbird55]

@thomaslevesque
I just used A(Of Object).Ignored to keep the example as simple as possible.
The original test used this code:

A.CallTo(
    Sub() attHandler.ElementStart(A(Of String).Ignored,
                                  A(Of CAttributes).That.Matches(Function(pAtt) pAtt.Value("Nr") = "200"))
    ).MustHaveHappened()

And this code worked with version 1.23
I've created some new tests with a newer version of FakeItEasy but we mentioned that a few old tests didn't work.

And I think your expectation is right, I forgot that this was inside a lambda. But honestly I'm not sure what VB does inside lamda statements.

[blairconrad]

I renamed this to make the title a little more accurate (as I understand things). And labelled it as a discussion so we don't lose it.
At some point, we should decide whether it's a bug or... I don't know what. If we decide we can't fix the problem, we should document it.

Also, I'm a little confused. Based on my understanding of how That and Ignored work, I don't know how @thunderbird55's latest example would work under FakeItEasy 1.23. But I've not had/taken the time to reproduce.

[thomaslevesque]

I think we should close this issue as wontfix, because there's no way we can fix it, short of a complete overhaul of how argument constraints work.

[blairconrad]

At the very least, I'd update the docs, explaining the shortcoming for the VB users.
I don't mind taking that on.

[blairconrad]

Late-breaking news!

The cause of the problem isn't what we thought it was. And it's a bug. The fixes for #220 and #559 conspired together to cause it.

When we try to get the value to implicitly assign to the ref parameter, we end up invoking A.That outside the scope of the capturing expression, and we throw.
A blunt-force approach to fixing it is to catch the exception and use a default value.
I'll see if I can work up something more elegant.

[thomaslevesque]

The cause of the problem isn't what we thought it was.

When we try to get the value to implicitly assign to the ref parameter, we end up invoking A.That outside the scope of the capturing expression, and we throw.

Isn't it what we were saying the cause was?

[thomaslevesque]

OK, I see what you mean now. The problem seems to be here: https://github.com/FakeItEasy/FakeItEasy/blob/master/src/FakeItEasy/Expressions/ExpressionArgumentConstraintFactory.cs#L39
Basically, we shouldn't be calling argument.Value outside the constraint trap, because it ends up evaluating the expression.

I have a fix ready, I'll send a PR ASAP

[blairconrad]

We could also consider backporting to the 2.x branch.
Unless it's having a dire effect on @thunderbird55, I'd prefer not to. (Because lazy.)
@thunderbird55, would you be able to move up to a 3.0.0-beta003 (or thereabouts) as easily as something like 2.3.2?

[thomaslevesque]

I don't mind backporting the fix to 2.x, it's not a big deal.

[blairconrad]

I don't mind backporting the fix to 2.x, it's not a big deal.

It is a nice gesture. If I thought 3.0 were coming out real soon, I'd say skip it.

But if you're game, I'll happily do the review/merge. Thanks!

[blairconrad]

Oh, if you do decide to port to 2.3.2, beware of #515.
Manual creation of the release is the way to go, I think.

[thomaslevesque]

Oh, if you do decide to port to 2.3.2, beware of #515.
Manual creation of the release is the way to go, I think.

I'll take care of it tomorrow if I can. For now I just created the support-2 branch and cherry-picked the fix.

[blairconrad]

Merged and made release, milestone, release issue, PR to set version. I think we are in good shape.

[thomaslevesque]

Merged and made release, milestone, release issue, PR to set version. I think we are in good shape.

Thanks!

[thunderbird55]

Thank you very much.
I just tried version 2.3.2 and now it works :)
Unfortunately some other tests fail now but I already know the reason and will open a new ticket for that.