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 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
    • Patch Submitted:
      Yes
    • Number of attachments :
      2

      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

        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 ).
        Hide
        blackdrag blackdrag 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
        blackdrag blackdrag 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.

          People

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

            Dates

            • Created:
              Updated:
              Resolved: