Adopt jspecify + NullAway null-checking on fdb-record-layer-spatial - #4582
Draft
arnaud-lacurie wants to merge 2 commits into
Draft
Conversation
arnaud-lacurie
changed the title
arnaud-lacurie
commented
Sep 7, 2026
| protected boolean shouldSwapLatLong(Key.Evaluated arguments) { | ||
| // Boolean.TRUE.equals(...) rather than unboxing, since a null argument value (unlikely, but not | ||
| // statically impossible) would otherwise NPE here; treat it the same as an absent/false argument. | ||
| return arguments.size() > 1 && Boolean.TRUE.equals(arguments.getObject(1, Boolean.class)); |
Collaborator
Author
There was a problem hiding this comment.
This was a genuine latent NPE, not just an annotation gap: shouldSwapLatLong() used to unbox arguments.getObject(1, Boolean.class) directly in the &&, which throws if the argument value is legitimately null (e.g. an unbound query parameter) rather than simply absent. The fix switches to Boolean.TRUE.equals(...), which treats a null value the same as an absent/false argument instead of crashing.
| // GeophileRecordImpl with a null entry as an internal Geophile scratch/comparison object), but | ||
| // records yielded by this iterator are always built from a real index entry (see | ||
| // GeophileCursorImpl#next()). | ||
| return recordCursor.map(record -> Objects.requireNonNull(record.getIndexEntry())); |
Collaborator
Author
There was a problem hiding this comment.
getIndexEntry() is @Nullable in general because GeophileIndexImpl#newRecord() builds an internal scratch/comparison GeophileRecordImpl with a null entry. Records yielded by this iterator (from GeophileCursorImpl#next()) are always backed by a real index entry though, so wrapping with Objects.requireNonNull(...) here documents and enforces that actual invariant rather than papering over it with a suppression.
Companion change to the fdb-relational-grpc / fdb-relational-jdbc null-checking adoption. jspecify + NullAway wired via net.ltgt.errorprone, scoped to this module only; com.apple.foundationdb.record.spatial.common and .geophile are marked @NullMarked; javax.annotation.Nonnull/Nullable usages replaced with jspecify's @nullable. Notable findings while compiling with NullAway: - DoubleValueOrParameter.getValue() was already documented ("@return a double value or {@code null}") but not annotated @nullable; now it and its two overrides are. - GeophileSpatialFunctionKeyExpression.shouldSwapLatLong() unboxed a Key.Evaluated argument that could genuinely be null, a latent NPE; fixed with Boolean.TRUE.equals(...) instead of suppressing. - GeophileSpatialIndexJoinPlan.fetchIndexRecords() dereferenced Pair.getLeft()/ getRight(), which are statically @nullable (GeophileRecordImpl.getIndexEntry() can be null for Geophile's internal scratch records); guarded with Objects.requireNonNull() since a completed spatial join only emits records built from real index entries. - The test proto (test_records_geo.proto) generates TestRecordsGeoProto.java directly into com.apple.foundationdb.record.spatial.geophile (same package as hand-written sources, not a dedicated subpackage), so it couldn't be carved out via NullAway:UnannotatedSubPackages like generated grpc/protobuf code was in other modules; excluded by path instead (errorprone excludedPaths), matching the existing **/generated/** exclusion used for checkstyle/PMD. - NullAway/JSpecify doesn't reliably track @nullable on byte[] parameters when overriding unannotated-interface methods (IndexMaintainer#scan, RecordQueryPlanWithIndex#executeEntries); suppressed with a comment, per the same known limitation seen on the grpc/jdbc modules. - A qualified generic return type (SpatialJoin.Filter<...>) with @nullable on its own line hit a javac "scoping construct cannot be annotated" error; fixed by static-importing Filter and using the simple name.
… contracts Mirrors the fdb-record-layer-lucene fix: NullAway now sees fdb-record-layer-core's corrected nullability contracts from spatial's own NullAway-annotated package for the first time, surfacing errors in main and test sources. Real fix: - GeophileSpatialJoin#recordCursor(SpatialObject, SpatialIndex): assert with Objects.requireNonNull that getIndexEntry() is non-null for records yielded by this iterator (GeophileRecordImpl#getIndexEntry is @nullable in general because GeophileIndexImpl#newRecord() constructs an internal scratch/comparison record with a null entry, but records produced by GeophileCursorImpl#next() are always built from a real index entry). The remaining two errors are the same documented array (byte[]) continuation-parameter tooling gap seen throughout this rollout (IndexMaintainer#scan / RecordQueryPlan#execute), fixed with @SuppressWarnings("NullAway") plus an explanatory comment, and (in the test) correcting a mistyped byte[] continuation local to byte @nullable [].
arnaud-lacurie
force-pushed
the
apple/arnaud-lacurie/jspecify-nullaway/spatial
branch
from
September 8, 2026 00:07
5805e0d to
525870a
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.
9th of a 14-PR stack adopting jspecify + NullAway null-checking, stacked on #4581 (
fdb-record-layer-core). Same treatment applied tofdb-record-layer-spatial. See inline comments for specific findings.