Fail if NullAway is run in JSpecify mode with an incompatible javac version / configuration#1317
Fail if NullAway is run in JSpecify mode with an incompatible javac version / configuration#1317
Conversation
WalkthroughIntroduces a new test helper NullAwayJSpecifyConfig that centralizes JSpecify-related compiler flags and updates many tests to use its withJSpecifyModeArgs(...) helper instead of embedding flags inline. Adds runtime validation in NullAway to fail early when JSpecify mode is enabled but the Javac/JDK configuration is incompatible (requires JDK 22+ or the -XDaddTypeAnnotationsToSymbol flag). Adds a unit test that asserts the error when JSpecify mode is enabled on older JDKs without the required flag. Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1317 +/- ##
============================================
+ Coverage 88.44% 88.47% +0.02%
- Complexity 2541 2550 +9
============================================
Files 94 95 +1
Lines 8466 8488 +22
Branches 1673 1678 +5
============================================
+ Hits 7488 7510 +22
Misses 494 494
Partials 484 484 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
nullaway/src/test/java/com/uber/nullaway/jspecify/JSpecifyArrayTests.java (1)
709-712: Consolidate the -XD flag in a shared helper/constant.Looks good. To avoid typos across many suites, consider a shared helper in NullAwayTestsBase (e.g., makeJSpecifyArgs()) or a constant like ADD_TYPE_ANNOS_FLAG.
nullaway/src/test/java/com/uber/nullaway/jspecify/JSpecifyVarargsTests.java (1)
643-646: Prefer centralizing repeated -XD flag wiring.Mirrors other suites; consider reusing a single source of truth (helper/constant).
nullaway/src/test/java/com/uber/nullaway/ErrorProneCLIFlagsConfigTest.java (1)
62-72: Good guard: JSpecify requires JDK 22+ or -XD flag on older JDKs.Assertion string matches NullAway’s message; test runs only on JDK<22. Consider adding a complementary test that asserts no failure on JDK≥22 to cover the positive path.
nullaway/src/main/java/com/uber/nullaway/NullAway.java (1)
2829-2846: Validator handles both JDK≥22 and older with -XD flag.Minor: consider lifting the error message into a static constant to reuse in tests and avoid drift.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
jdk-annotations/jdk-integration-test/src/test/java/com/uber/nullaway/jdkannotations/JDKIntegrationTest.java(8 hunks)library-model/library-model-generator-integration-test/src/test/java/com/uber/nullaway/libmodel/LibraryModelIntegrationTest.java(6 hunks)nullaway/src/main/java/com/uber/nullaway/NullAway.java(4 hunks)nullaway/src/test/java/com/uber/nullaway/CustomLibraryModelsTests.java(1 hunks)nullaway/src/test/java/com/uber/nullaway/ErrorProneCLIFlagsConfigTest.java(2 hunks)nullaway/src/test/java/com/uber/nullaway/SerializationTest.java(3 hunks)nullaway/src/test/java/com/uber/nullaway/jspecify/GenericInheritanceTests.java(1 hunks)nullaway/src/test/java/com/uber/nullaway/jspecify/GenericMethodTests.java(1 hunks)nullaway/src/test/java/com/uber/nullaway/jspecify/GenericsTests.java(1 hunks)nullaway/src/test/java/com/uber/nullaway/jspecify/JSpecifyArrayTests.java(1 hunks)nullaway/src/test/java/com/uber/nullaway/jspecify/JSpecifyLibraryModelsTests.java(1 hunks)nullaway/src/test/java/com/uber/nullaway/jspecify/JSpecifyVarargsTests.java(2 hunks)nullaway/src/test/java/com/uber/nullaway/jspecify/NullMarkednessTests.java(2 hunks)nullaway/src/test/java/com/uber/nullaway/jspecify/SuggestedFixesTests.java(1 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:
nullaway/src/test/java/com/uber/nullaway/jspecify/JSpecifyLibraryModelsTests.javanullaway/src/test/java/com/uber/nullaway/jspecify/NullMarkednessTests.javajdk-annotations/jdk-integration-test/src/test/java/com/uber/nullaway/jdkannotations/JDKIntegrationTest.javanullaway/src/test/java/com/uber/nullaway/jspecify/SuggestedFixesTests.javalibrary-model/library-model-generator-integration-test/src/test/java/com/uber/nullaway/libmodel/LibraryModelIntegrationTest.javanullaway/src/test/java/com/uber/nullaway/jspecify/JSpecifyArrayTests.javanullaway/src/test/java/com/uber/nullaway/jspecify/GenericInheritanceTests.javanullaway/src/test/java/com/uber/nullaway/jspecify/JSpecifyVarargsTests.javanullaway/src/test/java/com/uber/nullaway/jspecify/GenericsTests.javanullaway/src/test/java/com/uber/nullaway/ErrorProneCLIFlagsConfigTest.javanullaway/src/test/java/com/uber/nullaway/jspecify/GenericMethodTests.javanullaway/src/main/java/com/uber/nullaway/NullAway.java
⏰ 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 macos-latest
- GitHub Check: Build and test on ubuntu-latest
- GitHub Check: Build caffeine with snapshot
- GitHub Check: Build and test on windows-latest
- GitHub Check: Build spring-framework with snapshot
🔇 Additional comments (17)
library-model/library-model-generator-integration-test/src/test/java/com/uber/nullaway/libmodel/LibraryModelIntegrationTest.java (1)
140-140: LGTM! Systematic addition of the javac flag for JSpecify mode support.The addition of
-XDaddTypeAnnotationsToSymbol=trueto all tests using JSpecifyMode is consistent and correct. This enables JSpecify tests to work on JDK versions prior to 22 by ensuring type annotations are properly propagated to symbols during compilation. The pattern is applied uniformly across all JSpecify-enabled tests in this file.Note: Line 167 also adds
JSpecifyMode=truetolibraryModelNullableUpperBoundsWithoutJarInferTest(), which is appropriate given the test uses@Nullabletype annotations.Also applies to: 167-168, 190-190, 215-215, 243-243, 270-270
jdk-annotations/jdk-integration-test/src/test/java/com/uber/nullaway/jdkannotations/JDKIntegrationTest.java (1)
126-151: Cross-repo verification identified incomplete flag coverage.While the JDKIntegrationTest.java changes are consistent, a search across the repository reveals that
ErrorProneCLIFlagsConfigTest.java(line 67) usesJSpecifyMode=truewithout the-XDaddTypeAnnotationsToSymbol=trueflag. This suggests the bytecode annotation flag was not systematically applied to all JSpecifyMode tests. Confirm whether:
- This test file was intentionally excluded or requires the flag added
- Any other test files with JSpecifyMode lack the bytecode annotation flag
nullaway/src/test/java/com/uber/nullaway/jspecify/SuggestedFixesTests.java (1)
24-24: No issues found. Change is correct and consistent with JSpecify mode requirements.The codebase already validates that the
-XDaddTypeAnnotationsToSymbol=trueflag works as an alternative to JDK 22+ for enabling type annotation support. The production code in NullAway.java confirms this by accepting either JDK 22+ OR the flag on older JDKs. Existing tests verify this requirement, and the flag is consistently used across all JSpecify-related test files.nullaway/src/test/java/com/uber/nullaway/jspecify/NullMarkednessTests.java (2)
870-877: LGTM! Consistent test configuration update.The addition of
-XDaddTypeAnnotationsToSymbol=truecorrectly enables type-annotation propagation for JSpecify mode testing, consistent with the broader PR changes.
904-911: LGTM! Consistent test configuration update.The addition of
-XDaddTypeAnnotationsToSymbol=truecorrectly enables type-annotation propagation for JSpecify mode testing, consistent with the broader PR changes.nullaway/src/test/java/com/uber/nullaway/CustomLibraryModelsTests.java (1)
269-276: LGTM! Consistent test configuration update.The addition of
-XDaddTypeAnnotationsToSymbol=truecorrectly enables type-annotation propagation for JSpecify mode testing, consistent with the broader PR changes.nullaway/src/test/java/com/uber/nullaway/jspecify/JSpecifyLibraryModelsTests.java (1)
32-38: LGTM! Consistent test configuration update.The addition of
-XDaddTypeAnnotationsToSymbol=trueto themakeHelper()method correctly enables type-annotation propagation for all JSpecify library model tests, consistent with the broader PR changes.nullaway/src/test/java/com/uber/nullaway/jspecify/GenericsTests.java (1)
2705-2711: LGTM! Consistent test configuration update.The addition of
-XDaddTypeAnnotationsToSymbol=trueto themakeHelper()method correctly enables type-annotation propagation for all generics tests in JSpecify mode, consistent with the broader PR changes.nullaway/src/test/java/com/uber/nullaway/jspecify/GenericMethodTests.java (1)
1400-1415: LGTM! Consistent test configuration updates.The addition of
-XDaddTypeAnnotationsToSymbol=trueto bothmakeHelper()andmakeHelperWithInferenceFailureWarning()correctly enables type-annotation propagation for all generic method tests in JSpecify mode, consistent with the broader PR changes.nullaway/src/test/java/com/uber/nullaway/SerializationTest.java (3)
2141-2142: LGTM: ensure symbols carry type-use annotations in JSpecify mode.
2184-2185: LGTM: adds -XDaddTypeAnnotationsToSymbol for local var case.
2227-2228: LGTM: adds -XDaddTypeAnnotationsToSymbol for lambda case.nullaway/src/test/java/com/uber/nullaway/jspecify/JSpecifyVarargsTests.java (1)
468-469: LGTM: add -XD flag for restrictive varargs scenario.nullaway/src/test/java/com/uber/nullaway/jspecify/GenericInheritanceTests.java (1)
258-261: LGTM: include -XDaddTypeAnnotationsToSymbol in JSpecify helper.nullaway/src/main/java/com/uber/nullaway/NullAway.java (3)
231-231: State flag is fine.Single boolean to avoid repeated checks is OK; config is immutable per instance.
97-97: Import usage is appropriate.Using com.sun.tools.javac.util.Options is consistent with existing javac integrations.
1709-1718: Code implementation is correct—flag name verified.The hidden option
-XDaddTypeAnnotationsToSymbolis confirmed to exist and be honored on JDK 19, 20, and 21 (OpenJDK), with the key spelled exactly as "addTypeAnnotationsToSymbol". The gate correctly checks JSpecify mode and throws a fail-fast error with clear messaging.
|
@sdeleuze FYI |
|
This change will help developers to have the right arrangement, and avoid a lot of false positive in the bug tracker. Looks great. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
nullaway/src/main/java/com/uber/nullaway/testhelper/NullAwayJSpecifyConfig.java (1)
7-12: Clear documentation of the helper's purpose.The Javadoc explains that both flags must be passed together. Consider adding a brief note about why (e.g., "Required for javac to correctly read type-use annotations from bytecode in JSpecify mode") for better context, though the current doc is acceptable.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
guava-recent-unit-tests/src/test/java/com/uber/nullaway/guava/NullAwayGuavaParametricNullnessTests.java(2 hunks)jdk-annotations/jdk-integration-test/src/test/java/com/uber/nullaway/jdkannotations/JDKIntegrationTest.java(9 hunks)library-model/library-model-generator-integration-test/src/test/java/com/uber/nullaway/libmodel/LibraryModelIntegrationTest.java(7 hunks)nullaway/src/main/java/com/uber/nullaway/testhelper/NullAwayJSpecifyConfig.java(1 hunks)nullaway/src/test/java/com/uber/nullaway/CustomLibraryModelsTests.java(2 hunks)nullaway/src/test/java/com/uber/nullaway/SerializationTest.java(4 hunks)nullaway/src/test/java/com/uber/nullaway/jspecify/BytecodeGenericsTests.java(2 hunks)nullaway/src/test/java/com/uber/nullaway/jspecify/GenericInheritanceTests.java(2 hunks)nullaway/src/test/java/com/uber/nullaway/jspecify/GenericMethodTests.java(2 hunks)nullaway/src/test/java/com/uber/nullaway/jspecify/GenericsTests.java(2 hunks)nullaway/src/test/java/com/uber/nullaway/jspecify/JSpecifyArrayTests.java(2 hunks)nullaway/src/test/java/com/uber/nullaway/jspecify/JSpecifyLibraryModelsTests.java(2 hunks)nullaway/src/test/java/com/uber/nullaway/jspecify/JSpecifyVarargsTests.java(3 hunks)nullaway/src/test/java/com/uber/nullaway/jspecify/NullMarkednessTests.java(3 hunks)nullaway/src/test/java/com/uber/nullaway/jspecify/SuggestedFixesTests.java(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- nullaway/src/test/java/com/uber/nullaway/jspecify/JSpecifyArrayTests.java
- nullaway/src/test/java/com/uber/nullaway/jspecify/GenericInheritanceTests.java
- nullaway/src/test/java/com/uber/nullaway/jspecify/JSpecifyVarargsTests.java
- nullaway/src/test/java/com/uber/nullaway/SerializationTest.java
- nullaway/src/test/java/com/uber/nullaway/jspecify/NullMarkednessTests.java
- nullaway/src/test/java/com/uber/nullaway/jspecify/JSpecifyLibraryModelsTests.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.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:
nullaway/src/test/java/com/uber/nullaway/jspecify/GenericsTests.javanullaway/src/test/java/com/uber/nullaway/jspecify/GenericMethodTests.javalibrary-model/library-model-generator-integration-test/src/test/java/com/uber/nullaway/libmodel/LibraryModelIntegrationTest.javanullaway/src/test/java/com/uber/nullaway/jspecify/BytecodeGenericsTests.javajdk-annotations/jdk-integration-test/src/test/java/com/uber/nullaway/jdkannotations/JDKIntegrationTest.javanullaway/src/main/java/com/uber/nullaway/testhelper/NullAwayJSpecifyConfig.javanullaway/src/test/java/com/uber/nullaway/CustomLibraryModelsTests.javaguava-recent-unit-tests/src/test/java/com/uber/nullaway/guava/NullAwayGuavaParametricNullnessTests.javanullaway/src/test/java/com/uber/nullaway/jspecify/SuggestedFixesTests.java
🔇 Additional comments (14)
nullaway/src/test/java/com/uber/nullaway/jspecify/GenericsTests.java (1)
708-709: LGTM! Clean refactoring to centralize JSpecify mode configuration.The use of
NullAwayJSpecifyConfig.withJSpecifyModeArgs()ensures both-XepOpt:NullAway:JSpecifyMode=trueand-XDaddTypeAnnotationsToSymbol=trueare consistently supplied together, improving maintainability.nullaway/src/test/java/com/uber/nullaway/jspecify/GenericMethodTests.java (1)
1403-1404: LGTM! Consistent use of the JSpecify configuration helper.Both test helper methods correctly use
withJSpecifyModeArgs()to centralize flag management.Also applies to: 1409-1412
nullaway/src/test/java/com/uber/nullaway/CustomLibraryModelsTests.java (1)
272-276: LGTM! JSpecify configuration properly centralized.The helper correctly wraps the test-specific arguments while adding the required JSpecify mode flags.
guava-recent-unit-tests/src/test/java/com/uber/nullaway/guava/NullAwayGuavaParametricNullnessTests.java (1)
62-66: LGTM! Clean integration of the JSpecify configuration helper.Note: This uses
List.of()instead ofArrays.asList()(both produce immutable lists, so functionally equivalent).library-model/library-model-generator-integration-test/src/test/java/com/uber/nullaway/libmodel/LibraryModelIntegrationTest.java (1)
136-141: LGTM! Consistent refactoring across all test methods.All test methods correctly adopt the centralized JSpecify configuration helper, maintaining the same argument structure while delegating flag management to
NullAwayJSpecifyConfig.Also applies to: 163-167, 184-189, 208-213, 235-240, 261-266
nullaway/src/test/java/com/uber/nullaway/jspecify/SuggestedFixesTests.java (1)
19-34: LGTM! Appropriate use ofjspecifyModeArgs()for flexible argument ordering.This test uses
jspecifyModeArgs()directly (rather thanwithJSpecifyModeArgs()) because it needs to append additional flags (SuggestSuppressions) after the JSpecify configuration. The mutableArrayListapproach correctly handles this requirement.jdk-annotations/jdk-integration-test/src/test/java/com/uber/nullaway/jdkannotations/JDKIntegrationTest.java (1)
130-135: LGTM! Comprehensive adoption of centralized JSpecify configuration.All test methods consistently use
withJSpecifyModeArgs()to manage JSpecify-related flags, improving maintainability across the test suite.Also applies to: 157-161, 178-183, 202-207, 229-234, 255-260, 284-289, 310-315
nullaway/src/main/java/com/uber/nullaway/testhelper/NullAwayJSpecifyConfig.java (4)
15-19: Well-designed constants and internal flag list.The public constants allow direct access when needed, while the private
JSPECIFY_MODE_ARGSencapsulates the combined list. UsingList.of()ensures immutability by default.
21-21: Good use of private constructor.Prevents instantiation of this utility class, following best practices for utility/helper classes.
24-26: Clean accessor for JSpecify mode arguments.Returns the immutable internal list directly, which is safe since
List.of()creates an unmodifiable list.
33-38: Robust implementation ofwithJSpecifyModeArgs.Correctly creates a new
ArrayList(preventing modification of the input), pre-sizes it efficiently, and returns an unmodifiable view. This ensures both immutability and good performance.nullaway/src/test/java/com/uber/nullaway/jspecify/BytecodeGenericsTests.java (3)
5-5: LGTM! Import is correctly added.The import for
NullAwayJSpecifyConfigis properly added and used in themakeHelper()method below.
250-254: LGTM! Clean refactoring to centralize JSpecify configuration.The migration to
NullAwayJSpecifyConfig.withJSpecifyModeArgs()correctly centralizes the JSpecify mode configuration and should automatically provide the necessary flags (including-XDaddTypeAnnotationsToSymbol=truefor JDK < 22).
79-101: Consider removingAssume.assumeTruechecks given the new configuration.Three test methods (
genericsChecksForFieldAssignments,genericsChecksForParamPassingAndReturns, andcallMethodTakingJavaUtilFunction) skip execution on JDK < 21 usingAssume.assumeTrue(Runtime.version().feature() >= 21).Given that:
NullAwayJSpecifyConfignow provides the necessary flags for JSpecify mode (including-XDaddTypeAnnotationsToSymbol=true)- The PR introduces an early failure mechanism for incompatible JSpecify configurations
- These checks may now be redundant if the configuration helper enables proper bytecode annotation reading on JDK 21+
Should these
Assume.assumeTruechecks be removed to let tests fail explicitly when run on incompatible JDK versions, rather than silently skipping? This would align with the preference to fail tests on unsupported versions to ensure CI catches regressions and enforces proper JDK usage.Based on learnings
Also applies to: 104-128, 233-248
lazaroclapp
left a comment
There was a problem hiding this comment.
LGTM!
A fun trick we could also try as an alternative here is to create a new jar artifact that NullAway depends on, and test the loading of bytecode annotations, rather than check JDK version/config. Not sure how easy/robust that is compared to this approach. It would be more precise in cases where the JDK distro or small version doesn't support -XDaddTypeAnnotationsToSymbol, but would add the brittleness of needing another dependency and the cost of loading it at compile time.
I think an issue with this is that we'd need to inject that artifact into the classpath of the program being compiled. If only NullAway itself depends on the artifact, I don't think we'll be able to reference it with |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
nullaway/src/main/java/com/uber/nullaway/NullAway.java (1)
2829-2845: Flag parsing is correct; document that “=true” is required, and consider a tiny polish.
- Boolean.parseBoolean(opts.get(key)) enforces -XDaddTypeAnnotationsToSymbol=true (bare -XDaddTypeAnnotationsToSymbol remains false). Good. A short comment will prevent regressions.
- Optional: lift the key into a private static final String to avoid typos and aid reuse.
Comment-only diff to clarify the “=true” requirement:
if (version.feature() < 22) { Options opts = Options.instance(state.context); - String key = "addTypeAnnotationsToSymbol"; + String key = "addTypeAnnotationsToSymbol"; // Requires -XDaddTypeAnnotationsToSymbol=true. + // Note: a bare -XDaddTypeAnnotationsToSymbol (no '=true') yields an empty value, + // and Boolean.parseBoolean("") is false, which is intended. if (!opts.isSet(key)) { return false; } return Boolean.parseBoolean(opts.get(key)); } else {Optional constant (outside this range):
// Near other constants: private static final String ADD_TYPE_ANNOTS_TO_SYMBOL = "addTypeAnnotationsToSymbol";Then reference ADD_TYPE_ANNOTS_TO_SYMBOL in this method.
Also consider moving this helper to a small utility (e.g., JavacCompat) to reduce NullAway.java size.
🧹 Nitpick comments (1)
nullaway/src/main/java/com/uber/nullaway/NullAway.java (1)
1709-1718: Good fail-fast placement; add a brief comment explaining why this isn’t in the constructor.
You need VisitorState to read javac Options; documenting that here will preempt questions from future readers.Apply this comment-only diff:
if (codeAnnotationInfo == null) { codeAnnotationInfo = CodeAnnotationInfo.instance(state.context); } - if (!checkedJDKVersionForJSpecifyMode) { + // Run this check here (not in the constructor) because we need VisitorState.context + // to read javac Options. Throwing here fails the build early if JSpecify mode isn't + // supported by the current javac configuration. + if (!checkedJDKVersionForJSpecifyMode) { checkedJDKVersionForJSpecifyMode = true; if (config.isJSpecifyMode() && !isValidJavacConfigForJSpecifyMode(state)) { String msg = "Running NullAway in JSpecify mode requires either JDK 22+" + " or passing the flag -XDaddTypeAnnotationsToSymbol=true to an older JDK that supports it;" + " see https://github.com/uber/NullAway/wiki/JSpecify-Support#supported-jdk-versions for details."; throw new IllegalStateException(msg); } }This aligns with the preference to fail rather than silently skip on unsupported JDKs. Based on learnings.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
nullaway/src/main/java/com/uber/nullaway/NullAway.java(4 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:
nullaway/src/main/java/com/uber/nullaway/NullAway.java
⏰ 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 caffeine with snapshot
- GitHub Check: Build and test on macos-latest
- GitHub Check: Build and test on windows-latest
- GitHub Check: Build spring-framework with snapshot
- GitHub Check: Build and test on ubuntu-latest
🔇 Additional comments (2)
nullaway/src/main/java/com/uber/nullaway/NullAway.java (2)
97-97: LGTM: Options import is appropriate for reading -XD flags.
Consistent with existing javac APIs used throughout the file.
231-232: One-time guard is fine.
Single-instance, single-threaded checker assumption is already documented below; this boolean is enough to avoid repeated checks per compilation.If EP ever parallelizes matchers for a single checker instance, we’d need volatile or other synchronization. Not required today.
|
@coderabbitai talk to me. |
|
Tip For best results, initiate chat on the files or code changes. @nimakarimipour! Happy to discuss this PR with you. Overview of PR #1317: This PR addresses issue #1310 by making NullAway fail fast when run in JSpecify mode on incompatible javac configurations. The core problem was that on JDK versions before 22 (without the Key Changes:
Observations:
Is there anything specific you'd like me to focus on or help with regarding this PR? For example:
🧠 Learnings used |
Fixes #1310
NullAway now fails with an exception if run on an incompatible javac configuration, i.e., a configuration that does not support reading type use annotations from bytecode (see https://github.com/uber/NullAway/wiki/JSpecify-Support#supported-jdk-versions). Specifically, NullAway fails if it is running on JDK version before 22 and the
-XDaddTypeAnnotationsToSymbol=truehas not been passed tojavac. Note thatjavacversions that do not recognize the-XDaddTypeAnnotationsToSymbolflag simply ignore it, so this check does not ensure we are on a compatible JDK. But, it's better than nothing, and doing a more careful check would be hard, since some distributions of the same JDK version may not support the flag. Also, there is hope for this flag to be backported to JDK 17u, and this way we won't need to change our logic when that happens. Documentation of supported versions is at https://github.com/uber/NullAway/wiki/JSpecify-Support#supported-jdk-versions and that URL is included in the error message. When this PR lands, we will update the docs to indicate the stronger version requirement.Rather than passing
"-XDaddTypeAnnotationsToSymbol=true"in many places in our test code, we introduce aJSpecifyJavacConfigtest helper class and use it wherever we want to run tests in JSpecify mode, to ensure the right flags are passed together.Summary by CodeRabbit
Bug Fixes
Tests