Checker to ensure explicit null marking of every class#1323
Checker to ensure explicit null marking of every class#1323
Conversation
WalkthroughAdds a new Error Prone BugChecker Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (5)📓 Common learnings📚 Learning: 2025-08-29T18:41:43.584ZApplied to files:
📚 Learning: 2025-08-28T04:54:20.953ZApplied to files:
📚 Learning: 2025-08-14T18:50:06.159ZApplied to files:
📚 Learning: 2025-10-29T23:56:18.236ZApplied to files:
🔇 Additional comments (5)
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
nullaway/src/test/java/com/uber/nullaway/RequireExplicitNullMarkingTest.java (1)
58-66: Consider adding test coverage for module-level annotations.The test correctly verifies package-level annotation behavior. However, the PR description mentions that the checker also supports module-level annotations ("its containing package, and its containing module (if present)"), but there's no test case for this scenario.
Additionally, consider adding test coverage for:
- Enums, interfaces, and records (to ensure
getNestingKind() == TOP_LEVELhandles all top-level type declarations)- Module-level
@NullMarkedannotation (viamodule-info.java)Example test case for module-level annotation:
@Test public void moduleLevelAnnotationAllowsMissingClassAnnotation() { compilationHelper .addSourceLines( "module-info.java", "@org.jspecify.annotations.NullMarked", "module test.module {", " requires org.jspecify;", "}") .addSourceLines( "test/ModuleMarkedClass.java", "package test;", "class ModuleMarkedClass {}") .doTest(); }nullaway/src/main/java/com/uber/nullaway/RequireExplicitNullMarking.java (1)
65-68: UseNullabilityUtilconstants instead of string literals for consistency.The original concern about simple name matching is incorrect. Simple name matching for these annotations is intentional and consistently used throughout the codebase. No custom annotations with conflicting names exist in the repository.
However,
RequireExplicitNullMarkingshould follow the established pattern used everywhere else in the codebase: replace the hardcoded strings withNullabilityUtil.NULLMARKED_SIMPLE_NAMEandNullabilityUtil.NULLUNMARKED_SIMPLE_NAMEconstants (lines 66–67).private static boolean hasNullMarkedOrNullUnmarkedAnnotation(Symbol symbol) { return ASTHelpers.hasDirectAnnotationWithSimpleName(symbol, NullabilityUtil.NULLMARKED_SIMPLE_NAME) || ASTHelpers.hasDirectAnnotationWithSimpleName(symbol, NullabilityUtil.NULLUNMARKED_SIMPLE_NAME); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
nullaway/src/main/java/com/uber/nullaway/RequireExplicitNullMarking.java(1 hunks)nullaway/src/test/java/com/uber/nullaway/RequireExplicitNullMarkingTest.java(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: msridhar
Repo: uber/NullAway PR: 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.
Learnt from: msridhar
Repo: uber/NullAway PR: 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
Repo: uber/NullAway PR: 1245
File: guava-recent-unit-tests/src/test/java/com/uber/nullaway/guava/NullAwayGuavaParametricNullnessTests.java:101-102
Timestamp: 2025-08-14T18:50:06.159Z
Learning: In NullAway JSpecify tests, when JDK version requirements exist due to bytecode annotation reading capabilities, prefer failing tests over skipping them on unsupported versions to ensure CI catches regressions and enforces proper JDK version usage for developers.
📚 Learning: 2025-08-29T18:41:43.584Z
Learnt from: msridhar
Repo: uber/NullAway PR: 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:
nullaway/src/main/java/com/uber/nullaway/RequireExplicitNullMarking.javanullaway/src/test/java/com/uber/nullaway/RequireExplicitNullMarkingTest.java
📚 Learning: 2025-08-28T04:54:20.953Z
Learnt from: msridhar
Repo: uber/NullAway PR: 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:
nullaway/src/main/java/com/uber/nullaway/RequireExplicitNullMarking.javanullaway/src/test/java/com/uber/nullaway/RequireExplicitNullMarkingTest.java
📚 Learning: 2025-08-14T18:50:06.159Z
Learnt from: msridhar
Repo: uber/NullAway PR: 1245
File: guava-recent-unit-tests/src/test/java/com/uber/nullaway/guava/NullAwayGuavaParametricNullnessTests.java:101-102
Timestamp: 2025-08-14T18:50:06.159Z
Learning: In NullAway JSpecify tests, when JDK version requirements exist due to bytecode annotation reading capabilities, prefer failing tests over skipping them on unsupported versions to ensure CI catches regressions and enforces proper JDK version usage for developers.
Applied to files:
nullaway/src/main/java/com/uber/nullaway/RequireExplicitNullMarking.javanullaway/src/test/java/com/uber/nullaway/RequireExplicitNullMarkingTest.java
📚 Learning: 2025-10-29T23:56:18.236Z
Learnt from: msridhar
Repo: uber/NullAway PR: 1316
File: jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java:261-293
Timestamp: 2025-10-29T23:56:18.236Z
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.
Applied to files:
nullaway/src/main/java/com/uber/nullaway/RequireExplicitNullMarking.javanullaway/src/test/java/com/uber/nullaway/RequireExplicitNullMarkingTest.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 spring-framework with snapshot
- GitHub Check: Build caffeine with snapshot
- GitHub Check: Build and test on macos-latest
- GitHub Check: Build and test on ubuntu-latest
- GitHub Check: Build and test on windows-latest
🔇 Additional comments (7)
nullaway/src/main/java/com/uber/nullaway/RequireExplicitNullMarking.java (3)
22-34: LGTM!Package and imports are appropriate for an Error Prone checker implementation.
35-46: LGTM!The class structure and annotations follow Error Prone conventions correctly. The SUGGESTION severity is appropriate for an opt-in checker intended for migration purposes.
48-63: LGTM!The logic correctly:
- Filters to only top-level classes using
getNestingKind()- Walks up the enclosing symbol hierarchy (class → package → module) to find null-marking annotations
- Short-circuits appropriately when annotations are found
The implementation aligns with the PR objectives to enforce explicit marking at any level of the containment hierarchy.
nullaway/src/test/java/com/uber/nullaway/RequireExplicitNullMarkingTest.java (4)
14-18: LGTM!Standard Error Prone test setup is correct.
20-32: LGTM!Good test coverage for the core requirement. The nested class check ensures the
TOP_LEVELfiltering works correctly.
34-44: LGTM!Correctly verifies that direct
@NullMarkedannotation satisfies the requirement.
46-56: LGTM!Correctly verifies that direct
@NullUnmarkedannotation also satisfies the requirement.
…rking.java Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
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)
nullaway/src/main/java/com/uber/nullaway/RequireExplicitNullMarking.java(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: msridhar
Repo: uber/NullAway PR: 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.
Learnt from: msridhar
Repo: uber/NullAway PR: 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
Repo: uber/NullAway PR: 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
Repo: uber/NullAway PR: 1316
File: jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java:261-293
Timestamp: 2025-10-29T23:56:18.236Z
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.
📚 Learning: 2025-08-29T18:41:43.584Z
Learnt from: msridhar
Repo: uber/NullAway PR: 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:
nullaway/src/main/java/com/uber/nullaway/RequireExplicitNullMarking.java
📚 Learning: 2025-08-28T04:54:20.953Z
Learnt from: msridhar
Repo: uber/NullAway PR: 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:
nullaway/src/main/java/com/uber/nullaway/RequireExplicitNullMarking.java
📚 Learning: 2025-08-14T18:50:06.159Z
Learnt from: msridhar
Repo: uber/NullAway PR: 1245
File: guava-recent-unit-tests/src/test/java/com/uber/nullaway/guava/NullAwayGuavaParametricNullnessTests.java:101-102
Timestamp: 2025-08-14T18:50:06.159Z
Learning: In NullAway JSpecify tests, when JDK version requirements exist due to bytecode annotation reading capabilities, prefer failing tests over skipping them on unsupported versions to ensure CI catches regressions and enforces proper JDK version usage for developers.
Applied to files:
nullaway/src/main/java/com/uber/nullaway/RequireExplicitNullMarking.java
📚 Learning: 2025-10-29T23:56:18.236Z
Learnt from: msridhar
Repo: uber/NullAway PR: 1316
File: jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java:261-293
Timestamp: 2025-10-29T23:56:18.236Z
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.
Applied to files:
nullaway/src/main/java/com/uber/nullaway/RequireExplicitNullMarking.java
🔇 Additional comments (4)
nullaway/src/main/java/com/uber/nullaway/RequireExplicitNullMarking.java (4)
23-34: LGTM!Imports are appropriate and necessary for the checker implementation.
36-47: LGTM!The checker declaration is well-structured with appropriate severity level (SUGGESTION for opt-in) and clear documentation.
49-64: LGTM!The matchClass logic correctly implements the checker's contract by filtering to top-level classes and walking enclosing symbols (package, module) to detect explicit null-marking annotations.
66-69: LGTM!The simple name matching approach for annotations is consistent with Error Prone and NullAway patterns, providing flexibility without hard dependencies on specific annotation packages.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
Tests are failing here due to an annoying interaction with explicitly passing An alternative would be to integrate this checking into NullAway and enable it when the @ben-manes @sdeleuze would be interested if you have any thoughts here (and anyone else who is reading...) |
|
I know for large code bases that are converting it requires gradually opting in. Doing so using If the intent is to instead require a package-info on all packages and be |
|
Thanks @ben-manes! The way this check is written it would indeed require either All that said, seems safer to make this check opt-in at first, which works well with the separate checker implementation. If lots of people run into this problem and aren't finding how to opt in, we could think about making it opt-out instead. |
|
Useful feature, but yeah I think this check should indeed be opt-in, to allow annotating initially packages after packages or classes after classes without too much friction. |
…tests (#1326) This seems to have a global side effect that causes weird test failures in #1323, and it's a bit of a hack. Instead, move the relevant tests to the `test-library-model` module where no such hacks are required. Test-only changes. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added capability to suggest removal of unnecessary non-null casts from library models * **Tests** * Expanded test coverage for library model scenarios with new test cases * **Chores** * Updated build dependencies and test configuration <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1323 +/- ##
============================================
- Coverage 88.42% 88.41% -0.01%
- Complexity 2566 2574 +8
============================================
Files 96 97 +1
Lines 8605 8617 +12
Branches 1708 1713 +5
============================================
+ Hits 7609 7619 +10
- Misses 500 501 +1
- Partials 496 497 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
BTW, another reason to prefer |
NullAway has long supported the
AnnotatedPackagessetting to indicate that all packages within a namespace should be treated as "annotated" (i.e., "null-marked"), with packages treated as hierarchical. JSpecify instead requires writing@NullMarkedannotations on every class / package / module to achieve the same effect, with packages not treated as hierarchical (matching Java). While usingAnnotatedPackagesis more succinct, it is not standard, and has the disadvantage that for a library, client code will not see the code as null-marked if it lacks the JSpecify marking annotations. However, using JSpecify marking annotations with NullAway has another disadvantage: if you forget to annotate a class / package / module as@NullMarkedyou won't get any warning; NullAway will simply treat the code as@NullUnmarkedand not check it for nullness issues.This PR adds a
RequireExplicitNullMarkingError Prone checker to address the last problem. It reports an issue for any top-level class that does not have an explicit@NullMarkedor@NullUnmarkedannotation on itself, its containing package, and its containing module (if present). So, classes being compiled can no longer be defaulted into being@NullUnmarked; they need to be explicitly marked as such. This checker is opt-in and off by default, so there is no backward compatibility issue. This check will be helpful for code bases that want to transition from using theAnnotatedPackagessetting to using standard JSpecify annotations.Note there is the related
AddNullMarkedToPackageInfocheck built into Error Prone, which is complementary to this one.Summary by CodeRabbit
New Features
Tests