Groovy
  1. Groovy
  2. GROOVY-3481

Groovy Support for annotations on local variable declarations

    Details

    • Type: New Feature New Feature
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.7-beta-1
    • Fix Version/s: 1.7.0
    • Component/s: ast builder
    • Labels:
      None
    • Environment:
      All

      Description

      Groovy should support annotations on local variable declarations. It is syntactically legal to annotate a local variable, but the AST produced does not carry that annotation.

      My use case with the AST builder. Either we'd like to annotate a local variable, like this:

      @AstSource(CompilePhase.CONVERSION)
      def source = { println "compiled on: ${new Date()}" }
      

      Or annotate a property within a closure (which is a DeclarationExpression), like this:

      def result = new AstBuilder().build {
                 phase = CompilePhase.CONVERSION
                 @AstSource
                 source = { println "compiled on: ${new Date()}" }
       }
      

      A getAnnotations() method should probably be added to DeclarationExpression to support this.

        Issue Links

          Activity

          Hamlet D'Arcy created issue -
          Paul King made changes -
          Field Original Value New Value
          Description Groovy should support annotations on local variable declarations. It is syntactically legal to annotate a local variable, but the AST produced does not carry that annotation.

          My use case with the AST builder. Either we'd like to annotate a local variable, like this:
          @AstSource(CompilePhase.CONVERSION)
          def source = { println "compiled on: ${new Date()}" }

          Or annotate a property within a closure (which is a DeclarationExpression), like this:

          def result = new AstBuilder().build {
                     phase = CompilePhase.CONVERSION
                     @AstSource
                     source = { println "compiled on: ${new Date()}" }
           }

          A getAnnotations() method should probably be added to DeclarationExpression to support this.
          Groovy should support annotations on local variable declarations. It is syntactically legal to annotate a local variable, but the AST produced does not carry that annotation.

          My use case with the AST builder. Either we'd like to annotate a local variable, like this:
          {code}
          @AstSource(CompilePhase.CONVERSION)
          def source = { println "compiled on: ${new Date()}" }
          {code}

          Or annotate a property within a closure (which is a {{DeclarationExpression}}), like this:

          {code}
          def result = new AstBuilder().build {
                     phase = CompilePhase.CONVERSION
                     @AstSource
                     source = { println "compiled on: ${new Date()}" }
           }
          {code}

          A {{getAnnotations()}} method should probably be added to {{DeclarationExpression}} to support this.
          Hide
          Hamlet D'Arcy added a comment -

          please note: this JIRA is not holding up the AstBuilder work. I would still like to have this JIRA completed but no planned features are dependent on it.

          Show
          Hamlet D'Arcy added a comment - please note: this JIRA is not holding up the AstBuilder work. I would still like to have this JIRA completed but no planned features are dependent on it.
          Hide
          Paul King added a comment -

          Cool, good to know. I need to catch up on where you have got up to on AstBuilder.

          WRT this issue, what puzzles me on best way to handle this issue is usage from Groovy code itself.
          Everywhere else we have an easy way to get to the annotation, e.g. Foo.class.annotations[0].value()
          but here it would be almost impossible to peak at from the user code. This isn't necessarily a
          problem at all just feels different to existing cases.

          Show
          Paul King added a comment - Cool, good to know. I need to catch up on where you have got up to on AstBuilder. WRT this issue, what puzzles me on best way to handle this issue is usage from Groovy code itself. Everywhere else we have an easy way to get to the annotation, e.g. Foo.class.annotations [0] .value() but here it would be almost impossible to peak at from the user code. This isn't necessarily a problem at all just feels different to existing cases.
          Hide
          Hamlet D'Arcy added a comment -

          My usage is in AST Transforms, so the annotation never appears at runtime. However, I too have considered how there is not API to retrieve this beyond looking at AST. I'm not recommending this yet, but you could add an API to the java Method class for:
          Method#getVariables() : Variable
          and introduce a new Variable class to complement Class, Method, and Field.
          But then what do you do about scope? There can be two foos defined in a single method and both can carry different annotations. I'm not sure there is a good runtime api to retrieve this stuff.

          Show
          Hamlet D'Arcy added a comment - My usage is in AST Transforms, so the annotation never appears at runtime. However, I too have considered how there is not API to retrieve this beyond looking at AST. I'm not recommending this yet, but you could add an API to the java Method class for: Method#getVariables() : Variable and introduce a new Variable class to complement Class, Method, and Field. But then what do you do about scope? There can be two foos defined in a single method and both can carry different annotations. I'm not sure there is a good runtime api to retrieve this stuff.
          Hide
          Jochen Theodorou added a comment -

          java and Groovy have lexical scopes, so you cannot define two foo variables.

          Show
          Jochen Theodorou added a comment - java and Groovy have lexical scopes, so you cannot define two foo variables.
          Hide
          Hamlet D'Arcy added a comment -

          You can define 2 foo variables within the scope of a single method... the have a different VariableScope in the AST, but for the purposes of defining an API that allows you to retrieve the variables defined in a Method object you can have 2 foos.

          class A {
          public void foo() {
          if (true)

          { @LocalAnnotation String foo = "foo1" }

          else

          { @SomeOtherAnnotation String foo = "foo2" }

          }
          }

          Show
          Hamlet D'Arcy added a comment - You can define 2 foo variables within the scope of a single method... the have a different VariableScope in the AST, but for the purposes of defining an API that allows you to retrieve the variables defined in a Method object you can have 2 foos. class A { public void foo() { if (true) { @LocalAnnotation String foo = "foo1" } else { @SomeOtherAnnotation String foo = "foo2" } } }
          Hide
          Jochen Theodorou added a comment -

          ah, yes, of course... I am very sorry for commenting so very misleading. two foos in nested scopes is impossible, but siblings can have each a foo of course.

          But... IMHO Java does allow annotations on declarations, but gives no support for annotations on local variables. Why should Groovy have that beyond the AST?

          Show
          Jochen Theodorou added a comment - ah, yes, of course... I am very sorry for commenting so very misleading. two foos in nested scopes is impossible, but siblings can have each a foo of course. But... IMHO Java does allow annotations on declarations, but gives no support for annotations on local variables. Why should Groovy have that beyond the AST?
          Hide
          Hamlet D'Arcy added a comment -

          i personally do not have a need for the metadata beyond AST. The problem I am trying to solve is the difficulty in finding AST transform insertion points at a more granular level than the method.

          Show
          Hamlet D'Arcy added a comment - i personally do not have a need for the metadata beyond AST. The problem I am trying to solve is the difficulty in finding AST transform insertion points at a more granular level than the method.
          Hide
          Paul King added a comment -

          Initial patch attached. Summary of contents:

          • Created a new interface Annotated (replaces most usages of AnnotatedNode)
          • AnnotatedNode and VariableExpression both implement Annotated
          • AntlrParserPlugin stores annotations into VvariableExpression nodes
          • a little bit of refactoring (no time to split that out yet)
          • Grab modified to know about above (I believe global transforms will be fine)

          What still isn't working?

          • with local transforms, variable expressions in a script (not an explicit class) may be processed before AST annotation class is resolved (still investigating)
          Show
          Paul King added a comment - Initial patch attached. Summary of contents: Created a new interface Annotated (replaces most usages of AnnotatedNode ) AnnotatedNode and VariableExpression both implement Annotated AntlrParserPlugin stores annotations into VvariableExpression nodes a little bit of refactoring (no time to split that out yet) Grab modified to know about above (I believe global transforms will be fine) What still isn't working? with local transforms, variable expressions in a script (not an explicit class) may be processed before AST annotation class is resolved (still investigating)
          Paul King made changes -
          Attachment groovy3481_annotations_local_variable_declarations.patch [ 43493 ]
          Hide
          Paul King added a comment -

          reworked file with refactorings removed

          Show
          Paul King added a comment - reworked file with refactorings removed
          Paul King made changes -
          Attachment groovy3481_annotations_local_variable_declarations-B.patch [ 43501 ]
          Paul King made changes -
          Attachment groovy3481_annotations_local_variable_declarations.patch [ 43493 ]
          Hide
          Paul King added a comment -

          OK, reworked patch makes Expression extend AnnotationNode but only populates and visits VariableExpressions

          Show
          Paul King added a comment - OK, reworked patch makes Expression extend AnnotationNode but only populates and visits VariableExpressions
          Hide
          Paul King added a comment -

          Committed reworked version to trunk. So basic functionality is there.
          More work required for this to work nicely in local AST transforms for Scripts

          Show
          Paul King added a comment - Committed reworked version to trunk. So basic functionality is there. More work required for this to work nicely in local AST transforms for Scripts
          Paul King made changes -
          Assignee Guillaume Laforge [ guillaume ] Paul King [ paulk_asert ]
          Fix Version/s 1.7-beta-1 [ 14014 ]
          Guillaume Delcroix made changes -
          Fix Version/s 1.7-beta-1 [ 14014 ]
          Fix Version/s 1.7-beta-x [ 15538 ]
          Hide
          Guillaume Delcroix added a comment -

          Can it be closed?

          Show
          Guillaume Delcroix added a comment - Can it be closed?
          Hide
          Paul King added a comment -

          I haven't done anything yet to make local AST transforms in Scripts work.
          So if you have, e.g.:

          @ClassScope foo
          

          in a script, then ClassScope won't have been resolved at the time the script is processed and will be basically ignored. I probably won't get time to look at this until after I get back from SpringOne2GX.

          Show
          Paul King added a comment - I haven't done anything yet to make local AST transforms in Scripts work. So if you have, e.g.: @ClassScope foo in a script, then ClassScope won't have been resolved at the time the script is processed and will be basically ignored. I probably won't get time to look at this until after I get back from SpringOne2GX.
          Hide
          Hamlet D'Arcy added a comment -

          This change still requires a lot of the Expression subclasses to extend AnnotatedNode so that the annotations can be retrieved during a transformation.

          Show
          Hamlet D'Arcy added a comment - This change still requires a lot of the Expression subclasses to extend AnnotatedNode so that the annotations can be retrieved during a transformation.
          Paul King made changes -
          Link This issue is related to GROOVY-3952 [ GROOVY-3952 ]
          Hide
          Paul King added a comment -

          Split remaining work into its own issue.

          Show
          Paul King added a comment - Split remaining work into its own issue.
          Hide
          Paul King added a comment -

          Initial part of this issue has been completed.

          Show
          Paul King added a comment - Initial part of this issue has been completed.
          Paul King made changes -
          Fix Version/s 1.7.0 [ 14665 ]
          Fix Version/s 1.7.X [ 15538 ]
          Affects Version/s 1.7-beta-1 [ 14014 ]
          Affects Version/s 1.7.0 [ 14665 ]
          Paul King made changes -
          Status Open [ 1 ] Resolved [ 5 ]
          Resolution Fixed [ 1 ]
          Paul King made changes -
          Status Resolved [ 5 ] Closed [ 6 ]
          Mark Thomas made changes -
          Project Import Sun Apr 05 13:32:57 UTC 2015 [ 1428240777691 ]
          Mark Thomas made changes -
          Workflow jira [ 12732474 ] Default workflow, editable Closed status [ 12744225 ]
          Mark Thomas made changes -
          Project Import Mon Apr 06 02:11:23 UTC 2015 [ 1428286283443 ]
          Mark Thomas made changes -
          Workflow jira [ 12970225 ] Default workflow, editable Closed status [ 12978002 ]
          Mark Thomas made changes -
          Assignee paulk_asert [ paulk_asert ] Paul King [ paulk ]
          Transition Time In Source Status Execution Times Last Executer Last Execution Date
          Open Open Resolved Resolved
          244d 9h 22m 1 Paul King 21/Dec/09 05:46
          Resolved Resolved Closed Closed
          1d 43m 1 Paul King 22/Dec/09 06:29

            People

            • Assignee:
              Paul King
              Reporter:
              Hamlet D'Arcy
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development