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

Early-bound named parameters not resolved? #3937

Closed
retailcoder opened this issue Apr 16, 2018 · 2 comments · Fixed by #5003
Closed

Early-bound named parameters not resolved? #3937

retailcoder opened this issue Apr 16, 2018 · 2 comments · Fixed by #5003
Labels
bug Identifies work items for known bugs difficulty-02-ducky Resolving these involves the internal API, but with relatively easy problems to solve. help wanted resolver Issue is easier to resolve with knowledge of the internal resolver API and the Antlr4 parse trees. up-for-grabs Use this label in conjunction with a difficulty level label, e.g. difficulty-02-ducky user-interface This issue explicitly relates to the visible interface of Rubberduck.

Comments

@retailcoder
Copy link
Member

We correctly resolve Excel.Range.Cells calls, for example:

foo = currentRow.Range.Cells(12).Value

However given named parameters:

foo = currentRow.Range.Cells(ColumnIndex:=12).Value

Selecting the ColumnIndex identifier doesn't update the status bar with the expected ColumnIndex (parameter: Long), which makes me wonder if we resolve named parameters.

We are resolving user code's named parameters (although it doesn't seem to work for recursive calls).

This isn't a high-priority bug at all, since it doesn't affect inspections or refactorings (perhaps it's exactly why we didn't bother with resolving those), however it does affect the context-sensitive reference count in the status bar, making it feel as though something isn't working as it should.

I haven't looked at the source, but it seems to me this could be a fairly easy thing to fix - likely the resolver is simply deliberately working off user declarations when resolving named parameters; if that's the case then the fix is just a matter of making it include the referenced declarations too.

@retailcoder retailcoder added bug Identifies work items for known bugs help wanted user-interface This issue explicitly relates to the visible interface of Rubberduck. up-for-grabs Use this label in conjunction with a difficulty level label, e.g. difficulty-02-ducky difficulty-02-ducky Resolving these involves the internal API, but with relatively easy problems to solve. resolver Issue is easier to resolve with knowledge of the internal resolver API and the Antlr4 parse trees. labels Apr 16, 2018
@retailcoder retailcoder changed the title Eaerly-bound named parameters not resolved? Early-bound named parameters not resolved? Apr 16, 2018
@bclothier
Copy link
Contributor

I've reviewed this and I think this is actually more complicated than it seems.

Consider those 3 cases:

Public Sub foo()
    Dim sht As Excel.Worksheet
    
    sht.Paste Link:=True
End Sub

Public Sub foo1()
    Sheet1.Paste Link:=True
End Sub

Public Sub foo2()
    Dim v As Variant
    Dim sht As Excel.Worksheet
    
    v = sht.Cells(ColumnIndex:=12)
End Sub

In the first case, it resolves correctly and shows as Parameter when selecting the Link

In the second case, it doesn't, showing the procedure. This might need to be fixed by TypeLib due to not knowing what Sheet1 is.

In the third case, it doesn't resolve because if you look very carefully, the .Cells() returns a Range object, and creating a ColumnIndex actually references the _Default default member of the Range object.
image

So I think it's more to do with the default member access than external library references.

bclothier added a commit to bclothier/Rubberduck that referenced this issue Apr 22, 2018
@bclothier
Copy link
Contributor

Note that the PR #3893 adds a failing unit test which is currently ignored; Identify_NamedParameter_Parameter_FromExcel_DefaultAccess that covers the third case. We also have a (currently passing) unit test for first case (Identify_NamedParameter_Parameter_FromExcel) to prove that external references are being resolved correctly. Originally I was going to fix this in the PR #3893 because it seemed similar enough but based on the finding above, I think a separate PR is best.

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 difficulty-02-ducky Resolving these involves the internal API, but with relatively easy problems to solve. help wanted resolver Issue is easier to resolve with knowledge of the internal resolver API and the Antlr4 parse trees. up-for-grabs Use this label in conjunction with a difficulty level label, e.g. difficulty-02-ducky user-interface This issue explicitly relates to the visible interface of Rubberduck.
Projects
Development

Successfully merging a pull request may close this issue.

2 participants