Rubberduck: Code Inspection: Incorrect recommendation to switch from ByRef to ByVal for array

[ghost]

Seems like this is broken again

Code inspector recommends changing to ByVal for this_items() in

Public Sub Report(ByVal this_formatter As String, ByRef this_items() As Variant)

but changing to ByVal gives non compilable code (flagged in red by VBA)

Version 2.3.1.4431
OS: Microsoft Windows NT 10.0.17763.0, x64
Host Product: Microsoft Office x64
Host Version: 16.0.11126.20266
Host Executable: WINWORD.EXE

[Vogel612]

Not quite sure what you mean by "again". Could you point me to the closed issue where this was allegedly fixed? Thanks!

[ghost]

This was one of the items that popped up when I entered my title

https://github.com/rubberduck-vba/Rubberduck/issues/1191

[comintern]

I tried to replicate this with the following code...

Sub foo()
    Dim bar() As Variant
    Report "foo", bar
End Sub

Public Sub Report(ByVal this_formatter As String, ByRef this_items() As Variant)
    Debug.Print this_items(0)
End Sub

...and got no result. This is also literally the first thing that is checked in the inspection, so I'm a bit perplexed. If you select this_items in the code pane, does it give the correct information in the toolbar?

[ghost]

Change 'ByRef this_items()' to 'ByVal this_items()' as recommended by the Code Inspector. The line will turn red indicating that it is not compilable. The VBA error message is 'Compile error: Array argument must be byRef'

[comintern]

Right, but I can't replicate the inspection result when it's passed ByRef. The compiler error would be expected if it's ByVal, which is why we filter the results for arrays. Is it showing the correct information in the toolbar when it's declared ByRef?

[ghost]

Yes. I was just pointing out that the recommendation is incorrect.

The inspection report is

Suggestion | RegCMC | LoggerMemberInterface | Parameter 'this_items' can be passed by value. | 12 | 57

[bclothier]

@SteveLaycock we're still missing the crucial data - what does the toolbar, as Comintern pointed in his screenshot above says about that parameter? We need that to know if there's a discrepancy in how RD sees that parameter because it should not be even suggested in the first place.

[ghost]

Sorry, that wasn't clear. The toolbar reports exactly the same as shown above.

[comintern]

Do you have anything else in the project that is named this_items?

[ghost]

Yes. But this is the only one that is flagged

Summary below. I removed lines that were not procedure/variable declarations

Search "this_items" (27 hits in 6 files)
C:\Projects\Code\RubberDuckSnapshots\RegCMC 2018-09-04 rebuild\Repository\temp\LoggerMain.cls (11 hits)
Line 219: ParamArray this_items() As Variant
Line 232: Public Sub Info(ByVal this_location As LocationType, ByVal this_formatter As String, ParamArray this_items() As Variant)
Line 246: Public Sub Warn(ByVal this_location As LocationType, ByVal this_formatter As String, ParamArray this_items() As Variant)
Line 258: Public Sub Ditch(ByVal this_location As LocationType, ByVal this_formatter As String, ParamArray this_items() As Variant)
Line 309: Private Sub DoLogging(ByVal this_formatter As String, ByVal this_concern As String, ByRef this_items() As Variant)
C:\Projects\Code\RubberDuckSnapshots\RegCMC 2018-09-04 rebuild\Repository\temp\LoggerMemberInterface.cls (1 hit)
Line 21: Public Sub Report(ByVal this_formatter As String, ByRef this_items() As Variant)
C:\Projects\Code\RubberDuckSnapshots\RegCMC 2018-09-04 rebuild\Repository\temp\LoggerMemberToImmediate.cls (4 hits)
Line 68: Private Sub LoggerMemberInterface_Report(ByVal this_formatter As String, ByRef this_items() As Variant)
Line 75: Public Sub Report(ByVal this_formatter As String, ByRef this_items() As Variant)
C:\Projects\Code\RubberDuckSnapshots\RegCMC 2018-09-04 rebuild\Repository\temp\LoggerMemberToTextFile.cls (4 hits)
Line 34: Private Sub LoggerMemberInterface_Report(ByVal this_formatter As String, ByRef this_items() As Variant)
C:\Projects\Code\RubberDuckSnapshots\RegCMC 2018-09-04 rebuild\Repository\temp\Striing.bas (4 hits)
Line 388: Public Function AsCommaList(ParamArray this_items() As Variant) As String

[retailcoder]

If the resolver state label describes the parameter as an array, then RD knows the parameter is an array. As pointed out by @comintern, when a declaration is an array, it's explicitly excluded from results, literally the first thing being checked:

https://github.com/rubberduck-vba/Rubberduck/blob/64ae145a041062bb4f57381198d9999d3b13521b/Rubberduck.CodeAnalysis/Inspections/Concrete/ParameterCanBeByValInspection.cs#L55-L65

Seeing that no one can reproduce the problem and that the inspection results aren't consistent, closing as "no-repro". If the parameter wasn't correctly resolving as an array (note the () after the identifier name in the toolbar label), then the bug would be consistent and reproducible.

Thanks for reporting anyway!

[comintern]

Missed the clue from the toolbar that this is on an interface. Reopening.

[MDoerner]

MCVE:

Interface:

Public Sub Whatever(ByRef someArray() As Variant)
End Sub

Class:

Implements Interface

Private Sub Interface_Whatever(ByRef someArray() As Variant)
End Sub

The problem is that the part checking the special cases of interfaces and events, for which all implementing members must be able to pass by value in order to make this an option for the interface/event, does not check that the parameters are not arrays.