Adopt jspecify + NullAway null-checking on fdb-record-layer-debugger - #4580
Draft
arnaud-lacurie wants to merge 1 commit into
Conversation
arnaud-lacurie
commented
Sep 7, 2026
| final PlannerPhase plannerPhase = getPlannerPhase(); | ||
| plannerRepl.printKeyValue("plannerPhase", | ||
| (getPlannerPhase() == null ? getPlannerPhase().name() : "any").toLowerCase(Locale.ROOT)); | ||
| (plannerPhase == null ? "any" : plannerPhase.name()).toLowerCase(Locale.ROOT)); |
Collaborator
Author
There was a problem hiding this comment.
Real bug fixed here: the previous ternary was (getPlannerPhase() == null ? getPlannerPhase().name() : "any") — backwards on both sides. When the phase was null it called .name() on the null value (guaranteed NullPointerException), and when it was non-null it printed the literal string "any" instead of the actual phase name, so OnPhaseBreakPoint.onList() never displayed the real planner phase. The fix captures getPlannerPhase() once into a local and corrects the ternary to (plannerPhase == null ? "any" : plannerPhase.name()), matching what the null-check should have guarded in the first place.
Same treatment as the grpc/jdbc branches: jspecify + NullAway wired via net.ltgt.errorprone, scoped to this module only; the com.apple.foundationdb.record.query.plan.cascades.debug package is marked @NullMarked; javax.annotation.Nonnull/Nullable usages replaced with jspecify's @nullable. Compiling with NullAway surfaced a real bug in PlannerRepl.OnPhaseBreakPoint: onList() had its null-check ternary backwards (getPlannerPhase() == null ? getPlannerPhase().name() : "any"), which would NPE whenever "break list" was run after setting a phase-less "phase" breakpoint; fixed the logic (and a similar hashCode() case flagged by SpotBugs) using a local variable instead of re-invoking the getter. Debugger.setDebugger(null) (fdb-record-layer-core, outside this module) documents and implements null-acceptance to clear the debugger, but its parameter isn't annotated @nullable; suppressed at the two call sites in this module's tests with a comment, since fixing the declaration is out of scope.
arnaud-lacurie
force-pushed
the
apple/arnaud-lacurie/jspecify-nullaway/debugger
branch
from
September 8, 2026 00:07
9605a6c to
65fe185
Compare
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
1 participant
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.
7th of a 14-PR stack adopting jspecify + NullAway null-checking, stacked on #4579 (
fdb-extensions). Same treatment applied tofdb-record-layer-debugger. See inline comments for specific findings.