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.91k stars 300 forks source link

Parser Error reported on certain For Next loop structures #4066

Closed chrisneilsen closed 6 years ago

chrisneilsen commented 6 years ago

Write a nested For Next in the form

For x = a To b
For y = c To d
    ' ...
Next y, x

Or

For x = a To b
' some code
For y = c To d
    ' ...
Next y, x

Both of which are valid syntax

throws a Parser Error on the Next y, x

extraneous input ',' expecting {, ':', REM, NEWLINE, ''', WS, LINE_CONTINUATION}

RubberduckLog.txt

Version 2.2.6672.28001 OS: Microsoft Windows NT 10.0.17134.0, x64 Host Product: Microsoft Office 2016 x86 Host Version: 16.0.9330.2087 Host Executable: EXCEL.EXE

retailcoder commented 6 years ago

Not sure how to tweak the ForNextStmt parser rule to handle this arguably demonic construct, without making the Next token optional...

Nice catch!

chrisneilsen commented 6 years ago

It gets worse: this also works (though why you would ever do that I don't know)

For x1 = a To b
   aa = 1
For x2 = c To d
   aa = 1
For x3 = e To f
   aa = 1
For x4 = g To h
   aa = 1
For x5 = i To i
   aa = 1
For x6 = k To l
   aa = 1
For x7 = m To n
   aa = 1
For x8 = o To p
   aa = 1
For x9 = q To r
   aa = 1
   For bb = 1 To 10
   For cc = 1 To 10
   Next cc, bb
For x10 = s To t
    Debug.Print x1, x2, x3, x4, x5, x6, x7, x8, x9, x10
Next x10, x9, x8, x7, x6, x5, x4, x3, x2, x1
chrisneilsen commented 6 years ago

One thought on handling this: the parser requires compilable code, right? Then when the Parser hits Next ... , ... it must be syntactically valid to have compiled. So just throw a warning: "arguably demonic For Next construct"

retailcoder commented 6 years ago

Technically the parser assumes compilable code - truth is, the grammar is rather robust and we only use to "ok screw it we're not writing a compiler" as a last resort =)

If/when this construct parses, we'll definitely implement an inspection+quickfix combo to cure this!

retailcoder commented 6 years ago

@chrisneilsen the thing is, we can't just dismiss the parser exception here: firstly because any parse exception is configured to halt that module's parse task and report an error state, but also and more importantly because we need these identifiers to be tokenized properly, so that doing refactor/rename on aa picks up these references as well. If we don't tokenize them, they won't exist for the rest of RD either. So.. It's an edge case indeed, but as a tool that wants to be there for folks that inherit terrible VBA code (and help them bring it into shape), we can't let our users down on such an evil syntax! ...I wonder how many other tools this Next statement breaks...

Vogel612 commented 6 years ago

The current forNextStmt looks as follows:

// expression EQ expression refactored to expression to allow SLL
forNextStmt : 
    FOR whiteSpace expression whiteSpace TO whiteSpace expression stepStmt? whiteSpace* endOfStatement 
    block
    statementLabelDefinition? whiteSpace? NEXT (whiteSpace expression)?
; 

The problem here is that the next statement can be absorbed into an "inner" next. To emulate this, one could try to replace the last line with:

(statementLabelDefinition? whiteSpace? NEXT (whiteSpace expression)? | COMMA whiteSpace expression)

This should work, because expression does not include a statementSeparator. I'm pretty sure that this messes with how we deal with the NEXT expression in any inspections involving the forNextStmt.

MDoerner commented 6 years ago

I will try whether that works, but insert a whiteSpace? in front of the COMMA. It certainly looks cleaner than what I had thought of.

Moreover, I will do the same for ForEachStmt.

bclothier commented 6 years ago

Just for record, this compiles and runs. :(

Public Sub ThisIsHorrid()
    Dim x As Long
    Dim y As Long
    Dim i As Variant
    Dim c As VBA.Collection

    Set c = New VBA.Collection
    c.Add 1
    c.Add 2

    For x = 0 To 3
        For Each i In c
            For y = 0 To 1
        Debug.Print "iterated"
    Next y, i, x

End Sub
retailcoder commented 6 years ago

From chat conversation: this Next statement from Hell is also messing with the indenter. Indentation logic for Next statements needs to count the number of Next statements on the current line (excluding comments), then count the number of commas in each Next statement - then decrease the current indent level by the number of Next statements + the number of commas in them.