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

On Error statement in error path #3048

Open retailcoder opened 7 years ago

retailcoder commented 7 years ago

Consider this code:

For i = 0 To 20
    On Error GoTo Handler
    Debug.Print 42/0
Handler:
Next

The happy path overlaps the error-handling path, resulting in unexpected execution behavior. Rubberduck should be able to warn about that. Not sure if a quickfix is possible, but warning about it would already be nice.

retailcoder commented 7 years ago

A first version could be looking for procedures containing On Error GoTo statements that aren't On Error GoTo 0, that can execute in an error state. To do this we need to annotate block statements as we traverse them, flagging them as "HappyPath" or "ErrorPath", using a cumulative flag: if a procedure contains an instruction that has both flags, we have an inspection result.

We can identify the error path by picking up the On Error GoTo label; if that label is located inside a conditional or loop block, we're looking at a serious prospect; before we can determine that paths overlap in a wrong way, we just need to establish that there's no Err.Clear or Resume statement in that code path.

For example, a procedure with this code wouldn't be flagged, even if Next runs in both error paths:

For i = 0 To 20
    On Error GoTo Handler
    Debug.Print 42/0
Handler:
    Err.Clear
    On Error GoTo 0
Next

...or perhaps it should, but that would be a different kind of inspection result I guess. Key point being that On Error GoTo Handler never executes in an error state, which is "ok". Better code would untangle the error-handling code from the actual procedure's code, but that's more of a readability/maintainability issue, while the On Error executing in error state is definitely a code quality issue.