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

          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)
          Hide
          Paul King added a comment -

          reworked file with refactorings removed

          Show
          Paul King added a comment - reworked file with refactorings removed
          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
          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.
          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.

            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