Fix JDK 20-ea build compatibility by Stephan202 · Pull Request #3610 · google/error-prone

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

+    // 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

  @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

  @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

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


nit: ReflectiveOperationException

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

+    // 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

+    // 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

Conversation

Stephan202 commented Dec 17, 2022

Uh oh!

Stephan202 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cushon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Stephan202 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants