groovy
  1. groovy
  2. GROOVY-5197

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
    • Number of attachments :
      1

      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

          Activity

          Hide
          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
          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
          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
          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
          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
          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
          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
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: