Groovy Support for annotations on package declarations and imports

One question. Java already has a ElementType.PACKAGE enum value but no ElementType.IMPORT value. Do we need a GroovyElementType?
This would allow, for instance, an AST macro to check that an annotation is applicable to e.g. an import.

Attached patch provides a potential implementation. It doesn't offer a solution for ElementType.IMPORT nor does it hook up traversal of the annotations into the shared class visitors. It thought that might be safer to do that later. It is hooked up specifically for the Grapes for the moment, so the example above works.

I'll probably apply the patch in a day or so if I have no feedback.

The solution works with normal qualified imports and 'as' aliases. Currently it doesn't work for static imports or * imports.
We currently don't represent those using ImportNode nodes. Perhaps we should.

I also wonder whether we should have a special PackageNode class. It wouldn't have much in it but would make things nicer.

The problem with those changes are that they probably bigger than what belong in 1.6 and I was hoping to merge the changes
onto the 1_6_X branch. So perhaps the way to go is to merge the current patch onto both trunk and the 1.6 branch (which has
some limitations) and then refactor trunk as we become more familiar with what usage patterns this new feature may bring.

first a comment to the patch.... removing public void addImport(String alias, ClassNode type) is no good idea, better to have the annotations optional. Many use this method already, so it would be a breaking change. Next thing... please don't reformat or correct spelling errors in such a patch. It makes it difficult to see what the real change is. Also looking at the grammar change I think we should think of doing a bigger transformation of the grammar. Let the annotation be its own element and attach it later to a node. As an example:

class Foo {
}
@MyAnnotation

This annotation then annotates the module node and no real class is needed. Of course that is a bigger change than the one you suggest here. We also said that we want to allow annotations everywhere, but if not supported by Java, then it should be a source annotation only. You already change ModuleNode to be an AnnotatedNode, but I think that is without need. With my suggestions it makes sense of course. Also changing all imports to use ImportNodes is a good thing to do.

Revised patch sans typos plus reinstated the old addImport API method for b/c reasons

OK, I have removed the typo noise. I am not sure I understand everything you were suggesting, so I'll move one step at a time.

Just an overall disclaimer. I have a main goal of providing a way for 1_6_X scripts to not require creating an artificial method or class,
so I would like a version close to attached patch which doesn't require too much refactoring. We should obviously make trunk
as refactored as makes sense.

Let's just look at packages first.

Java allows package annotations. Runtime annotations can be accessed using code like:

println String.package.annotations // => [ ] as java.lang package has no runtime annotations

The output of this script worries me:

package foo
class Bar { }
println Bar.package // => null

Seems to me we should fix this up. Introducing a PackageNode to hold annotations might be a reasonable thing to do as part of that.
At the moment, I have just shoe-horned the package annotations into the ModuleNode.

We should also support the package-info.groovy variant for creating package annotations at some point as per here:
http://tech.puredanger.com/2007/02/28/package-annotations/

See also this interesting Coin proposal which is relevant:
http://mail.openjdk.java.net/pipermail/coin-dev/2009-March/000318.html

Paul, the package problem was reported and I thought it is already fixed. Didn't Roshan add some addPackage statements to GCL to fix this issue?

As for the patch...I am not fully getting why you have visitAnnotations(sourceUnit.getAST()); Currently you cannot use that, or am I wrong? I mean currently there is no way to use the source code to add an annotation to the ModuleNode, or not? Ah wait, I see now that you add package annotations to the module. I think we should not do that, it will hinder us in further development. You sure say that we should do as less as possible for 1.6.x regarding refactoring, but this would introduce an API that becomes invalid in 1.7 again and since we already know that, there is IMHO no point in adding that then. It would be a more clean solution to introduce a PackageNode and annotate that. That is backwards compatible and future proof (at last for one release).

As for the package-info.java, I think that should be a new issue, it is enough if we support here source level annnotations first. Related to that file groovydoc would need an update too. As for resolving classes used by the import.. this reminds me of a possible bug in your patch. Since you did not overwrite visitAnnotations in the default visitor I doubt that ResolveVisitor will look at those annotations. That means there will be unresolved classes. As long as they are ignored by ACG it is problem not a problem, but I think we should do a good solution. And that would be to fix the visitors to visit that code too and we should resolve the used classes just like any normal class... meaning with all imports.

Ah yes, I forgot about Roshan's patch. That fixes it. I was testing on 1.6.3.

I'll create a new issue for the package-info processing. Groovydoc does basic processing of those files already.

I'll add a PackageNode and refactor.

Patch I appended seems to be missing diffs for one file. Will upload a new patch shortly - just not while still happy from CITCON drinks.

Paul, did I see right and you replaced hasPackageName() by hasPackage()? That is a public method and should not be replaced. Again you can add the method and in his case maybe mark the other as deprecated. But simply removing is not ok. Also if I see right, then the package node you create in ASTHelper gets no line/col information set. I think there is a configureAST method for this. either add this to the patch or create a follow up issue so we don't forget it. the whole purpose of introducing such a node is for one more correct annotation handling and second for setting line/col information correctly for IDE usage later. Then one more thing in ASTHelper... protected void importClass(ClassNode type, String name, String as) should not be removed either. Better make it deprecated only.

Besides that I think the patch is also missing visitor code for annotations. Again this should be either added (including testcases of course), or a follow up issue should be created.

Paul I hope you are not annoyed by me being so strict here

Hi Jochen, patch C is just a placeholder. I am on holidays at the beach this week and didn't want the only code to be on my machine. Still quite a bit to do when I get time again.

ah, good. Paul, enjoy your vocation

Still no visiting yet in this patch.

Another temporary patch

Current patch - includes a little bit of refactoring (sorry - haven't had time to split that out yet).
Status: handles @Grab and @Grapes on all kinds of imports and on package statements

Questions: Should there be a common abstract class or interface for:
ImportStaticNode
ImportNode
ImportStarNode
ImportStaticStarNode

Currently we tend not to use interfaces in this part of the code but we might need interfaces to make VariableExpression annotated soon anyway.

The alternative which I should have considered earlier would be a common class with isStar and isStatic properties. It would be a little bit of a strange beast though as it would have a strange mix of fields only used depending on the values of the flags. Even so, one slightly awkward class might be less cumbersome than 4 classes. The idea would be to make the default values of the isStar and isStatic fields such that it would default back to the current behavior of the class.

Should the redundancy in ModuleNode be removed? e.g. staticImportFields could be derived from staticImportNodes

Which methods should be made deprecated? Or leave in for convenience?

Is current visiting logic acceptable? It will visit imports/package def'ns multiple times for the case of multiple classes in a file. Alternative would be some kind of visitModule logic.

An updated patch with all ImportNode variants now in the one class.

I am fairly comfortable merging the imports part of this patch to trunk and possibly 1_6_X branch.

The package part I am less sure of. Even though our short term goal is to have the info available at compilation time only, I think we would want to spike out runtime capture of this information so that the decisions we make now don't cause trouble further down the track.

I am commenting the G patch now... you added some maps to ModuleNode, but they are not really used. I think that's not worth the trouble of having a list and a map structure for the same thing. It adds very much to the API without real use and makes it probably more complicated later. Then I suggest that setPackage in APP does configure the AST, not doing it outside later if possible... that is unless I did look wrong and it is not the same node, then forget about that comment.

Besides these small things the patch looks quite good now.

Has been added to trunk

Not currently planned for 1_6_X.