groovy-eclipse icon indicating copy to clipboard operation
groovy-eclipse copied to clipboard

Parrot parser not used with maven-compiler-plugin when configured as documented

Open cstamas opened this issue 5 years ago • 13 comments

So if a the doco is followed to set up your Maven, the Parrot parser seems not used, see https://github.com/groovy/groovy-eclipse/issues/1082

I believe this is worth documenting, as it causes HUGE discrepancy between what user reads on Groovy3 and whar Maven "eats" when configured as documented. Moreover, IDEs like Idea, when setup like this, does NOT behave as this (follows Groovy doco, does not need this system property to enable parrot parser).

Hence, this should be documented, on a very visible place. Plus: doco should get into detail how to setup maven compiler plugin to get "real" Groovy 3 as well.

cstamas avatar Apr 30 '20 13:04 cstamas

I'll definitely need some clarification on your use case. Parrot Parser is disabled by default in Eclipse IDE and this has been stated in the release notes for every release that includes the new parser. The Maven batch compiler was not released until Groovy 3 was released and the compiler artifact was designed and tested to use the Parrot Parser. Since live editing is not involved, there was no reason to disable the new parser. If you are not using Eclipse IDE or Maven builds, what is your situation where the new parser is expected but is not appearing to be used?

eric-milles avatar Apr 30 '20 14:04 eric-milles

Am using it as documented on this site, but try-with-resource does NOT compile (on CLI, using mvn clean install), just like in referenced issue. Moreover, it DOES compile if I append -Dgroovy.antlr4=true to my command line. Hence, this shows that when used with Maven only (so no IDE involved), it still needs this system property.

cstamas avatar Apr 30 '20 14:04 cstamas

So here is an example project https://github.com/cstamas/maven-eclipse-compiler-1107

Is basic archetype + groovy compiler

  • hopefully is configured right
  • but does not compile

I get error

[ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.8.1:compile (default-compile) on project maven-eclipse-compiler-1107: Compilation failure: Compilation failure: 
[ERROR] /home/cstamas/tmp/maven-with-groovy/maven-eclipse-compiler-1107/src/main/java/org/cstamas/test/App.groovy:[10,5] 
[ERROR] 1. ERROR in /home/cstamas/tmp/maven-with-groovy/maven-eclipse-compiler-1107/src/main/java/org/cstamas/test/App.groovy (at line 10)
[ERROR] 	try (FileInputStream fis = new FileInputStream('test')) {
[ERROR] 	    ^
[ERROR] Groovy:expecting '{', found '('
[ERROR] Found 1 error and 0 warnings.
[ERROR] -> [Help 1]

but, when I do mvn install -Dgroovy.antlr4=true, then it compiles just fine.

Am I doing something wrong?

cstamas avatar Apr 30 '20 15:04 cstamas

And setting it for "real" (the thing I mention on OP) is quite cumbersome, as setting a system property from POM alone is not possible, unless you get an extra plugin, and use that.

Why not just plain compiler argument instead (that maven-compiler-plugin already supports)? Also, why this (IMO wrong) default when used from Maven?

cstamas avatar Apr 30 '20 15:04 cstamas

If you want to enable with no system property, you can use configScript option. Your script only needs one line:

configuration.pluginFactory = org.codehaus.groovy.control.ParserPluginFactory.antlr4()

I am re-checking the maven integration. It was supposed to work with no additional config, but has clearly failed.

eric-milles avatar Apr 30 '20 15:04 eric-milles

Thanks @eric-milles Am fine for now with properties-maven-plugin doing it, but clearly was surprised with CLI Maven results, AND would be delighted to remove this "workaround" as well (as just adds bloat to project).

cstamas avatar Apr 30 '20 15:04 cstamas

The long term goal is to support recovery for the new parser and at that point there will be no reason to disable it by default.

eric-milles avatar Apr 30 '20 15:04 eric-milles

I am commenting here instead of in my duplicate #1137, in order to return something to the helpful community and hopefully for the benefit of others stumbling upon this problem too. Here is how I configure my IDE IntelliJ IDEA to use -Dgroovy.antlr4=true for both Maven and IDEA builds:

image

image

I do agree with @cstamas though: It should be a compiler option I can set directly in my Maven POM via Maven Compiler plugin.

kriegaex avatar Jul 05 '20 16:07 kriegaex

@cstamas Could you explain how you've used the properties-maven-plugin to set groovy.antlr4? Seeing a snippet of POM would be incredibly helpful.

brandones avatar Nov 20 '20 22:11 brandones

Howdy, of course:

        <!-- Set system properties -->
        <plugin>
          <groupId>org.codehaus.mojo</groupId>
          <artifactId>properties-maven-plugin</artifactId>
          <version>1.0.0</version>
          <executions>
            <execution>
              <goals>
                <goal>set-system-properties</goal>
              </goals>
              <configuration>
                <properties>
                  <!-- Make groovy-eclipse-compiler use Groovy 3 compiler -->
                  <property>
                    <name>groovy.antlr4</name>
                    <value>true</value>
                  </property>
                </properties>
              </configuration>
            </execution>
          </executions>
        </plugin>

cstamas avatar Nov 20 '20 22:11 cstamas

Perfect, thanks so much!

brandones avatar Nov 20 '20 23:11 brandones

A couple notes that might be useful:

  1. If using maven to compile, the forked mode of the compiler does indeed enable the Parrot Parser for Groovy 3+. This can be done by adding <maven.compiler.fork>true</maven.compiler.fork> to your properties element or <fork>true</fork> to your maven-compiler-plugin's configuration element.

  2. As noted earlier, you can also enable the Parrot Parser by adding a compiler config script. This can be done by adding <compilerArguments><configScript>config.groovy</configScript></compilerArguments> to your maven-compiler-plugin's configuration element. config.groovy should contain:

    configuration.pluginFactory = org.codehaus.groovy.control.ParserPluginFactory.antlr4()
    
  3. IntelliJ does not use the Main-Class as specified by the groovy-eclipse-batch jar. This means it bypasses the extra config that was put in place to have the batch compiler use Parrot Parser by default.

eric-milles avatar Jun 03 '21 18:06 eric-milles

Thanks for the Maven Compiler fork setting. It works nicely for me. So now in IntelliJ IDEA I no longer need to set the property in the Maven runner VM settings as described in https://github.com/groovy/groovy-eclipse/issues/1107#issuecomment-653906682. For the Groovy-Eclipse compiler, I still need the property, though. I do not want to use a compiler config script, because that would be even more complicated and just be another extra IDE setting I need, i.e. nothing would improve that way with regard to my goal "simply import the Maven project and it works".

@eric-milles, I created IDEA-270925 in order to track this problem and get it fixed in IntelliJ IDEA.

On a side note: Setting the property in a main method which is meant to be used via CLI, while integrating clients normally do what IDEA does, i.e. to extend the class and call super(), is quite counter-intuitive for any tool integrator. Would it not be better to pull the logic for selecting the correct parser default (i.e. Parrot, if the property is unset) into the constructor? Then the problem would simply go away for all integrators and magically be fixed in IDEA and Maven Compiler, too. No extra compiler parameters, no sacred knowledge about the need to fork. Compiling in process is actually the preferable thing to do in most cases.

kriegaex avatar Jun 06 '21 02:06 kriegaex

I also ran into this problem and only found this issue after a lot of searching. I think it would make sense to just update the wiki's example POM or at least add a comment there. It's an easy fix once someone knows that the problem is that they need to enable the antlr4 parser, but as it stands you have to dig through the issues or old release notes to find it.

mattventura avatar Aug 17 '23 19:08 mattventura

Actually, I am not sure why the idea from the last paragraph of my previous comment https://github.com/groovy/groovy-eclipse/issues/1107#issuecomment-855323869 was not implemented yet. It would be simple and fix the problem in several use cases.

kriegaex avatar Aug 17 '23 21:08 kriegaex

@cstamas @mattventura I updated the docs to make specific mention of the fork setting: https://github.com/groovy/groovy-eclipse/wiki/Groovy-Eclipse-Maven-plugin#how-to-use-the-compiler-plugin---setting-up-the-pom

@kriegaex The design of this was to respect the system properties of a shared jvm. When forking the compiler process, it is safe enough to set the system property that enables antlr4 parser. It was unanticipated that IntelliJ uses a different entry point. The long-term goal is still to improve parser error recovery and then enable it by default -- as groovyc does.

eric-milles avatar Aug 23 '23 14:08 eric-milles