Adopt jspecify + NullAway null-checking on fdb-record-layer-core - #4581
Draft
arnaud-lacurie wants to merge 190 commits into
Draft
Adopt jspecify + NullAway null-checking on fdb-record-layer-core#4581arnaud-lacurie wants to merge 190 commits into
arnaud-lacurie wants to merge 190 commits into
Conversation
arnaud-lacurie
changed the title
arnaud-lacurie
commented
Sep 7, 2026
| // lockRef is only set once acquire()'s future completes successfully; if acquire() itself fails, | ||
| // thenCompose's lambda never runs and lockRef stays unset, so guard against that here. | ||
| final var acquiredLock = lockRef.get(); | ||
| if (acquiredLock != null) { |
Collaborator
Author
There was a problem hiding this comment.
Real bug: previously this whenComplete callback unconditionally called lockRef.get().release(). If acquire() itself failed before the thenCompose lambda ever ran, lockRef was never set, and the resulting NPE masked the real underlying failure from callers. The fix adds a null guard so the original exception from acquire() propagates instead of being replaced by a confusing NPE.
| @Nonnull | ||
| @Override | ||
| public LoggableTimeoutException addLogInfo(@Nonnull String description, Object object) { | ||
| public LoggableTimeoutException addLogInfo(String description, @Nullable Object object) { |
Collaborator
Author
There was a problem hiding this comment.
Real bug: this override previously declared object as @Nonnull/unannotated while the overridden LoggableKeysAndValues.addLogInfo declares it @Nullable — a Liskov substitution violation that could NPE callers relying on the (correct) supertype contract. Fixed by widening the override's parameter to @Nullable Object object, matching the superclass.
| @@ -164,13 +155,10 @@ public static UUID stringToUuidValue(String value) { | |||
| try { | |||
| return UUID.fromString(value); | |||
| } catch (IllegalArgumentException ex) { | |||
| SemanticException.fail(SemanticException.ErrorCode.INVALID_UUID_VALUE, value); | |||
| throw new SemanticException(SemanticException.ErrorCode.INVALID_UUID_VALUE, value, ex); | |||
Collaborator
Author
There was a problem hiding this comment.
Real bug: this catch (IllegalArgumentException ex) block previously called SemanticException.fail(...), constructing a new exception without chaining ex as the cause, so the original stack trace (and reason UUID.fromString failed) was silently dropped. Fixed to throw new SemanticException(SemanticException.ErrorCode.INVALID_UUID_VALUE, value, ex), preserving the cause. Caught by a PMD PreserveStackTrace violation.
| RecordType newRecordType = newMetaData.getRecordType(newRecordTypeName); | ||
| Integer sinceVersion = newRecordType.getSinceVersion(); | ||
| if (sinceVersion == null || newRecordType.getSinceVersion() <= oldMetaData.getVersion()) { | ||
| if (sinceVersion == null || sinceVersion <= oldMetaData.getVersion()) { |
Collaborator
Author
There was a problem hiding this comment.
Real bug: the condition previously re-called newRecordType.getSinceVersion() a second time instead of reusing the already-null-checked local sinceVersion, defeating the null check on the left side of the || and risking an NPE on unboxing when the since-version is absent. Fixed to reuse the local variable in both operands.
| runner.close(); | ||
| // Guard against buildRepairRunner() throwing before assigning runner: closing a null runner | ||
| // here would mask the original exception with an unrelated NullPointerException. | ||
| if (runner != null) { |
Collaborator
Author
There was a problem hiding this comment.
Real bug (test): the finally block previously called runner.close() unconditionally, but runner remains null if buildRepairRunner() itself throws before assigning it — masking the real failure being tested with an unrelated NPE. Fixed with a null check before calling close().
| // called; validate() does not currently check for that, so assert it here with a clear message | ||
| // rather than let a confusing NPE surface deeper in IndexingCommon's constructor. | ||
| return new OnlineIndexer( | ||
| Objects.requireNonNull(getRunner(), "runner must be set before calling build()"), |
Collaborator
Author
There was a problem hiding this comment.
Real bug: Builder.build() had no precondition check that runner/recordStoreBuilder had actually been set via setDatabase/setRecordStore. Calling build() without one would previously propagate a null straight into OnlineIndexer's constructor and surface as a confusing NPE deep inside IndexingCommon. Fixed with Objects.requireNonNull(...) guards that fail fast with a clear message at the build() call site.
| // already guaranteed non-null by validateIndex() above, but that invariant isn't visible here | ||
| // without an explicit check either. | ||
| return new OnlineIndexScrubber( | ||
| Objects.requireNonNull(getRunner(), "runner must be set before calling build()"), |
Collaborator
Author
There was a problem hiding this comment.
Same class of bug as OnlineIndexer.Builder.build(): no precondition check that runner/recordStoreBuilder were set before constructing OnlineIndexScrubber, which would previously surface as a confusing NPE deep inside the constructor instead of a clear error at the build() call site. Fixed with explicit Objects.requireNonNull(...) guards.
| @@ -102,7 +104,7 @@ public Issue(final KeyValueLogMessage logMessage, final FDBStoreTimer.Counts tim | |||
| * @param result an item that was returned by a cursor provided by {@link #getCursor} | |||
| * @return null if the result valid, an {@link Issue} if not. | |||
| */ | |||
| CompletableFuture<Issue> handleOneItem(FDBRecordStore store, RecordCursorResult<T> result); | |||
| CompletableFuture<@Nullable Issue> handleOneItem(FDBRecordStore store, RecordCursorResult<T> result); | |||
Collaborator
Author
There was a problem hiding this comment.
The declared return type CompletableFuture<Issue> didn't match this method's own documented contract ("@return null if the result valid") or its actual implementations (ValueIndexScrubbingToolsDangling/ValueIndexScrubbingToolsMissing), both of which do return null on the valid-result path. Widened to CompletableFuture<@Nullable Issue> so the signature reflects the real, intentional behavior instead of silently relying on unchecked nulls.
…pace/cursors packages
Scoped to com.apple.foundationdb.record.query.plan.cascades (root package), its .explain/.matching.graph/.typing/.debug/.values.translation subpackages, com.apple.foundationdb.record.query.plan.cascades.rules, and com.apple.foundationdb.record.query.combinatorics. Fixes real latent bugs (missing null checks backed by invariants that NullAway can't see, e.g. containsKey-then-get races, a genuine LSP violation in Correlated.BoundEquivalence.equals(), a None/Relation type code missing a getJavaClass() override, an AggregateIndexExpansionVisitor.aggregateValue() that could NPE instead of returning Optional.empty()), widens a few over-narrow @nullable annotations to match actual behavior (AliasMap#getTarget, TreeLike#replace/replaceLeavesMaybe, Debugger#mapDebugger), and adds targeted NullAway suppressions only where the mismatch is a tooling limitation (Guava's Iterables.get/getFirst null-default idiom, IdentityBiMap wrapping a delegate BiMap, PlanHashable's vararg null-tolerance, LinkedIdentityMap#get's long-standing non-null contract).
Resolves all ~112 NullAway errors under com.apple.foundationdb.record.query.plan.cascades.values and its .simplification subpackage, as part of the fdb-record-layer-core jspecify + NullAway rollout. Notable fixes beyond mechanical annotation/narrowing: - CastValue.eval: widened the overridden `store` parameter to @nullable to match Value.eval's contract (Liskov substitution violation). - MessageHelpers.TransformationTrieNode.semanticEquals: fixed a latent NPE where comparing a non-leaf node against a leaf node (mismatched children-map nullness) would dereference a null map. - CollateValue.eval/getTextCollator: propagate NULL string input to a NULL result, and fall back to default locale/strength when those children evaluate to NULL at runtime, mirroring the existing compile-time literal handling in getInvariableCollator. - JavaCallFunction: guard against a NULL function-name literal, which previously caused an unhelpful NullPointerException from Class.forName(null) instead of a domain exception. - RelOpValue.UnaryPhysicalOperator: removed the redundant per-constant Objects::isNull/Objects::nonNull evaluation function (every constant is an IS_NULL/NOT_NULL variant already fully determined by `type`), which also sidesteps a NullAway/Objects-reference nullability mismatch. - Several `SemanticException.check(x != null, ...)` followed by unguarded use of `x` were converted to throw directly or to Objects.requireNonNull, since the check helper doesn't narrow nullability for the compiler. - Widened AbstractRuleCall/AbstractValueRuleCall/ValueSimplificationRuleCall/ ValueComputationRuleCall's constraint-lookup Function to @Nullable-returning, matching the null-tolerant handling already present in getQueryPlanConstraint(). Suppressions (tooling limitations, not real bugs): - PlanHashable.objectsPlanHash's Object... varargs isn't @Nullable-annotated even though it's element-wise null-tolerant; suppressed at each call site passing a legitimately-nullable field (CountValue, CollateValue, LiteralValue, PromoteValue). - FirstOrDefaultStreamingValue/RangeValue: NullAway doesn't reliably track @nullable on byte[] parameters, even for already-@Nullable-annotated ones. No NullAway errors remain under values/ or values/simplification/ (verified via :fdb-record-layer-core:compileJava); the module as a whole still fails due to other packages being fixed in parallel by other agents.
…,synthetic,visitor} (partial) Progress checkpoint on the non-cascades query-plan NullAway scope: fixes Comparisons.java, ScanComparisons.java, QueryKeyExpression.java, OrderQueryKeyExpression.java, AndOrComponent.java, RecordQueryPlannerConfiguration.java, IndexKeyValueToPartialRecord.java, QueryPlanInfo.java, PlanOrderingKey.java, BindingFunction.java, BooleanNormalizer.java, InExtractor.java, and the plans/, sorting/, synthetic/, and visitor/ subpackages. Remaining: RecordQueryPlanner.java, TextScan.java, QueryToKeyMatcher.java, RankComparisons.java, and the bitmap/ package.
Continues the non-cascades query-plan NullAway scope checkpoint: fixes a genuinely nullable filterMask dereference pattern in QueryToKeyMatcher, adds missing @nullable on two extractXxxKey helpers there, and fixes RankComparisons' getScanComparisons/ getScoreForRank nullability (both provable via construction-time invariants). Remaining: RecordQueryPlanner.java, TextScan.java, and the bitmap/ package.
…mapIndexQueryPlan.java MergeCursorState.getResult() is @nullable before the state's onNext future resolves; whenAll() (and the resultStates selection logic) guarantee it's populated by the time these cursors dereference it, so the fixes wrap those with Objects.requireNonNull(). The OperatorComposer.operate() overrides are correctly declared @nullable byte[] already; their null-literal/ternary returns trip NullAway's known byte[] array tracking limitation, so those are suppressed with an explanatory comment.
…BitmapIndexContinuation.java indexNodes is lazily initialized together with bitmapIndexes and never cleared, so it's provably non-null wherever bitmapIndexes is; NullAway can track that narrowing for bitmapIndexes itself (checked directly) but not for the sibling field, so those call sites use Objects.requireNonNull (captured into a local before use inside a lambda, since field narrowing doesn't survive into a lambda body). The from(byte[], ...) continuation parser hits the same known byte[]-narrowing-through-null-check gap seen elsewhere in this scope; suppressed with an explanatory comment. This finishes the bitmap/ package for the non-cascades query-plan NullAway scope. Remaining: RecordQueryPlanner.java (25 errors) and TextScan.java (6 errors).
The Function<byte[], RecordCursor<IndexEntry>> lambdas returned by scanTokenPrefix/ scanToken genuinely accept a null continuation (it flows straight into store.scanIndex), but their declared type argument is a plain (non-@nullable) byte[] to stay assignable to the List<Function<byte[], ...>> that ProbableIntersectionCursor/UnionCursor/UnorderedUnionCursor/IntersectionMultiCursor (out of scope) expect; suppressed at the call sites with an explanatory comment rather than widening the shared type and risking an invariant-generics mismatch against those out-of-scope constructors. Also fixes the PlanHashable vararg nullability gap (same pattern as other files in this scope). This clears everything in this scope except RecordQueryPlanner.java.
Widens the private plan()/planNoFilter()/planOther()/planText() helpers to accept @nullable filter/sort where their bodies already null-check them (real signature gaps, not tooling issues) and adds a defensive filter==null guard in planText(). Mirrors planOrderedUnion()'s commonPrimaryKey==null handling in planFilterWithInUnion(), which was missing it. Fixes planExtractedInsFilter()'s missing @nullable return (callers already null-check it). Captures query.getAllowedIndexes() into a local before a deferred lambda, since its null-check doesn't survive into the lambda body. Wraps two provably-non-null (via the actual argument, not just the declared signature) calls with Objects.requireNonNull. ~9 of 25 errors fixed; continuing with the remainder.
Adds missing @nullable on planThenNestedField/planNestingNestedField/ planAndWithThen (both overloads) — all delegate to already-@nullable helpers. Fixes an unproven-but-provable sortOnlyPlan/planComparisonRanges invariant with Objects.requireNonNull. Mirrors planOr's allHaveSameBasePlan/commonFilteredBasePlan invariant the same way. Widens RankComparisons.planComparisonSubstitute (and its caller matchCandidateScan's filter param) to @nullable — both already pass null through unchanged, just weren't annotated. Documents PlanContext.rankComparisons as a lateinit field (set by planExtractedInsFilterOnce/planCoveringAggregateIndex before any reader runs) via a suppression with rationale, rather than threading it through the constructor across a wide call graph. Makes the getRequiredResults() precondition in planCoveringAggregateIndex explicit via requireNonNull. 18 of 25 errors fixed; 7 remain.
Widens the planNestedFieldOrComponentChild abstract method (and both overrides) to Function<@nullable KeyExpression, @nullable ScoredMatch> — both overrides already called .apply(null) and handled a null result, just weren't annotated to match. Fixes the resulting nextComparisons/scoredMatch paired-nullability gap in one override the same way as similar cases elsewhere in this file. Wraps adjustedComparisons.getLeft() with requireNonNull (Pair.getLeft()/getRight() are unconditionally @nullable regardless of the actual contract, same as the other Pair call sites fixed earlier in this scope). This clears the last of the ~163 NullAway errors in this scope (non-cascades query-plan packages): query.expressions, query.plan (root) + plans/planning/ sorting/synthetic/explain/visitor/bitmap/serialization subpackages, and query (root).
… (part 1) Covers FDBDatabase, FDBDatabaseFactory(Impl), FDBRecordContext, FDBTransactionContext, FDBMetaDataStore, IndexAggregateGroupKeys, IndexDeferredMaintenanceControl, IndexFunctionHelper, MetaDataCache, MetaDataProtoEditor, SubspaceProviderBySubspace, and the leaderboard/ recordrepair/runners.throttled subpackages, per the fix-e1 filename split. Real bugs fixed (not just annotation gaps): - FDBTransactionContext#transaction and FDBDatabase#database/fdb/reverseDirectoryCache are genuinely nullable after close()/before open() but were declared non-null, causing every dereference outside ensureActive()/database() to be a latent risk; annotated @nullable and routed access through the safe accessors. - FDBDatabaseFactory#getDatacenterId() was genuinely nullable (never set by default) but declared non-null; FDBDatabaseFactoryImpl unconditionally forwarded it to FDBDatabase#setDatacenterId(), which would NPE-risk on a fresh factory -- now guarded with a null check. - FDBMetaDataStore#parseMetaDataProto() passed a possibly-null extension registry into protobuf's parseFrom(), which its own javadoc says can NPE under proto3; now falls back to ExtensionRegistry.getEmptyRegistry(). - Generic asyncToSync(...) on FDBDatabase/FDBRecordContext was annotated @nullable on T itself, forcing every caller (even ones instantiating T non-null) to treat the result as nullable; changed to <T extends @nullable Object> so nullability follows the actual instantiation. - TimeWindowLeaderboardIndexMaintainer.UpdateState#directory follows a two-phase init contract; restructured setDirectory() to assign the field exactly once, after it's proven non-null, instead of bouncing it through null mid-method. Suppressions added only for the documented byte[]/@nullable array-tracking gap (MetaDataCache#getCachedSerialized, FDBRecordContext#addVersionMutation/ removeVersionMutation, FDBMetaDataStore.PendingCacheUpdate#serialized/run(), RecordRepair#cursorFactory(), a small noInnerContinuation() helper in TimeWindowLeaderboardIndexMaintainer), each with a comment explaining why. No remaining NullAway errors in these files as of this commit.
The Events/DetailEvents/Waits/Counts enum constructors take a genuinely optional logKey (falling back to the interface default when null), and Counts additionally takes an optional createIfNotExists-style flag combo -- annotate the delegating single-arg constructors' logKey parameters @nullable to match. recordTimeout() calls getCounter/getTimeoutCounter with createIfNotExists=true, which their javadoc guarantees is non-null, but the declared return type is @nullable regardless (StoreTimer, out of this agent's scope, already uses Objects.requireNonNull for the same pattern) -- apply the same here. No remaining NullAway errors in this file as of this commit.
Builder follows a two-phase-init contract (setters populate optional fields, prepare() validates/defaults them, then getters are called) -- annotated the setter-populated fields @nullable and suppressed NullAway.Init at the class level for the prepare()-only fields (transaction, limitManager, streamingMode, begin, end), consistent with the pattern already used elsewhere in this rollout. getInnerContinuation() returns null if and only if its argument is null; fixed its declared return type to @nullable and used Objects.requireNonNull at the one call site (prepare(), which already null-checks the argument). Suppressions limited to the documented byte[]/@nullable array-tracking gap (lastKey field init, toBytes()/getInnerContinuation() return, and the continuation/lowBytes/highBytes Builder field initializers), each commented. No remaining NullAway errors in this file as of this commit.
- Added a private noContinuation() helper (byte[] + @nullable is not reliably tracked for null literals passed to @nullable byte[] params) and routed the ~8 "no continuation" call sites through it instead of a raw null literal. - remoteFetchFallbackFrom's continuation parameter was genuinely nullable (passed through from callers that accept @nullable byte[] continuation) but declared non-null; annotated it correctly. - BaseBuilder#getContext() is genuinely @nullable until setContext() is called; added a requireContext() default method (throws with a clear message) and routed uncheckedOpen()/create()/open()/createOrOpen() through it instead of dereferencing getContext() directly. - loadIndexEntryRecord's ERROR-orphan-behavior path unconditionally dereferenced the genuinely-@nullable getSubspaceProvider() twice when building the exception's log info; restructured to null-check once, matching the existing pattern already used a few lines above in this same file. No remaining NullAway errors in this file as of this commit.
- Added a loggable(byte[]) helper (ByteArrayUtil2.loggable() genuinely returns null only for a null byte[], but every call site here passes a never-null subspace.pack()/getKey()) and routed the ~27 addLogInfo(SUBSPACE/KEY, ...) call sites through it, since LoggableException#addLogInfo's value parameter does not accept null. - KeyValueUnsplitter#continuation was declared @nullable and left null until the first inner result, but RecordCursorResult#withoutNextValue requires a real continuation; switched to initializing it with RecordCursorStartContinuation.START (the documented sentinel for exactly this "haven't produced a continuation yet" case) instead of null, removing a latent NPE risk on an immediately-stopped cursor. - addLogInfo(KEY_TUPLE, nextKey) is called from several exception-construction sites where nextKey can genuinely be null (per an existing code comment); reused the file's own established "nextKey == null ? "null" : nextKey" fallback idiom (already used for the continuation field a few lines above) rather than force a non-null assumption that doesn't always hold. - Objects.requireNonNull at several genuinely-invariant-backed call sites (next/nextSubspace/nextKey/kv/innerNoNextReason), each commented with the invariant that makes the dereference safe (e.g. next and nextKey are always set/cleared together). - Suppressions limited to the documented byte[]/@nullable array-tracking gap (SingleKeyUnsplitter#result and KeyValueUnsplitter#nextPrefix field init and later re-derefs/reassignment), each commented. No remaining NullAway errors in this file as of this commit.
….java Real bugs fixed (not just annotation gaps): - deleteRecordsWhereCheckRecordTypes() returns null when allRecordTypes is empty (annotated it @nullable to reflect that), but its only caller unconditionally dereferenced the result a few lines later (evaluated.values().subList(...)), which would NPE with a confusing stack trace instead of a clear error. Added an explicit null check that throws a descriptive RecordCoreException instead. - FDBStoredRecordBuilder#setVersion(FDBRecordVersion) required non-null, but the field it sets, and getVersion(), are both @nullable (no version is a valid, common state per VersionstampSaveBehavior.NO_VERSION); widened the parameter to @nullable to match its own field/getter contract, fixing two call sites in FDBRecordStore that legitimately pass a null version. - handleOrphanEntry's return type was declared non-null but its SKIP branch returns null; the only caller's cursor pipeline already filters nulls out specifically for the SKIP case (RecordCursor.filter(Objects::nonNull)), confirming null is an intentional sentinel here -- annotated the return type @nullable to match. recordStoreStateRef/newStoreHeaderRef/oldStoreHeaderRef (AtomicReference fields) and several evaluated/indexEvaluated/context accesses required Objects.requireNonNull at points where an established invariant (an earlier null check earlier in the same method, an AtomicReference.updateAndGet that always populates a ref before it's read, etc.) guarantees non-null but doesn't survive across separate calls/lambdas -- each documented with the specific invariant that makes it safe. Suppressions limited to the documented byte[]/@nullable array-tracking gap: - Constructor-time field-init false positives (SingleKeyUnsplitter-style patterns don't appear here, but indexDeferredMaintenanceControl-style lazy-init fields do -- fixed with plain @nullable, no suppression needed). - A shared noBytes() helper for null-literal byte[] arguments into already-@Nullable-annotated APIs in other packages/modules that this scope can't touch (IndexingRangeSet#insertRangeAsync, RecordCursor#flatMapPipelined, KeyValueCursorBase.Builder#setContinuation). - ListCursor's continuation constructor param (fdb-record-layer's .cursors package, out of this scope) isn't annotated @nullable even though null is the correct "start fresh" value; suppressed at the one call site with an explanatory comment. No remaining NullAway errors in these files as of this commit.
…dContextConfig, FDBRecordVersion - FDBDatabaseRunner#asyncToSync is a @Nullable-generic interface method also overridden by SynchronizedSessionRunner (out of this scope), so it can't be narrowed to <T extends @nullable Object> the way FDBDatabase/FDBRecordContext's asyncToSync were; fixed at the two call sites in FDBDatabaseRunnerImpl instead, since handle()'s future always completes with a concrete true/false. - FDBExceptions: Throwable#getMessage() is genuinely @nullable, but the RecordCoreException-family constructors used require non-null; added a messageOrToString() helper (falls back to Throwable#toString(), which always includes the exception's class name) instead of forcing a null message through. - FDBRecordContextConfig.Builder#listener was assigned @nullable config.listener/never explicitly set, matching the (already correctly @nullable) outer class field and getTransactionListener() getter, but was declared non-null itself; annotated to match. - FDBRecordVersion#withCommittedVersion already null-checks committedVersion before use, but NullAway does not reliably narrow @nullable byte[] parameters through that check (a known array-type tracking gap); added Objects.requireNonNull at the point of use. No remaining NullAway errors in these files as of this commit.
- FDBRecordVersion#equals(Object) is a standard equals() override and must tolerate null per contract; annotated the parameter @nullable. This also fixes FDBStoredRecord#equals, which calls recordVersion.equals(that.recordVersion) with a genuinely-nullable argument, with no change needed there. - FDBRecordVersion#withCommittedVersion already null-checks committedVersion, but NullAway does not reliably narrow a @nullable byte[] parameter through that check even via Objects.requireNonNull (a known array-type tracking gap); suppressed at the method with a comment. - FDBReverseDirectoryCache#rebuild called fdb.asyncToSync(null, null, ...), passing null for the required Wait event and skipping timer instrumentation entirely; switched to context.asyncToSync with a real Wait event (WAIT_KEYSPACE_PATH_RESOLVE), consistent with how subspace resolution is instrumented elsewhere in this codebase. - FDBStoreBase#getSubspace's assignment to the @nullable subspace field was letting the assignment's target type influence generic inference of asyncToSync's type parameter, conflicting with getSubspaceAsync()'s own non-null return type; introduced an explicitly-typed intermediate local to break the ambiguity. - FDBSystemOperations' local asyncToSync(...) wrapper delegates to FDBDatabaseRunner#asyncToSync, which is blanket-@nullable (an interface method also implemented by SynchronizedSessionRunner, outside this scope, so it can't be narrowed the way FDBDatabase/FDBRecordContext's asyncToSync were); kept it blanket-@nullable to match, which is also what every caller here already expects. - FDBTypedRecordStore.Builder#uncheckedOpenAsync/createOrOpenAsync dereferenced the builder's @nullable typedSerializer field without the same "must be specified" validation that build() already has; extracted a shared requireTypedSerializer() helper and used it consistently across all three methods that construct an FDBTypedRecordStore, plus similarly for getUntypedSerializer()/getStoreStateCache() against the underlying untypedStoreBuilder's @Nullable-but-always-defaulted getters. - MinimumTupleSizeKeyChecker/SortedRecordSerializer/FDBRecordContextConfig: routine @nullable annotations and a shared loggable() helper for the byte[]-null-literal array-tracking gap, matching the pattern already used in SplitHelper.java. No remaining NullAway errors in files under this agent's scope as of this commit (verified via a full :fdb-record-layer-core:compileJava run).
Wrap Throwable.getMessage() and Map.get() dereferences with Objects.requireNonNull (genuinely @nullable on the JDK/Map contract, but guaranteed present here: the message is always set on the caught exception, and the stamp lookup follows a containsAll() check against the same key set).
Suppress NullAway (with comment) on tests/helper that pass a null byte[] continuation to assertSameResults()/RecordQueryPlan.execute() - known tooling limitation, even though the continuation parameter is already correctly annotated @nullable. Wrap RecordCursorResult.get() with Objects.requireNonNull (guaranteed non-null once hasNext() is true).
Suppress NullAway (with comment) on helpers/tests that pass a null byte[] continuation through to RangeCursor/RecursiveCursor.create() - known tooling limitation, even though the parameters are already correctly annotated @nullable. Convert the RecursiveValue::getValue method reference to a lambda wrapped in Objects.requireNonNull (the traversal never actually produces a null value in this test), and wrap AtomicReference.get() similarly (asList(ref) always populates it before returning).
Suppress NullAway (with comment) on the six tests that pass a null byte[] continuation to DedupCursor's constructor - known tooling limitation, even though the constructor's continuation parameter is already correctly annotated @nullable.
Wrap Map.get() unboxing with Objects.requireNonNull (test invariant: the field is always present in the query result). Suppress NullAway (with comment) on the two tests that pass a null byte[] continuation to queryRecordsWithHeader() (defined in a testFixtures base class outside this fix's scope, so its signature can't be changed here). Make the private Holder<T> helper's value field @nullable with a bounded type parameter, matching its use as a byte[] continuation holder that starts uninitialized and can become null again once the scan is exhausted.
…RegisterTest OnlineIndexerBuildValueIndexTest: type indexValue's Function result as @nullable Integer (the lambda genuinely returns null when the field is absent); wrap the three Map.get(value2) calls with Objects.requireNonNull (group() populates an entry for every key that appears among the same records the lookup key is drawn from). TaskEventRegisterTest: suppress NullAway (with comment) on the two tests that intentionally pass a null Transaction - per the class javadoc, these tests only verify that the composed register mechanically forwards whatever it is given, without touching the transaction itself.
Wrap recordStore.loadRecord() and planner.fromStoredType() dereferences with Objects.requireNonNull (both genuinely @nullable APIs, but guaranteed present here: the record was just saved earlier in the same test, and its stored type always corresponds to a synthetic record type in this test's schema).
…ppress instead Marking indexValue as Function<..., @nullable Integer> surfaced a new mismatch at the group() call site (declared in OnlineIndexerBuildIndexTest, outside this fix's scope), since its keyFunction parameter isn't annotated @nullable. Revert to the original non-null-declared type and suppress NullAway at the enclosing method instead, with a comment explaining why.
Wrap the createCycle().getCause() chain with Objects.requireNonNull. Throwable.getCause() is genuinely @nullable on the JDK, but createCycle() explicitly wires up a three-exception cycle via initCause()/constructor chaining, so each cause in the chain is guaranteed non-null.
…electionTest FDBSystemOperationsTest: bound run()'s generic type parameter with <T extends @nullable Object> so it can be reused with both nullable-returning (getPrimaryDatacenter, getClusterFilePath, getConnectionString) method references. FDBSortQueryIndexSelectionTest: suppress NullAway (with comment) on tests that pass a null byte[] continuation to executeQuery() - known tooling limitation. Wrap RecordCursorResult.get() with Objects.requireNonNull (guaranteed non-null once hasNext() is true).
…erTests, RecordValidateOnlyTest FDBSystemOperationsTest: suppress NullAway (with comment) at the three call sites where run() is used with a @Nullable-returning method reference - NullAway can't infer a @nullable T from method-reference target-type context even with run()'s bounded type parameter. MessageHelperTests: wrap PromoteValue.computePromotionsTrie() with Objects.requireNonNull (declared @nullable, but these helpers always set up types that genuinely require promotion). RecordValidateOnlyTest: wrap getExceptionCaught()/getCaughtException() with Objects.requireNonNull (test asserts an exception was caught, so it is always present).
…plicatorTest Both follow the standard JUnit test-fixture lifecycle already established for FDBRecordStoreTestBase: fields are left unset by the constructor and populated by a setup method (or, for the abstract ResolverMappingReplicatorTest, by a subclass's own @beforeeach) before any test method runs. Suppress NullAway.Init at the class level with a comment, matching that precedent. ResolverMappingReplicatorTest also suppresses NullAway on seedDirectoryLayer() for the byte[]-returning MetadataHook's null branch - known tooling limitation, even though the hook's own declared type already permits a null result.
Wrap Throwable.getMessage() dereferences with Objects.requireNonNull (genuinely @nullable on the JDK, but the message is always set on these caught exceptions).
Suppress NullAway (with comment) on testCoveringIndexFunction() for the intentional null continuation passed to scanIndexRecords() - known byte[] tooling limitation, even though the parameter is already correctly annotated @nullable.
- ArithmeticValueTest, JoinedRecordTypeTest: wrap Throwable.getMessage()/getCause() with Objects.requireNonNull (guaranteed present by the test's own setup/assertions). - KeySpacePathImportDataTest, SlidingWindowTestHelpers: suppress NullAway (with comment) for intentional null byte[] continuation arguments - known tooling limitation. - DebuggerWithSymbolTablesTest: suppress NullAway on tearDown()'s deliberate Debugger.setDebugger(null) cleanup - the main-source parameter isn't annotated @nullable and is out of this fix's scope. - MetaDataEvolutionValidatorTest: suppress NullAway for an intentional null passed to Index.setPrimaryKeyComponentPositions(), whose int[] parameter is already correctly annotated @nullable but hits the same array-tracking tooling limitation.
… MultidimensionalIndexTestBase TransactionalRunnerTest: adding @nullable to assertValue()/expectValues() parameters alone didn't satisfy NullAway at call sites - the byte[] tooling limitation strikes even when both sides are already correctly annotated. Suppress NullAway (with comment) on the affected tests and on Conflicter.expectValues() itself. MultidimensionalIndexTestBase: mark HypercubeScanParameters' minsInclusive/maxsInclusive array fields @nullable Long[] to match the already-@nullable Long... varargs values being written into them (unbounded interval endpoints are represented as null elements).
Suppress NullAway on assertValue() itself for the byte[] mismatch against JUnit's unannotated assertArrayEquals() - value is legitimately null when asserting a key has no value.
…ives in MapWhileCursorTest
…tore, SplitHelper fdb-extensions is now fully jspecify/NullAway-annotated, and its real @nullable contracts (ByteArrayUtil2, RankedSet, etc.) surface new NullAway errors in fdb-record-layer-core that AcknowledgeRestrictiveAnnotations previously masked. - LoggableTimeoutException.addLogInfo: widen the Object parameter to @nullable to match the LoggableKeysAndValues superclass contract (genuine Liskov violation; the implementation already handles null values safely). - TupleRange.equals: replace manual null-check-then-equals with Objects.equals, which NullAway/JSpecify model as accepting nullable arguments (Tuple's own equals(Object) does not get the same treatment). - TupleRange/SplitHelper: suppress NullAway on byte[]-heavy methods that call into ByteArrayUtil/ByteArrayUtil2 with a documented comment; this is the known, pervasive NullAway/JSpecify array-nullability tracking gap hit throughout this rollout. - FDBRecordStore: use Objects.requireNonNull for Index/FormerIndex getSubspaceTupleKey() call sites (never actually null given the classes' constructor invariants) and Objects.equals for the previousKey dedup check.
…Cache, ScanComparisons, TimeWindowLeaderboardIndexMaintainer Same two categories as the previous commit: the byte[]-array NullAway/JSpecify tracking gap on calls into RangeSet/ByteArrayUtil (suppressed with a documented comment), plus a couple of Tuple.equals(Object)/Tuple.from(Object) call sites where the argument is now correctly-but-conservatively flagged by NullAway even though Tuple genuinely tolerates a null element/argument (also suppressed, or switched to Objects.equals where that is the cleaner fix).
Covers FDBRecordStoreBase, RecordType, RecordCursorIterator, RecordQueryInJoinPlan, RecordQueryIndexPlan, KeyValueCursorBase, ResolverResult, KeySpaceDirectory, FDBDatabaseFactoryImpl, FDBMetaDataStore, MultidimensionalIndexScanBounds, ResolverValidator, and TextIndexMaintainer, plus a correction to the SplitHelper fix from the first commit (the real culprit at that line was the byte[] array-tracking gap on packedVersion, not getVersionstamp()'s return). - RecordCursorIterator.next(): genuine override mismatch against com.apple.foundationdb.async.AsyncIterator (external, unannotated fdb-java interface); NullAway's conservative default of treating its unannotated next() as @nonnull for override-checking is not a real declared contract from that library, and RecordCursorResult documents a genuinely-nullable next value, so the wider override is intentional (suppressed with comment). - Everywhere else: either the byte[] array-tracking gap on calls into ByteArrayUtil/ByteArrayUtil2/BunchedMap (suppressed), or Tuple.from/addObject being fed a value that is genuinely allowed to be null by design (also suppressed), or a value that's non-null by class invariant but declared defensively nullable (Objects.requireNonNull, e.g. RecordType's getRecordTypeKeyTuple).
Same ripple, now surfacing in src/test and src/testFixtures: FDBTestEnvironment (fdb-test-utils) genuinely returns a @nullable cluster file, and a large batch of test data deliberately exercises Tuple's support for null elements (KeyType.NULL directories, nullable IN-list/outer-join values, etc.), which NullAway's conservative treatment of Tuple.from's unannotated varargs misflags as requiring non-null. - FDBDatabaseExtension.defaultClusterFile: widen to @nullable to match FDBTestEnvironment.randomClusterFile()'s real, already-nullable contract (getDatabase(@nullable String) already handles a null cluster file). - FDBRecordStoreClearIndexDataTest.sentinelKey: Objects.requireNonNull on index.getSubspaceTupleKey(), mirroring the FDBRecordStore.java fix. - VersionIndexTest: Objects.requireNonNull on tupleBytesPair.getRight(), mirroring the existing requireNonNull on getLeft() two lines above. - DataInKeySpacePathUtilTest: Objects.equals instead of Tuple.equals(Object). - Everywhere else: @SuppressWarnings("NullAway") with a comment, for either the Tuple-accepts-null-by-design pattern or the standard "Transaction -> Void lambda returning null" idiom used throughout the FDB test suite.
…lAway fix PMD's UnusedNullCheckInEquals flags "lowGroup != null && Objects.equals(lowGroup, highGroup)" as redundant (lowGroup is already known non-null), but switching to lowGroup.equals(highGroup) directly reintroduces the NullAway complaint fixed in an earlier commit, since Tuple.equals(Object)'s argument is conservatively treated as requiring non-null while highGroup may genuinely be null. Suppress the PMD rule here, matching the existing precedent in TupleRange.isEquals().
spotbugsTestFixtures had never been run before, so these 6 genuine null-dereference-on-some-path findings went unnoticed. Each one calls FDBRecordContext#getTimer() (correctly @nullable) once to null-check it and then calls getTimer() again to dereference the result; SpotBugs (like NullAway) can't relate the two separate calls, so the second one looks like an unchecked dereference of a @nullable value. Capture getTimer() in a local variable once and null-check/dereference that same local in TestHelpers#assertDiscardedAtMost/assertLoadRecord/ assertDiscardedAtLeast/assertDiscardedExactly/assertDiscardedNone and in FDBRecordStoreQueryTestBase#clearStoreCounter.
…n resolve jspecify annotation classes from its migrated dependencies
arnaud-lacurie
force-pushed
the
apple/arnaud-lacurie/jspecify-nullaway/core
branch
from
September 8, 2026 00:07
abedaa4 to
425df13
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.
8th of a 14-PR stack adopting jspecify + NullAway null-checking, stacked on #4580 (
fdb-record-layer-debugger). Same treatment applied tofdb-record-layer-core, the largest module in this rollout — split into 7 independently-worked package chunks then merged, followed by an integration pass to resolve cross-chunk interactions and a cross-module ripple pass againstfdb-extensions's now-finalized contracts. See inline comments for specific findings.