Fix JDK 20-ea build compatibility #3610

Stephan202 · 2022-12-17T17:34:02Z

With these changes the JDK 20-ea build on GitHub Actions passes again.

Stephan202

NB: I'd be nice to have a new Error Prone release that includes df033f0.

Stephan202 · 2022-12-17T17:36:03Z

core/src/main/java/com/google/errorprone/refaster/UEnhancedForLoop.java

+    // On JDK 20 and above the `EnhancedForLoopTree` interface contains a additional method
+    // `getDeclarationKind()`, referencing a  type not available prior to JDK 20. AutoValue
+    // generates a corresponding field and accessor for this property. Here we find and invoke the
+    // generated constructor with the appropriate arguments, depending on context.
+    // See https://github.com/openjdk/jdk20/commit/2cb64a75578ccc15a1dfc8c2843aa11d05ca8aa7.
+    // TODO: Simplify this logic once JDK 19 and older are no longer supported.
+    return isCompiledWithJdk20Plus()
+        ? createJdk20PlusEnhancedForLoop(variable, elements, statement)
+        : createPreJdk20EnhancedForLoop(variable, elements, statement);


Not happy with all the reflection going on here, but found no better way.

We've mostly been using reflection for similar workarounds recently.

I've been thinking about whether could use Multi-Release jars. At some point it may be worth branches source files like this when there are significant API changes, to avoid the reflection. But at least the last time I looked into it, setting maven up to build mr-jars didn't seem trivial.

