groovy

AST Transforms: @GroovyASTTransformationClass annotation should allow parameters of type Class[] not just String[]

Details

  • Type: Improvement Improvement
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Fixed
  • Affects Version/s: 1.7-beta-2
  • Fix Version/s: 1.6.6, 1.7-rc-1
  • Component/s: Compiler
  • Labels:
    None
  • Environment:
    all
  • Number of attachments :
    1

Description

The annotation @GroovyASTTransformationClass marks another annotation as an AST Transformation. The annotation has a required parameter, a String[]. The parameters stores the fully qualified class name that will be invoked during the transformation phases.

As a convenience, the parameter type should be Class[] instead of String[]. It allows programmers to specify a class not a String that represents a class.

We cannot remove String[] because that would break backwards compatibility. We could add a 2nd parameter and then require 1 (and only one) of the two parameters to be specified.

The was originally requested by Venkat Subramanium at 2gx 2009.

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Roshan Dawrani added a comment -

Attaching a patch for review that allows AST transforms to annotated using the class references as an alternative to using the String class names. There is no backwards compatibility broken - in specifying ast transforms by class name strings - as indicated by no failures in the test suite run.

Here is the summary of changes:
1) GroovyASTTransformationClass - This annotation now allows a new attribute "classes" of type Class[]
2) ASTTransformationCollectorCodeVisitor - Modified to collect the ast transform classes - either using class names or class references. Also errors out if neither class names nor classes are specified or if both are specified.
3) G3839Transform1/G3839Transform2/G3839Transform3 - Dummy transforms that add fields f1/f2/f3 to the class that they are annotated on.
4)

  • G3839A1 - Test annotation that adds an ast transform using new "classes" attribute
  • G3839A2 - Test annotation that adds multiple ast transforms using new "classes" attribute
  • G3839A3 - Test annotation that neither specifies ast transforms by class names nor by classes. Using it results in an error.
  • G3839A4 - Test annotation that neither specifies ast transforms by both class names and classes. Using it results in an error.
Show
Roshan Dawrani added a comment - Attaching a patch for review that allows AST transforms to annotated using the class references as an alternative to using the String class names. There is no backwards compatibility broken - in specifying ast transforms by class name strings - as indicated by no failures in the test suite run. Here is the summary of changes: 1) GroovyASTTransformationClass - This annotation now allows a new attribute "classes" of type Class[] 2) ASTTransformationCollectorCodeVisitor - Modified to collect the ast transform classes - either using class names or class references. Also errors out if neither class names nor classes are specified or if both are specified. 3) G3839Transform1/G3839Transform2/G3839Transform3 - Dummy transforms that add fields f1/f2/f3 to the class that they are annotated on. 4)
  • G3839A1 - Test annotation that adds an ast transform using new "classes" attribute
  • G3839A2 - Test annotation that adds multiple ast transforms using new "classes" attribute
  • G3839A3 - Test annotation that neither specifies ast transforms by class names nor by classes. Using it results in an error.
  • G3839A4 - Test annotation that neither specifies ast transforms by both class names and classes. Using it results in an error.
Hide
Permalink
Roshan Dawrani added a comment -

Jochen, any feedback on this?

Show
Roshan Dawrani added a comment - Jochen, any feedback on this?
Hide
Permalink
blackdrag blackdrag added a comment -

from your code I can see that transformClassNames and transformClasses can be null, but the for-loops you use don't take this in account. Is that not a source for NullPointerExceptions?

On the issue itself. The reason String[] is used is, because The transformation may use a different classpath from the normal compilation process. Removing the String[] part makes no sense if you consider this. That also means using the Class[] results in the transform sharing the same classpath as the normal code. Well, since many use cases will use the same path it is probably ok to have the Class[] version.

Show
blackdrag blackdrag added a comment - from your code I can see that transformClassNames and transformClasses can be null, but the for-loops you use don't take this in account. Is that not a source for NullPointerExceptions? On the issue itself. The reason String[] is used is, because The transformation may use a different classpath from the normal compilation process. Removing the String[] part makes no sense if you consider this. That also means using the Class[] results in the transform sharing the same classpath as the normal code. Well, since many use cases will use the same path it is probably ok to have the Class[] version.
Hide
Permalink
Roshan Dawrani added a comment -

Actually transformClassNames and transformClasses can not be null because both getTransformClassNames() and getTransformClasses() send back empty arrays in case of exceptions and then GroovyASTTransformationClass itself defines defaults for both "value" and "classes" attributes as empty arrays. These defaults I had introduced later when I ran into an issue, that's why that null check has remained in "(transformClassNames != null) && (transformClassNames.length > 0)". It can as well be "(transformClassNames.length > 0)" but its harmless anyway. (same for transformClasses).

String[] is not being removed in this improvement. It's just Class[] that is being introduced as an alternative for people who want to use it.

Show
Roshan Dawrani added a comment - Actually transformClassNames and transformClasses can not be null because both getTransformClassNames() and getTransformClasses() send back empty arrays in case of exceptions and then GroovyASTTransformationClass itself defines defaults for both "value" and "classes" attributes as empty arrays. These defaults I had introduced later when I ran into an issue, that's why that null check has remained in "(transformClassNames != null) && (transformClassNames.length > 0)". It can as well be "(transformClassNames.length > 0)" but its harmless anyway. (same for transformClasses). String[] is not being removed in this improvement. It's just Class[] that is being introduced as an alternative for people who want to use it.
Hide
Permalink
blackdrag blackdrag added a comment -

if they cannot be null, then why checking for null then? I suggest removing that part, it is misleading. After that small change I tink the patch is ok.

yes, I was aware that String[] is not removed, just wanted to point out why it was String[] and not Class[] in the fist place for documentation.

Show
blackdrag blackdrag added a comment - if they cannot be null, then why checking for null then? I suggest removing that part, it is misleading. After that small change I tink the patch is ok. yes, I was aware that String[] is not removed, just wanted to point out why it was String[] and not Class[] in the fist place for documentation.
Hide
Permalink
Roshan Dawrani added a comment -

Thanks for the review comments. I will remove that "misleading" null check. It is not required.

Is there any piece of doc that should be updated for it? If yes, it will be nice if you can point out to the relevant portion of the doc.

Thanks.

Show
Roshan Dawrani added a comment - Thanks for the review comments. I will remove that "misleading" null check. It is not required. Is there any piece of doc that should be updated for it? If yes, it will be nice if you can point out to the relevant portion of the doc. Thanks.
Hide
Permalink
Roshan Dawrani added a comment -

Done

Show
Roshan Dawrani added a comment - Done

People

Vote (0)
Watch (0)

Dates

  • Created:
    Updated:
    Resolved:
Atlassian JIRA (v4.4.3#663-r165197) | Report a problem
Powered by a free Atlassian JIRA open source license for Codehaus. Try JIRA - bug tracking software for your team.