Source location incorrect for statement after a label

Details

  • Type: Bug Bug
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Fixed
  • Affects Version/s: 1.8.4
  • Fix Version/s: 1.8.6, 2.0-beta-3
  • Component/s: parser-antlr
  • Labels:
    None

Description

Take this code and put it into the groovy console:

def meth() {
  label:
    assert i == 9
}

Now inspect the AST and navigate to the assert statement. You will see that the assert statement has a lineNumber and lastLineNumber are 2, but it should be 3. This is incorrect. It doesn't seem like this affects Groovy itself too much, but this is causing an exception in Groovy-Eclipse. See GRECLIPSE-1270.

The fix is simple enough.

Issue Links

is related to

Bug - A problem which impairs or prevents the functions of the product. GRECLIPSE-1270 IndexOutOfBoundsException with assert keyword in Spock test

  • Minor - Minor loss of function, or other problem where easy workaround is present.
  • Resolved - A resolution has been taken, and it is awaiting verification by reporter. From here issues are either reopened, or are closed.

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Andrew Eisenberg added a comment - - edited

Fix: in org.codehaus.groovy.antlr.AntlrParserPlugin.statement(AST) at the end of the method, change:

if (statement != null) {
    configureAST(statement, node);
}

to:

if (statement != null && type != LABELED_STAT) {
    configureAST(statement, node);
}

This problem is coming about because the source location of statement is being set twice: once from within the call to labeledStatment using the correct source location and once again using the source location for just the label in the call to statement.

This leads me to question if explicitly calling configureAST is necessary at all in the statement method since all calls to create the different kinds of statements already call configureAST.

Show
Andrew Eisenberg added a comment - - edited Fix: in org.codehaus.groovy.antlr.AntlrParserPlugin.statement(AST) at the end of the method, change: 
if (statement != null) {
    configureAST(statement, node);
}
to: 
if (statement != null && type != LABELED_STAT) {
    configureAST(statement, node);
}
This problem is coming about because the source location of statement is being set twice: once from within the call to labeledStatment using the correct source location and once again using the source location for just the label in the call to statement. This leads me to question if explicitly calling configureAST is necessary at all in the statement method since all calls to create the different kinds of statements already call configureAST.
Hide
Permalink
Paul King added a comment -

Proposed patch - a slight variation of what Andrew proposed. The code for the statement method is a little strange in that despite each branch of the switch deriving the respective source positioning depending on the particular branch chosen, it then overrides that information with the information for the entire node. The more refined info is useful in other contexts.

Show
Paul King added a comment - Proposed patch - a slight variation of what Andrew proposed. The code for the statement method is a little strange in that despite each branch of the switch deriving the respective source positioning depending on the particular branch chosen, it then overrides that information with the information for the entire node. The more refined info is useful in other contexts.
Hide
Permalink
blackdrag blackdrag added a comment -

I guess the patch is ok then

Show
blackdrag blackdrag added a comment - I guess the patch is ok then
Hide
Permalink
Paul King added a comment -

Well, it is more of a philosophical decision I think. At the statement level, we tend to keep track of the whitespace around the statement. Here we would not be doing this. There are no LineColumn checks that test this area. There is only a very minimal SourcePrinter test. I will do another sanity check with some more elaborate SourcePrinter tests before applying the patch. The alternative is that we need to document that any statement may contain a label and it would be up to other tools, e.g. CodeNarc etc. to step around the label. We could provide a utility to make it easier I guess.

Show
Paul King added a comment - Well, it is more of a philosophical decision I think. At the statement level, we tend to keep track of the whitespace around the statement. Here we would not be doing this. There are no LineColumn checks that test this area. There is only a very minimal SourcePrinter test. I will do another sanity check with some more elaborate SourcePrinter tests before applying the patch. The alternative is that we need to document that any statement may contain a label and it would be up to other tools, e.g. CodeNarc etc. to step around the label. We could provide a utility to make it easier I guess.
Hide
Permalink
Andrew Eisenberg added a comment -

From the tooling perspective, including whitespace in AST nodes is problematic (specifically in expression nodes and too a lesser extent in statement nodes). There are many situations where trailing whitespace must be worked around.

When there is an easy fix, I patch the groovy compiler and try to send you guys a patch, but often I need some hairy workaround code in Groovy-Eclipse.

Is there a reason why many AST nodes keep trailing whitespace? Or is this largely legacy? (From my investigations, it seems to be built into the parser.)

Show
Andrew Eisenberg added a comment - From the tooling perspective, including whitespace in AST nodes is problematic (specifically in expression nodes and too a lesser extent in statement nodes). There are many situations where trailing whitespace must be worked around. When there is an easy fix, I patch the groovy compiler and try to send you guys a patch, but often I need some hairy workaround code in Groovy-Eclipse. Is there a reason why many AST nodes keep trailing whitespace? Or is this largely legacy? (From my investigations, it seems to be built into the parser.)
Hide
Permalink
blackdrag blackdrag added a comment -

I think it is mainly because it is not easy to tell the grammar to not to include the whitespace and since we didn't really need that information to a level where trailing whitespace matters, we simply didn't put in the energy to prevent that from happening.

Show
blackdrag blackdrag added a comment - I think it is mainly because it is not easy to tell the grammar to not to include the whitespace and since we didn't really need that information to a level where trailing whitespace matters, we simply didn't put in the energy to prevent that from happening.
Hide
Permalink
Paul King added a comment -

Patch applied - I tried playing around with different source texts to confuse the SourcePrinterTests but could't spot any obvious breakages.

Show
Paul King added a comment - Patch applied - I tried playing around with different source texts to confuse the SourcePrinterTests but could't spot any obvious breakages.

People

  • Assignee:
    Paul King
    Reporter:
    Andrew Eisenberg
Vote (0)
Watch (1)

Dates

  • Created:
    Updated:
    Resolved: