google-java-format (and removeUnusedImports) broken on JDK 16+ (has workaround)

[andrej-urvantsev]

When running under JDK 16 I get this error message:

Task :spotlessJava FAILED
Step 'google-java-format' found problem in 'src/main/java/se/inoviagroup/v2t/gateway/api/AuthUtils.java':
        ...
Caused by: java.lang.IllegalAccessError: class com.google.googlejavaformat.java.JavaInput (in unnamed module @0x615edab4) cannot access class com.sun.tools.javac.parser.Tokens$TokenKind (in module jdk.compiler) because module jdk.compiler does not export com.sun.tools.javac.parser to unnamed module @0x615edab4
        at com.google.googlejavaformat.java.JavaInput.buildToks(JavaInput.java:349)
        at com.google.googlejavaformat.java.JavaInput.buildToks(JavaInput.java:334)
        at com.google.googlejavaformat.java.JavaInput.<init>(JavaInput.java:276)
        at com.google.googlejavaformat.java.Formatter.getFormatReplacements(Formatter.java:280)
        at com.google.googlejavaformat.java.Formatter.formatSource(Formatter.java:267)
        at com.google.googlejavaformat.java.Formatter.formatSource(Formatter.java:233)
        ... 142 more

EDIT:

• workaround Gradle
• workaround maven

EDIT 2 & 3

• we thought no workaround necessary starting with plugin-gradle 6.5.1 and plugin-maven 2.22.3, but that was incorrect, the workaround is still required

[nedtwigg]

I think this can be worked-around if we somehow add these flags to the JVM which runs Gradle.

  --add-exports jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED \
  --add-exports jdk.compiler/com.sun.tools.javac.file=ALL-UNNAMED \
  --add-exports jdk.compiler/com.sun.tools.javac.parser=ALL-UNNAMED \
  --add-exports jdk.compiler/com.sun.tools.javac.tree=ALL-UNNAMED \
  --add-exports jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED \

The deeper fix will be related to #724.

[jmaicher]

This can be done in maven via MAVEN_OPTS or ${maven.projectBasedir}/.mvn/jvm.config, see https://maven.apache.org/configure.html

[dymk]

@nedtwigg The following can be added to your gradle.properties to work-around the bug:

org.gradle.jvmargs=--add-exports jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED \
  --add-exports jdk.compiler/com.sun.tools.javac.file=ALL-UNNAMED \
  --add-exports jdk.compiler/com.sun.tools.javac.parser=ALL-UNNAMED \
  --add-exports jdk.compiler/com.sun.tools.javac.tree=ALL-UNNAMED \
  --add-exports jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED

[ZacSweers]

Edit: Nevermind! I had an override of jvmargs in my gradle home that was overridding this

[anuraaga]

I tried playing with the Worker API for running Spotless. As a PoC, I could get GoogleJavaFormatIntegrationTest to pass

https://github.com/diffplug/spotless/compare/main...anuraaga:worker?expand=1

It was nice seeing how so much of the configuration is serializable, presumably to satisfy Gradle cache, which is a requirement for the Worker API. However, not all of it is, for example

https://github.com/diffplug/spotless/compare/main...anuraaga:worker?expand=1#diff-fb99f7a4cb3e09aa538ffb82c60c7119d0eddfb92fd93af98cb6337df1e87476R47
https://github.com/diffplug/spotless/compare/main...anuraaga:worker?expand=1#diff-b8ce946c3dca62f457228d3530c14e442ccdefe8e6619965cd3c2bcebf9e0d94R103

Can any team member skim through that PR to see if it seems feasible at all? If it is I can continue with it but it's quite invasive so want to get a sanity check first.

[nedtwigg]

Thanks for the investigation! It is promising. In your draft, there are a few places where a transient String/File path becomes non-transient. These classes are compared for equality based on their serialized representation. By making them non-transient, the remote build cache will no longer work, because task inputs are now keyed on absolute paths.

The root of this problem is LazyForwardingEquality. Pretty much everything in Spotless derives from this. This class allowed us to avoid implementing .equals() and .hashCode() in a lot of places, and it also allowed us to do lazy configuration back in Gradle 2.x, before Gradle had task configuration avoidance.

I think there are two different, but not exactly mutually exclusive paths forward:

1. Rename LazyForwardingEquality to LazySerializedEquality, and make a new class LazyDelegatedEquality which actually calls .equals() on the state object rather than comparing serialized bytes. This will require writing more boilerplate code, but it allows us to put absolute paths into objects without breaking remote build cache.

2. Give up on FormatterStep supporting .equals(), and instead have it provide List<File> inputFiles() and Object inputProp(). We would explicitly pass these to Gradle's own up-to-date mechanisms, rather than swallowing them up into our own .equals().

If you are interested in continuing the experiment, open a draft PR and we can continue from there. These improvements are also on the path to properly supporting the Gradle configuration cache.