Adopt jspecify + NullAway null-checking on fdb-extensions - #4579
Draft
arnaud-lacurie wants to merge 17 commits into
Draft
Adopt jspecify + NullAway null-checking on fdb-extensions#4579arnaud-lacurie wants to merge 17 commits into
arnaud-lacurie wants to merge 17 commits into
Conversation
…ompileTestJava compileJava is fully clean. compileTestJava fixes in progress (BunchedMapTest.java partially done). Temporary -Xmaxerrs bump in fdb-extensions.gradle still present for debugging visibility; will be removed once compileTestJava is fully clean.
CallbackUtilsTest, CloseableUtilsTest, LoggableExceptionTest, MoreAsyncUtilTest now compile clean under NullAway. Mostly Objects.requireNonNull() at genuine Throwable.getCause() dereferences, plus a couple of narrowly-scoped suppressions for known tool limitations (nested generic type-argument annotations read across compilation units; an internal null-tolerance test that intentionally exercises behavior wider than dedupIterable's public <T> bound).
Group D complete: clientlog (ClientLogEventCounterTest, DatabaseClientLogEventsTest) and hnsw (TestHelpers, OperationsTest, SiftTest, CardinalityTest) now compile clean under NullAway. Mostly @tempdir field suppressions (JUnit-injected, not constructor-visible), db.run()-returns-null suppressions, and real Objects.requireNonNull() fixes for genuinely-nullable external API results (FDB.open(cluster), ArgumentsAccessor.get(), TupleKeyCountTree.getParent()).
…-extensions - DatabaseClientLogEvents.java: avoid binding the Transaction field to a named local (PMD CloseResource false positive -- the field is still closed via tr.close() elsewhere; only the null-check needed the requireNonNull, not a persistent alias). - RTree.java: fix indentation in fetchUpdatePathToLeaf() left inconsistent by the earlier NullAway-motivated block-lambda restructuring. pmdMain and checkstyleMain are clean.
… by NullAway migration SpotBugs reads jspecify nullability annotations too, so several pre-existing but previously-undetected null-dereference patterns in fdb-extensions became visible once the mechanical swap changed Node.getParentNode()/NodeOrAdjust accessors to use type-use @nullable: - AbstractNode.toString(), RTree.depth(), and two RTree lambdas (insertOrUpdateSlot, deleteSlotIfExists) each called a getter twice (once to null-check, once to dereference); capture the result once instead so the two uses provably agree. - DeleteNeighborsChangeSet/InsertNeighborsChangeSet.merge()/writeDelta() call their own getParent() override, which is intentionally narrowed to always be non-null (backed by a non-null field) -- SpotBugs's interprocedural analysis doesn't seem to account for this covariant narrowing and falls back to the wider @nullable contract declared by NeighborsChangeSet.getParent(); suppressed with a comment explaining why, matching the existing SpotBugsSuppressWarnings convention. - Lens.java: removed a now-stale @SpotBugsSuppressWarnings that an earlier real NullAway-motivated fix (avoiding Lens.this.get(null)) made unnecessary, surfaced by SpotBugs's own US_USELESS_SUPPRESSION_ON_METHOD check. spotbugsMain is clean (verified via a from-scratch recompile after discovering that Gradle's incremental compilation can otherwise mask/produce spurious NullAway diagnostics in this errorprone setup).
…nd BunchedMapTest.java
sum.add(v) was discarded because RealVector.add() is immutable and returns a new vector rather than mutating the receiver, so `sum` never actually accumulated past the first non-null vector. Assign the result back to `sum` and add the same Objects.requireNonNull narrowing used for `v` and `q` so NullAway can see `sum` is non-null after the loop.
Tuple.from(Object...) is from the unannotated fdb-java client library and genuinely supports null elements, but its varargs parameter is treated as @nonnull by NullAway's defaults; introduce a small tupleFromNullable(...) helper (suppressed at its single declaration) in TupleHelpersTest, TupleTest, and BunchedTupleSerializerTest to construct the intentionally-null-containing tuples used by those tests. Also narrow two @nullable String results from ByteArrayUtil2.loggable() with Objects.requireNonNull in ByteArrayUtil2Test, since the inputs are non-null literals so the result can never actually be null.
Apply the established patterns from this rollout: - db.run(Function<Transaction, T>) side-effect-only lambdas that return null are assigned to a @SuppressWarnings("NullAway") final Void, since no void-returning overload exists. - @tempdir fields are injected by JUnit's TempDirectory extension via reflection, which NullAway cannot see; suppress at the field declaration with an explanatory comment (matches the hnsw test files already fixed on this branch). - RangeSet.insertRange's begin/end parameters are declared @nullable byte[], but NullAway does not reliably honor @nullable on array-typed parameters, so the null-literal call is misflagged; suppress on the narrow local declaration. - GuardiannStructureAsserts.snapshotStructure() is @nullable by contract (null means "no clusters yet"); narrow with Objects.requireNonNull at the call site, consistent with existing call sites elsewhere in this package.
Same established patterns as the previous guardiann commit: suppress @tempdir field "not initialized" false positives, assign side-effect-only db.run(...) lambdas to a suppressed Void, and narrow GuardiannStructureAsserts.snapshotStructure()'s @nullable result with Objects.requireNonNull at the call site. Files: DeleteReplicationPersistenceTest, DeterministicReplayTest, ReassignScenarioTest, SplitMergeSplitScenarioTest.
Same established patterns as the rest of the guardiann package: suppress the @tempdir field's "not initialized" false positive, and assign each side-effect-only db.run(...) lambda to a suppressed Void since Function<Transaction, T> has no void-returning overload. Also suppress the onWriteListener field: unlike the static db field (assigned in a @BeforeAll method, which NullAway recognizes as an initializer), onWriteListener is assigned by the instance helper newGuardiann() -- not a JUnit lifecycle method -- so NullAway's field initialization check does not see it as initialized, even though every test calls newGuardiann() before touching the field.
- RankedSetTest: assign side-effect-only db.run/db.read/tc.run lambdas to a suppressed Void, per the established pattern. - RTreeModificationTest/RTreeScanTest: suppress the byte[]/array-type NullAway limitation for RTree.scan/BunchedMap.scan continuations and add a tupleFromNullable(...) helper for Tuple.from(...) calls that intentionally include a null coordinate (RTree.Point.getCoordinate is genuinely @nullable). Also fix a stale double array-read in RTreeScanTest.queryTopNWithFilters (items[i] used again after being read once through Objects.requireNonNull(items)[i]) by capturing the element once instead. - HalfTest: suppress the one call that deliberately passes null to Half.valueOf(String) to verify its defensive NullPointerException behavior at the API boundary. - BunchedMapScanTest: finish the remaining continuationWithDeletes/ testScanMulti fixes (byte[] continuation limitation, and BunchedMapScanEntry.getSubspaceTag() narrowing via Objects.requireNonNull -- this test's splitter always derives a non-null Long tag even though the accessor is generically @nullable). This brings :fdb-extensions:compileTestJava to a clean build.
…oke null-containing streams An earlier NullAway fix on this branch (738a760d9) added Objects.requireNonNull(next) to filterRemaining()'s next(), silencing a compile-time NullAway warning but breaking dedupIterable() (which is built on filterIterable/filterRemaining and is documented/tested to support streams with null elements): every call to next() on a filtered/deduped stream whose current element was null now threw NPE at runtime instead of returning null. filterIterable/filterRemaining/dedupIterable's type parameter is reused across both non-null and nullable-element instantiations, so per this rollout's established pattern it needs <T extends @nullable Object> rather than plain <T>. The override of AsyncIterator.next() still can't be declared @nullable (NullAway infers @nonnull for that unannotated external method), so the method is suppressed instead of narrowed with requireNonNull, since requireNonNull would reproduce the same bug it replaces. Caught by MoreAsyncUtilTest.dedupIterableEmitsLeadingNullAndDoesNotCollapseAdjacentNulls, which was failing on this branch (unrelated to the FDB-cluster- connectivity failures expected elsewhere in :fdb-extensions:test).
Checkstyle flagged the import as unused; nothing in the file needs an explicit @nullable annotation (the one intentional-null case uses a @SuppressWarnings("NullAway")-annotated local instead).
arnaud-lacurie
changed the title
arnaud-lacurie
commented
Sep 7, 2026
| // re-declared @Nullable; Objects.requireNonNull() would be wrong too, since it would incorrectly | ||
| // reject that legitimate null case, so the method is suppressed instead. | ||
| @SuppressWarnings("NullAway") | ||
| public T next() { |
Collaborator
Author
There was a problem hiding this comment.
Regression-then-fix worth calling out: an earlier commit on this branch silenced a NullAway warning here by doing return Objects.requireNonNull(next);, but next can legitimately be null when this iterator wraps a nullable-element stream (e.g. via dedupIterable()), so every next() call on a null element started throwing NullPointerException at runtime. That was caught by MoreAsyncUtilTest.dedupIterableEmitsLeadingNullAndDoesNotCollapseAdjacentNulls. The fix here widens filterIterable/filterRemaining/dedupIterable's type parameter to <T extends @Nullable Object> and suppresses (rather than narrows) this override, since AsyncIterator.next() is unannotated external API that NullAway infers as @NonNull, and requireNonNull would just reintroduce the bug.
| @@ -325,11 +321,12 @@ void encodeManyWithEstimationsTest(final long seed, final int numDimensions, fin | |||
| if (sum == null) { | |||
| sum = v; | |||
| } else { | |||
| sum.add(v); | |||
| sum = sum.add(v); | |||
Collaborator
Author
There was a problem hiding this comment.
Real bug fixed here: RealVector.add() is immutable and returns a new vector rather than mutating in place, so the original sum.add(v); (discarding the return value) meant sum never accumulated past the first vector added to it — the centroid computed a few lines below was silently wrong. Fixed to sum = sum.add(v); so the running sum actually accumulates.
| final RTree.Point point = Objects.requireNonNull(items)[i].getPoint(); | ||
| if (query.contains(point)) { | ||
| expectedResultsQueue.add(items[i]); | ||
| final Item item = Objects.requireNonNull(items)[i]; |
Collaborator
Author
There was a problem hiding this comment.
Minor correctness cleanup: the old code read Objects.requireNonNull(items)[i] to get the point, then indexed the bare (unchecked) items[i] again a few lines later to add to the results queue — two separate accesses to the same element via inconsistent null-safety. This capture the element once into item and reuses it for both the point check and the queue add, avoiding the redundant/inconsistent re-read.
fdb-extensions now depends on jspecify, but that compileOnly dependency doesn't propagate to consumers, so SpotBugs can't resolve org.jspecify.annotations.Nullable when analyzing not-yet-migrated downstream modules that reference fdb-extensions' compiled classes. Declare jspecify compileOnly for every module so the class is always resolvable regardless of migration order. Also suppresses NP_METHOD_PARAMETER_TIGHTENS_ANNOTATION, which SpotBugs raises whenever a JSR-305-annotated override disagrees with a jspecify-annotated superclass parameter even when both intend the same nullability -- and separately suppresses one exception-hierarchy site where the diagnostic is unrelated to the migration but was only now resolvable through the newly-annotated ByteArrayUtil2.loggable.
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.
6th of a 14-PR stack adopting jspecify + NullAway null-checking, stacked on #4578 (
fdb-record-layer-icu). Same treatment applied tofdb-extensions, split into two independently-worked chunks (async/vector-search packages; linear/math and misc-utility packages) then merged. See inline comments for specific findings.