rubberduck-vba / Rubberduck

Every programmer needs a rubberduck. COM add-in for the VBA & VB6 IDE (VBE).
https://rubberduckvba.com
GNU General Public License v3.0
1.92k stars 301 forks source link

"Variable {x} is used but not assigned" finding variables in comments that follow an annotation #5091

Open daFreeMan opened 5 years ago

daFreeMan commented 5 years ago

Rubberduck version information Version 2.4.1.4848 OS: Microsoft Windows NT 10.0.15063.0, x64 Host Product: Microsoft Office 2016 x64 Host Version: 16.0.4873.1000 Host Executable: MSACCESS.EXE

Description A variable was added to a comment (at the end of an '@Ignore... and the code parsed - RD picked up the variable name from the comment and gave an inspection warning.

To Reproduce Steps to reproduce the behavior:

  1. Write some code with a variable declaration
  2. Add a comment with that variable name in it
  3. Parse
  4. Profit!!! See error

Expected behavior Commented text, especially after an inspection '@Ignore should not be picked up for inspection.

Screenshots

Logfile

Additional context I will have to figure out a way to attach screen shots & the log. It seems corporate has blocked uploads to imgur and github - I get a GH message stating "Something went really wrong, and we can't process that file. Try again."

Vogel612 commented 5 years ago

This is a reproduction of #3248 that was originally closed as norepro

daFreeMan commented 5 years ago

Actually it appears that additional text in the '@Ignore comment line causes the issue.

This code causes an inspection result:

  Dim noteElement As WebElement
  this.IsLoggedIn = IsLoginEnabled(noteElement)

  If Not this.IsLoggedIn Then
'@Ignore UnassignedVariableUsage -- RD can't do enough code path analysis yet to know that `noteElement` is assigned in `IsLoginEnabled`, therefore, the following @Ignore
    If InStr(1, noteElement.Text, "disabled") Then

Additionally, this code causes an inspection result (note the space in the commented variable name note Element):

  Dim noteElement As WebElement
  this.IsLoggedIn = IsLoginEnabled(noteElement)

  If Not this.IsLoggedIn Then
'@Ignore UnassignedVariableUsage -- RD can't do enough code path analysis yet to know that `note Element` is assigned in `IsLoginEnabled`, therefore, the following @Ignore
    If InStr(1, noteElement.Text, "disabled") Then

Even this code causes the error:

  Dim noteElement As WebElement
  this.IsLoggedIn = IsLoginEnabled(noteElement)

  If Not this.IsLoggedIn Then
'RD can't do enough code path analysis yet to know that `note Element` is assigned in `IsLoginEnabled`, therefore, the following @Ignore
'@Ignore UnassignedVariableUsage -- RD can't do enough
    If InStr(1, noteElement.Text, "disabled") Then

Likewise, this causes the error (note removal of ' from can't to cant)

  Dim noteElement As WebElement
  this.IsLoggedIn = IsLoginEnabled(noteElement)

  If Not this.IsLoggedIn Then
'RD can't do enough code path analysis yet to know that `note Element` is assigned in `IsLoginEnabled`, therefore, the following @Ignore
'@Ignore UnassignedVariableUsage -- RD can't do enough
    If InStr(1, noteElement.Text, "disabled") Then

Even removing the SQL comment -- didn't remove the error. The only thing that seems to allow RD to note the '@Ignore is removing all comments following it. Spaces, tabs, text - they all trip up the duck.

Vogel612 commented 5 years ago

Using : as a separator after intended annotation arguments works around this issue by explicitly delineating part of the comment as comment and not belonging to the annotation..

The following avoids the bug manifesting:

'@Ignore UnassignedVariableUsage :-- RD can't do enough
daFreeMan commented 5 years ago

I'd strongly suggest, then, that the : be added in by default when @Ignores are added. This will help eliminate future confusion and consternation on the end-user's part, since needing a colon here is entirely non-intuitive.

daFreeMan commented 5 years ago

I knew I wasn't imagining things!

      '@Ignore VariableNotAssigned            returned by PrepareExcelSourceData
      Dim Address As String

Works just fine! (and has been in my code for quite some time).

Note the lack of : after the inspection name, yet following comments and there's no failure of the parser to recognize and honor the '@Ignore here.

It appears that there's a difference between how the UnassignedVariableUsage and the VariableNotAssigned inspections are handling and honoring the @Ignore annotation...

MDoerner commented 5 years ago

Are you sure that you get a result in the above case after removing the annotation comment?

According to the grammar, and my tests, that annotation parses as a comment.

daFreeMan commented 5 years ago

After breaking the annotation syntax ' @ Ignore ... and parsing, I do NOT get a VariableNotAssigned inspection result. It appears that this particular false positive was fixed, therefore it's not parsing the annotation?

TBH, though, that is they way I originally added the comment (after RD inserted the annotation for me), and it's been processed like that for quite some time. (For some value of "quite" definitely > 4 months, probably < 12 months, but no guarantee of the exact time frame.)