Groovy
  1. Groovy
  2. GROOVY-2767

Preserve line/column information for user-defined occurrences of variables this,super and literals null,true,false

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.6-beta-1
    • Fix Version/s: 1.6-beta-1
    • Component/s: ast builder
    • Labels:
      None
    • Flags:
      Patch

      Description

      Currently, user-defined occurrences of variables this, super and literals null, true, false are represented as singletons in the AST (VariableExpression.THIS_EXPRESSION, ConstantExpression.NULL, etc.). This means that their line/column information is lost, causing troubles for tools relying on that information (IDEs, code analyzers, transformations, etc.). It isn't even easy to detect that line/column information is missing for these expressions, as they will return "arbitrary" values.
      Attached is a patch that changes AntlrParserPlugin so that it creates a new instance of VariableExpression/ConstantExpression for every user-defined occurrence of the variables/literals mentioned above, thus preserving line/column information. The singletons can still be used for internal purposes (like representing an implicit this in a method call); however, in order to test for the presence of one of the expressions, it is now necessary to call one of the new methods isXXXExpression (e.g. myVar.isThisExpression()) instead of comparing against the singleton (e.g. myVar == VariableExpression.THIS_EXPRESSION). This keeps changes to the code base to a minimum while avoiding the problem of defining equality of ASTNode's by way of overriding equals(). All affected code in groovy-core has been adapted accordingly.
      Groovy-core builds successfully (ant install) after applying the patch on my local machine. Initial manual tests indicate that the patch works as intended. Of course an automated test would be preferable - is there already an easy way to get to an expression's AST node from a unit test? If not, I might write a transformation that allows to capture a declaration's AST node in a certain compiler phase.
      Please have a look at the patch and give me some feedback. Thanks!

        Activity

        Peter Niederwieser created issue -
        Guillaume Delcroix made changes -
        Field Original Value New Value
        Assignee Guillaume Laforge [ guillaume ] Jochen Theodorou [ blackdrag ]
        Jochen Theodorou made changes -
        Fix Version/s 1.6-beta-1 [ 14008 ]
        Hide
        Peter Niederwieser added a comment -

        (Copied from groovy-dev)

        Some time ago I wrote a helper class for testing AST-related code. Now I have produced a polished version called AstInspector. A quick example:

        def inspector = new AstInspector()
        inspector.compilePhase = CompilePhase.SEMANTIC_ANALYSIS
        inspector.load("def foo()

        { label: println 'hi!' }

        ")
        def expr = inspector.getExpression("label")
        assert expr instanceof MethodCallExpression

        With the help of AstInspector I have written JUnit tests for:

        • handling of annotation retention by the compiler (used to be broken in 1.6 trunk but works fine now)
        • my patch for GROOVY-2767

        I think this might be useful for others, so if you are interested, I'd be glad to donate the code (I have attached it to GROOVY-2767).

        Show
        Peter Niederwieser added a comment - (Copied from groovy-dev) Some time ago I wrote a helper class for testing AST-related code. Now I have produced a polished version called AstInspector. A quick example: def inspector = new AstInspector() inspector.compilePhase = CompilePhase.SEMANTIC_ANALYSIS inspector.load("def foo() { label: println 'hi!' } ") def expr = inspector.getExpression("label") assert expr instanceof MethodCallExpression With the help of AstInspector I have written JUnit tests for: handling of annotation retention by the compiler (used to be broken in 1.6 trunk but works fine now) my patch for GROOVY-2767 I think this might be useful for others, so if you are interested, I'd be glad to donate the code (I have attached it to GROOVY-2767 ).
        pnw made changes -
        Attachment inspector.zip [ 34185 ]
        Hide
        Jochen Theodorou added a comment -

        I applied your patch to 1.6-beta1, so I am closing this for now. You said you may want to provide some tests based on your code that we can use for our build. If you do so I suggest you open a new issue and attach the code there.

        Show
        Jochen Theodorou added a comment - I applied your patch to 1.6-beta1, so I am closing this for now. You said you may want to provide some tests based on your code that we can use for our build. If you do so I suggest you open a new issue and attach the code there.
        Jochen Theodorou made changes -
        Resolution Fixed [ 1 ]
        Status Open [ 1 ] Closed [ 6 ]
        Mark Thomas made changes -
        Project Import Sun Apr 05 13:32:57 UTC 2015 [ 1428240777691 ]
        Mark Thomas made changes -
        Workflow jira [ 12731791 ] Default workflow, editable Closed status [ 12743723 ]
        Mark Thomas made changes -
        Flags Patch [ 10430 ]
        Patch Submitted Yes [ 10763 ]
        Mark Thomas made changes -
        Project Import Mon Apr 06 02:11:23 UTC 2015 [ 1428286283443 ]
        Mark Thomas made changes -
        Workflow jira [ 12967793 ] Default workflow, editable Closed status [ 12975561 ]
        Transition Time In Source Status Execution Times Last Executer Last Execution Date
        Open Open Closed Closed
        11d 12h 4m 1 Jochen Theodorou 02/May/08 07:14

          People

          • Assignee:
            Jochen Theodorou
            Reporter:
            Peter Niederwieser
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development