groovy

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

Details

  • Type: Improvement Improvement
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Fixed
  • Affects Version/s: 1.8-rc-2
  • Fix Version/s: 1.8.6, 2.0-beta-3
  • Component/s: Compiler
  • Labels:
    None
  • Number of attachments :
    4

Description

Groovy-Eclipse runs a patched form of groovyc. I want to start moving those patches back into groovyc itself, with the medium term goal of running an unpatched groovyc. Initially I'm collected together a few small things here, just to see if we have a process that can work for us. This patch was created on 1.8.0 rc2 and as far as I can tell still passes all the groovy tests (I simply ran 'ant install' to check). What does it contain:

GrabAnnotationTransformation. A null check. Under groovy-eclipse we operate a recoverable compiler so that we don't totally give up on the first problem encountered. This means sometimes code now runs that has to make an attempt to cope with errors having happened earlier on. Usually this is just a 'check if null, don't do this operation' type thing. This null check is one such case of that.

VariableScope. Extra helper method for accessing an iterator over the declared variables.

CompilationUnit/GroovyClass. When eclipse is processing 'GroovyClass' objects, it is very helpful to know where they came from. The patches here cause GroovyClass to know its originating classNode and SourceUnit.

ResolveVisitor. Under eclipse we have a subclass of resolve visitor that resolves types using JDT information. To do its work it needs to override some methods - this patch changes some methods from private to protected.

.classpath The .classpath currently in 1.8.0rc2 is out of date, some dependencies are missing and some haven't been added. This fixes those.

In addition it would be nice to get GROOVY-4553 fixed - is it definetly going to be in 1.8-rc-4? (that is the current target)

I'm not saying these changes need to be in 1.8.0, because you are in RC phase, but sometime in the early 1.8 streams would be nice. I tried to pick non-controversial changes initially - the tricky stuff is still to come. As of the 1.8.0 patch job I just did we currently modify around 50 groovy files, every little we can do to reduce that will help.

Activity

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

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.

Show
blackdrag blackdrag added a comment - 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.
Hide
Permalink
Andy Clement added a comment -

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

Show
Andy Clement added a comment - doh! missing attachment, I got carried away writing the text.
Hide
Permalink
Andy Clement added a comment -

> 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.

Show
Andy Clement added a comment - > 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.
Hide
Permalink
blackdrag blackdrag added a comment - - edited

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

Show
blackdrag blackdrag added a comment - - edited 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
Hide
Permalink
Andy Clement added a comment -

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?

Show
Andy Clement added a comment - 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?
Hide
Permalink
Andy Clement added a comment -

3 small patches, mentioned in previous comment.

Show
Andy Clement added a comment - 3 small patches, mentioned in previous comment.
Hide
Permalink
Paul King added a comment - - edited

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

Show
Paul King added a comment - - edited 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
Hide
Permalink
blackdrag blackdrag added a comment -

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

Show
blackdrag blackdrag added a comment - 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
Hide
Permalink
Paul King added a comment -

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.

Show
Paul King added a comment - 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.

People

  • Assignee:
    Paul King
    Reporter:
    Andy Clement
Vote (0)
Watch (0)

Dates

  • Created:
    Updated:
    Resolved: