groovy

Scala+Groovy+Java joint compiler

Details

  • Type: New Feature New Feature
  • Status: Open Open
  • Priority: Major Major
  • Resolution: Unresolved
  • Affects Version/s: 1.6.1, 1.7-beta-1
  • Fix Version/s: 1.8.x, 2.x
  • Component/s: Compiler
  • Labels:
    None
  • Patch Submitted:
    Yes
  • Number of attachments :
    1

Activity

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

Let me first try to summarize how this works

src/main/META-INF/services/org.codehaus.groovy.tools.javac.ExternalCompilerPlugin
org.codehaus.groovy.tools.scalac.ScalacExternalCompilerPlugin
org.codehaus.groovy.tools.javac.JavacExternalCompilerPlugin

defines the plugins, javac.ExternalCompilerPlugin is the interface/abstract class for the plugins. This is according to the SPI convention used in java5. This class contains a method createCompilationUnit, which obviously means that each plugin should create its own CompilationUnit variation. This seems to imply, that one plugin decides for the others how to compile, as Groovy does not support multiple CompilationUnits for one compilation... The CompilationUnit is the compilation, so it wouldn't make sense to do so. But if we cannot have multiple plugins active, then why the SPI stuff? The getFileExt() (returns the file extension used for the compilation) IMHO suggests that only one file extension name is possible... what will then happen for java files?

Since ExternalCompilationUnit is taking a JavaAwareCompilationUnit and a plugin in the constructor, I would have expected that ExternalCompilationUnit is working as some kind of proxy or at last uses the JavaAwareCompilationUnit for configuration, but nothing of that seems to be the case. Instead protected fields are set, usage unclear. ExternalCompilationUnit seems to manage its own sources, addSource and clearSources suggest that. Unclear to me is why this has to be a Set (we used List so far).

Both compiler plugins seem to use JavaCompiler based classes, which theoretically enables the usage from ant and command line. But in case of the command line I see no change that would finally link that in and it does not happen automatically. So this seems to be something that we still have to do

The orchestration of the change should then be in one of the CompilationUnit... since only JavaAwareCompilationUnit is changed, it must happen there. JavaAwareCompilationUnit used a compilerFactory property to enable to plugin a different java compiler. The ant task is using that for example to use ant's javac frontend instead of any internal lookup done by Groovy directly. I also suggested that usage for the eclipse plugin and I think GMaven wanted to use that too. Since that part is important for compatibility any change here is to be seen as bad. From looking at current groovyc I see here a regression, probably because of the changes for a forked compilation mode. This needs to be fixed. But anyway... even if the compilerFactory property is currently not used internally, doesn't mean it can be just removed. Back to the orchestration... ExternalCompilerPlugin.getPlugins is used to get a list of plugins (2 by default), we do stub generation if any of these plugins has sources. As long as any plugin has sources we do compilation with each plugin even if the plugin has no sources. sources are added using ExternalCompilationUnit#addSource, so each "plugin" decides if it adds the source or not. In fact it will return true if it does, so a source consumed by one plugin is no longer available to another plugin. Since we have two plugins active by default, this means we have scala sources going to the scala plugin, java sources to the java plugin and all other sources are then added to the controlling CompilationUnit... in a mixed compilation of java, scala and groovy sources this means then all groovy sources. Since only one file extension is supported by this code the scala plugin cannot use steal java files. That also means that any joint compilation probably done by the scala plugin won't happen.

So once joint compilation is activated the groovyc task and the command line compiler will both be able to use joint complication with java sources.

Then some notes and discussion points:

  • a initial patch should be more or less minimal and should not contain dead code or surplus code
  • The code reading the plugin duplicates the code used for global transforms. Obviously we should make that code into a utility class instead and use it for both. But this should be done independent of this issue. See ExternalCompilerPlugin.getPlugins
  • ExternalCompilerPlugin.getPlugins does a loop through all classloaders... why? The code for global transforms does not do this. So I see that as possible bug either here or there... or at last as surplus code, which means the patch has to be changed.
  • public abstract ExternalCompilationUnit createCompilationUnit (JavaAwareCompilationUnit compilationUnit) in ExternalCompilerPlugin takes a JavaAwareCompilationUnit, isn't that leaky abstraction? It makes sense for the scala version alone, but we try to define here something more general and the above means, that watever it is, it has to depend on a CompilationUnit created for joint compilation of groovy and java files. And that would be the case even if no java files are involved.
  • ExternalCompilerPlugin(String name, List<String> before, List<String> after) uses two list which are assigned to protected fields, but those are never used in your patch. This looks like dead code, left over from refactorings on your side. That means it can be removed, which is a good thing, since the protected fields would leak abstraction.
  • ExternalCompilerPlugin#getPlugins does operate on a List and is itself void. It does populate the list from an empty one, so why does it not return the list and create it inside in the first place? Also this works on a List<ExternalCompilationUnit>, which is not the plugins itself, but the CompilationUnit. If we forget that the name obviously does not fit, why add the compilation units and not the plugins?
  • After before/after is removed JavacExternalCompilerPlugin will no longer have to use Collections.EMPTY_LIST. Collections.EMPTY_LIST creates a list, yes, but one you can't add an element to. This IMHO strengthens my interpretation that before/after has no meaning.
  • If ExternalCompilerPlugin.getName() and the static method are the only methods with an actual implementation, then it should be considered to make the abstract class into an interface.
  • ExternalCompilerPlugin.getFileExt() should probably renamed to getDefaultFileExtension. For one we shouldn't fear the long method name and for second it is questionable if the mapping extension->language is always unique. So we shold think of a configuration option here. This doesn't have to be included in the initial design, but it should be considered when doing the infrastructure.
  • ExternalCompilationUnit as a private field suffix and mask can be extracted from the plugin. In case of suffix keeping the value is IMHO ok, because it is a internal information, but in case of mask it is returned outside and there should be the question if that really is needed. Also depending here on getFileExt naturally excludes configurable file extensions. Mabye that is a bad idea after all?
  • ScalacScalaCompiler#makeParameters seems to give all options to the scala compiler as they are, only target not. target instead goes to "jvm-"+namedValues[i + 1]. Scala seems to support jvm-1.5,jvm-1.4, msil and cldc, I think we should react with an error for the other two at last. And maybe we should do something about target 1.2 for example too, since that is a quite common option.
  • ScalacScalaCompiler does overwrite the only visible method of JavacJavaCompiler and does not use super... then why extend JavacJavaCompiler at all?
  • ScalacScalaCompiler implements ExternalCompiler, which more or less duplicates JavaCompiler. Now I agree that the naming doesn't fit any more, but ExternalCompiler is used only there and no where else. Not in a cast or method method definition - nowhere. This makes it surplus and should be removed. No need to add misleading dead code.
  • in JavacJavaCompiler a field is changed from protected to private, this looks very surplus
  • in JavacJavaCompiler the method signature is changed to accept a Set... no need to break with compatibility here, the same applies to JavaCompiler and the field changes in JavaAwareCompilationUnit for sources
  • in FileSystemCompiler JavaAwareCompilationUnit is constructed with an additional null argument. The old constructor did the same. IMHO this change is surplus and touches just another files, that doesn't need to be touched. So that change should be removed.
  • in build.xml the call order of -includeResources and -stagecompile is now switched... is this really intended?
  • the changes to groovyc do need a rework, so I would exclude all related changes from the patch for now.
Show
blackdrag blackdrag added a comment - Let me first try to summarize how this works src/main/META-INF/services/org.codehaus.groovy.tools.javac.ExternalCompilerPlugin org.codehaus.groovy.tools.scalac.ScalacExternalCompilerPlugin org.codehaus.groovy.tools.javac.JavacExternalCompilerPlugin defines the plugins, javac.ExternalCompilerPlugin is the interface/abstract class for the plugins. This is according to the SPI convention used in java5. This class contains a method createCompilationUnit, which obviously means that each plugin should create its own CompilationUnit variation. This seems to imply, that one plugin decides for the others how to compile, as Groovy does not support multiple CompilationUnits for one compilation... The CompilationUnit is the compilation, so it wouldn't make sense to do so. But if we cannot have multiple plugins active, then why the SPI stuff? The getFileExt() (returns the file extension used for the compilation) IMHO suggests that only one file extension name is possible... what will then happen for java files? Since ExternalCompilationUnit is taking a JavaAwareCompilationUnit and a plugin in the constructor, I would have expected that ExternalCompilationUnit is working as some kind of proxy or at last uses the JavaAwareCompilationUnit for configuration, but nothing of that seems to be the case. Instead protected fields are set, usage unclear. ExternalCompilationUnit seems to manage its own sources, addSource and clearSources suggest that. Unclear to me is why this has to be a Set (we used List so far). Both compiler plugins seem to use JavaCompiler based classes, which theoretically enables the usage from ant and command line. But in case of the command line I see no change that would finally link that in and it does not happen automatically. So this seems to be something that we still have to do The orchestration of the change should then be in one of the CompilationUnit... since only JavaAwareCompilationUnit is changed, it must happen there. JavaAwareCompilationUnit used a compilerFactory property to enable to plugin a different java compiler. The ant task is using that for example to use ant's javac frontend instead of any internal lookup done by Groovy directly. I also suggested that usage for the eclipse plugin and I think GMaven wanted to use that too. Since that part is important for compatibility any change here is to be seen as bad. From looking at current groovyc I see here a regression, probably because of the changes for a forked compilation mode. This needs to be fixed. But anyway... even if the compilerFactory property is currently not used internally, doesn't mean it can be just removed. Back to the orchestration... ExternalCompilerPlugin.getPlugins is used to get a list of plugins (2 by default), we do stub generation if any of these plugins has sources. As long as any plugin has sources we do compilation with each plugin even if the plugin has no sources. sources are added using ExternalCompilationUnit#addSource, so each "plugin" decides if it adds the source or not. In fact it will return true if it does, so a source consumed by one plugin is no longer available to another plugin. Since we have two plugins active by default, this means we have scala sources going to the scala plugin, java sources to the java plugin and all other sources are then added to the controlling CompilationUnit... in a mixed compilation of java, scala and groovy sources this means then all groovy sources. Since only one file extension is supported by this code the scala plugin cannot use steal java files. That also means that any joint compilation probably done by the scala plugin won't happen. So once joint compilation is activated the groovyc task and the command line compiler will both be able to use joint complication with java sources. Then some notes and discussion points: a initial patch should be more or less minimal and should not contain dead code or surplus code The code reading the plugin duplicates the code used for global transforms. Obviously we should make that code into a utility class instead and use it for both. But this should be done independent of this issue. See ExternalCompilerPlugin.getPlugins ExternalCompilerPlugin.getPlugins does a loop through all classloaders... why? The code for global transforms does not do this. So I see that as possible bug either here or there... or at last as surplus code, which means the patch has to be changed. public abstract ExternalCompilationUnit createCompilationUnit (JavaAwareCompilationUnit compilationUnit) in ExternalCompilerPlugin takes a JavaAwareCompilationUnit, isn't that leaky abstraction? It makes sense for the scala version alone, but we try to define here something more general and the above means, that watever it is, it has to depend on a CompilationUnit created for joint compilation of groovy and java files. And that would be the case even if no java files are involved. ExternalCompilerPlugin(String name, List<String> before, List<String> after) uses two list which are assigned to protected fields, but those are never used in your patch. This looks like dead code, left over from refactorings on your side. That means it can be removed, which is a good thing, since the protected fields would leak abstraction. ExternalCompilerPlugin#getPlugins does operate on a List and is itself void. It does populate the list from an empty one, so why does it not return the list and create it inside in the first place? Also this works on a List<ExternalCompilationUnit>, which is not the plugins itself, but the CompilationUnit. If we forget that the name obviously does not fit, why add the compilation units and not the plugins? After before/after is removed JavacExternalCompilerPlugin will no longer have to use Collections.EMPTY_LIST. Collections.EMPTY_LIST creates a list, yes, but one you can't add an element to. This IMHO strengthens my interpretation that before/after has no meaning. If ExternalCompilerPlugin.getName() and the static method are the only methods with an actual implementation, then it should be considered to make the abstract class into an interface. ExternalCompilerPlugin.getFileExt() should probably renamed to getDefaultFileExtension. For one we shouldn't fear the long method name and for second it is questionable if the mapping extension->language is always unique. So we shold think of a configuration option here. This doesn't have to be included in the initial design, but it should be considered when doing the infrastructure. ExternalCompilationUnit as a private field suffix and mask can be extracted from the plugin. In case of suffix keeping the value is IMHO ok, because it is a internal information, but in case of mask it is returned outside and there should be the question if that really is needed. Also depending here on getFileExt naturally excludes configurable file extensions. Mabye that is a bad idea after all? ScalacScalaCompiler#makeParameters seems to give all options to the scala compiler as they are, only target not. target instead goes to "jvm-"+namedValues [i + 1] . Scala seems to support jvm-1.5,jvm-1.4, msil and cldc, I think we should react with an error for the other two at last. And maybe we should do something about target 1.2 for example too, since that is a quite common option. ScalacScalaCompiler does overwrite the only visible method of JavacJavaCompiler and does not use super... then why extend JavacJavaCompiler at all? ScalacScalaCompiler implements ExternalCompiler, which more or less duplicates JavaCompiler. Now I agree that the naming doesn't fit any more, but ExternalCompiler is used only there and no where else. Not in a cast or method method definition - nowhere. This makes it surplus and should be removed. No need to add misleading dead code. in JavacJavaCompiler a field is changed from protected to private, this looks very surplus in JavacJavaCompiler the method signature is changed to accept a Set... no need to break with compatibility here, the same applies to JavaCompiler and the field changes in JavaAwareCompilationUnit for sources in FileSystemCompiler JavaAwareCompilationUnit is constructed with an additional null argument. The old constructor did the same. IMHO this change is surplus and touches just another files, that doesn't need to be touched. So that change should be removed. in build.xml the call order of -includeResources and -stagecompile is now switched... is this really intended? the changes to groovyc do need a rework, so I would exclude all related changes from the patch for now.
Hide
Permalink
blackdrag blackdrag added a comment -

The above was to take a look at the potential problems of the change and how it works, since you gave no detailed explanation and no code comments.

One thing that bothers me a bit is the order in which the plugins are executed. Thanks to in src/main/META-INF/services/org.codehaus.groovy.tools.javac.ExternalCompilerPlugin you define the scala plugin before the java plugin.. IMHO that means the scala part won't be able to depend on parts in Groovy or Java, but if your test runs fine, then I am wrong with that. because in your test the scala class extends the groovy class. So another possibility is, that the scala compiler will do a lookup for java files on its own and will then compile them by itself as scala joint compilation. AFAIK scala does joint compilation with java, but it does not write the class files, which means normal java compilation is required after. Of course compiling the java class then again using our java joint compilation is a bit surplus, but required. What worries me here is, that if we depend on the file system lookup made by the scala compiler we will run into problems if one source path containing a java file the scala file depends on is not known to the scala compiler. In that case the scala compiler will fail to compile and it will be quite a problem for outsiders to understand why. For the joint compilation part from the command line the solution is easy, the scala compilation unit simply should also accept java sources and not return true for them, meaning another unit will handle them too. So this part can be solved...

But assuming there is a plugin for language XY then the fact if XY can depend on scala classes will depend on if the scala compiler was run before XY or not. Of course you can get a similar problem with scala depending on XY. That again means that defining any kind of order is useless and unless they provide a stub style working compiler there won't be a solution to the problem.

But if that is the case, then what kind of problem are we actually trying to solve here? If it is only joint compilation of Java+Groovy+Scala, or maybe even Java+Groovy+XY (assuming XY understands java source files), then we can do that without these plugins. In fact the compiler factory property could be used as it is now. XY would then behave as a normal java compiler, just that it does a bit more.

If we want to solve the case of Java+Groovy+Scala+XY, where each can depend on the other, then this patch is IMHO not enough and does not provide the infrastructure for it. But if it fails to provide the infrastructure I fail to see why we should try to solve the problem in the way the patch suggests it. We could try to define a fixed order to not to be trapped by how resources are found in the class loaders, but the lack of two way communication will still frustrate users.

IMHO a "full" solution would have to be one in that each plugin generates stubs too. Because in that case we get the same level of integration we have with Groovy. But who would like to write a stub compiler for Scala and XY?

Show
blackdrag blackdrag added a comment - The above was to take a look at the potential problems of the change and how it works, since you gave no detailed explanation and no code comments. One thing that bothers me a bit is the order in which the plugins are executed. Thanks to in src/main/META-INF/services/org.codehaus.groovy.tools.javac.ExternalCompilerPlugin you define the scala plugin before the java plugin.. IMHO that means the scala part won't be able to depend on parts in Groovy or Java, but if your test runs fine, then I am wrong with that. because in your test the scala class extends the groovy class. So another possibility is, that the scala compiler will do a lookup for java files on its own and will then compile them by itself as scala joint compilation. AFAIK scala does joint compilation with java, but it does not write the class files, which means normal java compilation is required after. Of course compiling the java class then again using our java joint compilation is a bit surplus, but required. What worries me here is, that if we depend on the file system lookup made by the scala compiler we will run into problems if one source path containing a java file the scala file depends on is not known to the scala compiler. In that case the scala compiler will fail to compile and it will be quite a problem for outsiders to understand why. For the joint compilation part from the command line the solution is easy, the scala compilation unit simply should also accept java sources and not return true for them, meaning another unit will handle them too. So this part can be solved... But assuming there is a plugin for language XY then the fact if XY can depend on scala classes will depend on if the scala compiler was run before XY or not. Of course you can get a similar problem with scala depending on XY. That again means that defining any kind of order is useless and unless they provide a stub style working compiler there won't be a solution to the problem. But if that is the case, then what kind of problem are we actually trying to solve here? If it is only joint compilation of Java+Groovy+Scala, or maybe even Java+Groovy+XY (assuming XY understands java source files), then we can do that without these plugins. In fact the compiler factory property could be used as it is now. XY would then behave as a normal java compiler, just that it does a bit more. If we want to solve the case of Java+Groovy+Scala+XY, where each can depend on the other, then this patch is IMHO not enough and does not provide the infrastructure for it. But if it fails to provide the infrastructure I fail to see why we should try to solve the problem in the way the patch suggests it. We could try to define a fixed order to not to be trapped by how resources are found in the class loaders, but the lack of two way communication will still frustrate users. IMHO a "full" solution would have to be one in that each plugin generates stubs too. Because in that case we get the same level of integration we have with Groovy. But who would like to write a stub compiler for Scala and XY?
Hide
Permalink
Robert Fischer added a comment -

What kind of output would the "stub compiler" be forced to put out?

Show
Robert Fischer added a comment - What kind of output would the "stub compiler" be forced to put out?

People

  • Assignee:
    Unassigned
    Reporter:
    Alex Tkachman
Vote (8)
Watch (9)

Dates

  • Created:
    Updated: