Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Inconsistent false positive on 'passed ByVal and assigned a value' inspection #4030

Closed
daFreeMan opened this issue May 22, 2018 · 10 comments
Closed
Labels
bug Identifies work items for known bugs inspection-false-positive A bug where an inspection result appears, even though it is incorrect. resolver Issue is easier to resolve with knowledge of the internal resolver API and the Antlr4 parse trees. status-norepro The issue could not be reproduced. If there is a stable repro, please add that as a new comment.

Comments

@daFreeMan
Copy link
Contributor

Win10, Excel2016 (desktop), RD .3280

Public Sub foo(ByVal myRange As Range)
  myRange.Value = "Bar"
End Sub

Generates no inspection. However, my real code (snippet):

Public Sub SetSummaryCell(ByVal SummaryRange As Range, ByVal value As Variant, ByVal FormatFlag As String)

  SummaryRange.value = CStr(value)
'other stuff
End Sub

generates the inspection.

@MDoerner
Copy link
Contributor

MDoerner commented May 22, 2018

This is not inconsistent, but still wrong. In the code snippet, for some reason the inspection thinks that the member value refers to the parameter value, which gets passed by value. Since the potential MCVE does not have the parameter value it does not reproduce the issue.

@retailcoder
Copy link
Member

Hmm, so a resolver bug more than an inspection issue then?

@retailcoder
Copy link
Member

Could also be an artifact of how the older inspections kind of tried to do the resolver's job - this looks like it could be a case of the inspection not using the resolved member call; investigation is needed.

@Vogel612 Vogel612 added bug Identifies work items for known bugs inspection-false-positive A bug where an inspection result appears, even though it is incorrect. labels May 23, 2018
@MDoerner
Copy link
Contributor

It is the resolver. The inspection just looks for by value parameters with an assignment reference.

@MDoerner MDoerner added the resolver Issue is easier to resolve with knowledge of the internal resolver API and the Antlr4 parse trees. label May 23, 2018
@MDoerner
Copy link
Contributor

MDoerner commented May 23, 2018

The problem seems to be that member access resolution generally fails for members of parameters. More precisely, the MemberAccessDefaultBinding does not take into consideration that a lExpression with classification Variable might have the DeclarationType Parameter.
I will see whether that really is the cause when I am back home tonight.

@daFreeMan
Copy link
Contributor Author

I'm going to go out on a limb and say it is inconsistent. After noting the conversation, and especially the comment about the MCVE not having the Variant parameter, I made this:

Public Sub foo(ByVal myRange As Range, ByVal value As Variant)
  myRange.value = "Bar"
End Sub

and it does not generate the error.

More food for thought.

@MDoerner
Copy link
Contributor

Are you sure your code snippet triggers the inspection? I do not get the inspection result for the snippet.

@daFreeMan
Copy link
Contributor Author

The MCVE does not generate the error, but my real code most certainly does.

rd byval issue

Maybe this has been obvious to the two of you, but it just hit me. The inspection is legit - I'm passing ByVal SummaryRange As Range, then assigning to SummaryRange.Value, so I am assigning to a parameter passed by value.

However, the parameter is an object, so passing the pointer to the object ByVal really shouldn't be picked up by the inspection when I'm assigning to a property of said object, only if I'm trying Set SummaryRange = SomeOtherRange.

Right?

@retailcoder
Copy link
Member

@daFreeMan no, you're not assigning the SummaryRange reference, which is what the inspection means to be flagging. You're assigning a member of SummaryRange, i.e. SummaryRange.Value, which shouldn't trigger the inspection in any way. This is absolutely a false positive.

@MDoerner
Copy link
Contributor

Is there anything else in the method that could trigger the inspection? I cannot reproduce the issue with the snippet provided, i.e. the following code does not cause an inspection result for me, neither in a standard module nor in a class module.

Public Sub SetSummaryCell(ByVal SummaryRange As Range, ByVal value As Variant, ByVal FormatFlag As String)

  SummaryRange.value = CStr(value)
'other stuff
End Sub

@MDoerner MDoerner self-assigned this Aug 19, 2019
@MDoerner MDoerner removed their assignment Sep 9, 2019
@retailcoder retailcoder added the status-norepro The issue could not be reproduced. If there is a stable repro, please add that as a new comment. label Sep 9, 2019
Semi-automatic bug tracker automation moved this from ToDo to Done Sep 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Identifies work items for known bugs inspection-false-positive A bug where an inspection result appears, even though it is incorrect. resolver Issue is easier to resolve with knowledge of the internal resolver API and the Antlr4 parse trees. status-norepro The issue could not be reproduced. If there is a stable repro, please add that as a new comment.
Projects
Development

No branches or pull requests

4 participants