Skip to content
New issue

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

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

Already on GitHub? # to your account

False Positive AssignmentNotUsed inspection #5456

Closed
daFreeMan opened this issue Apr 3, 2020 · 3 comments · Fixed by #5502
Closed

False Positive AssignmentNotUsed inspection #5456

daFreeMan opened this issue Apr 3, 2020 · 3 comments · Fixed by #5502
Labels
bug Identifies work items for known bugs

Comments

@daFreeMan
Copy link
Contributor

Rubberduck version information

Version 2.5.0.5410
OS: Microsoft Windows NT 10.0.17134.0, x64
Host Product: Microsoft Office 2016 x64
Host Version: 16.0.4966.1000
Host Executable: EXCEL.EXE

Description
A variable assignment in an error handler is flagged as Not Used, even though that's the Function's "return" variable and it is used in cleaning up the return.

To Reproduce

Public Function Foo() as Long
  Dim Sum as Long
  On Error Goto ErrorHandler
  Sum = 1/0
CleanExit:
  Foo = Sum
  Exit Function
ErrorHandler:
  Sum = -1 
  Resume CleanExit
End Function

Expected behavior
The Sum = -1 assignment in the ErrorHandler should not be flagged as unused.

Screenshots
N/A

Logfile
RubberduckLog.txt

Additional context
This will probably require full CPA to fix.

@daFreeMan daFreeMan added the bug Identifies work items for known bugs label Apr 3, 2020
@MDoerner
Copy link
Contributor

MDoerner commented May 5, 2020

The problem here seems to be that the code path analysis does not account for jump statements like GoTo and Resume.

@BZngr
Copy link
Contributor

BZngr commented May 16, 2020

There can be a reduction in false positives due to jump statements by evaluating the referenced 'jump-to' labels. If there is a potential of usage in the code below the jump-to label, the unused-assignment-candidate can be removed from the inspection results.

In the same way that ProcedureNotUsedInspection filters all Public Standard Module procedures from inspection results to avoid false positives - the above approach may avoid some false positives at the potential cost of missing some actual unused assignments. (Granted: the filter applied to ProcedureNotUsedInspection is a more significant 'win')

I have a local branch that accomplishes this. It evaluates Resume and GoTo statements that reference labels or linenumbers. Not included in the evaluation are On...GoTo statements.

I'm commenting here to get some feedback as to whether there is interest in reducing the false positives using this approach before creating a PR.

@daFreeMan
Copy link
Contributor Author

(Just noticed your comment.)

I'm all for it.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
bug Identifies work items for known bugs
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants