Adopt jspecify + NullAway null-checking on fdb-relational-core - #4585
Draft
arnaud-lacurie wants to merge 42 commits into
Draft
Conversation
arnaud-lacurie
changed the title
arnaud-lacurie
commented
Sep 7, 2026
| } | ||
| return Optional.of(literalReverseLookup.get(value)); | ||
| // Note: this must be ofNullable, not of() -- a missing key legitimately means "no duplicate found". | ||
| return Optional.ofNullable(literalReverseLookup.get(value)); |
Collaborator
Author
There was a problem hiding this comment.
getFirstValueDuplicateMaybe() wrapped literalReverseLookup.get(value) in Optional.of(...); since literalReverseLookup legitimately has no entry for values that aren't duplicates, a cache miss would NPE inside Optional.of instead of correctly reporting "no duplicate found". Changed to Optional.ofNullable(...). The identical fix is applied to getFirstDuplicateOfConstantIdMaybe() just below (line 201), which had the same bug.
| // enforces that with a clear RelationalException, but NullAway can't see through it since Assert lives | ||
| // in the not-yet-migrated fdb-relational-api module. | ||
| @SuppressWarnings("NullAway") | ||
| private static Type.Record maximumRecordTypeOrFail(final Type.Record left, final Type.Record right) { |
Collaborator
Author
There was a problem hiding this comment.
validateUnionTypes() had two branches computing the promoted union type: the requiresPromotion branch cast Type.maximumType(...)'s result directly with no null check, while the other branch already guarded it with Assert.notNullUnchecked (maximumType can return null for genuinely incompatible types). The unguarded branch would NPE on an invalid UNION instead of raising the intended UNION_INCOMPATIBLE_COLUMNS error. Both branches now go through this shared maximumRecordTypeOrFail() helper, so they fail consistently and cleanly.
| } | ||
| } | ||
| } else { | ||
| } else if (array != null) { |
Collaborator
Author
There was a problem hiding this comment.
In the wrapped-ARRAY branch of toDynamicMessage(), array (from struct.getArray(i + 1)) was used unconditionally once the fd.isRepeated() branch was ruled out, even though a NULL array-valued column makes getArray(...) return null. That would NPE on array.getMetaData() a few lines down. Now it's guarded with else if (array != null), consistent with how the other nullable field cases (bytes, uuid, struct) in this same method skip setting the field when the value is null.
|
|
||
| @Override | ||
| @Nullable | ||
| public RelationalArray getArray(int columnIndex) throws SQLException { |
Collaborator
Author
There was a problem hiding this comment.
getArray(int)/getArray(String) (and similarly getUUID and getStruct below) simply return delegate.getXxx(...), but the delegate's real contract (RelationalStruct) declares these @nullable -- a NULL column is a legitimate result, not just an error-path corner case. Declaring the override without @nullable would silently violate that contract under the new null-checking. These are now annotated @nullable to match the delegate.
| @Override | ||
| public RelationalResultSet getSchemas() throws SQLException { | ||
| return getSchemas(conn.getPath().getPath(), null); | ||
| return getSchemas(Objects.requireNonNull(conn.getPath()).getPath(), null); |
Collaborator
Author
There was a problem hiding this comment.
getSchemas() called conn.getPath().getPath() directly, but RelationalConnection#getPath() is genuinely nullable (a connection isn't always associated with a resolved path). Now wrapped in Objects.requireNonNull(...), turning a potential silent NPE deep in the metadata call into an explicit, attributable failure at the point of use.
| final var dbAndSchema = SemanticAnalyzer.parseSchemaIdentifier(schemaId); | ||
| Assert.thatUnchecked(dbAndSchema.getLeft().isPresent(), ErrorCode.UNKNOWN_DATABASE, () -> String.format(Locale.ROOT, "invalid database identifier in '%s'", ctx.uid().getText())); | ||
| return ProceduralPlan.of(metadataOperationsFactory.getDropSchemaConstantAction(dbAndSchema.getLeft().get(), dbAndSchema.getRight(), Options.NONE)); | ||
| final Optional<URI> databaseUri = Objects.requireNonNull(dbAndSchema.getLeft()); |
Collaborator
Author
There was a problem hiding this comment.
visitDropSchemaStatement() called dbAndSchema.getLeft() twice: once inside Assert.thatUnchecked(...isPresent()...) and again via .get() on the next line. If getLeft() weren't a plain immutable field accessor (or is ever changed to compute something), the value seen by isPresent() and the value used by get() could diverge, defeating the null-check entirely. Caching it in a local databaseUri once removes that risk. The same pattern (and fix) appears above in visitCreateSchemaStatement (line ~555).
| final var fieldAliases = new ArrayList<>(trie.getChildrenMap().keySet()); | ||
| // A record's trie node always has a populated children map -- one entry per field -- which is | ||
| // exactly what we index into below. | ||
| final var nonNullChildrenMap = Objects.requireNonNull(childrenMap, "record type's field-access trie node has no children map"); |
Collaborator
Author
There was a problem hiding this comment.
setFieldNamesInternal() called trie.getChildrenMap() three separate times: once for the null/isEmpty check near the top of the method, then again for .keySet() and .get(fieldAlias) further down -- with no guarantee the same value is observed each time. The fix captures it once in a local (childrenMap / nonNullChildrenMap) and reuses that, closing the gap between the null-check and the actual use.
Adds NullAway + jspecify wiring in fdb-relational-core.gradle
(AnnotatedPackages = com.apple.foundationdb.relational.{api,recordlayer,
transactionbound,util}), fills two @NullMarked gaps left by the mechanical
phase (api/exceptions, api/options package-info), and works through the
resulting compileJava errors file by file.
Note: fdb-relational-core.gradle currently has a TEMP-DIAGNOSTIC -Xmaxerrs
bump to see the full error list past javac's 100-error cap; this must be
removed before this branch is considered done.
This is a WIP checkpoint mid compile-fix loop -- compileJava is not yet
clean. Not all files are finished (RecordLayerIterator, MessageTuple,
LogicalOperator, functions/*, RecordLayerStoreCatalog fixed; QueryPlan,
CopyPlan, PlanGenerator, MutablePlanGenerationContext, Expression and a
few others still outstanding).
Both use Assert.notNullUnchecked/Assert.thatUnchecked in ways NullAway can't see through (Assert lives in the not-yet-migrated fdb-relational-api module); one spot in LogicalOperator switched to Objects.requireNonNull directly since it doesn't need Assert's custom exception type.
…Context ctor RecordLayerIterator.java: 'RecordCursor.NoNextReason noNextReason' needs @nullable placed as 'RecordCursor.@nullable NoNextReason' (type-use annotation on the simple name), not as a prefix on the whole qualified type -- the latter is a genuine javac error ("scoping construct cannot be annotated with type-use annotation"), not just a NullAway finding. MutablePlanGenerationContext.java: suppress the constructor's continuation=null assignment; NullAway/JSpecify doesn't reliably track @nullable on array-typed (byte[]) fields even though the field is already correctly declared @nullable.
…MessageTuple/RecordLayerIterator - Plan.java: widen T to <T extends @nullable Object> (ProceduralPlan instantiates it with Void, whose only value is null). - ContinuationImpl.parseContinuation: add missing @nullable on the bytes param; it already handled null internally (returns BEGIN). - MessageTuple.getObject: revert to non-@nullable signature (overriding Row#getObject(int), which NullAway treats as implicitly @nonnull) and suppress at the method instead, since it genuinely returns null for a SQL NULL/absent field. - RecordLayerIterator.next(): use Objects.requireNonNull instead of a bare @SuppressWarnings for the fetched value, since suppressing an assignment alone doesn't stop the nullness from resurfacing at the next use (lesson learned the hard way across several files). - QueryPlan.java / CopyPlan.java: several Assert.notNull(Unchecked) cross-module-narrowing fixes, byte[]/vararg array-nullability suppressions (ArrayRow, CopyPlan's continuation field/ctor), and a requireNonNull for the schema-must-be-selected invariant.
- CopyPlan.convertDataToRow: drop the dead null-check/param -- the sole caller (via RecordLayerIterator, whose next() now guarantees a non-null fetched value) never actually passes null, matching RecordLayerStoreCatalog#transformDatabaseInfo's already-non-null style. - ProceduralPlan.executeInternal: explicitly @nullable Void return type, since Void's only value is null. - MutablePlanGenerationContext: two array/struct prepared-statement-param methods reassign their resolved type through Objects.requireNonNull right after the try block (real invariant: resolved either from the passed-in type or from introspecting the JDBC value, in both cases non-null by the time it's used). - PlanGenerator/QueryPlan: more getMessage()-requireNonNullElse fixes, Assert.notNullUnchecked cross-module suppressions, and ArrayRow vararg-element suppressions (properly wrapping the whole ArrayRow construction this time, not just an intermediate variable -- suppressing a declaration does not stop the value from being re-flagged wherever it is used next).
… code CopyPlan.convertDataToRow: forgot to drop the method's own @nullable return annotation when the dead null-check/param was removed earlier. ProceduralPlan.executeInternal: revert to plain (unannotated) Void return with a method-level suppression -- NullAway still treats the plain type variable T from Plan#executeInternal as @nonnull at this override despite Plan's generic bound now allowing @nullable Object, so @nullable Void here is an override-compatibility violation, not a fix.
In-progress fixes across BasicMetadataTest, DelegatingVisitorTest, ProtobufDataBuilderTest, SchemaTemplateSerDeTests, RecordLayerStoreCatalogTestBase, BackingLocatableResolverStoreTest, OfflinePlanGenerationTest, PlanGenerationStackTest, SqlVisitorTests, ConstraintValidityTests. Committing now to avoid losing progress; some of these files/batches are not yet fully clean.
- BasicMetadataTest: suppress at JDBC-null-arg call sites (getSchemas/ getTables/getColumns accept null filter params per JDBC javadoc, but the unmigrated fdb-relational-api interface isn't annotated to say so) - DelegatingVisitorTest: suppress on anonymous BaseVisitor overrides that intentionally return null (only the invocation is being asserted, not the return value)
- UpdateTest: switch Pair<Continuation, Integer> to NonnullPair since values are never actually null; suppress the two intentional updateValue.apply(null) calls where the lambda ignores its argument. - StandardQueryTests: fix a latent bug where a caught exception's message was checked with .contains(...) but the result was never asserted (partiqlAccessingNestedFieldWithInnerRepeatedFieldsFails); use Objects.requireNonNullElse for other getMessage() dereferences; switch Review.endorsements to NonnullPair since it's always constructed with literal non-null values.
…Test, ExpressionTests
…ValueTupleTest, UniqueIndexTests - KeySpacePathParsingTest: add @nullable to createDirectory()/ambiguousHalf() helper params (both already flow into constructors/methods that accept null); suppress at cross-module Pair.getLeft()/Map.get()/asyncToSync() call sites that are provably non-null in context but not annotated as such - CaseSensitivityTest: suppress at JDBC-null-arg and ArrayRow varargs call sites (unmigrated fdb-relational-api types) - ValueTupleTest: suppress testEquals() which deliberately calls equals(null) to test the equals() contract - UniqueIndexTests: fix real nullness gap -- Throwable.getMessage() can return null; use Objects.requireNonNullElse(..., e.toString()) before calling .contains()
In-progress fixes across DdlTestUtil, AbstractRecordLayerResultSetTest, CopyCommandTest, KeyBuilderTest, RecordLayerStoreCatalogWithNoTemplateOperationsTest, StoreTimerMetricCollectorFromFDBRecordContextTest. Committing now to avoid losing progress; some of these files/batches are not yet fully clean.
- Wrap ConnectionUtils.getFromCatalog()/EmbeddedRelationalConnection#getMetricCollector()/ EmbeddedRelationalExtension#getDriver() results with Objects.requireNonNull() at call sites, since those methods are declared @nullable but are known non-null in these test contexts; the declaring classes are out of scope to annotate differently here. - Same treatment for ContinuationImpl#getPlanHash() unboxing.
- StoreTimerMetricCollectorFromFDBRecordContextTest: use Objects.requireNonNull for getMetricCollector()/continuation, both known non-null by test invariant. - DdlTestUtil: extract Assert.notNullUnchecked result to a suppressed local (Assert lives in the not-yet-migrated fdb-relational-api module); use Objects.requireNonNull for getSchema(); replace dead "fail() then return null" with "return fail()" in ParsedSchema.getType/getTable (assertj's fail() returns <T> T, so no unreachable null return is needed). - ConcurrentCacheTests/MultiStageCacheTests: suppress at call sites where pickFirst's @nullable return is passed to AbstractCache.reduce()'s unannotated Function<Stream<V>, V> parameter (main code, out of scope); removed dead/broken unused produceAnimal/produceLandform/produceCapital helpers in MultiStageCacheTests (they indexed entries by the wrong key level and were never called); extracted nested fixture-map lookups in readCache() to a small helper using Objects.requireNonNull.
…nalArrayTest, RecordTypeTableSerDeTest - StructDataMetadataTest: require non-null continuation before serialize() (all call sites pass numBaseQueryRuns >= 1 alongside numContinuationRuns, so it's always populated by then). - RowStructTest: suppress at the ArrayRow(null, 1L) call site -- the null vararg element is intentional, representing a SQL NULL for the wasNullWorks test; ArrayRow is out of scope. - RelationalArrayTest: mark the nullArrayElements helper parameter @nullable since callers intentionally pass null to mean "expect a null array". - RecordTypeTableSerDeTest: require non-null on fieldData.get("A") -- "A" is the primary key column and always present and non-null across every call site.
…llectorFromMetricRegistryTest
…ational util.Assert)
…edRelationalExtension, ContinuationTest - StatementBuilderTests: use Objects.requireNonNull(whereClause) after the existing assertThat(whereClause).isNotNull() check (AssertJ's isNotNull() isn't understood by NullAway as narrowing). - LogAppenderRule/EmbeddedRelationalExtension: suppress NullAway.Init on constructors whose fields are set by beforeEach()/setup() (JUnit extension lifecycle, not the constructor); suppress the manual beforeEach(null)/ afterEach(null) invocations (JUnit's ExtensionContext parameter isn't annotated @nullable, but these implementations ignore the context). - ContinuationTest: widen assertContinuation's underlying parameter to @nullable Object (it's called with null in two tests); suppress the fromUnderlyingBytes(null) call site (its byte[] parameter is already @nullable but NullAway doesn't reliably track array nullability).
Replace dead "fail() then use null local" with "keys = fail(...)" (JUnit's fail() returns <V> V, so no unreachable null assignment is needed); suppress the two Assert.notNullUnchecked(recordLayerIndex.getPredicate()) call sites (Assert lives in the not-yet-migrated fdb-relational-api module).
…ySpacePathParsingTest
- ProceduralPlan.java: remove now-unused jspecify Nullable import (PMD UnnecessaryImport / Checkstyle UnusedImportsCheck). - ExpressionVisitor.java: drop a dead "= null" initializer whose value is always overwritten before use in every branch (PMD UnusedAssignment). - WindowSpecExpression.java / WithPlanGenerationSideEffects.java / CaseInsensitiveCharStream.java / NoOpMetricCollector.java: restore the blank line between the last import and the following class/interface Javadoc, lost during the mechanical jspecify-import swap (Checkstyle EmptyLineSeparatorCheck). - RecordMetadataSerializer.java / QueryPlan.java: SpotBugs NP_NULL_ON_SOME_PATH_FROM_RETURN_VALUE -- both called a @nullable getter (getPredicate()/getCompiledStatement()) twice in a null-check ternary and used the second, unchecked call in the non-null branch; hoist to a single local variable instead.
setContinuation(continuationProto) -- both sides already correctly declared @nullable byte[] -- still tripped NullAway's known array-type matching limitation. Extracted into a small suppressed private helper (setContinuationOnContext) instead of a broad method-level suppression, since the call is a bare void-method statement that can't otherwise carry a @SuppressWarnings.
…rIterator PMD 6.44.0 cannot parse 'RecordCursor.@nullable NoNextReason' (a type-use annotation on a qualified nested type) and throws a ParseException for the whole file instead of just skipping the annotation. Unlike RecordLayerIndex.Builder (which collides with ImmutableMap.Builder's simple name and must keep the qualified form), NoNextReason has no simple-name collision here, so import it directly and use the simple name everywhere in the file (including the now-plain '@nullable NoNextReason' field and every other RecordCursor.NoNextReason reference), matching the documented workaround.
fdb-relational-api's finalized contracts (RelationalStruct/RelationalArray/ RelationalResultSet methods that legitimately return @nullable, and RelationalDriver#connect()) are now visible to fdb-relational-core's NullAway pass. - ErrorCapturingResultSet: widen getArray/getUUID/getStruct to @nullable to match the real delegate contract in RelationalStruct. - RecordTypeTable#toDynamicMessage: fix a real latent NPE risk, a NULL array-valued column reaching the non-repeated/wrapped ARRAY branch would previously dereference a null array; now skipped consistently with the other nullable-field cases. - RelationalKeyspaceProvider#getSchemaName: use Objects.requireNonNull with a message, KeySpacePath#getValue() is @nullable in general but is guaranteed non-null once the directory name is confirmed to be SCHEMA_DIR. - QueryPlan, QueryExecutor, RecordLayerStoreSchemaTemplateCatalog, RecordLayerStoreCatalog, BackingRecordStore, BackingLocatableResolverStore, CopyPlan: suppress with explanation, byte[] continuation values are genuinely @nullable on both sides of these calls, but NullAway/JSpecify does not reliably track @nullable across array-typed parameters (known tooling limitation).
…ct() nullability) RelationalDriver#connect() legitimately returns @nullable RelationalConnection (a driver may decline a URL it doesn't handle), matching java.sql.Driver's contract. Test code assumed a non-null result and dereferenced it immediately; wrap with Objects.requireNonNull() (a null connection here is a real test setup failure that should fail fast with a clear message) in: EmbeddedRelationalExtension, QueryPropertiesTest, OptionScopeTest, QueryLoggingTest, CursorTest, ExecutePropertyTests, UpdateTest. Also: - RelationalConnectionRule: same fix for its connection field assignment, plus widen getPath() to @nullable to match RelationalConnection#getPath()'s real contract. - EmbeddedRelationalExtension: widen clusterFile field/constructor/makeDatabase param to @nullable, matching FDBTestEnvironment#randomClusterFile() and FDBDatabaseFactory#getDatabase(String)'s real contracts (null means "use the default cluster file"). - DdlRecordLayerSchemaTemplateTest: Objects.requireNonNull around a genuinely-nullable RelationalArray column value before dereferencing it.
- ExplainTests, ExpressionTests: Debugger#setDebugger(Debugger) is documented to accept null (removes the current debugger) but its parameter isn't annotated @nullable in fdb-record-layer-core; that upstream annotation gap is outside this fix's scope, so isolate the necessary suppression in a small wrapper (ExplainTests) / directly on the one call site (ExpressionTests) rather than at every call. - CaseSensitivityQueryTests: Objects.requireNonNull around a genuinely-nullable RelationalStruct column value before dereferencing it. - V2PlanGeneratorTests: same pattern, and de-duplicate repeated getStruct("i")/ getStruct("loc") calls into local variables.
StandardQueryTests and PreparedStatementTests dereference RelationalResultSet#getArray()/ getStruct() results (both genuinely @nullable per RelationalStruct's real contract) without a null check. Wrap with Objects.requireNonNull() at each call site; these are test assertions where a null result would indicate the test itself is broken, so failing fast with a clear message is correct.
…ctDataMetadataTest) RelationalResultSet#getStruct()/getArray() are genuinely @nullable per RelationalStruct's real contract; wrap chained/dereferenced calls with Objects.requireNonNull(), or add the matching Assertions.assertNotNull() JUnit already uses elsewhere in this file (NullAway recognizes it as a narrowing assertion) where a reassignment lacked one. This completes :fdb-relational-core:compileTestJava, which is now clean, in addition to :fdb-relational-core:compileJava fixed in earlier commits on this branch.
Apply the established @SuppressWarnings("NullAway.Init") convention to benchmark state fields that are populated by JMH's @Setup/@PARAM lifecycle (or an equivalent up()/init() method) rather than by the constructor: EmbeddedRelationalBenchmark.Driver, BenchmarkConnHolder, SimplePlanCachingBenchmark, RelationalScanBenchmark (and its nested RelationalConnHolder), IndexScanVsQueryBenchmark, and SetSchemaBenchmark.RelationalConnHolder. Mark EmbeddedRelationalBenchmark.Driver#planCache and SimplePlanCachingBenchmark#getPlanCache() as @nullable to reflect their real, already-supported contract: a null plan cache is a meaningful value ("no cache configured") that RecordLayerEngine already accepts as @nullable. Suppress the RecordLayerScanBenchmark#scan() NullAway false positive on the null continuation passed to scanRecords(), using the same "byte[] nullability isn't tracked reliably" rationale and pattern already established in IndexingMultiTargetByRecords.
…b-relational-core Widened @nullable contracts elsewhere in the rollout (EmbeddedRelationalConnection, NonnullPair-returning helpers, CompatibleTypeEvolutionPredicate.FieldAccessTrieNode) let SpotBugs's interprocedural analysis see real null-dereference paths that were previously masked by now-redundant @nonnull annotations. Fix each with an explicit Objects.requireNonNull() or by capturing a nullable getter's result in a single local variable instead of calling it twice (once to null-check, once to dereference). Also remove the now-useless @SpotBugsSuppressWarnings("NP_METHOD_PARAMETER_TIGHTENS_ANNOTATION") on EmbeddedRelationalStruct$Builder: its justification (fdb-relational-core still using javax.annotation.Nullable while fdb-relational-api used jspecify's) no longer applies now that this class uses org.jspecify.annotations.Nullable too.
arnaud-lacurie
force-pushed
the
apple/arnaud-lacurie/jspecify-nullaway/relational-core
branch
from
September 8, 2026 00:07
f9cdbdb to
0526e8c
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.
12th of a 14-PR stack adopting jspecify + NullAway null-checking, stacked on #4584 (
fdb-relational-api). Same treatment applied tofdb-relational-core, split into 3 independently-worked package chunks then merged, followed by a cross-module ripple pass againstfdb-relational-api's now-finalized contracts. See inline comments for specific findings.