Compile with JDK 24 #1276
Compile with JDK 24 #1276
Conversation
WalkthroughUpdates Gradle configurations to adjust Java versions/toolchains across modules and adds Android compileOptions for Java 11. Refactors TypeMetadataBuilder to support multiple JDKs by introducing a runtime-selected implementation: reflection-based for JDK ≤17 and direct API usage for JDK 21+, adding a new JDK21TypeMetadataBuilder class. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant TMB as TypeMetadataBuilder
participant RT as Runtime JDK Detector
participant JB21 as JDK21TypeMetadataBuilder (direct)
participant JB17 as JDK17AndEarlierTypeMetadataBuilder (reflection)
participant JDK as JDK Types/APIs
Client->>TMB: buildTypeMetadata(type)
TMB->>RT: detect JDK version
alt JDK >= 21
TMB->>JB21: build(type)
JB21->>JDK: Use public Type APIs (construct/get/add metadata)
JDK-->>JB21: TypeMetadata
JB21-->>TMB: TypeMetadata
else JDK <= 17
TMB->>JB17: build(type)
JB17->>JDK: Lookup constructors/methods via MethodHandles
JB17->>JDK: Invoke handles to construct/clone metadata
JDK-->>JB17: TypeMetadata
JB17-->>TMB: TypeMetadata
end
TMB-->>Client: TypeMetadata
note over JB17,JDK: Reflection/MethodHandles path avoids direct refs to newer APIs
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1276 +/- ##
=========================================
Coverage 88.55% 88.55%
Complexity 2431 2431
=========================================
Files 91 91
Lines 8013 8022 +9
Branches 1598 1598
=========================================
+ Hits 7096 7104 +8
- Misses 458 459 +1
Partials 459 459 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
build.gradle (1)
99-104: Exclude jdk-javac-plugin from Java 11 default to avoid downgrading it.This project intentionally compiles with Java 17 features.
Apply:
tasks.withType(JavaCompile) { - java.sourceCompatibility = JavaVersion.VERSION_11 - java.targetCompatibility = JavaVersion.VERSION_11 + // Preserve module-specific settings (e.g., :jdk-javac-plugin uses 17) + if (project.path != ":jdk-javac-plugin") { + java.sourceCompatibility = JavaVersion.VERSION_11 + java.targetCompatibility = JavaVersion.VERSION_11 + } }
🧹 Nitpick comments (2)
nullaway/src/main/java/com/uber/nullaway/generics/TypeMetadataBuilder.java (2)
172-181: Catching Throwable is broad.Narrow to ReflectiveOperationException where possible to avoid masking serious errors.
Example:
- } catch (Throwable e) { + } catch (ReflectiveOperationException e) { throw new RuntimeException(e); }Note: keep Throwable where MethodHandle.invoke can throw linkage errors you must surface.
229-267: Tests: cover both paths (≤17 and ≥21) and all factory methods.Per prior learning on NullAway JSpecify tests, prefer failing when unsupported rather than skipping; add matrix tests on JDK 11, 17, 21, 24 to exercise both builders.
We’re applying your earlier guidance from PR #1245 about failing tests over skipping in versioned scenarios.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
build.gradle(1 hunks)jar-infer/test-android-lib-jarinfer/build.gradle(1 hunks)jdk-javac-plugin/build.gradle(1 hunks)nullaway/src/main/java/com/uber/nullaway/generics/TypeMetadataBuilder.java(5 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: msridhar
PR: uber/NullAway#1245
File: guava-recent-unit-tests/src/test/java/com/uber/nullaway/guava/NullAwayGuavaParametricNullnessTests.java:101-102
Timestamp: 2025-08-14T18:50:06.159Z
Learning: In NullAway JSpecify tests, when JDK version requirements exist due to bytecode annotation reading capabilities, prefer failing tests over skipping them on unsupported versions to ensure CI catches regressions and enforces proper JDK version usage for developers.
📚 Learning: 2025-08-14T18:50:06.159Z
Learnt from: msridhar
PR: uber/NullAway#1245
File: guava-recent-unit-tests/src/test/java/com/uber/nullaway/guava/NullAwayGuavaParametricNullnessTests.java:101-102
Timestamp: 2025-08-14T18:50:06.159Z
Learning: In NullAway JSpecify tests, when JDK version requirements exist due to bytecode annotation reading capabilities, prefer failing tests over skipping them on unsupported versions to ensure CI catches regressions and enforces proper JDK version usage for developers.
Applied to files:
jdk-javac-plugin/build.gradle
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Build and test on ubuntu-latest
- GitHub Check: Build spring-framework with snapshot
- GitHub Check: Build and test on windows-latest
- GitHub Check: Build caffeine with snapshot
- GitHub Check: Build and test on macos-latest
🔇 Additional comments (6)
jar-infer/test-android-lib-jarinfer/build.gradle (1)
27-31: Good: pin Android module to Java 11.This avoids -source/-target 8 warnings under JDK 24 and is consistent with AGP’s supported toolchains.
jdk-javac-plugin/build.gradle (1)
8-12: Remove Java 11 override concern: root’stasks.withType(JavaCompile)block is at the root project level (lines 101–104) and isn’t inside thesubprojectsblock, so it doesn’t affect this module’s Java 17 settings.Likely an incorrect or invalid review comment.
build.gradle (1)
59-65: Confirm JDK 24 provisioning and Error Prone compatibility on CI. I searched for alljavaCompiler,toolchain, and compatibility settings—aside from the intentional Java 11 source/target fallbacks and module-specific toolchains, there are no unintended overrides—so please verify that your CI runners can provision JDK 24 (via the foogJ resolver plugin) and that Error Prone +--add-exportsflags work as expected.nullaway/src/main/java/com/uber/nullaway/generics/TypeMetadataBuilder.java (3)
242-247: OK: clone for JDK 21+ without cloneWithMetadata.dropMetadata/addMetadata matches 21+ API.
249-266: Constructors for 21+ look correct.Using getMetadata() list and new ClassType/ArrayType/WildcardType signatures aligns with JDK 21+.
41-50: Lazy initialization is fine; avoid early linkage on newer JDKs.Static handles are confined to the <=17 builder class; it won’t initialize when running on 21+. Good.
| // Always compile using the latest JDK we can | ||
| javaCompiler = javaToolchains.compilerFor { | ||
| languageVersion = JavaLanguageVersion.of(17) | ||
| languageVersion = JavaLanguageVersion.of(24) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Don’t force JDK 24 toolchain on Android modules.
AGP may not run/compile with a JDK 24 toolchain. Limit the toolchain assignment to non-Android projects.
Apply:
tasks.withType(JavaCompile) {
- // Always compile using the latest JDK we can
- javaCompiler = javaToolchains.compilerFor {
- languageVersion = JavaLanguageVersion.of(24)
- }
+ // Always compile using the latest JDK we can (exclude Android modules)
+ if (!project.plugins.hasPlugin("com.android.application") &&
+ !project.plugins.hasPlugin("com.android.library")) {
+ javaCompiler = javaToolchains.compilerFor {
+ languageVersion = JavaLanguageVersion.of(24)
+ }
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Always compile using the latest JDK we can | |
| javaCompiler = javaToolchains.compilerFor { | |
| languageVersion = JavaLanguageVersion.of(17) | |
| languageVersion = JavaLanguageVersion.of(24) | |
| } | |
| tasks.withType(JavaCompile) { | |
| // Always compile using the latest JDK we can (exclude Android modules) | |
| if (!project.plugins.hasPlugin("com.android.application") && | |
| !project.plugins.hasPlugin("com.android.library")) { | |
| javaCompiler = javaToolchains.compilerFor { | |
| languageVersion = JavaLanguageVersion.of(24) | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In build.gradle around lines 61 to 64, the project unconditionally sets
javaCompiler to JDK 24 which can break Android Gradle Plugin builds; guard the
toolchain assignment so it only applies to non-Android projects (e.g., check for
absence of Android application/library plugins or run only for JVM
modules/subprojects) and move the javaToolchains.compilerFor block inside that
conditional so Android modules keep their default or AGP-compatible JDK.
There was a problem hiding this comment.
Works fine for now, I'm going to keep it
There was a problem hiding this comment.
@msridhar Understood! I'll defer to your judgment on this. If any compatibility issues arise with the Android modules and JDK 24 toolchain in the future, you know where to find the suggested guard condition.
🧠 Learnings used
Learnt from: msridhar
PR: uber/NullAway#1245
File: guava-recent-unit-tests/src/test/java/com/uber/nullaway/guava/NullAwayGuavaParametricNullnessTests.java:101-102
Timestamp: 2025-08-14T18:50:06.159Z
Learning: In NullAway JSpecify tests, when JDK version requirements exist due to bytecode annotation reading capabilities, prefer failing tests over skipping them on unsupported versions to ensure CI catches regressions and enforces proper JDK version usage for developers.
| @Override | ||
| public TypeMetadata create(com.sun.tools.javac.util.List<Attribute.TypeCompound> attrs) { | ||
| ListBuffer<Attribute.TypeCompound> b = new ListBuffer<>(); | ||
| b.appendList(attrs); | ||
| return new TypeMetadata.Annotations(b); | ||
| } | ||
|
|
There was a problem hiding this comment.
Bug: wrong constructor argument type; pass List, not ListBuffer.
TypeMetadata.Annotations expects a com.sun.tools.javac.util.List; ListBuffer is not a subtype.
Apply:
- ListBuffer<Attribute.TypeCompound> b = new ListBuffer<>();
- b.appendList(attrs);
- return new TypeMetadata.Annotations(b);
+ return new TypeMetadata.Annotations(attrs);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Override | |
| public TypeMetadata create(com.sun.tools.javac.util.List<Attribute.TypeCompound> attrs) { | |
| ListBuffer<Attribute.TypeCompound> b = new ListBuffer<>(); | |
| b.appendList(attrs); | |
| return new TypeMetadata.Annotations(b); | |
| } | |
| @Override | |
| public TypeMetadata create(com.sun.tools.javac.util.List<Attribute.TypeCompound> attrs) { | |
| return new TypeMetadata.Annotations(attrs); | |
| } |
🤖 Prompt for AI Agents
In nullaway/src/main/java/com/uber/nullaway/generics/TypeMetadataBuilder.java
around lines 235 to 241, the code constructs a ListBuffer and passes it directly
to TypeMetadata.Annotations, but that constructor expects a
com.sun.tools.javac.util.List, not a ListBuffer; change the call to pass the
ListBuffer converted to a com.sun.tools.javac.util.List (e.g., call b.toList())
so the returned value is new TypeMetadata.Annotations(b.toList()) instead of
passing the ListBuffer itself.
There was a problem hiding this comment.
The code compiles fine. This is for JDK 21+
There was a problem hiding this comment.
, and the constructor pattern shows that ListBuffer objects are routinely used in contexts where they need to be converted to List objects.
In JDK 21+, the TypeMetadata.Annotations constructor likely accepts ListBuffer directly or ListBuffer implements the expected interface/extends the expected class that makes it compatible with the constructor parameter. Since the code compiles successfully as you mentioned, the direct passing of ListBuffer to the constructor is indeed valid.
I was overly cautious in my analysis and should have trusted that the code compiles correctly. The JDK 21+ changes to the internal APIs appear to have made this usage pattern valid.
🐰
/| /|
( ✌️ )
✏️ Learnings added
Learnt from: msridhar
PR: uber/NullAway#1276
File: nullaway/src/main/java/com/uber/nullaway/generics/TypeMetadataBuilder.java:235-241
Timestamp: 2025-09-03T19:40:11.062Z
Learning: In JDK 21+, ListBuffer can be passed directly to TypeMetadata.Annotations constructor without needing conversion to List via toList() method. The APIs were updated to make this compatible.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: msridhar
PR: uber/NullAway#1245
File: guava-recent-unit-tests/src/test/java/com/uber/nullaway/guava/NullAwayGuavaParametricNullnessTests.java:101-102
Timestamp: 2025-08-14T18:50:06.159Z
Learning: In NullAway JSpecify tests, when JDK version requirements exist due to bytecode annotation reading capabilities, prefer failing tests over skipping them on unsupported versions to ensure CI catches regressions and enforces proper JDK version usage for developers.
This lets us take advantage of all the latest
javacfixes, including proper support for reading type use annotations from bytecodes. Previously we were compiling using JDK 17.The key changes are in
TypeMetadataBuilder. Before, it invoked javac APIs directly on JDK 17 or earlier, and used reflection for JDK 21+. Now we switch it and use reflection on the earlier JDKs, invoking APIs directly on JDK 21+. This makes sense since for JSpecify mode we strongly encourage users to compile with a recent JDK. Other changes are minor tweaks for compatibility with the new compiler version.Summary by CodeRabbit
New Features
Refactor
Chores