Collection of false positives for inspection: "Assignment is not used"

[Inarion]

I recently upgraded from 2.3.1.4441 to 2.4.0.4488 and I'm reasonably confident that these false positives haven't been showing up before the upgrade.

For reference, the inspection tells me: An assignment is immediately overridden by another assignment or is never used.
I hope upon inspecting the following procedures you will agree with me that the above assessment is objectively false. Also please disregard their mostly nonsensical nature - I tried to condense them as far as possible. And please don't try to actually execute them. ;)

Option Explicit

Public Function IssueInIfBlock(ByVal someInput As Boolean) As Boolean

    If someInput Then
        Dim HasProblems As Boolean
        HasProblems = True ' triggers "Assignment is not used" inspection
    End If

    IssueInIfBlock = someInput And HasProblems

End Function

Public Function IssueInSelectStatement(ByVal someInput As Long) As Boolean

    Select Case someInput
        Case True
            Dim HasProblems As Boolean
            HasProblems = True ' triggers "Assignment is not used" inspection
    End Select

    IssueInSelectStatement = someInput And HasProblems

End Function

Public Function IssueWithStaticVariable() As Boolean

    Static SomeProperty As String
    If SomeProperty = "dummy" Then
        IssueWithStaticVariable = True
    End If
    SomeProperty = "new dynamic value" ' triggers "Assignment is not used" inspection

End Function

Public Sub IssueInWithBlock()

    Dim SomeObject As Object
    With SomeObject
        Dim HasProblems As Boolean
        HasProblems = SomeObject.SomeProperty ' triggers "Assignment is not used" inspection
    End With
    Debug.Print HasProblems

End Sub

Public Sub IssueInDoLoop()

    Dim Idx As Long
    Idx = 1
    Do
        Dim SomeBreakCondition As Boolean
        SomeBreakCondition = True ' triggers "Assignment is not used" inspection
        Idx = Idx + 1
    Loop Until SomeBreakCondition

End Sub

Edit: As detailed in the comments, moving the Dim statements before the blocks where their variables appear in will make the inspection disappear. Only the inspection for IssueWithStaticVariable will remain.

[bclothier]

While I'm not sure if that's the intention of the current inspection, I think this definitely should be a different altogether inspection. Something like InconsistentDimScopingInspection -- the main problem is that the Dimmed variables are apparently used outside the blocks where the statements are. For that reason, dimming them inside a block doesn't really make sense.

