Jdk javac plugin: don't serialize info for classes / methods without annotations #1316
Conversation
WalkthroughAdds JSpecify-aware deep annotation detection to NullnessAnnotationSerializer. New helpers recursively inspect type parameters and TypeMirror structures (arrays, declared types with type arguments, wildcards, type variables, intersections) to detect Nullable/NonNull annotations. visitClass and visitMethod track and record TypeParamInfo for class and method type parameters, and they gate creation/persistence of ClassInfo and MethodInfo on detected annotations (and on non-empty method lists for classes). Tests and expected outputs were updated to include richer signatures, fully-qualified annotated types, and TypeParamInfo. Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java (1)
42-44: Remove NONNULL_NAME; JSpecify has no @nonnull.Keeping org.jspecify.annotations.NonNull can mislead and complicates future maintenance. Drop the constant and reference in hasJspecifyAnnotation.
- private static final String NULLABLE_NAME = "org.jspecify.annotations.Nullable"; - private static final String NONNULL_NAME = "org.jspecify.annotations.NonNull"; + private static final String NULLABLE_NAME = "org.jspecify.annotations.Nullable";- if (fqn.equals(NULLABLE_NAME) - || fqn.equals(NONNULL_NAME) - || fqn.equals(NULLMARKED_NAME) - || fqn.equals(NULLUNMARKED_NAME)) { + if (fqn.equals(NULLABLE_NAME) + || fqn.equals(NULLMARKED_NAME) + || fqn.equals(NULLUNMARKED_NAME)) { return true; }Also applies to: 236-241
jdk-javac-plugin/src/test/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializerTest.java (3)
157-157: Avoid usingStringas a type parameter name.It shadows
java.lang.Stringand is confusing. Prefer a neutral name likeS.- private class PrivateInner<String extends @Nullable Object> { } + private class PrivateInner<S extends @Nullable Object> { }
250-261: Prefer order-insensitive assertions for robustness.The test currently compares methods using
containsExactlyEntriesOf(), which relies onList.equals()(order-sensitive). While the current AST visitor pattern ensures deterministic source-order traversal, future changes to the serializer implementation could alter method ordering. Use AssertJ'scontainsExactlyInAnyOrder()or compare method sets by name/signature to make tests resilient to non-breaking refactorings.This applies to both test cases at lines 250-261 and 271-293.
119-124: Make publicMethod explicitly public to clarify intent.The method is currently package-private but named
publicMethod, which creates confusion in a test specifically about excluding private methods. Adding thepublicmodifier clarifies that it should be accessible.- void publicMethod(@Nullable String s) {} + public void publicMethod(@Nullable String s) {}The concern about JDK consistency is not applicable here. The project explicitly requires JDK 21 only per the build.gradle comment: "We require JDK 21, since we rely on some internal String formatting of javac types and don't support the pre-JDK-21 formats." The CI workflow confirms no multi-JDK matrix. The assertion format is therefore deterministic.
Also applies to: 139-144
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java(8 hunks)jdk-javac-plugin/src/test/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializerTest.java(5 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
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.
📚 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-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.javajdk-javac-plugin/src/test/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializerTest.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-javac-plugin/src/test/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializerTest.java
🔇 Additional comments (5)
jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java (1)
131-137: Class-level gating logic looks good.Only persisting classes with JSpecify presence (class-level or via collected methods) meets the PR goal.
jdk-javac-plugin/src/test/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializerTest.java (4)
71-71: LGTM: expectation matches filtered output policy.Keeping only the NullMarked class under the "unnamed" module aligns with “write only when JSpecify is used.”
164-171: LGTM: anonymous classes skipped, non‑private inner captured.Asserting only
Foo.InnerandFooensures the anonymous class was not serialized.
205-246: LGTM: class‑level filtering behavior validated.Only classes with JSpecify usage (annotated members or annotated type params) plus the NullMarked outer class are retained.
173-184: The annotation style difference reflects correct use of different javac APIs.The review comment assumes an inconsistency that is actually intentional design. The codebase correctly uses:
TypeMirror.toString()for method return/parameter types (line 152) → produces fully-qualified names like[email protected] StringTree.toString()for type parameter bounds (lines 193-194) → produces source-as-written format like@Nullable ObjectThis difference is a natural consequence of javac's APIs:
TypeMirrorrepresents resolved type information and includes FQNs, whileTreenodes represent the source code structure. The tests correctly expect these different formats.No changes or documentation are needed; the implementation is working as designed.
Likely an incorrect or invalid review comment.
jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java
Show resolved
Hide resolved
jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java
Show resolved
Hide resolved
msridhar
left a comment
msridhar
left a comment
There was a problem hiding this comment.
Thanks for this! See the comment below
jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
jdk-javac-plugin/src/test/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializerTest.java (1)
157-157: Consider renaming the type parameter to avoid shadowing.Using
Stringas a type parameter name shadowsjava.lang.String, which may be confusing. While legal and potentially testing edge cases, consider using a more conventional name likeTorSfor clarity.Apply this diff if you'd like to improve readability:
- private class PrivateInner<String extends @Nullable Object> { } + private class PrivateInner<T extends @Nullable Object> { }jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java (1)
161-167: Add early exit for minor performance improvement.Once an annotated parameter is found, the loop continues checking remaining parameters unnecessarily.
Apply this diff to exit early:
// check each parameter annotations if (!methodHasAnnotations) { for (Symbol.VarSymbol vSym : mSym.getParameters()) { if (hasJspecifyAnnotationDeep(vSym.asType())) { methodHasAnnotations = true; + break; } } }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java(8 hunks)jdk-javac-plugin/src/test/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializerTest.java(5 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 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/src/test/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializerTest.java
📚 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-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java
🔇 Additional comments (16)
jdk-javac-plugin/src/test/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializerTest.java (9)
58-72: LGTM!The test correctly verifies that a
@NullMarkedclass is serialized even without methods, which aligns with the optimization goal of capturing JSpecify annotation usage.
74-111: LGTM!The test correctly validates module-aware serialization with proper fully-qualified type-use annotation syntax.
114-145: LGTM!The test correctly validates that private methods are excluded while public methods with JSpecify annotations are captured.
164-171: LGTM!The assertions correctly validate that only non-private classes with relevant annotations are serialized, excluding both the private
PrivateInnerand the anonymousRunnableclass.
173-203: LGTM!The test comprehensively validates type parameter serialization with JSpecify annotations, including both class-level and method-level generic signatures and the new
TypeParamInfostructure.
205-247: LGTM!The test effectively validates the core optimization feature: classes without JSpecify annotations (
NoAnnotation) are skipped, while classes with annotations (directly or through members/type parameters) are captured.
249-294: LGTM!The test comprehensively validates method-level filtering, ensuring that only methods with JSpecify annotations (on type parameters, return types, or parameters) are serialized.
296-351: LGTM!The test thoroughly validates deep annotation scanning across complex type structures (arrays, wildcards, type variables, intersection types), ensuring the serializer detects JSpecify annotations in nested contexts.
353-376: LGTM!The helper method correctly parses JSON output using Gson with proper error handling and resource management.
jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java (7)
11-11: LGTM!The new imports are necessary for the type parameter annotation detection logic.
Also applies to: 14-14
42-43: LGTM!The constants are properly defined and used consistently in the annotation detection logic.
107-141: LGTM!The class filtering logic correctly implements the optimization goal. Classes are only added to the output when they or their members contain JSpecify annotations, and the nested class context handling is preserved correctly.
149-187: LGTM!The method filtering logic correctly implements deep annotation detection across return types, parameters, and type parameters. Methods are only added to the class when they contain JSpecify annotations.
205-217: LGTM!The type parameter annotation detection correctly uses deep traversal and includes proper null safety checks.
219-233: LGTM!Clean helper that consolidates all JSpecify annotation checks with proper null safety.
235-280: Verify recursive type bound handling is safe or add cycle detection.The deep traversal implementation handles type bound recursion (TYPEVAR, WILDCARD cases at lines 262-265 and 257-260). While javac's TypeMirror API likely prevents infinite loops internally, and similar patterns in
TypeVarWithSymbolCollectorwork without issues, this remains worth confirming—especially for edge cases like<T extends Comparable<T>>where recursive bound resolution could theoretically cycle.Either:
- Confirm that javac's TypeMirror prevents cycles internally, or
- Add cycle detection (e.g., visited set or depth limit) for defensive robustness
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
jdk-javac-plugin/src/test/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializerTest.java (1)
353-376: Pick the latest JSON file, not the first.Use last-modified ordering to read the JSON produced by the current compile.
- try (Stream<Path> stream = Files.list(Paths.get(tempPath))) { - return stream - .filter(path -> path.toString().endsWith(".json")) - .findFirst() + try (Stream<Path> stream = Files.list(Paths.get(tempPath))) { + return stream + .filter(path -> path.toString().endsWith(".json")) + .max(java.util.Comparator.comparingLong(p -> { + try { return Files.getLastModifiedTime(p).toMillis(); } catch (IOException e) { return Long.MIN_VALUE; } + })) .<Map<String, List<ClassInfo>>>map( path -> { try { return new GsonBuilder() .create() .fromJson( new String(Files.readAllBytes(path), StandardCharsets.UTF_8), new TypeToken<Map<String, List<ClassInfo>>>() {}.getType()); } catch (IOException e) { throw new RuntimeException(e); } }) .orElseThrow();
🧹 Nitpick comments (5)
jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java (3)
236-281: Handle UNION types in deep scan.Covers ARRAY/DECLARED/WILDCARD/TYPEVAR/INTERSECTION, but not UNION. Add UNION to avoid missing annotations in multi-catch types.
private boolean hasJspecifyAnnotationDeep(TypeMirror type) { @@ - case INTERSECTION -> { + case INTERSECTION -> { for (TypeMirror b : ((javax.lang.model.type.IntersectionType) type).getBounds()) { if (hasJspecifyAnnotationDeep(b)) { return true; } } return false; } + case UNION -> { + for (TypeMirror b : + ((javax.lang.model.type.UnionType) type).getAlternatives()) { + if (hasJspecifyAnnotationDeep(b)) { + return true; + } + } + return false; + } default -> { return false; }
236-281: Prefer a TypeVisitor to future-proof traversal.Switch is fine but easy to drift. A SimpleTypeVisitor/TypeKindVisitor keeps this robust as Java adds kinds; centralize recursion and annotation checks there. See maintainer guidance in earlier discussion.
Based on past review comments
183-186: Fix misleading comment.Comment says “only add currentClass,” but this block adds a method.
- // only add currentClass if there are annotations in it + // only add this method if it uses JSpecify annotations if (currentClass != null && methodHasAnnotations) { currentClass.methods().add(methodInfo); }jdk-javac-plugin/src/test/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializerTest.java (2)
43-55: Prevent stale JSON interference between tests.Multiple compiles write multiple JSONs; picking an arbitrary one can flake tests. Clean old JSONs in setup or write deterministically.
@Before public void setup() { String tempPath = temporaryFolder.getRoot().getAbsolutePath(); + // Clean any prior JSON artifacts + try (java.util.stream.Stream<java.nio.file.Path> s = java.nio.file.Files.list(java.nio.file.Paths.get(tempPath))) { + s.filter(p -> p.toString().endsWith(".json")).forEach(p -> { + try { java.nio.file.Files.deleteIfExists(p); } catch (IOException ignore) {} + }); + } catch (IOException ignore) {} compilationTestHelper =
296-351: Add coverage for @NullnessUnspecified.Once serializer recognizes it, add a focused test to guard behavior.
+ @Test + public void nullnessUnspecifiedCoverage() { + compilationTestHelper + .addSourceLines( + "Foo.java", + "import org.jspecify.annotations.*;", + "@NullMarked", + "class Foo {", + " @NullnessUnspecified String f(@NullnessUnspecified String s) { return s; }", + "}") + .doTest(); + Map<String, List<ClassInfo>> moduleClasses = getParsedJSON(); + assertThat(moduleClasses).containsExactlyEntriesOf( + Map.of("unnamed", + List.of(new ClassInfo( + "Foo", "Foo", true, false, List.of(), + List.of(new MethodInfo( + "[email protected] String", + "f([email protected] String)", + false, false, List.of())))))); + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java(8 hunks)jdk-javac-plugin/src/test/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializerTest.java(5 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 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/src/test/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializerTest.java
📚 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-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java
🔇 Additional comments (1)
jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java (1)
42-44: The review comment is incorrect; no changes needed.JSpecify defines org.jspecify.annotations.NonNull, so including
NONNULL_NAMEin the serializer is correct. More importantly, there is no org.jspecify.annotations.NullnessUnspecified annotation; "unspecified nullness" is the state of an unannotated type (or of code in a NullUnmarked scope), not a separate annotation. The proposed addition of aNULLNESS_UNSPECIFIED_NAMEconstant is therefore not applicable.The code at lines 42–44 is correct as-is, and the
hasJspecifyAnnotationmethod already handles the appropriate JSpecify annotations.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java (1)
236-281: Deep traversal correctly handles main type kinds; consider edge cases.The implementation properly recurses through nested type structures and addresses the previous review feedback. The coverage of ARRAY, DECLARED, WILDCARD, TYPEVAR, and INTERSECTION cases handles the majority of scenarios.
Two minor refinements to consider:
UNION types (from multi-catch clauses) could theoretically carry type-use annotations but aren't currently handled. This is likely a rare edge case.
TypeVariable lower bound (line 266): Type variables in Java don't have meaningful lower bounds (the lower bound is always the null type). The check is harmless but unnecessary.
If you want to add UNION support:
case INTERSECTION -> { for (TypeMirror b : ((javax.lang.model.type.IntersectionType) type).getBounds()) { if (hasJspecifyAnnotationDeep(b)) { return true; } } return false; } + case UNION -> { + for (TypeMirror alt : + ((javax.lang.model.type.UnionType) type).getAlternatives()) { + if (hasJspecifyAnnotationDeep(alt)) { + return true; + } + } + return false; + } default -> { return false; }To simplify TypeVariable handling:
case TYPEVAR -> { TypeVariable tv = (TypeVariable) type; - return hasJspecifyAnnotationDeep(tv.getUpperBound()) - || hasJspecifyAnnotationDeep(tv.getLowerBound()); + return hasJspecifyAnnotationDeep(tv.getUpperBound()); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
jdk-annotations/astubx-generator/src/test/java/com/uber/nullaway/jdkannotations/AstubxTest.java(3 hunks)jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java(8 hunks)
✅ Files skipped from review due to trivial changes (1)
- jdk-annotations/astubx-generator/src/test/java/com/uber/nullaway/jdkannotations/AstubxTest.java
🧰 Additional context used
🧠 Learnings (1)
📚 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-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java
🔇 Additional comments (3)
jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java (3)
107-141: LGTM! Class filtering logic correctly handles annotations.The implementation correctly tracks whether a class has JSpecify annotations (directly on the class, on its type parameters, or in its methods) and only persists
ClassInfowhen annotations are present. The stack-based approach properly handles nested class contexts.
149-188: LGTM! Method filtering correctly uses deep annotation detection.The implementation properly checks for JSpecify annotations across return types, parameters, and type parameters using the deep traversal method, and only adds methods with annotations. The early-break optimization for parameter scanning is a nice touch.
206-218: LGTM! Type parameter checking correctly uses deep traversal.This helper properly obtains the type parameter's symbol and delegates to
hasJspecifyAnnotationDeep, ensuring nested annotations in bounds are detected.
jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java
Show resolved
Hide resolved
jdk-annotations/astubx-generator/src/test/java/com/uber/nullaway/jdkannotations/AstubxTest.java
Outdated
Show resolved
Hide resolved
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1316 +/- ##
============================================
- Coverage 88.47% 88.37% -0.11%
Complexity 2550 2550
============================================
Files 95 95
Lines 8488 8544 +56
Branches 1678 1705 +27
============================================
+ Hits 7510 7551 +41
- Misses 494 499 +5
- Partials 484 494 +10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java (2)
43-43: Remove NonNull; not part of JSpecify.JSpecify does not define org.jspecify.annotations.NonNull. Keeping it will misclassify code. Remove the constant and its check.
- private static final String NONNULL_NAME = "org.jspecify.annotations.NonNull";And in hasJspecifyAnnotation:
- if (fqn.equals(NULLABLE_NAME) - || fqn.equals(NONNULL_NAME) - || fqn.equals(NULLMARKED_NAME) - || fqn.equals(NULLUNMARKED_NAME)) { + if (fqn.equals(NULLABLE_NAME) + || fqn.equals(NULLMARKED_NAME) + || fqn.equals(NULLUNMARKED_NAME)) {Also applies to: 230-234
247-279: Deep scan misses TYPEVAR; annotations on T’s bounds won’t trigger emission.If a method uses T and T’s bounds carry JSpecify, hasJspecifyAnnotationDeep returns false (default branch). Add TYPEVAR traversal. UNION is also easy to cover.
switch (type.getKind()) { + case TYPEVAR -> { + TypeVariable tv = (TypeVariable) type; + return hasJspecifyAnnotationDeep(tv.getUpperBound()) + || hasJspecifyAnnotationDeep(tv.getLowerBound()); + } case ARRAY -> { return hasJspecifyAnnotationDeep( ((javax.lang.model.type.ArrayType) type).getComponentType()); } case DECLARED -> { @@ case INTERSECTION -> { for (TypeMirror b : ((javax.lang.model.type.IntersectionType) type).getBounds()) { if (hasJspecifyAnnotationDeep(b)) { return true; } } return false; } + // Optional completeness: + // case UNION -> { + // for (TypeMirror a : ((javax.lang.model.type.UnionType) type).getAlternatives()) { + // if (hasJspecifyAnnotationDeep(a)) return true; + // } + // return false; + // } default -> { return false; } }Alternatively, consider a DefaultTypeVisitor to avoid future misses. Based on learnings.
🧹 Nitpick comments (4)
jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java (4)
72-75: Validate args and ensure output directory exists to avoid runtime failures.Current code assumes args[0] and an existing directory. Guard and create the directory.
@Override public void init(JavacTask task, String... args) { - String outputDir = args[0]; + if (args == null || args.length < 1 || args[0].isBlank()) { + throw new IllegalArgumentException("NullnessAnnotationSerializer: missing output directory argument"); + } + String outputDir = args[0]; + Path outDir = Paths.get(outputDir); + try { + Files.createDirectories(outDir); + } catch (IOException ioe) { + throw new RuntimeException("Failed to create output directory: " + outDir, ioe); + }
151-156: Return type string may drop type-use annotations; consider pretty printing.TypeMirror.toString() often omits type-use annotations. Use a pretty-printing visitor (e.g., GenericTypePrettyPrintingVisitor-like) to retain them in JSON for fidelity.
183-186: Optionally skip constructors to keep output focused.If constructors aren’t needed, early-return when mSym.isConstructor() unless they contain relevant annotations.
@Override public Void visitMethod(MethodTree methodTree, Void unused) { MethodSymbol mSym = (MethodSymbol) trees.getElement(getCurrentPath()); + if (mSym.isConstructor()) { + return super.visitMethod(methodTree, null); + }
123-129: Use fully qualified class name to avoid collisions.simpleName can collide within a module. Consider using classSym.getQualifiedName().toString().
- simpleName.toString(), + classSym.getQualifiedName().toString(),
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
jdk-annotations/astubx-generator/src/test/java/com/uber/nullaway/jdkannotations/AstubxTest.java(3 hunks)jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- jdk-annotations/astubx-generator/src/test/java/com/uber/nullaway/jdkannotations/AstubxTest.java
🧰 Additional context used
🧠 Learnings (1)
📚 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-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java
🔇 Additional comments (1)
jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java (1)
131-137: Class emission gating logic looks solid.You defer adding until after visiting members and only emit if class-level JSpecify exists or methods list is non-empty. Good.
msridhar
left a comment
msridhar
left a comment
There was a problem hiding this comment.
This looks really good! Just a few minor-ish comments
jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java
Outdated
Show resolved
Hide resolved
jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java
Show resolved
Hide resolved
| return false; | ||
| } | ||
|
|
||
| private boolean hasJspecifyAnnotationDeep(TypeMirror type) { |
There was a problem hiding this comment.
Add some Javadoc for this method
...vac-plugin/src/test/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializerTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java (1)
43-43: Remove NONNULL; JSpecify does not define @nonnull.Including NONNULL_NAME and checking it inflates matches and misrepresents JSpecify usage. This was noted earlier; please drop the constant and its check.
- private static final String NONNULL_NAME = "org.jspecify.annotations.NonNull"; @@ - if (fqn.equals(NULLABLE_NAME) - || fqn.equals(NONNULL_NAME) - || fqn.equals(NULLMARKED_NAME) - || fqn.equals(NULLUNMARKED_NAME)) { + if (fqn.equals(NULLABLE_NAME) + || fqn.equals(NULLMARKED_NAME) + || fqn.equals(NULLUNMARKED_NAME)) {Also applies to: 230-234
🧹 Nitpick comments (4)
jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java (4)
200-204: Match annotation FQNs via elements, not toString().toString() can be non‑canonical. Use TypeElement.getQualifiedName() for robustness.
- private boolean hasAnnotation(Symbol sym, String fqn) { - return sym.getAnnotationMirrors().stream() - .map(AnnotationMirror::getAnnotationType) - .map(Object::toString) - .anyMatch(fqn::equals); - } + private boolean hasAnnotation(Symbol sym, String fqn) { + return sym.getAnnotationMirrors().stream() + .map(am -> (javax.lang.model.element.TypeElement) am.getAnnotationType().asElement()) + .map(te -> te.getQualifiedName().toString()) + .anyMatch(fqn::equals); + }
149-169: Do you intend to include methods that only reference annotated class type params via bounds?Current gating won’t include a method whose types use a class type variable with annotated bounds (e.g., return T; class T extends @nullable Object) unless you adopt TYPEVAR deep handling (above) or include class‑level TP signals when deciding methodHasAnnotations. Please confirm intended behavior.
If inclusion is desired, either:
- Rely on the TYPEVAR fix (preferred), or
- Also set methodHasAnnotations when any referenced class type parameter has JSpecify in its bounds.
Also applies to: 170-176, 184-186
71-76: Runtime robustness: validate args and ensure output directory exists.Guard against missing args and create the directory before writing.
public void init(JavacTask task, String... args) { - String outputDir = args[0]; + if (args == null || args.length == 0 || args[0].isBlank()) { + throw new IllegalArgumentException("NullnessAnnotationSerializer requires outputDir arg"); + } + String outputDir = args[0]; @@ - Path p = Paths.get(outputDir, jsonFileName); + Path p = Paths.get(outputDir, jsonFileName); + Files.createDirectories(p.getParent());Also applies to: 293-301
240-291: Optional: implement deep scan via a DefaultTypeVisitor for clarity/coverage.A visitor improves readability and makes it easy to add UNION/EXECUTABLE in future if needed. Not blocking.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java(8 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 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-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java
🔇 Additional comments (1)
jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java (1)
257-289: Source level check: arrow switch requires newer Java.You’re using arrow‑case switch. Ensure the module/toolchain compiles with a compatible --release/source (e.g., 17+).
Please confirm the jdk-javac-plugin module’s source/target in build files.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java (1)
43-44: Remove the invalid NONNULL_NAME constant.JSpecify does not define a
@NonNullannotation. The specification only provides@Nullable,@NullMarked, and@NullUnmarked. This was flagged in previous review comments.
🧹 Nitpick comments (1)
jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java (1)
207-223: Consider simplifying the final check for clarity.The final check
hasJSpecifyAnnotationDeep(tpSym.asType())at line 222 is potentially confusing. SincehasJSpecifyAnnotationDeepdoesn't have a TYPEVAR case, this call only checks top-level annotations on the TypeVariable itself (not bounds, which were already checked).Consider making the intent explicit:
-return hasAnnotation || hasJSpecifyAnnotationDeep(tpSym.asType()); +// Check for annotations directly on the type parameter (e.g., @Nullable T) +return hasAnnotation || typeHasJSpecifyAnnotation(tv.getAnnotationMirrors());
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
jdk-javac-plugin/build.gradle(1 hunks)jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java(8 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: haewiful
PR: uber/NullAway#1243
File: jdk-annotations/astubx-generator-cli/src/main/java/com/uber/nullaway/jdkannotations/AstubxGeneratorCLI.java:97-106
Timestamp: 2025-09-24T23:14:37.777Z
Learning: In the jdk-javac-plugin NullnessAnnotationSerializer, the plugin only creates one JSON file per compilation, so processing only the first JSON file is intentional behavior rather than a bug.
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#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.
📚 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-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java
📚 Learning: 2025-09-24T23:14:37.777Z
Learnt from: haewiful
PR: uber/NullAway#1243
File: jdk-annotations/astubx-generator-cli/src/main/java/com/uber/nullaway/jdkannotations/AstubxGeneratorCLI.java:97-106
Timestamp: 2025-09-24T23:14:37.777Z
Learning: In the jdk-javac-plugin NullnessAnnotationSerializer, the plugin only creates one JSON file per compilation, so processing only the first JSON file is intentional behavior rather than a bug.
Applied to files:
jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.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-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.javajdk-javac-plugin/build.gradle
📚 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-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.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 windows-latest
- GitHub Check: Build spring-framework with snapshot
- GitHub Check: Build and test on ubuntu-latest
- GitHub Check: Build caffeine with snapshot
🔇 Additional comments (3)
jdk-javac-plugin/build.gradle (1)
46-46: LGTM! JSpecify dependency added to support annotation detection.This dependency addition aligns with the PR objectives and enables the enhanced annotation detection logic in
NullnessAnnotationSerializer.jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java (2)
112-141: LGTM! Class filtering logic correctly implements the optimization.The logic properly gates class serialization on the presence of JSpecify annotations, checking:
- Class-level
@NullMarked/@NullUnmarked- Type parameter annotations
- Presence of annotated methods
The context save/restore mechanism (lines 109-110, 140) correctly handles nested classes.
150-188: Method filtering logic is sound, but depends on deep scan correctness.The gating logic correctly ensures only annotated methods are serialized. However, the effectiveness depends on
hasJSpecifyAnnotationDeepproperly detecting all annotations (see separate comment about missing TYPEVAR case).
| switch (type.getKind()) { | ||
| case ARRAY -> { | ||
| return hasJSpecifyAnnotationDeep( | ||
| ((javax.lang.model.type.ArrayType) type).getComponentType()); | ||
| } | ||
| case DECLARED -> { | ||
| for (TypeMirror arg : | ||
| ((javax.lang.model.type.DeclaredType) type).getTypeArguments()) { | ||
| if (hasJSpecifyAnnotationDeep(arg)) { | ||
| return true; | ||
| } | ||
| } | ||
| return false; | ||
| } | ||
| case WILDCARD -> { | ||
| javax.lang.model.type.WildcardType wt = | ||
| (javax.lang.model.type.WildcardType) type; | ||
| return hasJSpecifyAnnotationDeep(wt.getExtendsBound()) | ||
| || hasJSpecifyAnnotationDeep(wt.getSuperBound()); | ||
| } | ||
| case INTERSECTION -> { | ||
| for (TypeMirror b : | ||
| ((javax.lang.model.type.IntersectionType) type).getBounds()) { | ||
| if (hasJSpecifyAnnotationDeep(b)) { | ||
| return true; | ||
| } | ||
| } | ||
| return false; | ||
| } | ||
| default -> { | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical: Missing TYPEVAR case causes annotations on type variable bounds to be ignored.
The switch statement lacks a TYPEVAR case, causing false negatives when return types or parameters are type variables with annotated bounds.
Example of missed detection:
class Foo<T extends @Nullable Object> {
T get() { ... } // @Nullable on T's bound is NOT detected
}This issue was flagged in previous reviews and marked as "addressed," but the fix is not present in the current code.
Apply this fix:
switch (type.getKind()) {
+ case TYPEVAR -> {
+ TypeVariable tv = (TypeVariable) type;
+ return hasJSpecifyAnnotationDeep(tv.getUpperBound())
+ || hasJSpecifyAnnotationDeep(tv.getLowerBound());
+ }
case ARRAY -> {
return hasJSpecifyAnnotationDeep(
((javax.lang.model.type.ArrayType) type).getComponentType());🤖 Prompt for AI Agents
In
jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java
around lines 261-293, add a TYPEVAR branch to the switch so type-variable bounds
are inspected: cast the TypeMirror to javax.lang.model.type.TypeVariable, call
hasJSpecifyAnnotationDeep on its upper and lower bounds (checking for null) and
return true if either bound contains the annotation, otherwise return false;
place this branch alongside ARRAY/DECLARED/ WILDCARD/INTERSECTION so annotated
bounds on type parameters are detected.
There was a problem hiding this comment.
This is wrong. Type variable bounds at declarations are checked separately.
There was a problem hiding this comment.
@msridhar, you're absolutely right, and I apologize for the incorrect review comment.
Looking at the code more carefully, I can see that type variable bounds at their declaration sites are already handled by the typeParamHasAnnotation method, which is called for both class-level and method-level type parameters. That method explicitly checks the upper and lower bounds:
TypeVariable tv = (TypeVariable) tpSym.asType();
boolean hasAnnotation =
hasJSpecifyAnnotationDeep(tv.getUpperBound())
|| hasJSpecifyAnnotationDeep(tv.getLowerBound());The hasJSpecifyAnnotationDeep method is designed to check uses of types (like return types, parameter types, etc.), not type parameter declarations. When a type variable like T appears as a return type, the annotation information is already captured through the declaration-site check, so there's no need to recursively inspect bounds again at use sites.
Thank you for the correction!
✏️ Learnings added
Learnt from: msridhar
PR: uber/NullAway#1316
File: jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java:261-293
Timestamp: 2025-10-29T23:56:18.214Z
Learning: In NullAway's jdk-javac-plugin NullnessAnnotationSerializer, type variable bounds with annotations (e.g., `T extends Nullable Object`) are checked at their declaration sites by the typeParamHasAnnotation method for both class-level and method-level type parameters. The hasJSpecifyAnnotationDeep method is designed to check type uses (return types, parameters, etc.) and does not need a TYPEVAR case because type variable declaration bounds are already handled separately.
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.
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.
Learnt from: haewiful
PR: uber/NullAway#1243
File: jdk-annotations/astubx-generator-cli/src/main/java/com/uber/nullaway/jdkannotations/AstubxGeneratorCLI.java:97-106
Timestamp: 2025-09-24T23:14:37.777Z
Learning: In the jdk-javac-plugin NullnessAnnotationSerializer, the plugin only creates one JSON file per compilation, so processing only the first JSON file is intentional behavior rather than a bug.
msridhar
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
jdk-javac-plugin/src/test/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializerTest.java (1)
147-171: Consider adding a clarifying comment for private class exclusion.The
PrivateInner<T extends @Nullable Object>class (line 157) has an annotated type parameter but is correctly excluded from the expected output because it's private. While this behavior is correct and consistent withprivateMethodsExcluded, it might not be immediately obvious to readers why a class with jspecify annotations is excluded.Consider adding a comment above line 157 to clarify:
@NullUnmarked class Inner { } + // PrivateInner is excluded despite having annotated type parameters because it's private private class PrivateInner<T extends @Nullable Object> { }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
jdk-javac-plugin/src/test/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializerTest.java(5 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 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.
Learnt from: haewiful
PR: uber/NullAway#1243
File: jdk-annotations/astubx-generator-cli/src/main/java/com/uber/nullaway/jdkannotations/AstubxGeneratorCLI.java:97-106
Timestamp: 2025-09-24T23:14:37.777Z
Learning: In the jdk-javac-plugin NullnessAnnotationSerializer, the plugin only creates one JSON file per compilation, so processing only the first JSON file is intentional behavior rather than a bug.
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.
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.
📚 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/src/test/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializerTest.java
📚 Learning: 2025-09-24T23:14:37.777Z
Learnt from: haewiful
PR: uber/NullAway#1243
File: jdk-annotations/astubx-generator-cli/src/main/java/com/uber/nullaway/jdkannotations/AstubxGeneratorCLI.java:97-106
Timestamp: 2025-09-24T23:14:37.777Z
Learning: In the jdk-javac-plugin NullnessAnnotationSerializer, the plugin only creates one JSON file per compilation, so processing only the first JSON file is intentional behavior rather than a bug.
Applied to files:
jdk-javac-plugin/src/test/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializerTest.java
📚 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-javac-plugin/src/test/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializerTest.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-javac-plugin/src/test/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializerTest.java
🔇 Additional comments (1)
jdk-javac-plugin/src/test/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializerTest.java (1)
57-357: Excellent test coverage for the optimization feature!The test suite comprehensively validates the PR objective of writing JSON only when jspecify annotations are present. Key scenarios are well-covered:
- Classes and methods with/without annotations
- Type parameter annotations at class and method levels
- Deep annotation scanning (arrays, wildcards, type variables, intersections)
- Private member exclusion (visibility filtering before annotation filtering)
- The past review comment about testing unannotated class+method is properly addressed in
skipNonAnnotatedClassesThe tests correctly validate that the serializer:
- Excludes private members regardless of annotations (consistent across
privateMethodsExcludedandinnerAndAnonymousClasses)- Among non-private members, only includes those with jspecify annotations
- Captures type parameter information and fully-qualified annotated types
2 participants
This pull request will optimize the
jdk-javac-pluginby only writing class and method information on the JSON file when jspecify annotations are used.Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores