Improved class loading for AST transformations

Isn't it related to a previous issue you had opened?

In a sense yes, but this patch has a much smaller scope: the only thing it does is to allow compiler clients to optionally provide a custom class loader for loading AST transformations, just as they have always been able to provide a custom class loader for loading compile dependencies. As such it is a very natural extension of groovyc that both fixes immediate problems and simplifies future efforts such as GROOVY-3169, all without changing the class loading behavior of existing compiler clients.

there are some small adjustments to the patch I would like to see. SourceUnits may have a different ClassLoader then the CompileUnit or CompilationUnit. So I would like to see the transform loader in the same class.. Also I would probably name it to compilerExtensionLoader or better only extensionLoader. Also per default the laoder should be the same as what getClassLoader produces... these x==null?y:z checks I don't like very much.Then of course ASTTransformationCollectorCodeVisitor should not get the loader in the constructor, but request it from source as it does with the normal classloader.

> SourceUnits may have a different ClassLoader then the CompileUnit or CompilationUnit. So I would like to see the transform loader in the same class.
OK, so I should move transformLoader into ProcessingUnit and add another constructor there? Note that without further changes, initialization of transformLoader has to occur at construction time, because loading of global transforms is triggered from within CompilationUnit's constructor.

> Also I would probably name it to compilerExtensionLoader or better only extensionLoader.
I can do that, but "extension" already has an established meaning in the JVM (extension classpath, extension class loader).

> Also per default the loader should be the same as what getClassLoader produces... these x==null?y:z checks I don't like very much.
Neither do I, but the problem is that the regular class loader can also be set with ProcessingUnit.setClassLoader(), and I tried to support this form of initialization at least for local transforms. If that's not necessary then I can simplify the code.

the transforms loaded in the CompilationUnit are global ones. Of course CompilationUnit will still get the loader and that loader should be used to load the global transforms. IMHO ASTTransformationVisitor.addPhaseOperations should get that loader for this. As for the name "extensionLoader", you are right... then we should stick with compilerExtensionLoader.. or with astTransformationLoader.

A for ProcessingUnit.setClassLoader()... so you mean if someone uses null for the ast transformation loader, then it should use whatever is used for the normal classloader even if that is changed...hmm.... hadn't thought about that aspect.... But I don' think it is needed, better have a more simple code.

>A for ProcessingUnit.setClassLoader()... so you mean if someone uses null for the ast transformation loader, then it should use whatever is used for the normal classloader even if that is changed
I meant:

def unit = new CompilationUnit()
unit.classLoader = someClassLoader
unit.addSource(someFile)
unit.compile()

The initialization logic for ProcessingUnit/SourceUnit/CompilationUnit looks overly complex to me, with ProcessingUnit.setClassLoader() being especially evil. For example, if you call setClassLoader() after addSource(), the SourceUnit will end up with the "wrong" class loader. Should I just pretend that setClassLoader() doesn't exist?

One more thing that troubles me is that CompilationUnit.setSource(SourceUnit) always forces the CompilationUnit's class loader on the SourceUnit. Therefore, setting a class loader on the SourceUnit has no effect. Is this OK?

I'll let you apply this patch, and amend it as appropriate.

I now applied the patch, but with a few changes... Firstly I removed the reflective code you added. For a transform to work properly you need access to the compiler classes, so there is no point in excluding the annotation classes for the transform in th the compiler. Also I changed transform loader so it has to be a GroovyClassLoder. The loader used for transforms might be a GroovyClassLoader, but this loader has the ability compile scripts into classes on its own. But since we are currently in a compilation this must not happen. The result would be a deadlock or a cyclic compilation.... that means a file is to be compiled, which needs another file, which then is compiled, but depends on the first file. So if the compiler spawns a new compilation process outside the current one it would have to spawn new ones again and again. So a special loadClass method is used to avoid this cycle. The compiler then identifies classes to compile itself, instead of depending on GroovyClassLoader for this.

Of course that means the gmaven patch has to be changed to use GroovyClassLoader instead of URLClassLoader, but that shouldn't be a problem.

I close this issue as fixed now.... should we keep GROOVY-3169 open?

This patch makes local transforms work in the presence of a custom class loader. It also includes a thorough unit test.

patch applied