Generate astubx from JSON output of jdk-javac-plugin #1243
Conversation
WalkthroughAdds five jdk-annotations modules and includes them in settings.gradle: astubx-generator, astubx-generator-cli, test-annotated, test-unannotated, and jdk-integration-test. Introduces AstubxGenerator and AstubxGeneratorCLI to convert compiler-emitted JSON into .astubx files, wires a Gradle Exec task Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
msridhar
left a comment
msridhar
left a comment
There was a problem hiding this comment.
Some high level comments; haven't reviewed the actual logic. Looking forward to seeing the integration test working, and then I'll review in more detail
.../astubx-generator-cli/src/main/java/com/uber/nullaway/jdkannotations/AstubxGeneratorCLI.java
Outdated
Show resolved
Hide resolved
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1243 +/- ##
============================================
- Coverage 88.49% 88.39% -0.10%
- Complexity 2479 2516 +37
============================================
Files 93 94 +1
Lines 8228 8393 +165
Branches 1623 1657 +34
============================================
+ Hits 7281 7419 +138
- Misses 474 491 +17
- Partials 473 483 +10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
It's a good start! The comments below are a couple of initial minor comments. The major issue, though, is that I don't think the jdk-integration-test project is set up correctly. Right now, it depends on the test-annotated project. This means it sees the annotated code, which means we are not testing if the astubx file is being loaded correctly. Here is what should happen instead, at a high level.
test-annotatedshould be compiled with thejdk-javac-pluginas a javac plugin, producing a JSON file in some location. Then this JSON should be converted to an astubx fileoutput.astubxusing the new CLI you've written here. I think this is already happening.test-unannotatedshould not be compiled with thejdk-javac-plugin; there is no reason to do so, as it has no annotations. Instead, after it gets compiled, theoutput.astubxgenerated from step 1 should be added to its jar file. There is logic to do something like this here usingjar ufthat you can base this on.jdk-integration-testshould depend ontest-unannotated. After step 2, the jar fortest-unannotatedshould contain the unannotated class files plus theastubxwith the annotation info. Then, the tests should only pass if theastubxis getting loaded correctly.
If something doesn't make sense let me know.
.../astubx-generator-cli/src/main/java/com/uber/nullaway/jdkannotations/AstubxGeneratorCLI.java
Show resolved
Hide resolved
.../astubx-generator-cli/src/main/java/com/uber/nullaway/jdkannotations/AstubxGeneratorCLI.java
Outdated
Show resolved
Hide resolved
.../astubx-generator-cli/src/main/java/com/uber/nullaway/jdkannotations/AstubxGeneratorCLI.java
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
jdk-annotations/astubx-generator/src/main/java/com/uber/nullaway/jdkannotations/AstubxGenerator.java(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-08-28T04:54:20.953Z
Learnt from: msridhar
PR: uber/NullAway#1248
File: nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java:847-857
Timestamp: 2025-08-28T04:54:20.953Z
Learning: In NullAway's GenericsChecks.java, NewClassTree support for explicit type argument substitution requires more extensive changes beyond just modifying the conditional in compareGenericTypeParameterNullabilityForCall. The maintainers prefer to handle NewClassTree support in a separate follow-up rather than expanding the scope of PRs focused on specific issues like super constructor calls.
Applied to files:
jdk-annotations/astubx-generator/src/main/java/com/uber/nullaway/jdkannotations/AstubxGenerator.java
📚 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-annotations/astubx-generator/src/main/java/com/uber/nullaway/jdkannotations/AstubxGenerator.java
📚 Learning: 2025-08-29T18:41:43.584Z
Learnt from: msridhar
PR: uber/NullAway#1259
File: jdk-recent-unit-tests/src/test/java/com/uber/nullaway/jdk17/SwitchTests.java:318-321
Timestamp: 2025-08-29T18:41:43.584Z
Learning: Classes annotated with NullMarked are analyzed by NullAway even if they are not in packages specified by the AnnotatedPackages configuration. The NullMarked annotation guarantees NullAway analysis.
Applied to files:
jdk-annotations/astubx-generator/src/main/java/com/uber/nullaway/jdkannotations/AstubxGenerator.java
🧬 Code graph analysis (1)
jdk-annotations/astubx-generator/src/main/java/com/uber/nullaway/jdkannotations/AstubxGenerator.java (2)
jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java (1)
NullnessAnnotationSerializer(35-187)library-model/library-model-generator/src/main/java/com/uber/nullaway/libmodel/StubxWriter.java (1)
StubxWriter(15-153)
...tations/astubx-generator/src/main/java/com/uber/nullaway/jdkannotations/AstubxGenerator.java
Show resolved
Hide resolved
msridhar
left a comment
msridhar
left a comment
There was a problem hiding this comment.
Let's look at this one more comment, but aim to get this landed soon
...tations/astubx-generator/src/main/java/com/uber/nullaway/jdkannotations/AstubxGenerator.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
jdk-annotations/astubx-generator/src/main/java/com/uber/nullaway/jdkannotations/AstubxGenerator.java(1 hunks)jdk-annotations/astubx-generator/src/test/java/com/uber/nullaway/jdkannotations/AstubxTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- jdk-annotations/astubx-generator/src/main/java/com/uber/nullaway/jdkannotations/AstubxGenerator.java
🧰 Additional context used
🧬 Code graph analysis (1)
jdk-annotations/astubx-generator/src/test/java/com/uber/nullaway/jdkannotations/AstubxTest.java (1)
jdk-annotations/astubx-generator/src/main/java/com/uber/nullaway/jdkannotations/AstubxGenerator.java (1)
AstubxGenerator(34-309)
🔇 Additional comments (3)
jdk-annotations/astubx-generator/src/test/java/com/uber/nullaway/jdkannotations/AstubxTest.java (3)
23-44: LGTM!The test setup is well-structured. The DummyChecker provides the necessary BugChecker instance for CompilationTestHelper, and the setup method correctly configures the NullnessAnnotationSerializer plugin with the temporary directory for JSON output.
46-371: Comprehensive test coverage.The test suite covers a wide range of nullness annotation scenarios including nullable returns, parameters, upper bounds, generics, arrays, primitives, and void methods. The tests are well-structured with clear expectations and follow a consistent pattern that enhances readability.
311-314: Confirm asymmetric generic handling. AstubxGenerator intentionally strips generic parameters from method parameters (lines 212–231) but retains them in return types (lines 202–207). Verify this aligns with the astubx format specification.
jdk-annotations/astubx-generator/src/test/java/com/uber/nullaway/jdkannotations/AstubxTest.java
Outdated
Show resolved
Hide resolved
jdk-annotations/astubx-generator/src/test/java/com/uber/nullaway/jdkannotations/AstubxTest.java
Show resolved
Hide resolved
…ay/jdkannotations/AstubxTest.java Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
jdk-annotations/test-annotated/src/main/java/com/uber/nullaway/jdkannotations/ParameterAnnotation.java (1)
22-24: Redundant@NonNullannotation under@NullMarked.Under
@NullMarked, parameters are non-null by default, so the explicit@NonNullon line 22 is redundant. While not incorrect, removing it would align with JSpecify conventions where non-null is implicit within@NullMarkedscopes.Apply this diff to remove the redundant annotation:
- public static void printObjectString(@NonNull Object object) { + public static void printObjectString(Object object) {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
jdk-annotations/jdk-integration-test/src/test/java/com/uber/nullaway/jdkannotations/JDKIntegrationTest.java(1 hunks)jdk-annotations/test-annotated/src/main/java/com/uber/nullaway/jdkannotations/ParameterAnnotation.java(1 hunks)jdk-annotations/test-unannotated/src/main/java/com/uber/nullaway/jdkannotations/ParameterAnnotation.java(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 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-annotations/jdk-integration-test/src/test/java/com/uber/nullaway/jdkannotations/JDKIntegrationTest.java
🧬 Code graph analysis (2)
jdk-annotations/test-annotated/src/main/java/com/uber/nullaway/jdkannotations/ParameterAnnotation.java (2)
jdk-annotations/test-annotated/src/main/java/com/uber/nullaway/jdkannotations/ReturnAnnotation.java (1)
NullMarked(7-66)jdk-annotations/test-unannotated/src/main/java/com/uber/nullaway/jdkannotations/ParameterAnnotation.java (2)
ParameterAnnotation(5-52)Generic(32-41)
jdk-annotations/test-unannotated/src/main/java/com/uber/nullaway/jdkannotations/ParameterAnnotation.java (1)
jdk-annotations/test-annotated/src/main/java/com/uber/nullaway/jdkannotations/ParameterAnnotation.java (1)
Generic(36-45)
🔇 Additional comments (6)
jdk-annotations/test-annotated/src/main/java/com/uber/nullaway/jdkannotations/ParameterAnnotation.java (2)
7-8: LGTM!The
@NullMarkedannotation correctly establishes non-null as the default for all type uses in this class, aligning with JSpecify semantics.
10-12: LGTM!The method implementations correctly demonstrate various nullability scenarios for testing:
- Default non-null parameters (
add)- Explicit
@Nullableparameters (getNewObjectIfNull,takesNullArray,getString)- Array type annotations (
Object @Nullable []vsObject[])- Generic type parameter nullability
All implementations align with the test expectations in
JDKIntegrationTest.Also applies to: 14-20, 27-34, 36-45, 48-55
jdk-annotations/test-unannotated/src/main/java/com/uber/nullaway/jdkannotations/ParameterAnnotation.java (2)
3-3: Verify the intended purpose of annotations in the "unannotated" module.The
test-unannotatedmodule importsorg.jspecify.annotations.Nullableand uses it on line 44, which seems inconsistent with the module's "unannotated" designation. Please confirm whether:
- The module should be completely unannotated (in which case line 44's
@Nullableshould be removed)- The module intentionally includes minimal annotations for specific test scenarios (in which case the name might be misleading)
Looking at the test expectations in
JDKIntegrationTest.nullableGenericArrayTest()(lines 265-274), the test expectstakesNullGenericArray(null)to succeed andtakesNonNullGenericArray(null)to fail, suggesting the annotation on line 44 may be intentional for testing.
6-8: LGTM!The method implementations correctly mirror the annotated version's API surface, providing a baseline for testing NullAway's behavior with and without explicit annotations.
Also applies to: 10-16, 18-20, 23-30, 32-41, 44-51
jdk-annotations/jdk-integration-test/src/test/java/com/uber/nullaway/jdkannotations/JDKIntegrationTest.java (2)
17-20: LGTM!The setup correctly initializes
CompilationTestHelperwithNullAway.classfor each test, ensuring proper test isolation.
22-301: Enforce test failure on unsupported JDK versions
The build config currently disables thetestJdk17task viaonlyIf { false }. Per our JSpecify testing guidelines, unsupported JDKs should produce test failures (not be skipped) to catch regressions. Please replace the skip with a version check that fails on JDK < 21.
(jdk-annotations/jdk-integration-test/build.gradle)⛔ Skipped due to 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.Learnt from: msridhar PR: uber/NullAway#1243 File: jdk-annotations/astubx-generator/build.gradle:22-22 Timestamp: 2025-10-09T19:59:16.521Z Learning: When disabling testJdk17 tasks for modules requiring JDK 21, use `onlyIf { false }` to skip the task: ```gradle tasks.named("testJdk17").configure { onlyIf { false } } ``` Do not use `doFirst { throw new GradleException(...) }` as it will cause CI failures when the task is executed.
| ImmutableMap.of( | ||
| "ParameterizedTypeArray: ParameterizedTypeArray()", | ||
| MethodAnnotationsRecord.create(ImmutableSet.of(), ImmutableMap.of()), | ||
| "ParameterizedTypeArray:java.util.List<java.lang.String>[] getList()", |
There was a problem hiding this comment.
I'm wondering, why do we need java.util.List<java.lang.String>[] for the return type, but java.util.List[] without the generic for the parameter type? Is this some weirdness in astubx format?
There was a problem hiding this comment.
Yes, this is weird. I'll look into this.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
jdk-annotations/astubx-generator/src/test/java/com/uber/nullaway/jdkannotations/AstubxTest.java (1)
388-393: Unresolved: clarify or remove the fallback condition.This concern was raised in a previous review and remains unaddressed. The fallback logic allows the test to pass when the astubx file doesn't exist if all expectations are empty, which could mask file creation failures. Since
AstubxGenerator.writeToAstubxFilealways creates the file and no current test exercises the fallback condition (all tests have non-emptyexpectedMethodRecords), the fallback is effectively dead code that may hide bugs.Consider one of the following approaches:
Option 1: Always expect file creation (recommended)
Assert.assertTrue( "astubx file was not created", - Files.exists(Paths.get(astubxOutputDirPath, "output.astubx")) - || (expectedMethodRecords.isEmpty() - && expectedNullableUpperBounds.isEmpty() - && expectedNullMarkedClasses.isEmpty())); + Files.exists(Paths.get(astubxOutputDirPath, "output.astubx")));Option 2: Explicitly document and test the empty case
- Assert.assertTrue( - "astubx file was not created", - Files.exists(Paths.get(astubxOutputDirPath, "output.astubx")) - || (expectedMethodRecords.isEmpty() - && expectedNullableUpperBounds.isEmpty() - && expectedNullMarkedClasses.isEmpty())); + if (expectedMethodRecords.isEmpty() + && expectedNullableUpperBounds.isEmpty() + && expectedNullMarkedClasses.isEmpty()) { + // Document: for empty data, file creation is optional (or should not happen) + // Add explicit assertion here based on intended behavior + return; + } + Assert.assertTrue( + "astubx file was not created", + Files.exists(Paths.get(astubxOutputDirPath, "output.astubx")));
🧹 Nitpick comments (1)
jdk-annotations/astubx-generator/src/test/java/com/uber/nullaway/jdkannotations/AstubxTest.java (1)
377-377: Simplify redundant path construction.The
Paths.get(...).toString()wrapper is unnecessary sincegetAbsolutePath()already returns a String.Apply this diff to simplify:
- String astubxOutputDirPath = Paths.get(astubxFolder.getRoot().getAbsolutePath()).toString(); + String astubxOutputDirPath = astubxFolder.getRoot().getAbsolutePath();
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
jdk-annotations/astubx-generator/src/test/java/com/uber/nullaway/jdkannotations/AstubxTest.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
jdk-annotations/astubx-generator/src/test/java/com/uber/nullaway/jdkannotations/AstubxTest.java (1)
jdk-annotations/astubx-generator/src/main/java/com/uber/nullaway/jdkannotations/AstubxGenerator.java (1)
AstubxGenerator(34-309)
⏰ 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 caffeine with snapshot
- GitHub Check: Build and test on macos-latest
- GitHub Check: Build spring-framework with snapshot
- GitHub Check: Build and test on windows-latest
🔇 Additional comments (1)
jdk-annotations/astubx-generator/src/test/java/com/uber/nullaway/jdkannotations/AstubxTest.java (1)
1-371: Comprehensive test coverage.The test suite thoroughly covers various nullness annotation scenarios including nullable returns, upper bounds, parameters, generics, arrays, primitives, and void methods. The test structure using CompilationTestHelper with a DummyChecker to trigger the NullnessAnnotationSerializer plugin is appropriate and well-organized.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
jdk-annotations/astubx-generator/src/test/java/com/uber/nullaway/jdkannotations/AstubxTest.java (1)
388-393: Address the unresolved fallback condition in file existence assertion.The fallback logic allowing the test to pass when the astubx file doesn't exist (if all expectations are empty) was flagged in a previous review but remains unaddressed. This condition could mask file creation failures.
As previously recommended, consider one of these approaches:
Option 1: Always require file creation
Assert.assertTrue( "astubx file was not created", - Files.exists(Paths.get(astubxOutputDirPath, "output.astubx")) - || (expectedMethodRecords.isEmpty() - && expectedNullableUpperBounds.isEmpty() - && expectedNullMarkedClasses.isEmpty())); + Files.exists(Paths.get(astubxOutputDirPath, "output.astubx")));Option 2: Explicitly document the empty-data behavior
+// When all data collections are empty, file creation is optional Assert.assertTrue( "astubx file was not created", Files.exists(Paths.get(astubxOutputDirPath, "output.astubx")) || (expectedMethodRecords.isEmpty() && expectedNullableUpperBounds.isEmpty() && expectedNullMarkedClasses.isEmpty()));
🧹 Nitpick comments (1)
jdk-annotations/astubx-generator/src/test/java/com/uber/nullaway/jdkannotations/AstubxTest.java (1)
291-316: Document or align generics handling for parameters
getArgumentsAsArraydeliberately strips generic type arguments from parameters (e.g.java.util.List[]), while return types keep generics (e.g.java.util.List<java.lang.String>[]). If this asymmetry is part of the Astubx format, add a note in the spec; otherwise updategetArgumentsAsArrayto preserve parameter generics.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
jdk-annotations/astubx-generator/src/test/java/com/uber/nullaway/jdkannotations/AstubxTest.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
jdk-annotations/astubx-generator/src/test/java/com/uber/nullaway/jdkannotations/AstubxTest.java (1)
jdk-annotations/astubx-generator/src/main/java/com/uber/nullaway/jdkannotations/AstubxGenerator.java (1)
AstubxGenerator(34-309)
🔇 Additional comments (4)
jdk-annotations/astubx-generator/src/test/java/com/uber/nullaway/jdkannotations/AstubxTest.java (4)
1-35: LGTM!The test class structure, imports, and DummyChecker implementation are well-organized. The use of TemporaryFolder rules ensures proper test isolation and cleanup.
37-44: LGTM!The setup method correctly initializes the CompilationTestHelper with appropriate compiler arguments for testing the NullnessAnnotationSerializer plugin.
46-289: LGTM!The test methods provide comprehensive coverage of various nullness annotation scenarios including nullable returns, parameters, upper bounds, and different type contexts. The consistent test structure enhances maintainability.
318-371: LGTM!The tests for primitive return types and void returns correctly validate the handling of different return type categories.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
jdk-annotations/test-annotated/src/main/java/com/uber/nullaway/jdkannotations/ReturnAnnotation.java (1)
18-28: Return may be null; add return-level @nullable.Under @NullMarked, the method returns null for size <= 0 (Line 20) but the return type isn’t annotated @nullable. Annotate the method return in addition to the array element type.
- public Integer @Nullable [] generateIntArray(int size) { + public @Nullable Integer @Nullable [] generateIntArray(int size) {
🧹 Nitpick comments (1)
jdk-annotations/jdk-integration-test/src/test/java/com/uber/nullaway/jdkannotations/JDKIntegrationTest.java (1)
17-21: Optional: factor out repeated setArgs logic.Reduce duplication and make the flag matrix clearer with a small helper.
@Before public void setup() { compilationHelper = CompilationTestHelper.newInstance(NullAway.class, getClass()); } + + private static java.util.List<String> args( + org.junit.rules.TemporaryFolder tmp, boolean jspecify, boolean jarInfer) { + java.util.List<String> base = + new java.util.ArrayList<>( + java.util.Arrays.asList( + "-d", tmp.getRoot().getAbsolutePath(), "-XepOpt:NullAway:AnnotatedPackages=com.uber")); + if (jspecify) base.add("-XepOpt:NullAway:JSpecifyMode=true"); + if (jarInfer) base.add("-XepOpt:NullAway:JarInferEnabled=true"); + return base; + }Example usage (elsewhere in this class):
- setArgs(args(temporaryFolder, /* jspecify= / false, / jarInfer= */ true))
- setArgs(args(temporaryFolder, true, false))
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
jdk-annotations/jdk-integration-test/src/test/java/com/uber/nullaway/jdkannotations/JDKIntegrationTest.java(1 hunks)jdk-annotations/test-annotated/src/main/java/com/uber/nullaway/jdkannotations/ReturnAnnotation.java(1 hunks)jdk-annotations/test-unannotated/src/main/java/com/uber/nullaway/jdkannotations/ParameterAnnotation.java(1 hunks)jdk-annotations/test-unannotated/src/main/java/com/uber/nullaway/jdkannotations/ReturnAnnotation.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- jdk-annotations/test-unannotated/src/main/java/com/uber/nullaway/jdkannotations/ReturnAnnotation.java
🧰 Additional context used
🧠 Learnings (1)
📚 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-annotations/jdk-integration-test/src/test/java/com/uber/nullaway/jdkannotations/JDKIntegrationTest.java
🧬 Code graph analysis (2)
jdk-annotations/test-unannotated/src/main/java/com/uber/nullaway/jdkannotations/ParameterAnnotation.java (1)
jdk-annotations/test-annotated/src/main/java/com/uber/nullaway/jdkannotations/ParameterAnnotation.java (1)
Generic(36-45)
jdk-annotations/test-annotated/src/main/java/com/uber/nullaway/jdkannotations/ReturnAnnotation.java (2)
jdk-annotations/test-annotated/src/main/java/com/uber/nullaway/jdkannotations/ParameterAnnotation.java (1)
NullMarked(7-56)jdk-annotations/test-unannotated/src/main/java/com/uber/nullaway/jdkannotations/ReturnAnnotation.java (3)
ReturnAnnotation(5-61)InnerExample(31-35)UpperBoundExample(37-44)
🔇 Additional comments (5)
jdk-annotations/test-unannotated/src/main/java/com/uber/nullaway/jdkannotations/ParameterAnnotation.java (4)
8-14: LGTM!The null check is correctly implemented and the method safely handles null inputs.
32-34: LGTM!The method correctly handles null input with an explicit check and provides a safe fallback.
1-50: Verify the intent of missing null safety in test-unannotated module.This test-unannotated module lacks null checks in most methods, which contrasts with the test-annotated counterpart that uses
@Nullableannotations. If these NPE-prone methods are intentionally designed to test nullness checker behavior, consider adding documentation (class-level or method-level comments) to clarify this intent and prevent confusion.
41-49: NPE risk: generic array parameters can be null.Both methods will throw NullPointerException if the
objectsarray is null. Add null checks to handle this case safely.@SuppressWarnings("ArrayToString") public static void takesNullGenericArray(Generic<String>[] objects) { + if (objects == null) { + System.out.println("null array"); + return; + } System.out.println(objects); } @SuppressWarnings("ArrayToString") public static void takesNonNullGenericArray(Generic<String>[] objects) { + if (objects == null) { + throw new IllegalArgumentException("objects cannot be null"); + } System.out.println(objects); }Likely an incorrect or invalid review comment.
jdk-annotations/jdk-integration-test/src/test/java/com/uber/nullaway/jdkannotations/JDKIntegrationTest.java (1)
1-305: Solid coverage and correct use of NullAway options.End-to-end scenarios look good: returns (incl. arrays), inner classes, and generic upper bounds are exercised with/without JarInfer and JSpecify. No version-gating here, which aligns with our preference to fail rather than skip tests on unsupported JDKs. Based on learnings.
| public static UpperBoundExample<String> @Nullable [] returnNullableGenericArray() { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Also mark the array-returning method’s return as @nullable.
This method returns null but only the array element type is annotated. Add return-level @nullable.
- public static UpperBoundExample<String> @Nullable [] returnNullableGenericArray() {
+ public static @Nullable UpperBoundExample<String> @Nullable [] returnNullableGenericArray() {📝 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.
| public static UpperBoundExample<String> @Nullable [] returnNullableGenericArray() { | |
| return null; | |
| } | |
| public static @Nullable UpperBoundExample<String> @Nullable [] returnNullableGenericArray() { | |
| return null; | |
| } |
🤖 Prompt for AI Agents
In
jdk-annotations/test-annotated/src/main/java/com/uber/nullaway/jdkannotations/ReturnAnnotation.java
around lines 67 to 69, the method currently only annotates the array element
type as @Nullable but the method return itself can be null; add a return-level
@Nullable before the element type so the return type is annotated as nullable in
addition to the array element (i.e., apply @Nullable to the return type, not
just the component type).
| public static Integer add(Integer a, Integer b) { | ||
| return a + b; | ||
| } |
There was a problem hiding this comment.
NPE risk: unboxing null Integer parameters.
If either a or b is null, unboxing during the addition operation will throw a NullPointerException. Add null checks or use primitive int parameters if null inputs are not expected.
public static Integer add(Integer a, Integer b) {
+ if (a == null || b == null) {
+ throw new IllegalArgumentException("Parameters cannot be null");
+ }
return a + b;
}📝 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.
| public static Integer add(Integer a, Integer b) { | |
| return a + b; | |
| } | |
| public static Integer add(Integer a, Integer b) { | |
| if (a == null || b == null) { | |
| throw new IllegalArgumentException("Parameters cannot be null"); | |
| } | |
| return a + b; | |
| } |
| public static void printObjectString(Object object) { | ||
| System.out.println(object.toString()); | ||
| } |
There was a problem hiding this comment.
NPE risk: calling toString() on potentially null parameter.
If object is null, calling object.toString() will throw a NullPointerException. Add a null check before dereferencing.
public static void printObjectString(Object object) {
+ if (object == null) {
+ throw new IllegalArgumentException("object cannot be null");
+ }
System.out.println(object.toString());
}🤖 Prompt for AI Agents
In
jdk-annotations/test-unannotated/src/main/java/com/uber/nullaway/jdkannotations/ParameterAnnotation.java
around lines 16 to 18, the method dereferences the parameter and may NPE;
replace the direct object.toString() call with a null-safe approach (either
check if object == null and handle/print a placeholder, or use
String.valueOf(object) which returns "null" for null inputs) so the method does
not throw when passed null.
| @SuppressWarnings("ArrayToString") | ||
| public static void takesNullArray(Object[] objects) { | ||
| System.out.println(objects); | ||
| } | ||
|
|
||
| @SuppressWarnings("ArrayToString") | ||
| public static void takesNonNullArray(Object[] objects) { | ||
| String unused = objects.toString(); | ||
| } |
There was a problem hiding this comment.
NPE risk: array parameter can be null.
Both methods will throw NullPointerException if the objects array itself is null (not just its elements). Despite the method names suggesting null-handling intent, there are no parameter annotations or null checks.
@SuppressWarnings("ArrayToString")
public static void takesNullArray(Object[] objects) {
+ if (objects == null) {
+ System.out.println("null array");
+ return;
+ }
System.out.println(objects);
}
@SuppressWarnings("ArrayToString")
public static void takesNonNullArray(Object[] objects) {
+ if (objects == null) {
+ throw new IllegalArgumentException("objects cannot be null");
+ }
String unused = objects.toString();
}📝 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.
| @SuppressWarnings("ArrayToString") | |
| public static void takesNullArray(Object[] objects) { | |
| System.out.println(objects); | |
| } | |
| @SuppressWarnings("ArrayToString") | |
| public static void takesNonNullArray(Object[] objects) { | |
| String unused = objects.toString(); | |
| } | |
| @SuppressWarnings("ArrayToString") | |
| public static void takesNullArray(Object[] objects) { | |
| if (objects == null) { | |
| System.out.println("null array"); | |
| return; | |
| } | |
| System.out.println(objects); | |
| } | |
| @SuppressWarnings("ArrayToString") | |
| public static void takesNonNullArray(Object[] objects) { | |
| if (objects == null) { | |
| throw new IllegalArgumentException("objects cannot be null"); | |
| } | |
| String unused = objects.toString(); | |
| } |
🤖 Prompt for AI Agents
In
jdk-annotations/test-unannotated/src/main/java/com/uber/nullaway/jdkannotations/ParameterAnnotation
around lines 20 to 28, the methods take an Object[] parameter that can be null
and will cause an NPE when used; annotate the parameter as @Nullable and add an
explicit null check before dereferencing (e.g., return or handle the null case)
so the method safely handles a null array, or if the intent is to require
non-null, annotate as @NonNull and add a defensive
Objects.requireNonNull(objects, "...") at the start to fail fast; pick one
approach and apply it consistently to both methods.
| public void printObjectString(T t) { | ||
| System.out.println(t.toString()); | ||
| } |
There was a problem hiding this comment.
NPE risk: inconsistent null handling in Generic class.
Unlike getString (which safely checks t != null), this method calls t.toString() without a null check and will throw NullPointerException if t is null. Consider adding a null check for consistency.
public void printObjectString(T t) {
+ if (t == null) {
+ throw new IllegalArgumentException("t cannot be null");
+ }
System.out.println(t.toString());
}📝 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.
| public void printObjectString(T t) { | |
| System.out.println(t.toString()); | |
| } | |
| public void printObjectString(T t) { | |
| if (t == null) { | |
| throw new IllegalArgumentException("t cannot be null"); | |
| } | |
| System.out.println(t.toString()); | |
| } |
🤖 Prompt for AI Agents
In
jdk-annotations/test-unannotated/src/main/java/com/uber/nullaway/jdkannotations/ParameterAnnotation.java
around lines 36 to 38, printObjectString calls t.toString() without a null check
causing an NPE if t is null; modify the method to mirror getString by checking
if t == null and handling that case (e.g., return or print "null") before
calling toString(), ensuring consistent null handling in the generic class.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
jdk-annotations/astubx-generator/src/test/java/com/uber/nullaway/jdkannotations/AstubxTest.java (1)
391-397: Avoid masking failures with the empty-expectations fallbackAllowing the test to pass when the file is missing and expectations are empty can hide regressions. Prefer a clear policy (always expect file, or explicitly handle empty case).
Option 1 (always expect file):
- Assert.assertTrue( - "astubx file was not created", - Files.exists(Paths.get(astubxOutputDirPath, "output.astubx")) - || (expectedMethodRecords.isEmpty() - && expectedNullableUpperBounds.isEmpty() - && expectedNullMarkedClasses.isEmpty())); + Assert.assertTrue( + "astubx file was not created", + Files.exists(Paths.get(astubxOutputDirPath, "output.astubx")));Option 2 (explicitly return for empty case):
- Assert.assertTrue( - "astubx file was not created", - Files.exists(Paths.get(astubxOutputDirPath, "output.astubx")) - || (expectedMethodRecords.isEmpty() - && expectedNullableUpperBounds.isEmpty() - && expectedNullMarkedClasses.isEmpty())); + if (expectedMethodRecords.isEmpty() + && expectedNullableUpperBounds.isEmpty() + && expectedNullMarkedClasses.isEmpty()) { + return; + } + Assert.assertTrue( + "astubx file was not created", + Files.exists(Paths.get(astubxOutputDirPath, "output.astubx")));
🧹 Nitpick comments (3)
jdk-annotations/astubx-generator/src/main/java/com/uber/nullaway/jdkannotations/AstubxGenerator.java (3)
201-206: Handle unqualified @nullable on return types and normalize strippingReturn-type nullable detection only matches the fully-qualified annotation. If javac ever emits unqualified
@Nullable, we’ll miss it and leave the token in the signature. Detect both forms, and strip both consistently.- // check if return type has Nullable annotation - if (returnType.contains("@org.jspecify.annotations.Nullable")) { - returnType = returnType.replace("@org.jspecify.annotations.Nullable ", ""); - returnType = returnType.replaceAll("@Nullable\\s*", ""); - returnType = returnType.replace(" []", "[]"); // remove whitespace in Array types - returnTypeNullness = ImmutableSet.of("Nullable"); - } + // check if return type has Nullable annotation (qualified or unqualified) + boolean hasNullableReturn = + returnType.contains("@org.jspecify.annotations.Nullable") + || returnType.matches(".*@Nullable\\b.*"); + if (hasNullableReturn) { + returnType = returnType.replace("@org.jspecify.annotations.Nullable ", ""); + returnType = returnType.replaceAll("@Nullable\\s*", ""); + returnType = returnType.replace(" []", "[]"); // normalize array spacing + returnTypeNullness = ImmutableSet.of("Nullable"); + }
219-226: Match unqualified @nullable in parameter types (consistency with return types)Param annotation detection only checks FQ name. Support unqualified
@Nullabletoo to avoid missing annotations.- for (String token : signatureTokens) { + for (String token : signatureTokens) { if (token.contains("@")) { - if (token.contains("@org.jspecify.annotations.Nullable")) { + if (token.contains("@org.jspecify.annotations.Nullable") + || token.matches(".*@Nullable\\b.*")) { argAnnotation.put(i, ImmutableSet.of("Nullable")); } typeSignature += token.substring(0, token.indexOf('@')); } else { typeSignature += token; } }
141-142: Fix error message: not writing JSON hereThe message says “JSON file” but this block writes a .astubx binary.
- System.err.println("Error writing JSON file: " + outputFile.getAbsolutePath()); + System.err.println("Error writing astubx file: " + outputFile.getAbsolutePath());
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
jdk-annotations/astubx-generator/src/main/java/com/uber/nullaway/jdkannotations/AstubxGenerator.java(1 hunks)jdk-annotations/astubx-generator/src/test/java/com/uber/nullaway/jdkannotations/AstubxTest.java(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-08-28T04:54:20.953Z
Learnt from: msridhar
PR: uber/NullAway#1248
File: nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java:847-857
Timestamp: 2025-08-28T04:54:20.953Z
Learning: In NullAway's GenericsChecks.java, NewClassTree support for explicit type argument substitution requires more extensive changes beyond just modifying the conditional in compareGenericTypeParameterNullabilityForCall. The maintainers prefer to handle NewClassTree support in a separate follow-up rather than expanding the scope of PRs focused on specific issues like super constructor calls.
Applied to files:
jdk-annotations/astubx-generator/src/main/java/com/uber/nullaway/jdkannotations/AstubxGenerator.java
📚 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-annotations/astubx-generator/src/main/java/com/uber/nullaway/jdkannotations/AstubxGenerator.java
📚 Learning: 2025-08-29T18:41:43.584Z
Learnt from: msridhar
PR: uber/NullAway#1259
File: jdk-recent-unit-tests/src/test/java/com/uber/nullaway/jdk17/SwitchTests.java:318-321
Timestamp: 2025-08-29T18:41:43.584Z
Learning: Classes annotated with NullMarked are analyzed by NullAway even if they are not in packages specified by the AnnotatedPackages configuration. The NullMarked annotation guarantees NullAway analysis.
Applied to files:
jdk-annotations/astubx-generator/src/main/java/com/uber/nullaway/jdkannotations/AstubxGenerator.java
🧬 Code graph analysis (2)
jdk-annotations/astubx-generator/src/main/java/com/uber/nullaway/jdkannotations/AstubxGenerator.java (2)
jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java (1)
NullnessAnnotationSerializer(35-187)library-model/library-model-generator/src/main/java/com/uber/nullaway/libmodel/StubxWriter.java (1)
StubxWriter(15-153)
jdk-annotations/astubx-generator/src/test/java/com/uber/nullaway/jdkannotations/AstubxTest.java (1)
jdk-annotations/astubx-generator/src/main/java/com/uber/nullaway/jdkannotations/AstubxGenerator.java (1)
AstubxGenerator(34-322)
| // parse JSON file | ||
| Map<String, List<ClassInfo>> parsed = new HashMap<>(); | ||
| for (File jsonFile : jsonFiles) { | ||
| try { | ||
| String jsonContent = Files.readString(jsonFile.toPath()); | ||
| Map<String, List<ClassInfo>> parsedShard = gson.fromJson(jsonContent, parsedType); | ||
| for (Map.Entry<String, List<ClassInfo>> shardEntry : parsedShard.entrySet()) { | ||
| parsed | ||
| // if the key didn't exist, create a new list | ||
| .computeIfAbsent(shardEntry.getKey(), __ -> new ArrayList<>()) | ||
| .addAll(shardEntry.getValue()); // add values to the key | ||
| } |
There was a problem hiding this comment.
Defensive null handling when merging JSON shards
gson.fromJson(...) or shardEntry.getValue() can be null; current loop would NPE. Guard before merging.
- try {
+ try {
String jsonContent = Files.readString(jsonFile.toPath());
- Map<String, List<ClassInfo>> parsedShard = gson.fromJson(jsonContent, parsedType);
- for (Map.Entry<String, List<ClassInfo>> shardEntry : parsedShard.entrySet()) {
+ Map<String, List<ClassInfo>> parsedShard = gson.fromJson(jsonContent, parsedType);
+ if (parsedShard == null) {
+ continue;
+ }
+ for (Map.Entry<String, List<ClassInfo>> shardEntry : parsedShard.entrySet()) {
+ if (shardEntry.getValue() == null) {
+ continue;
+ }
parsed
// if the key didn't exist, create a new list
.computeIfAbsent(shardEntry.getKey(), __ -> new ArrayList<>())
.addAll(shardEntry.getValue()); // add values to the key
}
} catch (IOException e) {📝 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.
| // parse JSON file | |
| Map<String, List<ClassInfo>> parsed = new HashMap<>(); | |
| for (File jsonFile : jsonFiles) { | |
| try { | |
| String jsonContent = Files.readString(jsonFile.toPath()); | |
| Map<String, List<ClassInfo>> parsedShard = gson.fromJson(jsonContent, parsedType); | |
| for (Map.Entry<String, List<ClassInfo>> shardEntry : parsedShard.entrySet()) { | |
| parsed | |
| // if the key didn't exist, create a new list | |
| .computeIfAbsent(shardEntry.getKey(), __ -> new ArrayList<>()) | |
| .addAll(shardEntry.getValue()); // add values to the key | |
| } | |
| // parse JSON file | |
| Map<String, List<ClassInfo>> parsed = new HashMap<>(); | |
| for (File jsonFile : jsonFiles) { | |
| try { | |
| String jsonContent = Files.readString(jsonFile.toPath()); | |
| Map<String, List<ClassInfo>> parsedShard = gson.fromJson(jsonContent, parsedType); | |
| if (parsedShard == null) { | |
| continue; | |
| } | |
| for (Map.Entry<String, List<ClassInfo>> shardEntry : parsedShard.entrySet()) { | |
| if (shardEntry.getValue() == null) { | |
| continue; | |
| } | |
| parsed | |
| // if the key didn't exist, create a new list | |
| .computeIfAbsent(shardEntry.getKey(), __ -> new ArrayList<>()) | |
| .addAll(shardEntry.getValue()); // add values to the key | |
| } | |
| } catch (IOException e) { | |
| // existing error handling... | |
| } | |
| } |
🤖 Prompt for AI Agents
In
jdk-annotations/astubx-generator/src/main/java/com/uber/nullaway/jdkannotations/AstubxGenerator.java
around lines 170 to 181, the code assumes gson.fromJson(...) returns a non-null
parsedShard and that shardEntry.getValue() is non-null, which can cause NPEs;
modify the loop to first check if parsedShard is null and skip that file if so,
and then for each shardEntry skip entries whose value is null (or treat them as
empty lists) before calling computeIfAbsent(...).addAll(...), ensuring you only
call addAll when the value is non-null.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
jdk-annotations/jdk-integration-test/src/test/java/com/uber/nullaway/jdkannotations/JDKIntegrationTest.java (1)
22-330: LGTM! Comprehensive integration test coverage.The test suite provides excellent coverage of JDK annotation scenarios including nullable returns, parameters, arrays, generics, and inner classes. The tests correctly verify both positive cases (expecting errors) and negative cases (no errors expected), and include validation of flag behavior (JarInferEnabled, JSpecifyMode).
Optional: Consider extracting common argument setup to reduce duplication.
Many tests share similar argument lists. Consider extracting a helper method to reduce duplication:
private List<String> standardArgs(boolean jarInferEnabled, boolean jspecifyMode) { List<String> args = new ArrayList<>(Arrays.asList( "-d", temporaryFolder.getRoot().getAbsolutePath(), "-XepOpt:NullAway:AnnotatedPackages=com.uber")); if (jarInferEnabled) { args.add("-XepOpt:NullAway:JarInferEnabled=true"); } if (jspecifyMode) { args.add("-XepOpt:NullAway:JSpecifyMode=true"); } return args; }This would make tests more concise and easier to maintain:
compilationHelper .setArgs(standardArgs(true, false)) .addSourceLines(...) .doTest();
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
jdk-annotations/jdk-integration-test/src/test/java/com/uber/nullaway/jdkannotations/JDKIntegrationTest.java(1 hunks)jdk-annotations/test-annotated/src/main/java/com/uber/nullaway/jdkannotations/ReturnAnnotation.java(1 hunks)jdk-annotations/test-unannotated/src/main/java/com/uber/nullaway/jdkannotations/ReturnAnnotation.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- jdk-annotations/test-annotated/src/main/java/com/uber/nullaway/jdkannotations/ReturnAnnotation.java
- jdk-annotations/test-unannotated/src/main/java/com/uber/nullaway/jdkannotations/ReturnAnnotation.java
🧰 Additional context used
🧠 Learnings (1)
📚 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-annotations/jdk-integration-test/src/test/java/com/uber/nullaway/jdkannotations/JDKIntegrationTest.java
🔇 Additional comments (1)
jdk-annotations/jdk-integration-test/src/test/java/com/uber/nullaway/jdkannotations/JDKIntegrationTest.java (1)
1-20: LGTM! Test setup is properly configured.The test class is correctly configured with CompilationTestHelper and uses a TemporaryFolder for compilation artifacts. The setup follows standard patterns for Error Prone compilation tests.
msridhar
changed the title
2 participants
This support will be used to generate an astubx JDK model from the JSpecify annotated JDK. Includes extensive unit testing and integration testing of the support. We will likely need to add more features as we try to support all the annotations present in the JDK (on wildcards, etc.).
Summary by CodeRabbit
New Features
Chores
Tests