Rubberduck: Flag 'For' loop counter tampering in loop body

[retailcoder]

What
Let's have a code quality inspection that locates instances of a For loop variable being explicitly tampered with inside the loop body.

Why
A For loop that modifies its counter variable is poor practice and makes execution harder to follow and the code harder to debug when problems inevitably happen.

Example
This code should trigger the inspection:

Public Sub DoSomething()
    Dim i As Long
    For i = 1 To 3
        Debug.Print i
        i = i + 1
    Next
End Sub

Output is 1 on first iteration, 3 on the second; i is 5 after the loop.

---

QuickFixes

1. Specify a 'Step' increment
   When the in-body increment is unconditional, this fix should be applicable: we take the sum of manual unconditional increment(s), add +1, and that's the Step increment.

   Example code, after quickfix is applied:

   Public Sub DoSomething()
       Dim i As Long
       For i = 1 To 3 Step 2
           Debug.Print i
       Next
   End Sub
   

   Output is 1 on first iteration, then 3 on the second; i is still 5 after the loop.

If the tampering is conditional (even only in part), we can't really offer a quickfix (or can we?), but we can still warn and recommend restructuring that loop.

---

Resources

• InspectionNames: 'For' loop counter is being tampered with.
• InspectionInfo: Avoid tampering with a 'For' loop variable inside the body of that loop: it makes the code harder to follow, and can easily introduce bugs in the logic. If the assignment is unconditional, consider specifying a 'Step' increment. If the assignment is conditional, consider restructuring the flow control to avoid needing to modify the loop variable.
• InspectionResults: 'For' loop variable '{0}' is explicitly assigned in the loop body.

[bclothier]

An alternate quickfix that might cover the conditional modification is to convert from For...Next to a Do...Loop.

[Greedquest]

If this inspection is looking for 'tampering', not just incrementing (conditional or otherwise), then really you need to look out for any assignment to i, such as byRef in a sub or the return value of a function.

That makes a quick fix hard, as you need to trace the full assignment path and determine whether it resolves to a simple increment that can be replaced with a step, or a more complex expression with side effects - probably out of scope?

Refactoring the assignment line to include the loop step, and then swapping to a do loop as suggested would be a general fix though I think

[retailcoder]

@Greedquest good points. I was thinking of enabling the simple quickfix for the simplest scenarios, and just warn about the (potential) tampering otherwise (i.e. no quickfix for the more complex cases, at least for a first round).