Rubberduck: Argument to a ByRef parameter is passed by value

[retailcoder]

Given:

Sub DoSomething(ByRef foo As Integer)
    foo = 42
End Sub

Then this code:

DoSomething (bar)

Does not pass bar by reference, but rather passes the result of evaluating the expression bar, and because the caller does not hold on to a reference to that result, the ByRef argument is a reference that is effectively discarded, which makes the code behave exactly as if the parameter was ByVal: this can cause unexpected behavior, and should be avoided.

Sub Test()
    Dim bar As Integer
    DoSomething (bar)
    Debug.Print "Back into 'Test', bar is " & bar
End Sub

Sub DoSomething(ByRef foo As Integer)
    foo = 42
    Debug.Print "In 'DoSomething', foo is " & foo
End Sub

The output is as follows:

In 'DoSomething', foo is 42
Back into 'Test', bar is 0

Without the parentheses around (bar), the output is as expected:

In 'DoSomething', foo is 42
Back into 'Test', bar is 42

We need an inspection that locates ByRef arguments being force-passed by value, and suggests removing the extraneous parentheses.

[joshkreud]

this is .... why is this ?? ... why is vba doing things ?? so basically you say
DoSomething(bar)= byval
DoSomething bar = byref

but what if

if somefunction(byrefarg) = true then ... <-- there the parentheses is mandatory

[retailcoder]

The parens are mandatory in a _function_ call (where the return value is captured by the caller); in a _procedure_ call (or a function call where the return value is discarded by the caller) they basically force VBA to _evaluate_ the parens-enclosed argument as a _value_, and pass it as such, effectively overriding the ByRef specifier.

So assuming a Sub DoSomething(ByRef foo) signature, DoSomething (bar) is force-passed by value.

In the case of a function, the same holds true - assuming Function SomeFunction(ByRef foo), then If SomeFunction((bar)) is force-passing bar by value, again overriding the ByRef specifier.

[joshkreud]

i never understood the concept of omitting the parenthesis because
Call MySub(arg , arg2) looks a lot nicer to me then Mysub arg, arg2
And now you're saying me that they do behave so drastically different ?
This is just another creepy thing about vba :D

Funny side note: I've been doing the parenthesis thing everytime with the cancel thing and i never noticed this behavior.

[joshkreud]

This was the concept i was used to ...
call DoSomething(bar)= byref
DoSomething bar = byref
call DoSomething((bar))= byval <-- only this should force byval

https://msdn.microsoft.com/en-us/library/chy4288y.aspx

But that seems to be only VB.net right ?

[retailcoder]

It appears that behavior is inherited from VB.NET's VB6 ancestor, which VBA pretty much _is_.

[ghost]

If I understand this correctly, this issue is calling for an inspection for a procedure call with parens enclosing the arguments, for example:
DoSomething (bar)
or DoSomething((bar))
or Call DoSomething((bar))

but NOT:
Call DoSomething(bar)
DoSomething bar

The inspection will give a warning that bar will be passed as ByVar even if ByRef is specified in the corresponding Sub or Function.

But, does the inspection need to look and see if ByRef is specified in DoSomething? It seems like the inspection should provide a warning either way, as this seems something the developer shouldn't do unless they are certain of the behavior (if so, the inspection can be ignored.)

[MDoerner]

In VBA parameters are implicitly passed by reference. So, the only situation in which parentheses do not change the behaviour is the one where the parameter is specified with the ByVal keyword.

I think that there should always be a result because most of the time this is a bug. Whenever the parentheses are intentional, the user can use an ignore annotation, which also forces the user to indicate that something special is going on in that call.

If I see it correctly, we want to find every argumentExpression whose expression is a parenthesizedExpression.

[MDoerner]

For anyone wanting to implement this inspection, you will get most but not all occurrences by simply looking for arguments in argument lists enclosed in parentheses. The exception are line continued calls to a sub:

DoSomething _
    (arg) 

We interpret this as a line continued index expression. The parser cannot know that it is dealing with a sub to which an argument gets passed explicitly by value; it could just be a function to which an argument gets passed in a regular way. (In principle, it could actually know it, but that would need quite some context awareness in the parser.)

[retailcoder]

As per chat, this specific inspection should only flag parenthesized expressions that involve no operators (i.e. literal and simplename expressions).

Argument expression evaluation is redundant; the result of the parenthesized expression is passed as an argument, but discarded at the call site - if the argument is passed to a ByRef parameter, the caller cannot access that reference and the code behaves as if the argument was passed by value.

The quick-fix would be to remove the redundant parentheses around the expression.

When operators are involved, there's a potential legitimate readability concern, where parentheses indeed help visually distinguish arguments in an argument list:

MsgBox (foo & bar & twentyOtherThings), vbOkOnly, "something"

In such cases, it makes more sense to suggest addressing the readability issue otherwise:

The result of a parenthesized expression is passed as an argument: consider extracting expression into a local variable.

Where the quick-fix would be to prompt for a name and introduce a local variable (declared using the type of the parameter?):

Dim localExpression As String
localExpression = foo & bar & twentyOtherThings

MsgBox localExpression, vbOkOnly, "something"

[bclothier]

Just one more edge case. When calling a procedure declared with a Declare statement, we can suggest a quickfix to use ByVal instead of parentheses:

SomeExternalProcedure ByVal foo

That would be especially the case if the foo parameter is defined using either Any or ByRef and one wishes to override it using ByVal.

That may need to be its own inspection, though.