VariableNotUsedInspection: the false positives of v1.4, upcoming fixes of v2.0

One of my favorite features since we started working on this project, is the Code Inspections.

I like it because, well, it does find things.

The problem is that, sometimes, under specific circumstances, it makes false claims. Take this code for example:

Public Sub FormatChart(cht As ChartObject)
    Dim ax As Axis
    Set ax = cht.Axes(xlValue)
    ax.MajorGridlines.Border.Color = RGB(200, 200, 200)
    ax.MinorGridlines.Border.Color = RGB(230, 230, 230)
    ax.Crosses = xlAxisCrossesMinimum
End Sub

Here Rubberduck 1.4.3 would say “Variable ‘ax’ is never used” – and suggest a quick-fix to remove the supposedly unused declaration. A quick-fix which, of course, would break the code. Is there a bug in the VariableNotUsedInspection code?

Believe it or not, there isn’t. What makes the inspection fire up false positives, is a bug in the identifier reference resolver that causes member calls to ignore the “parent” reference.

Another common case, is caused by – again – the resolver treating For and For Each loops as assignments, but not as usages. So in code like this:

 Dim fYear As Integer
 Dim fQuarterOfYear As Integer
 Dim fMonthOfQuarter As Integer
 Dim fWeekOfMonth As Integer
 Dim fDayOfWeek As Integer
 
 For fYear = fStartYear To fStartYear + years - 1
     Set current = FiscalCalendarDate.Create(currentDate, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, fYear, vbNullString)
 
     For fQuarterOfYear = 1 To 4
 
         current.FiscalDayOfQuarter = 1
         current.FiscalWeekOfQuarter = 1
         current.FiscalMonthOfQuarter = 1
 
         For fMonthOfQuarter = 1 To 3
 
             current.FiscalDayOfMonth = 1
             current.FiscalWeekOfMonth = 1
 
             If IIf(IsLeapYear(current.calendarYear) And current.FiscalMonthOfYear = 12, True, fMonthOfQuarter Mod 2 = 0) Then
 
                 For fWeekOfMonth = 1 To 5
 ...

You get a “Variable ‘fWeekOfMonth’ is not used” and “Variable ‘fQuarterOfYear’ is not used”, because the only place they’re ever used is in For loops.

Then you have the forgotten edge cases:

Private WithEvents indicator As ProgressIndicator

WithEvents variables are likely to be assigned a reference, but not necessarily to be themselves referenced anywhere. And if they aren’t, well then, they’re reported as “never used”. Which is a problem, because you don’t want a quick-fix to go and delete the WithEvents declaration that provides all these events you’re handling. So we’re going to be ignoring WithEvents variables.

---

Okay… Got any Good news?

Totally. Ducky 2.0 code inspections are being completely revamped. And all these false positives are being addressed, which means…

…most inspections will support “Fix all” options. Of course one shouldn’t “fix all occurrences in project” without actually reviewing the inspection results. But so far, it’s looking very, very good. This UI is still preliminary: we’re happy to hear (and integrate) your feedback!

Resolver: still not perfect

I just found another bug in the rewritten 1.4.x resolver, and as much as I want to fix it (and I will)… part of me can’t help thinking if you encounter this bug, your code has greater problems than mine.

Picture this code:

Sub Foo()
     Dim Foo As New Foo
     With Foo
         With .Foo
             .Foo = 42
         End With 
         Bar .Foo.Foo
     End With
End Sub

Believe it or not, it compiles… and the 1.4.1 resolver works perfectly and correctly identifies who’s who and who’s calling what. Now what if we added a recursive call?

Sub Foo()
     Dim Foo As New Foo
     With Foo
         With .Foo
             .Foo = 42
         End With 
         Bar .Foo.Foo
     End With
     Foo
End Sub

The bug here, is that this Foo gets resolved as a call to the local variable, so renaming the procedure to DoSomething would leave that Foo unchanged.

Not too bad. I mean, nobody in their right minds would ever do that. (right?)

However, it gets worse: if we change Sub to Function, in the name of being able to identify a function’s return value, a matching identifier within the body of a function (or property getter) is resolved to the function (or getter) itself… so this part needs to be refined at bit.

Function Foo()
     Dim Foo As New Foo
     With Foo
         With .Foo
             .Foo = 42
         End With 
         Bar .Foo.Foo
     End With
     Foo
End Function

Every single Foo here, with the sole exception of the local declaration, resolves to a reference to the function.

I hope this bug doesn’t affect your code… for your sake – and that of whoever will be maintaining your code in the future.

If it does… v1.4.2 with certainly include a fix for this issue.

---

Update

The non-resolving recursive call isn’t a bug: