build: enable NullAway, scoped to packages that opt in with @NullMarked - #3367
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThe build now publishes JSpecify, enables NullAway for ChangesNullness tooling and logging contract
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The PR introduces package-scoped nullness checking and updates Logger annotations and published dependency metadata; the supplied verification reports successful full builds and tests, so no actionable merge-blocking risk remains beyond normal checks. Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
This was referenced Aug 15, 2026
TestNG has no nullness checking, so whether a method may return null is only discoverable by reading it. NullAway plugs into the Error Prone pass that already runs on every module, and OnlyNullMarked confines it to code declaring JSpecify's @NullMarked. No package declares it yet, so this commit touches no Java source and reports nothing. That scoping is the point. Nullness can now be adopted package by package, each step green on its own, instead of as a single sweep over the whole API. The earlier attempt in #2941 used the Checker Framework, which analyses every file of the compilation unit regardless of what the package opted into, and stalled on the size of the resulting diff. @NullMarked applies to a package rather than to a source set, and nine test packages share a name with a main one, so NullAway stays off for test compilation: marking org.testng or org.testng.internal would otherwise sweep in their test halves. JSpecify ships as a regular api dependency rather than compileOnly: its annotations are runtime-retained, so hiding them would strip nullness information from the published artifact instead of passing it to consumers.
First package to opt into the nullness checking wired up in the previous commit. It was picked for being small and confined to one module: most of TestNG's packages span several, and @NullMarked reaches every module where the package appears, through package-info.class on the compile classpath. Every Logger method hands its message straight to SLF4J as a "{}" argument, which renders null as the string "null" and never dereferences it. So null is accepted, and the parameters say so. The Throwable overloads keep a non-null throwable: nothing calls them otherwise. No behaviour changes -- annotations only.
krmahadevan
force-pushed
the
juherr/enable-nullaway
branch
from
August 16, 2026 03:26
c7d7263 to
481f404
Compare
This was referenced Aug 16, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TestNG has no nullness checking, so whether a method may return null is only discoverable by reading it. This wires up NullAway and takes the first package through it.
Supersedes #2941, which used the Checker Framework. That branch is now three years and 336 commits behind, 35 of its 59 files conflict, and
testng-assertshas since left the repository. More importantly the Checker Framework analyses every file of the compilation unit regardless of what a package opted into, so the change could not be split — the branch stalled on the size of the diff.Commit 1 — the wiring
NullAway plugs into the Error Prone pass that already runs on every module, so there is no new Gradle plugin.
OnlyNullMarkedconfines it to code declaring JSpecify@NullMarked, which makes this commit report nothing and touch no Java source.That scoping is the point: nullness can be adopted one package at a time, each step green on its own, instead of as one sweep over the whole API.
JSpecify ships as a regular
apidependency rather thancompileOnly. Its annotations are runtime-retained, so hiding them would strip nullness information from the published artifact instead of passing it to consumers.verifyPublishedPomDependenciesis updated accordingly.Commit 2 — the first package
org.testng.log4testng, picked for being small and confined to one module. That last part matters:@NullMarkedapplies to a package, and 10 of TestNG's 25 main-source packages appear in more than one module, so marking one half marks the other through the compile classpath.org.testng.internalalone spans four modules.Every
Loggermethod hands its message straight to SLF4J as a{}argument, which renders null as the string"null"and never dereferences it — verified against slf4j-simple 2.0.18. So the parameters are@Nullable. TheThrowableoverloads keep a non-null throwable.Verification
Full build green on both commits: 15726 tests in testng-core, 0 failures, including
testng-test-osgi, which exercises the bundle with the new dependency.The wiring was checked to be a real no-op rather than an inert one: a throwaway
@NullMarkedpackage with areturn nullmade NullAway fail the compile as expected, and was removed.Follow-up
Enabling the check on
org.testng.internal.reflectsurfaced a live NullPointerException, reported as #3361 and fixed separately in #3362.The follow-on packages are stacked on this branch rather than folded in, so each stays green on its own: #3365 takes
org.testng.collections,org.testng.cliandorg.testng.cli.jcommander.Replaces #3363, which was opened from a fork branch. GitHub requires a pull request's base to be a branch of the target repository, so the stack above it could not point at it. Identical commits, same SHAs.
Summary by CodeRabbit