part1: patches to enable groovy-eclipse to run unpatched groovy

I wonder if you planed to attache the patch here Andy. Without it it is a bit diffcult to tell about good or bad.

So it is for example unclear to me as of why GrabAnnotationTransformation needs an additional null and how that is connected to error recovery. In the best case the error recovery would still produce an AST that all phasses that are supposed to run with can still do so.

As for VariableScope. I guess such an iterator is good, for the other cases with have that already. What is needed though is an API test to ensure we won't do bad things here later.

As for .classpath.... how does that affect the groovy plugin? Sure, it is a good thing to fix that for other developers (I don't use it at all), but how is it related to the plugin?

As for ResolveVisitor... I really wonder if we should built in some abstraction instead of subclassing.... I mean sublcassing alone won't do it for you or not? You also have to "replace" the visitor.

doh! missing attachment, I got carried away writing the text.

> So it is for example unclear to me as of why GrabAnnotationTransformation needs an
> additional null and how that is connected to error recovery. In the best case the error
> recovery would still produce an AST that all phasses that are supposed to run with can
> still do so.

Ok, if you don't want that one now, I understand. I agree what you suggest would be nice, but I'll have to try and find the time to improve the error recovery further. The error recovery is far from perfect, it just gets improved over time.

> As for VariableScope. I guess such an iterator is good, for the other cases with have
> that already. What is needed though is an API test to ensure we won't do bad things
> here later.

Yes, tests would be nice. We have lots for this kind of thing in groovy-eclipse but none would tend to drop in to groovyc directly. I also find that because I can't build groovy itself as a groovy project in eclipse, it is quite awkward to work with (and thus awkward to create tests for inclusion with patches). I'd like to get to the point where we can work with groovy core as a real project in eclipse. But that's another issue I will slowly chip away at. So... will you not take this change without us also providing tests?

> As for .classpath.... how does that affect the groovy plugin? Sure, it is a good thing
> to fix that for other developers (I don't use it at all), but how is it related to the
> plugin?

Nothing to do with the plugin, just looked like no-one else was bothering to keep it up to date so I included the latest relevant changes in this patch whilst I had them. Part of me trying to actually get groovy core to build in eclipse as an eclipse project.

> As for ResolveVisitor... I really wonder if we should built in some
> abstraction instead of subclassing.... I mean sublcassing alone won't do it for
> you or not? You also have to "replace" the visitor.

We have other extensions (in CompilationUnit) that I've not yet included that allow the resolver to be configured (getter/setter for resolvevisitor). If you tell me the shape of what you have in mind, I can maybe explore moving towards something like it. Or if you have time to implement a more open resolution system for me, great.

It is tricky - unpicking all the various patches across all the modules into sensible chunks is hard as so many interoperate to make things work. I don't really want to dump them all on you in one go, so am going for a piece by piece approach. But if I do it this way you may not see the full picture when applying minimal pieces of the solution.

With all the changes we patch in groovyc, we try to do the absolute minimum because patching each release is not a good use of time. This can mean the implementation decisions are less than ideal due to the current primary goal of keeping the patch small.

I've been putting off this work for a bit too long and now it is particularly messy.

GrabAnnotationTransformation:
I think we should maybe consider having an artificial ModuleNode that so that always one is available.. what do you think? We have to rule those circumstances that produce problems for transforms on a case-by-case base out.

Nice tests:
WE do I want tests especially for the VariableScopeVisitor part? I want it so that we can there write that this method is used from the eclipse plugin and should not be changed or even removed without discussing with your guys first. But if there is no such test, this may not happen. That's why for API that is really used only outside of groovyc we need those API tests to ensure compatibility. We can do without, but then you should not wonder if something happens

Building Groovy directly in Eclipse:
Most of the time I do that almost. We have actions in the build files for generating DGM helper methods that are required for a Groovy program to run. I changed 1.8 so that it can at least start without them and will issue only a warning, but then not even println will be available. It works fine for running the compiler though, which is what I do most of the time. I have that hackish setup in which the ant build and eclipse share the class folder. Most of the times I can then do the following: ant build (including DGM step), incremental compiles from eclipse. Of course, once eclipse decides to do a full build I have to first do the ant one again. This is a bit annoying, but it works most of the time. Well sometimes the compiler actually doesn't do the incremental compile anymore... no compile at all... which is more than just a little bit annoying... and I have also no idea why eclipse thinks it has to make full builds so often... but all in all I can work with that setup.

ResolveVisitor:
Yes, that is the problem I have no idea about the full picture. of course that also means I have no idea about a good abstraction as well, since my usual working style is to look at the requirements at hand and not to make them up. In fact I would like to play the ball back and ask you for ideas on a more open resultion system, since you may have some ideas at least about the direction. Doesn't have to be anything concrete, but outlining the actual requirements would be nice. For joint compilation for example we do a hack, we have a JavaAwareResolvevisitor, that simply throws no error if a class is not resolveable. But in joint compilation mode we don't replace the original ResolveVisitor. So the only other usage we have is quite different I guess. I would imagine that for your case it would be more interesting to have a simple interface that can be used to query for a ClassNode with a certain name.... and we weave that then in... resolveToScript and such would then become external and a "plugin".

> It is tricky - unpicking all the various patches across all the
> modules into sensible chunks is hard as so many interoperate to make
> things work. I don't really want to dump them all on you in one go,
> so am going for a piece by piece approach. But if I do it this way
> you may not see the full picture when applying minimal pieces of the
> solution.

Maybe we should then do another approach... can you make a list describing all the changes made to CompilationUnit and why (in very short words)? Then I would get a picture of the missing connectors.

> With all the changes we patch in groovyc, we try to do the absolute
> minimum because patching each release is not a good use of time.

that is indeed true... also you want at some point at least come near the point where you can almost drop in the jar and be done

revisiting this to try and get something moving.

GrabAnnotationTransformation
Per your request I switched to using an empty module node rather than checking for null. The attached patch for SourceUnit does the job for my test scenario. I would have used a 'dummy' ModuleNode constant but as ModuleNode construction takes the SourceUnit as a parameter and I do have it around, I decided to use that and just build an empty one when this situation occurs.

ResolveVisitor
Ideally I would have a file system based resolver, not a classloader based one, for standard compilation. I want my compilation dependencies to be switchable and don't really want to actively load my dependencies as part of compiling the code. If my project depends on java 1.5 or java 1.6 I should be able to specify the JVM class library to use as configuration option on the compilation, right now I will end up tied in with whatever was used to launch the JVM that is running groovyc. But writing one of those seems more work than we have time for right now. I guess at minimum for the existing one we could open up the resolver you have to allow me to record dependencies (I have to record things that aren't found, as well as those that are, to allow for incremental compilation when these 'missing' types are added for a subsequent build) and lookup somewhere else other than through the classloader - probably be making 'resolveToClass' more flexible, delegating to some kind of lookup environment, where the default might be classloader based whilst mine will be file system based.
Let me refactor what I have to see if I can come up with something you'd rather integrate.

Following on from that, I would like to continue to try and reduce the getTypeClass() calls that are mostly unnecessary in the compiler as they cause real problems for a non-classloader based resolution system. To that end I'm attaching two other small patches:

AnnotationVisitor
For checking existence of enum values, remove the typeclass and usage of Enum directly, just use the available ClassNodes. The call to getFields() is currently necessary because of GROOVY-4814.

ExtendedVerifier
Two places where there are class literal comparisons (using getTypeClass()) when a string comparison would do. These strings could be pulled out into constants if there is a suitable constants class somewhere.

I've decided to separate these three changes into 3 different patches, maybe that is easier for you. I'll attach them in a moment, I've built groovy and run all the tests with these applied and tests are OK. In the meantime I'll review the resolver changes again.

I'm also still thinking about the VariableScope.getDeclaredVariablesIterator() method that we want to add. I can't see any unit tests for this class so can't add a test to indicate that it is still relevant and shouldn't be deleted in the future because someone doesn't think it is being used - do you have an opinion on how to address this? Would javadoc on the method being added be sufficient?

The 3 small patches have been applied - they didn't look contraversial. Changeset 22161 for trunk and 22163 for the 1_8_X branch. Applies to these source files:
AnnotationVisitor.java, ExtendedVerifier.java, SourceUnit.java

I assume Paul forgot to close the issue or wanted to wait for Andy to make another comment. Since that did not happen I close the issue now. It is no problem to reopen it of course

I don't think the three small patches covered everything in the original big patch - but we can certainly cover future changes in a subsequent issue. I don't think the original patch would be easy to understand/apply at this stage.