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

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.

Jochen, any feedback on this?

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.

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.

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.

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.

Done