Using Multi-Release JARs might be a nice idea indeed! I currently don't have any experience with those, so can't comment on the effort required for Maven. (But this does sound like a nice topic to dive into; might also be nice for Error Prone Support, where we're also discussing how to depend on/use newer JDKs while still supporting JDK 11.)

Stephan202 · 2022-12-17T17:36:11Z

core/src/main/java/com/google/errorprone/refaster/UEnhancedForLoop.java

  @Override
  public JCEnhancedForLoop inline(Inliner inliner) throws CouldNotResolveImportException {
-    return makeForeachLoop(
-        inliner.maker(),
-        getVariable().inline(inliner),
-        getExpression().inline(inliner),
-        getStatement().inline(inliner));
-  }
-
-  private static JCEnhancedForLoop makeForeachLoop(
-      TreeMaker maker, JCVariableDecl variable, JCExpression expression, JCStatement statement) {
-    try {
-      if (RuntimeVersion.isAtLeast20()) {
-        return (JCEnhancedForLoop)
-            TreeMaker.class
-                .getMethod("ForeachLoop", JCTree.class, JCExpression.class, JCStatement.class)
-                .invoke(maker, variable, expression, statement);
-      } else {
-        return (JCEnhancedForLoop)
-            TreeMaker.class
-                .getMethod(
-                    "ForeachLoop", JCVariableDecl.class, JCExpression.class, JCStatement.class)
-                .invoke(maker, variable, expression, statement);
-      }
-    } catch (ReflectiveOperationException e) {
-      throw new LinkageError(e.getMessage(), e);
-    }
+    return inliner
+        .maker()
+        .ForeachLoop(
+            getVariable().inline(inliner),
+            getExpression().inline(inliner),
+            getStatement().inline(inliner));


@cushon based on my tests (with JDK 20-ea build 28) the build also passes without these changes. Let me know if I should revert this part of the PR.

I think I prepared df033f0 to fix an error in the internal CI, which was building at the Java 11 language level and APIs and then running on a newer JDK. We might not need this with your patch, though. Let me think about this a little harder.

(I think there was some related discussion in an external bug about how our CI is compiling and testing at each JDK version, but we're only releasing one set of artifacts built at a fixed version, and maybe we should have the CI do one build that matches the releases and then run the tests at different versions.)

I see a regression when testing this compiled against the Java 11 APIs, and running on the latest version. I think that makes sense: the change to TreeMaker#ForeachLoop is source-compatible here, but if it resolves the Java 11 version at compile-time and sees a different version at runtime it will fail.

I think the reflection is still necessary, and I should also try to add that configuration to the external CI.

java.lang.NoSuchMethodError: 'com.sun.tools.javac.tree.JCTree$JCEnhancedForLoop com.sun.tools.javac.tree.TreeMaker.ForeachLoop(com.sun.tools.javac.tree.JCTree$JCVariableDecl, com.sun.tools.javac.tree.JCTree$JCExpression, com.sun.tools.javac.tree.JCTree$JCStatement)' at com.google.errorprone.refaster.UEnhancedForLoop.inline([UEnhancedForLoop.java:125](https://cs.corp.google.com/#search/&q=f:com/google/errorprone/refaster/UEnhancedForLoop.java:125&ws=cushon/225130&snapshot=5)) at com.google.errorprone.refaster.UEnhancedForLoop.inline([UEnhancedForLoop.java:34](https://cs.corp.google.com/#search/&q=f:com/google/errorprone/refaster/UEnhancedForLoop.java:34&ws=cushon/225130&snapshot=5)) at com.google.errorprone.refaster.USimpleStatement.inlineStatements([USimpleStatement.java:34](https://cs.corp.google.com/#search/&q=f:com/google/errorprone/refaster/USimpleStatement.java:34&ws=cushon/225130&snapshot=5)) at com.google.errorprone.refaster.BlockTemplate.replace([BlockTemplate.java:215](https://cs.corp.google.com/#search/&q=f:com/google/errorprone/refaster/BlockTemplate.java:215&ws=cushon/225130&snapshot=5)) at com.google.errorprone.refaster.BlockTemplate.replace([BlockTemplate.java:48](https://cs.corp.google.com/#search/&q=f:com/google/errorprone/refaster/BlockTemplate.java:48&ws=cushon/225130&snapshot=5)) at com.google.errorprone.refaster.RefasterScanner.scan([RefasterScanner.java:126](https://cs.corp.google.com/#search/&q=f:com/google/errorprone/refaster/RefasterScanner.java:126&ws=cushon/225130&snapshot=5)) at com.google.errorprone.refaster.RefasterScanner.scan([RefasterScanner.java:51](https://cs.corp.google.com/#search/&q=f:com/google/errorprone/refaster/RefasterScanner.java:51&ws=cushon/225130&snapshot=5)) at jdk.compiler/com.sun.source.util.TreeScanner.scanAndReduce([TreeScanner.java:96](https://cs.corp.google.com/#search/&q=f:com/sun/source/util/TreeScanner.java:96&ws=cushon/225130&snapshot=5)) at jdk.compiler/com.sun.source.util.TreeScanner.visitMethod([TreeScanner.java:224](https://cs.corp.google.com/#search/&q=f:com/sun/source/util/TreeScanner.java:224&ws=cushon/225130&snapshot=5)) at com.google.errorprone.refaster.RefasterScanner.visitMethod([RefasterScanner.java:88](https://cs.corp.google.com/#search/&q=f:com/google/errorprone/refaster/RefasterScanner.java:88&ws=cushon/225130&snapshot=5)) at com.google.errorprone.refaster.RefasterScanner.visitMethod([RefasterScanner.java:51](https://cs.corp.google.com/#search/&q=f:com/google/errorprone/refaster/RefasterScanner.java:51&ws=cushon/225130&snapshot=5)) at jdk.compiler/com.sun.tools.javac.tree.JCTree$JCMethodDecl.accept([JCTree.java:944](https://cs.corp.google.com/#search/&q=f:com/sun/tools/javac/tree/JCTree.java:944&ws=cushon/225130&snapshot=5)) at com.google.errorprone.refaster.RefasterScanner.visitClass([RefasterScanner.java:75](https://cs.corp.google.com/#search/&q=f:com/google/errorprone/refaster/RefasterScanner.java:75&ws=cushon/225130&snapshot=5)) at com.google.errorprone.refaster.RefasterScanner.visitClass([RefasterScanner.java:51](https://cs.corp.google.com/#search/&q=f:com/google/errorprone/refaster/RefasterScanner.java:51&ws=cushon/225130&snapshot=5)) at jdk.compiler/com.sun.tools.javac.tree.JCTree$JCClassDecl.accept([JCTree.java:851](https://cs.corp.google.com/#search/&q=f:com/sun/tools/javac/tree/JCTree.java:851&ws=cushon/225130&snapshot=5)) at jdk.compiler/com.sun.source.util.TreeScanner.scan([TreeScanner.java:92](https://cs.corp.google.com/#search/&q=f:com/sun/source/util/TreeScanner.java:92&ws=cushon/225130&snapshot=5)) at com.google.errorprone.refaster.RefasterScanner.scan([RefasterScanner.java:132](https://cs.corp.google.com/#search/&q=f:com/google/errorprone/refaster/RefasterScanner.java:132&ws=cushon/225130&snapshot=5)) at com.google.errorprone.refaster.RefasterScanner.scan([RefasterScanner.java:51](https://cs.corp.google.com/#search/&q=f:com/google/errorprone/refaster/RefasterScanner.java:51&ws=cushon/225130&snapshot=5)) at jdk.compiler/com.sun.source.util.TreeScanner.scan([TreeScanner.java:111](https://cs.corp.google.com/#search/&q=f:com/sun/source/util/TreeScanner.java:111&ws=cushon/225130&snapshot=5)) at jdk.compiler/com.sun.source.util.TreeScanner.scanAndReduce([TreeScanner.java:119](https://cs.corp.google.com/#search/&q=f:com/sun/source/util/TreeScanner.java:119&ws=cushon/225130&snapshot=5)) at jdk.compiler/com.sun.source.util.TreeScanner.visitCompilationUnit([TreeScanner.java:152](https://cs.corp.google.com/#search/&q=f:com/sun/source/util/TreeScanner.java:152&ws=cushon/225130&snapshot=5)) at jdk.compiler/com.sun.tools.javac.tree.JCTree$JCCompilationUnit.accept([JCTree.java:619](https://cs.corp.google.com/#search/&q=f:com/sun/tools/javac/tree/JCTree.java:619&ws=cushon/225130&snapshot=5)) at jdk.compiler/com.sun.source.util.TreeScanner.scan([TreeScanner.java:92](https://cs.corp.google.com/#search/&q=f:com/sun/source/util/TreeScanner.java:92&ws=cushon/225130&snapshot=5)) at com.google.errorprone.refaster.RefasterScanner.scan([RefasterScanner.java:132](https://cs.corp.google.com/#search/&q=f:com/google/errorprone/refaster/RefasterScanner.java:132&ws=cushon/225130&snapshot=5)) at com.google.errorprone.refaster.RefasterRule.apply([RefasterRule.java:139](https://cs.corp.google.com/#search/&q=f:com/google/errorprone/refaster/RefasterRule.java:139&ws=cushon/225130&snapshot=5)) at com.google.errorprone.refaster.CodeTransformerTestHelper.transform([CodeTransformerTestHelper.java:75](https://cs.corp.google.com/#search/&q=f:com/google/errorprone/refaster/CodeTransformerTestHelper.java:75&ws=cushon/225130&snapshot=5)) at com.google.errorprone.refaster.TemplateIntegrationTest.expectTransforms([TemplateIntegrationTest.java:68](https://cs.corp.google.com/#search/&q=f:com/google/errorprone/refaster/TemplateIntegrationTest.java:68&ws=cushon/225130&snapshot=5))

I could not reproduce this locally, but the stack trace speaks for itself. I added a commit to revert this section 👍

cushon

Thanks for this!

I want to do some internal testing to double-check it's OK to remove the stuff from df033f0, and then I'll try to get this merged soon.

cushon · 2022-12-17T21:20:44Z

core/src/main/java/com/google/errorprone/refaster/UEnhancedForLoop.java

  @Override
  public JCEnhancedForLoop inline(Inliner inliner) throws CouldNotResolveImportException {
-    return makeForeachLoop(
-        inliner.maker(),
-        getVariable().inline(inliner),
-        getExpression().inline(inliner),
-        getStatement().inline(inliner));
-  }
-
-  private static JCEnhancedForLoop makeForeachLoop(
-      TreeMaker maker, JCVariableDecl variable, JCExpression expression, JCStatement statement) {
-    try {
-      if (RuntimeVersion.isAtLeast20()) {
-        return (JCEnhancedForLoop)
-            TreeMaker.class
-                .getMethod("ForeachLoop", JCTree.class, JCExpression.class, JCStatement.class)
-                .invoke(maker, variable, expression, statement);
-      } else {
-        return (JCEnhancedForLoop)
-            TreeMaker.class
-                .getMethod(
-                    "ForeachLoop", JCVariableDecl.class, JCExpression.class, JCStatement.class)
-                .invoke(maker, variable, expression, statement);
-      }
-    } catch (ReflectiveOperationException e) {
-      throw new LinkageError(e.getMessage(), e);
-    }
+    return inliner
+        .maker()
+        .ForeachLoop(
+            getVariable().inline(inliner),
+            getExpression().inline(inliner),
+            getStatement().inline(inliner));


I think I prepared df033f0 to fix an error in the internal CI, which was building at the Java 11 language level and APIs and then running on a newer JDK. We might not need this with your patch, though. Let me think about this a little harder.

(I think there was some related discussion in an external bug about how our CI is compiling and testing at each JDK version, but we're only releasing one set of artifacts built at a fixed version, and maybe we should have the CI do one build that matches the releases and then run the tests at different versions.)

cushon · 2022-12-17T21:26:37Z

core/src/main/java/com/google/errorprone/refaster/UEnhancedForLoop.java

+              UExpression.class,
+              USimpleStatement.class)
+          .newInstance(declarationKind, variable, elements, statement);
+    } catch (IllegalAccessException


nit: ReflectiveOperationException

cushon · 2022-12-17T21:30:47Z

core/src/main/java/com/google/errorprone/refaster/UEnhancedForLoop.java

+    // On JDK 20 and above the `EnhancedForLoopTree` interface contains a additional method
+    // `getDeclarationKind()`, referencing a  type not available prior to JDK 20. AutoValue
+    // generates a corresponding field and accessor for this property. Here we find and invoke the
+    // generated constructor with the appropriate arguments, depending on context.
+    // See https://github.com/openjdk/jdk20/commit/2cb64a75578ccc15a1dfc8c2843aa11d05ca8aa7.
+    // TODO: Simplify this logic once JDK 19 and older are no longer supported.
+    return isCompiledWithJdk20Plus()
+        ? createJdk20PlusEnhancedForLoop(variable, elements, statement)
+        : createPreJdk20EnhancedForLoop(variable, elements, statement);


We've mostly been using reflection for similar workarounds recently.

I've been thinking about whether could use Multi-Release jars. At some point it may be worth branches source files like this when there are significant API changes, to avoid the reflection. But at least the last time I looked into it, setting maven up to build mr-jars didn't seem trivial.

Stephan202

Thanks for the quick review! Added a commit to use ReflectiveOperationException.

Stephan202 · 2022-12-18T09:32:36Z

core/src/main/java/com/google/errorprone/refaster/UEnhancedForLoop.java

+    // On JDK 20 and above the `EnhancedForLoopTree` interface contains a additional method
+    // `getDeclarationKind()`, referencing a  type not available prior to JDK 20. AutoValue
+    // generates a corresponding field and accessor for this property. Here we find and invoke the
+    // generated constructor with the appropriate arguments, depending on context.
+    // See https://github.com/openjdk/jdk20/commit/2cb64a75578ccc15a1dfc8c2843aa11d05ca8aa7.
+    // TODO: Simplify this logic once JDK 19 and older are no longer supported.
+    return isCompiledWithJdk20Plus()
+        ? createJdk20PlusEnhancedForLoop(variable, elements, statement)
+        : createPreJdk20EnhancedForLoop(variable, elements, statement);


Using Multi-Release JARs might be a nice idea indeed! I currently don't have any experience with those, so can't comment on the effort required for Maven. (But this does sound like a nice topic to dive into; might also be nice for Error Prone Support, where we're also discussing how to depend on/use newer JDKs while still supporting JDK 11.)

With these changes the JDK 20-ea build on GitHub Actions passes again. Fixes #3610 FUTURE_COPYBARA_INTEGRATE_REVIEW=#3610 from PicnicSupermarket:bugfix/jdk-20-enhanced-for-loop-compat 0b534d2 PiperOrigin-RevId: 498578771

This MR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [com.google.errorprone:error_prone_core](https://errorprone.info) ([source](https://github.com/google/error-prone)) | | minor | `2.16` -> `2.18.0` | | [com.google.errorprone:error_prone_annotations](https://errorprone.info) ([source](https://github.com/google/error-prone)) | compile | minor | `2.16` -> `2.18.0` | | [org.apache.maven.plugins:maven-failsafe-plugin](https://maven.apache.org/surefire/) | build | patch | `3.0.0-M7` -> `3.0.0-M8` | | [org.apache.maven.plugins:maven-surefire-plugin](https://maven.apache.org/surefire/) | build | patch | `3.0.0-M7` -> `3.0.0-M8` | --- ### Release Notes <details> <summary>google/error-prone</summary> ### [`v2.18.0`](https://github.com/google/error-prone/releases/tag/v2.18.0): Error Prone 2.18.0 [Compare Source](google/error-prone@v2.17.0...v2.18.0) New Checkers: - [`InjectOnBugCheckers`](https://errorprone.info/bugpattern/InjectOnBugCheckers) - [`LabelledBreakTarget`](https://errorprone.info/bugpattern/LabelledBreakTarget) - [`UnusedLabel`](https://errorprone.info/bugpattern/UnusedLabel) - [`YodaCondition`](https://errorprone.info/bugpattern/YodaCondition) Fixes issues: [#1650](google/error-prone#1650), [#2706](google/error-prone#2706), [#3404](google/error-prone#3404), [#3493](google/error-prone#3493), [#3504](google/error-prone#3504), [#3519](google/error-prone#3519), [#3579](google/error-prone#3579), [#3610](google/error-prone#3610), [#3632](google/error-prone#3632), [#3638](google/error-prone#3638), [#3645](google/error-prone#3645), [#3646](google/error-prone#3646), [#3652](google/error-prone#3652), [#3690](google/error-prone#3690) **Full Changelog**: google/error-prone@v2.17.0...v2.18.0 ### [`v2.17.0`](https://github.com/google/error-prone/releases/tag/v2.17.0): Error Prone 2.17.0 [Compare Source](google/error-prone@v2.16...v2.17.0) New Checkers: - [`AvoidObjectArrays`](https://errorprone.info/bugpattern/AvoidObjectArrays) - [`Finalize`](https://errorprone.info/bugpattern/Finalize) - [`IgnoredPureGetter`](https://errorprone.info/bugpattern/IgnoredPureGetter) - [`ImpossibleNullComparison`](https://errorprone.info/bugpattern/ProtoFieldNullComparison) - [`MathAbsoluteNegative`](https://errorprone.info/bugpattern/MathAbsoluteNegative) - [`NewFileSystem`](https://errorprone.info/bugpattern/NewFileSystem) - [`StatementSwitchToExpressionSwitch`](https://errorprone.info/bugpattern/StatementSwitchToExpressionSwitch) - [`UnqualifiedYield`](https://errorprone.info/bugpattern/UnqualifiedYield) Fixed issues: [#2321](google/error-prone#2321), [#3144](google/error-prone#3144), [#3297](google/error-prone#3297), [#3428](google/error-prone#3428), [#3437](google/error-prone#3437), [#3462](google/error-prone#3462), [#3482](google/error-prone#3482), [#3494](google/error-prone#3494) **Full Changelog**: google/error-prone@v2.16...v2.17.0 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever MR is behind base branch, or you tick the rebase/retry checkbox. 👻 **Immortal**: This MR will be recreated if closed unmerged. Get [config help](https://github.com/renovatebot/renovate/discussions) if that's undesired. --- - [ ] If you want to rebase/retry this MR, check this box --- This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).

Fix JDK 20-ea build compatibility

420ffef

Stephan202 commented Dec 17, 2022

View reviewed changes

Stephan202 mentioned this pull request Dec 17, 2022

Upgrade Arcmutate 1.0.1 -> 1.0.2 PicnicSupermarket/error-prone-support#418

Merged

1 task

cushon reviewed Dec 17, 2022

View reviewed changes

Simplify catch clause

26876c9

Stephan202 commented Dec 18, 2022

View reviewed changes

Revert the UEnhancedForLoop#inline changes

0b534d2

cushon approved these changes Dec 30, 2022

View reviewed changes

copybara-service bot mentioned this pull request Dec 30, 2022

Fix JDK 20-ea build compatibility #3635

Closed

copybara-service bot closed this in 145138b Dec 31, 2022

Stephan202 deleted the bugfix/jdk-20-enhanced-for-loop-compat branch December 31, 2022 09:40

cushon mentioned this pull request Feb 4, 2023

Consider using Multi-Release jars for Error Prone #3756

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix JDK 20-ea build compatibility #3610

Fix JDK 20-ea build compatibility #3610

Stephan202 commented Dec 17, 2022

Stephan202 left a comment

Stephan202 Dec 17, 2022

cushon Dec 17, 2022

Stephan202 Dec 18, 2022

Stephan202 Dec 17, 2022

cushon Dec 17, 2022

cushon Dec 19, 2022

Stephan202 Dec 20, 2022

cushon left a comment

cushon Dec 17, 2022

cushon Dec 17, 2022

cushon Dec 17, 2022

Stephan202 left a comment

Stephan202 Dec 18, 2022

Fix JDK 20-ea build compatibility #3610

Fix JDK 20-ea build compatibility #3610

Conversation

Stephan202 commented Dec 17, 2022

Stephan202 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cushon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stephan202 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment