Test reading JSpecify annotations from bytecodes on JDK 21 #1245
Conversation
WalkthroughUpdates test configurations and guards across multiple test suites to use List.of, add -XDaddTypeAnnotationsToSymbol=true, and gate certain tests on JDK feature >= 21. Removes a Gradle test-exclusion block so previously excluded jspecify tests now run. No public API changes. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1245 +/- ##
=========================================
Coverage 88.50% 88.50%
Complexity 2373 2373
=========================================
Files 90 90
Lines 7802 7802
Branches 1555 1555
=========================================
Hits 6905 6905
Misses 452 452
Partials 445 445 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (4)
nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java (1)
666-670: Good utility; minor doc clarification to avoid confusion on 22+.The helper is correct for gating the -XD flag on 21u8+. Consider clarifying in the Javadoc that this returns false for JDK 22+ intentionally, since the flag is not required there.
nullaway/src/test/java/com/uber/nullaway/jspecify/BytecodeGenericsTests.java (1)
233-236: Optional: centralize the repeated assumption into a helper.To avoid drift across tests and improve readability, you could add a single helper method and use it at all the guarded sites.
Add this method inside the class:
private static boolean jdkReadsTypeUseAnnotationsFromBytecode() { int feature = Runtime.version().feature(); return feature > 21 || NullabilityUtil.isJDK21Update8OrHigher(); }Then replace each occurrence with:
Assume.assumeTrue(jdkReadsTypeUseAnnotationsFromBytecode());Also applies to: 80-82, 105-107, 251-259
guava-recent-unit-tests/src/test/java/com/uber/nullaway/guava/NullAwayGuavaParametricNullnessTests.java (2)
59-71: Optional: centralize the guard to avoid duplication.Add a private helper in this test class and call it in all guarded tests to ensure future updates remain consistent.
Add:
private static boolean jdkReadsTypeUseAnnotationsFromBytecode() { int feature = Runtime.version().feature(); return feature > 21 || NullabilityUtil.isJDK21Update8OrHigher(); }Then replace the Assume lines with:
Assume.assumeTrue(jdkReadsTypeUseAnnotationsFromBytecode());Also applies to: 101-102, 149-150, 179-180, 261-262
59-71: Document the 21.0.8+ requirement in test comments.A brief comment noting “requires JDK 21.0.8+ (or 22+) to read type-use annotations from bytecode” will save future readers from rediscovering the guard rationale.
Also applies to: 98-123, 146-174, 176-197, 259-275
📜 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 settings in your CodeRabbit configuration.
📒 Files selected for processing (4)
guava-recent-unit-tests/src/test/java/com/uber/nullaway/guava/NullAwayGuavaParametricNullnessTests.java(6 hunks)nullaway/build.gradle(0 hunks)nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java(1 hunks)nullaway/src/test/java/com/uber/nullaway/jspecify/BytecodeGenericsTests.java(5 hunks)
💤 Files with no reviewable changes (1)
- nullaway/build.gradle
🧰 Additional context used
🧬 Code Graph Analysis (1)
nullaway/src/test/java/com/uber/nullaway/jspecify/BytecodeGenericsTests.java (2)
nullaway/src/test/java/com/uber/nullaway/NullAwayTestsBase.java (1)
NullAwayTestsBase(10-51)nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java (1)
NullabilityUtil(66-671)
🔇 Additional comments (2)
nullaway/src/test/java/com/uber/nullaway/jspecify/BytecodeGenericsTests.java (1)
251-259: Args builder and conditional -XD flag look good.Using ImmutableList.Builder enables clean conditional injection of -XDaddTypeAnnotationsToSymbol for 21u8+. This aligns with the JDK behavior described in the PR.
guava-recent-unit-tests/src/test/java/com/uber/nullaway/guava/NullAwayGuavaParametricNullnessTests.java (1)
59-71: Args builder + conditional -XD flag is correct and future-proof.This mirrors the JDK 21u8+ behavior and keeps 22+ untouched. Nice cleanup from Arrays.asList to builder for conditional logic.
...t-unit-tests/src/test/java/com/uber/nullaway/guava/NullAwayGuavaParametricNullnessTests.java
Show resolved
Hide resolved
...t-unit-tests/src/test/java/com/uber/nullaway/guava/NullAwayGuavaParametricNullnessTests.java
Show resolved
Hide resolved
...t-unit-tests/src/test/java/com/uber/nullaway/guava/NullAwayGuavaParametricNullnessTests.java
Show resolved
Hide resolved
...t-unit-tests/src/test/java/com/uber/nullaway/guava/NullAwayGuavaParametricNullnessTests.java
Show resolved
Hide resolved
nullaway/src/test/java/com/uber/nullaway/jspecify/BytecodeGenericsTests.java
Show resolved
Hide resolved
nullaway/src/test/java/com/uber/nullaway/jspecify/BytecodeGenericsTests.java
Show resolved
Hide resolved
nullaway/src/test/java/com/uber/nullaway/jspecify/BytecodeGenericsTests.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
nullaway/src/test/java/com/uber/nullaway/jspecify/BytecodeGenericsTests.java (3)
79-80: Tighten JDK guard: require 21u8+ (with -XD) or 22+
feature() >= 21will run on 21.0.0–21.0.7 where bytecode type-use annotations are not exposed; these tests will flake/fail there. Use the existing utility so the test only runs when supported.- // to read annotations properly from bytecode - Assume.assumeTrue(Runtime.version().feature() >= 21); + // Only run where bytecode type-use annotations are available: + // JDK 21.0.8+ (with -XDaddTypeAnnotationsToSymbol=true) or JDK 22+ + Assume.assumeTrue( + com.uber.nullaway.util.NullabilityUtil.isJDK21Update8OrHigher() + || Runtime.version().feature() > 21);
104-105: Same guard tightening needed hereAlign with the stricter check to avoid executing on 21.0.0–21.0.7.
- // to read annotations properly from bytecode - Assume.assumeTrue(Runtime.version().feature() >= 21); + // Only run where bytecode type-use annotations are available: + // JDK 21.0.8+ (with -XDaddTypeAnnotationsToSymbol=true) or JDK 22+ + Assume.assumeTrue( + com.uber.nullaway.util.NullabilityUtil.isJDK21Update8OrHigher() + || Runtime.version().feature() > 21);
233-234: Same guard tightening needed hereEnsure this also only runs on 21u8+ or 22+.
- // to read annotations properly from bytecode - Assume.assumeTrue(Runtime.version().feature() >= 21); + // Only run where bytecode type-use annotations are available: + // JDK 21.0.8+ (with -XDaddTypeAnnotationsToSymbol=true) or JDK 22+ + Assume.assumeTrue( + com.uber.nullaway.util.NullabilityUtil.isJDK21Update8OrHigher() + || Runtime.version().feature() > 21);
🧹 Nitpick comments (1)
nullaway/src/test/java/com/uber/nullaway/NullAwayTestsBase.java (1)
19-36: Gate -XDaddTypeAnnotationsToSymbol to JDK 21u8+ (or verify it’s harmless on older JDKs)You’re now passing the internal
-XDaddTypeAnnotationsToSymbol=trueflag via the default helper for all tests. While-XDgenerally accepts unknown keys, relying on that across older JDKs (e.g., 17) is brittle. Either:
- Gate the flag to only JDK 21 update 8+ (where it matters), or
- Confirm it’s a no-op on all JDKs in CI and keep as-is.
If you prefer gating, here’s a concrete refactor to keep args immutable at the call-site while conditionally adding the flag:
- makeTestHelperWithArgs( - List.of( + makeTestHelperWithArgs( + buildDefaultArgs( "-d", temporaryFolder.getRoot().getAbsolutePath(), "-XepOpt:NullAway:KnownInitializers=" + "com.uber.nullaway.testdata.CheckFieldInitNegativeCases.Super.doInit," + "com.uber.nullaway.testdata.CheckFieldInitNegativeCases" + ".SuperInterface.doInit2", "-XepOpt:NullAway:AnnotatedPackages=com.uber,com.ubercab,io.reactivex", // We give the following in Regexp format to test that support "-XepOpt:NullAway:UnannotatedSubPackages=com.uber.nullaway.[a-zA-Z0-9.]+.unannotated", "-XepOpt:NullAway:ExcludedClasses=" + "com.uber.nullaway.testdata.Shape_Stuff," + "com.uber.nullaway.testdata.excluded", "-XepOpt:NullAway:ExcludedClassAnnotations=com.uber.nullaway.testdata.TestAnnot", "-XepOpt:NullAway:CastToNonNullMethod=com.uber.nullaway.testdata.Util.castToNonNull", "-XepOpt:NullAway:ExternalInitAnnotations=com.uber.ExternalInit", "-XepOpt:NullAway:ExcludedFieldAnnotations=com.uber.ExternalFieldInit", - "-XDaddTypeAnnotationsToSymbol=true")); + "-XDaddTypeAnnotationsToSymbol=true"));And add this helper in the same class (outside the selected range), to keep
setup()clean and preserve immutability for callers:// Add near other helpers: private static List<String> buildDefaultArgs(String... baseArgs) { // Base immutable list List<String> args = List.of(baseArgs); // Only keep the -XD flag when it is relevant and safe (21u8+); skip on others. if (com.uber.nullaway.util.NullabilityUtil.isJDK21Update8OrHigher()) { return args; } // Remove the -XD flag if present (older JDKs or 22+ where it’s unnecessary) return args.stream() .filter(a -> !a.equals("-XDaddTypeAnnotationsToSymbol=true")) .toList(); }This ensures:
- Tests don’t rely on undocumented behavior of unknown
-XDkeys on older JDKs.- The flag remains present for JDK 21u8+ where the JSpecify bytecode reading fix exists and is gated by the flag.
Would you like me to push a follow-up commit that adds this helper and updates callers?
📜 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 (3)
guava-recent-unit-tests/src/test/java/com/uber/nullaway/guava/NullAwayGuavaParametricNullnessTests.java(6 hunks)nullaway/src/test/java/com/uber/nullaway/NullAwayTestsBase.java(2 hunks)nullaway/src/test/java/com/uber/nullaway/jspecify/BytecodeGenericsTests.java(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- guava-recent-unit-tests/src/test/java/com/uber/nullaway/guava/NullAwayGuavaParametricNullnessTests.java
🧰 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.094Z
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.094Z
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.094Z
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:
nullaway/src/test/java/com/uber/nullaway/NullAwayTestsBase.javanullaway/src/test/java/com/uber/nullaway/jspecify/BytecodeGenericsTests.java
🔇 Additional comments (2)
nullaway/src/test/java/com/uber/nullaway/jspecify/BytecodeGenericsTests.java (2)
5-6: LGTM on imports.Switching to
Listand addingAssumeimports is correct given the new usage.
251-254: LGTM: include -XDaddTypeAnnotationsToSymbol for jspecify testsSetting the flag here is correct since
setArgsreplaces the default args and these tests rely on reading type-use annotations from bytecode.List.of(...)usage also looks good.
|
@msridhar Does that mean |
|
@sdeleuze there is one test involving a downcast and an intersection type that only passes on JDK 23+ (see #1022). Beyond that one, all tests that pass on JDK 24 also pass on JDK 21.0.8+ with the extra flag. It's not high priority but if it became important we could probably fix #1022 on earlier JDKs as well. |
|
Good to know that we are pretty close. Let see how the potential Java 17 backport goes, that may increase the priority but we are not there yet. |
Some JSpecify tests now fail on earlier JDK 21 builds (see #1245). To avoid confusion, fail the whole build if one of these versions is being used.
As of JDK 21.0.8, the patch to properly expose type use annotations via standard APIs has landed, guarded behind the flag
-XDaddTypeAnnotationsToSymbol=true. This PR runs the relevant tests on JDK 21 by passing that flag where needed. This means NullAway regression tests will fail on versions of JDK 21 older than 21.0.8, but we can document that.Summary by CodeRabbit
Tests
Chores
Notes