This inspection is necessary because otherwise it hides a subtle "feature" of VBA -- the Dim statement is "global" to the entire procedure, even if you make it the very last statement of the procedure (though you'll get a compile error if you try to use the variable before its Dim statement).

If this is made into its own inspection, the Move Closer to Usage QF may also need to be updated, if necessary.

[Inarion]

Alright, so I'm totally aware that there's no thing like different scopes within a procedure (With blocks may be an exception). The only reason I put the Dims in those blocks is because it seemed the most logical place. (And some of them might even be the result of the Move Closer to Usage QF.)

But what about the IssueWithStaticVariable procedure? Is this a similar case? If so, how?

Edit: And of course, @bclothier is totally correct. Moving the Dim outside of the blocks will make the inspection disappear. Not completely understandable from my POV: If VBA doesn't care about the position of a Dim within a procedure - should RD do so?

[Vogel612]

It seems like the code analysis does not take Static locals into account. I'd assume this specific case never worked, though...

Note that the inspection is not about Dim statements, but about assignments. It seems that the code path analysis incorrectly scopes Dim statements to their direct parent node instead of propagating them up the "scope tree".

[bclothier]

I'm pretty sure IssueWithStaticVariable is a separate issue (and a legitimate one at that); it shouldn't have had fired any results.

As for inspection results disappearing when the Dim statement has been moved outside the block, this is more of a readability issue and a benefit for the programmer, not for the compiler. Seeing it inside a block makes me think it should be confined to only that block. So I want to be warned if I'm misusing a variable outside the block. One way we can do that is to adjust the position of the Dim statement, which as you correctly note makes absolutely no difference to the compiler.

[Inarion]

Seeing it inside a block makes me think it should be confined to only that block. So I want to be warned if I'm misusing a variable outside the block.

While I can follow your reasoning, I don't think it's globally applicable. Can I assume your primary coding background is in a language that offers such different scopes? As someone who basically learned coding in/from VBA, I wouldn't have called that misuse. (What, you can tell that from my MCVE? cough) I can see the merit in handling it more strictly, though.

[bclothier]

Indeed - VBA is in the minority when it comes to scoping resolution. Several languages normally scope the declarations to the same block it is declared and consider them invalid outside the block.

Even so, I started with VBA long before I learnt C# so I have been accustomed to the famous "wall of the declarations" that is very commonplace throughout VBA. Move closer to usage is one way to fix it but to put it at very first assignment when they are used later somewhere in the procedure feels funny to me. In fact, I think that might have been the primary motivation for the "wall of declaration", to emphasize that the variables are global to the procedure, regardless of whether you assigned or used them deep inside a block or not.

Keep in mind that is not the only refactoring - the other refactoring is to break up the method (that's where Extract Method refactoring would come in) into smaller methods. Often, in VBA-land, the habit is to make one huge procedure that does everything, and thus need 100 variables. In actuality, it shouldn't have been written that way to start with it. It might need to be 10 methods, or a class module or something. But breaking up also has the side effect of dramatically cutting on the "wall of the declarations" and for small enough methods, it might still suffice.

The assignment within a block is more of a special case, though. Consider for example:

'Assume we got rs As DAO.Recordset somehow....
If Not rs.EOF Then
  Dim x As Long
  x = rs.Fields("x").Value
End If

Debug.Print x 'What's the real value?

This has two subtle problems. We might be able to compile, and run and it works! Until the rs.Fields("x").Value contains a Null. We can't assign that to a Long variable... Now, obviously, we can fix this by changing the data type to Variant. But then when the recordset is empty, what is the actual value? It would be Empty, but Variant/Empty has funny properties where Empty = 0 and Empty = "" and IsNull(Empty) = False. That makes it very hard to tell whether we actually have any value.

Therefore a comprehensive fix might to be do something like:

'Assume we got rs As DAO.Recordset somehow....
If Not rs.EOF Then
  If Not IsNull(rs.Fields("x").Value) Then
    Dim x As Long
    x = rs.Fields("x").Value
    Debug.Print x
  Else
    Debug.Print "there was a record, but x was null"
  End If
Else
  Debug.Print "there was no records"
End If

This covers all possibilities and avoid providing incorrect data. But in practice, we don't always need all possible branches. I would count RD to help warn me about such cases so I can adjust accordingly. For example, maybe it's acceptable to have 0 as the default value for all other cases, so in this case:

'Assume we got rs As DAO.Recordset somehow....
Dim x As Long
If Not rs.EOF Then
    x = Nz(rs.Fields("x").Value, 0)
Else
   x = 0
End If

Debug.Print x

Note that the Else is technically redundant and can be omitted safely since the default value for the Long variable is 0, so we can just eliminate it and still get the same behavior as before.

So that's why I think we do need a separate inspections to warn about conditional assignments. If you don't care about that, you could set it to Do Not Show, of course.

[comintern]

Just tossing my $.02 in here, the Assignment is not used inspection should not care about the location of the Dim within the procedure at all. It is inspecting for a superfluous assignment and nothing more.

Re another inspection for where the Dim is placed within the module, if we want to inspect that, IMO it should be opened as another issue and tagged with enhancement. For that one, I'd suggest something more along the lines of "Variable used outside of implied scope", at a hint level severity with a description wording leaning toward the coding style instead of the compile time behavior.

I've personally gotten in the habit of declaring the variable within the block that it is intended to be used in so that using it outside of that block makes the usage look out of place (call it "Hungarian Location"). In my mind, variables that are assigned within loops in particular can be prone to coding mistakes if there is the assumption that scope is confined to block.

Whatever the behavior of the inspection or quickfix is, I'd suggest using the terminology "Implied scope" to describe it.

[retailcoder]

I'd suggest using the terminology "Implied scope" to describe it.

..but I liked "Hungarian Location" :rofl:

I would add that declaring inside the "implied scope" makes it much easier to later refactor and extract a method manually.

[Inarion]

So, to summarize:
All of the presented cases in the OP are in need of some smartening-up? And examples 1, 2, 4 and 5 warrant a new inspection about Hungarian Location, err.. Implied Scope.

Edit:

I would add that declaring inside the "implied scope" makes it much easier to later refactor and extract a method manually.

Agreed. Also, I've started to notice a certain smell when the same VariableThatIFoundHardToNameCorrectly appeared again in later sections of the code: There always seems to be a case for extracting parts of that procedure. (Probably what @bclothier meant with funny above.)

[retailcoder]

@Inarion TBH it looks like the inspection is somehow mistakenly tracking declarations instead of assignments; the reported false positives are indeed false positives - the goal behind this inspection is to warn about cases such as these:

'If bar Then '' conditional block makes no difference
    foo = 42 ' assignment not used
'End If
foo = 74

In other words, it means to identify instructions where a variable is assigned, and then assigned again before the value is read, in at least one code path.

That said this should also trigger the inspection:

Dim foo As Long ' variable is local
foo = 42
DoSomething bar
'the value foo is never read after it's assigned. If foo was global, trip the inspection if value isn't read anywhere.

[Inarion]

@retailcoder That was my understanding as well and what led me to opening this issue. :)

(That tangent on general scoping was interesting and educational, though.)

[bclothier]

@retailcoder but conditional block should be considered for this case, no?

foo = 74
If bar Then
  foo = 42
End If

Granted, this might be a case where it can be better rewritten as:

If bar Then
  foo = 42
Else
  foo = 74
End If

but the assignment before the conditional block is a common pattern for ensuring there's a sane value given in case where conditional block don't apply or something like that.

[retailcoder]

@bclothier why though? Whether the assignment is conditional or not makes no difference - this is different though:

foo = 72 'assignment is used in at least 1 execution path
If bar Then
    Debug.Print foo 
End If

IMO the above case shouldn't trip the inspection though. Or should it?

[bclothier]

The difference is that in all cases, the foo has 72 assigned, which may be used.

In the earlier example, we had

foo = 72
foo = 42

which would flag a result for lack of use of the first assignment. However,

If bar Then
  foo = 42
Else
  foo = 72
End If

shouldn't raise any result, even though there are potentially unused assignment depending on the value of bar. This can be transformed into this:

foo = 72
If bar Then
  foo = 42
End If

which is still semantically the same. If you agree the 2nd example is a non-result, then this should be, too, no?

[MDoerner]

@retailcoder I think that the definition you gave of what to report is slightly off, too. I think the following would make more sense.

It should report all assignments in any code path such that in all subsequent code paths the variable is either reassigned before use or never used at all.

[ChrisBrackett]

I'm getting what appears to be a false positive inspection result for the following code.

Sub TEST_ASSIGNMENT_NOT_USED()
    Dim temp As String
    temp = "Assignment Not Used"
    temp = Replace$(temp, "a", vbNullString)
    temp = Replace$(temp, "e", vbNullString)
    temp = Replace$(temp, "i", vbNullString)
    temp = Replace$(temp, "o", vbNullString)
    temp = Replace$(temp, "u", vbNullString)
End Sub

Does this fall under the same issue, or this this a separate issue (or am I misunderstanding the inspection)?

While it's true that the variable is being immediately overridden by another assignment, that other assignment is dependent on referencing it's last know value. I did not expect this in the inspection results.

Version 2.4.0.4578
OS: Microsoft Windows NT 10.0.17763.0, x64
Host Product: Microsoft Outlook x64
Host Version: 16.0.4810.1000
Host Executable: OUTLOOK.EXE

[MDoerner]

I am a bit ambivalent about whether this should be a new issue. On one hand, the problem is a different one than the two in this issue. On the other hand, this issue already covers multiple problems with the inspection.

Anyway, this inspection is not well thought threw and seriously undertested. For the next release we should either fix it properly with a lot more test cases, especially for situations where no results should be returned, or deactivate it for the time being.

[daFreeMan]

Just stumbled across this as another example of a false positive:

ExitHere:
  YtdClinicSatScore = Round(Score, 2)
  Exit Function

ErrHandle:
  Score = -1
  Resume ExitHere

End Function

Score = -1 is flagged in build .4666.

[retailcoder]

Current state of the PR, pasting the code in the OP in a new module produces exactly 1 result....

Public Sub IssueInDoLoop()

    Dim Idx As Long
    Idx = 1
    Do
        Dim SomeBreakCondition As Boolean
        SomeBreakCondition = True
        Idx = Idx + 1 '<~ this is now marked as unused
    Loop Until SomeBreakCondition

End Sub

All but the last Idx assignment are used, and SomeBreakCondition = True assignment isn't flagged; I deem these inspection results correct.

Note that Static locals aren't handled at all - they're just ignored until we further enhance the inspection to handle them.

@CHR-IS-B with the current inspection, only the last assignment to temp is flagged, ...and it's legit ;-)

@daFreeMan the walker is currently not handling GoTo, GoSub/Return, and Resume jumps, but that's definitely a note worth taking.

[MDoerner]

@retailcoder What became of the mentioned PR?

[retailcoder]

Buried under dayjob workload...