[CALCITE-7736] Replace the Checker Framework with NullAway and JSpecify - #5213
Draft
vlsi wants to merge 43 commits into
Draft
[CALCITE-7736] Replace the Checker Framework with NullAway and JSpecify#5213vlsi wants to merge 43 commits into
vlsi wants to merge 43 commits into
Conversation
The Checker Framework verified nullness through a Gradle plugin of its own, a set of `.astub` files that patched the nullness of the JDK and of third-party libraries, and two dedicated CI jobs. NullAway runs as an Error Prone check instead, so it needs no separate plugin and no stub files: it ships nullness models for the JDK and for popular libraries, and JSpecify supplies the annotations. Build: * drop the `org.checkerframework` plugin and its configuration block, and delete the 48 `.astub` files * add `com.uber.nullaway:nullaway` to the `errorprone` configuration and configure it for JSpecify, including the experimental generics support (`JSpecifyExperimental`, `HandleWildcardGenerics`, `JSpecifyJDKModels`, `WarnOnGenericInferenceFailure`) * replace `org.checkerframework:checker-qual` with `org.jspecify:jspecify` * raise Error Prone to 2.50.0 and the Error Prone plugin to 5.1.0, which NullAway requires NullAway is an error in the projects listed in `nullawayProjects` and is off elsewhere, so a nullness problem fails one CI job rather than every test job. CI: * drop the two `CheckerFramework` jobs * fold nullness verification into the `errorprone` job, which moves to JDK 21 because Error Prone 2.43 and later require it This commit only moves the tooling; the source still carries Checker Framework annotations and is migrated by the commits that follow.
… to JSpecify A purely mechanical rename of `org.checkerframework.checker.nullness.qual.Nullable` to `org.jspecify.annotations.Nullable`, and of the matching `NonNull`. Both annotations are `TYPE_USE`, so every existing position stays valid and no annotation moves. Autostyle rewrites either annotation to its JSpecify counterpart from now on, alongside the rule that already rewrote the jsr305 ones. The Checker Framework annotations that JSpecify does not define -- @polynull, @MonotonicNonNull, @RequiresNonNull and the rest -- are still here and are migrated by the next commit.
…fies
`@DefaultQualifier(NonNull, {FIELD, PARAMETER, RETURN})` is how the Checker
Framework said "types are non-null unless annotated". JSpecify says it with
`@NullMarked`, which covers every type position rather than three of them.
Only `calcite-linq4j` and `calcite-core` are verified, so only their main
packages get the annotation: `@NullMarked` claims that a package is fully
annotated, and in an unverified module nothing backs that claim. The 23
`package-info.java` files that carried `@DefaultQualifier` are converted, and
the remaining packages of those two modules get the annotation.
`babel`'s test `package-info.java` also carried `@DefaultQualifier`. It loses
the annotation rather than gaining `@NullMarked`, because `checker-qual` is
gone and test code is not verified.
NullAway skips a package that is not `@NullMarked`, so a missing annotation
costs coverage without saying a word. `LintTest.testLintNullMarked` walks the
source roots in `NULL_MARKED_ROOTS` and fails when a package there has no
`package-info.java`, or has one that does not declare `@NullMarked`. Widen that
list together with `nullawayProjects`.
…y does not define JSpecify defines @nullable, @nonnull and @NullMarked, and nothing else. The remaining Checker Framework annotations either move to a Calcite-owned equivalent or go away. Adds `org.apache.calcite.linq4j.annotations` with @contract, @MonotonicNonNull, @RequiresNonNull, @EnsuresNonNull and @EnsuresNonNullIf. NullAway matches these by the last component of their name rather than by their package, so Calcite declares its own and takes no dependency on the checker: * `ContractUtils.hasSimpleNameContract` compares the simple name * `Nullness.isMonotonicNonNullAnnotation` tests `endsWith(".MonotonicNonNull")` * the field-contract handlers pass `exactMatch=false` to `NullabilityUtil.findAnnotation`, which also compares the suffix Moved to that package: @MonotonicNonNull (32), @RequiresNonNull (21), @EnsuresNonNull (23) and @EnsuresNonNullIf (14). The two @EnsuresNonNull that named a parameter rather than a field are dropped, along with the four @EnsuresNonNullIf whose expression was a method call: NullAway supports fields only. @polynull (300) becomes @nullable. It says the result is null exactly when the argument is null, which @nullable weakens to "may be null"; the next commit restores the other half with @contract. Dropped, having no equivalent and no NullAway counterpart: @pure (81), the initialization annotations (80), @KeyFor and @UnknownKeyFor (18), @covariant (10), @HasQualifierParameter and @minlen. NullAway checks initialization on its own, so the receiver parameters that existed only to carry @UnderInitialization are removed with them. `<@nullable R>` becomes `<R extends @nullable Object>`. The Checker Framework reads an annotation on a type parameter declaration as a bound on the lower bound, so `<@nullable R>` there means R *must* be nullable; JSpecify tracks upper bounds only and can say no more than "may be". 215 @SuppressWarnings that named Checker Framework message keys such as `argument.type.incompatible` become "NullAway". `Nullness.castNonNull` keeps working as NullAway's `CastToNonNullMethod`; it loses @pure and its parameter-naming @EnsuresNonNull, and its blanket suppression narrows to the one method that needs it.
@polynull said the result is null exactly when the argument is null. The previous commit weakened it to @nullable, which makes every caller treat the result as nullable even when it passed a non-null argument. @contract states the direction that matters at a call site: @contract("!null, _ -> !null") public static @nullable Integer plus(@nullable Integer b0, int b1) NullAway is configured with `CheckContracts=true`, so it verifies each clause against the method body rather than trusting it, and none of the 108 clauses here is rejected. Not every @polynull converts. A clause describes arguments, so it cannot describe a receiver parameter, and it cannot describe a varargs method, whose call sites pass a different number of arguments. Nor does it reach a type argument such as `Enumerable<@nullable T>`, where the element rather than the result is polymorphic. Those keep the plain @nullable. Passing a clause whose length does not match the call crashes NullAway with an IndexOutOfBoundsException from `ContractHandler.onDataflowVisitMethodInvocation`, which reads arguments by the antecedent's length without checking the arity it was given. It validates that on declarations but not at call sites; worth reporting upstream.
…ramework inferred The two tools default an unwritten type parameter bound in opposite directions. The Checker Framework's CLIMB-to-top rule gives implicit bounds the top qualifier, so `<T>` there means `<T extends @nullable Object>`. JSpecify fills in `Object`, which under `@NullMarked` is non-null, and its user guide says as much: "`<E>` means `<E extends Object>` and that means it is not `@Nullable`". So every unbounded type parameter silently changed meaning. Calcite relied on the Checker Framework reading, for instance here: public interface SqlVisitor<R> { ... } public class SqlShuttle extends SqlBasicVisitor<@nullable SqlNode> { ... } public abstract <R> R accept(SqlVisitor<R> visitor); `SqlShuttle` is a `SqlVisitor<@nullable SqlNode>` and was passed to `accept` with no suppression, which only typechecks if R admits a nullable argument. Writes the bound out at 61 declarations: the Rex and Sql visitors and the 23 `accept` overrides, `Pair` and its factories, `PairList`, `ConsList`, `FlatLists`, `Holder`, `ImmutableNullableList`, `TryThreadLocal`, `Util.transform`, and the linq4j types `Enumerable`, `Enumerator`, `Queryable`, `Function0`, `Function1`, `Function2` and `Ord`. The erasure is unchanged, so this is binary compatible. This accounts for most of what NullAway reported: 1126 errors down to 576 in `calcite-core`. `Pair.of` alone was worth 132 -- its class already had the bounds, but a static factory declares type parameters of its own.
`Visitor<R>` computes a value by walking a node tree, and there is nothing to compute for an empty subtree: `Expressions.acceptNodes` returns the result of the last node, or null when the list is empty, and `VisitorImpl` returns a bare `null` for a `FunctionExpression` with no body and for a `GotoStatement` with no expression. `Visitor.visit` and `Node.accept(Visitor)` now say so, and the type variable takes the nullable bound the Checker Framework used to infer for it. Under the Checker Framework this was `VisitorImpl<@nullable R>`, which forced R to be nullable; JSpecify tracks upper bounds only, so the annotation moves to the result. `UseCounter` extended `VisitorImpl<Void>`, a type whose only value is null. It now extends `VisitorImpl<@nullable Void>`, like `MayThrowVisitor`. Nothing outside `org.apache.calcite.linq4j.tree` implements `Visitor` or calls the `accept` overload that takes one.
The seedless `aggregate` starts from the first element and returns null when there is none. The `min` and `max` overloads that call it returned a non-null type anyway, so an empty sequence produced a null the signature ruled out. Eight of them now say `@Nullable`; the other overloads either seed the accumulator or already wrap the call in `requireNonNull`, and are unchanged. Four `min` and `max` overloads were already annotated, so this makes the family consistent. `aggregate` declares its accumulator `Function2<@nullable TSource, TSource, TSource>`, and the reducers it is called with genuinely handle a null accumulator -- every `MIN` and `MAX` constant in `Extensions` opens with `v1 == null`. Their declarations now match; the `SUM` constants do not test for null and keep the non-null accumulator.
`aggregate(source, seed, func)` is the reduce that takes a starting value, and `min` and `max` start it at null. Its accumulator type variable was non-null, so the seed, the reducer and the result all disagreed with the call. `TAccumulate` and `TResult` take the nullable bound the Checker Framework used to infer for them. The reducer parameter becomes `Function2<TAccumulate, TSource, ? extends TAccumulate>`: the constants in `Extensions` accept a null accumulator but never produce one, and the wildcard is what lets a reducer with a non-null result feed a nullable seed. Three more `min` overloads return null for an empty sequence and now say so, and `long min(Enumerable, LongFunction1)` wraps the call in `requireNonNull`, which is what the other overloads that unbox the accumulator already do. `EqualityComparer<T>` takes the nullable bound as well; `Functions` builds comparers over nullable elements. The two `aggregate` bodies carry a NullAway suppression. Assigning the result of a call returning `? extends TAccumulate` to a `TAccumulate` local makes NullAway report the local as @nullable, even on `return result;` where the local's type is the return type. Spelling the type argument exactly reports nothing, so the wildcard is what triggers it.
NullAway reports a `castToNonNull` whose argument it can already prove non-null, which is how it flags a cast that has stopped earning its place. Four of them in `EnumerableDefaults`: `curAccumulator` is assigned from `accumulatorInitializer.apply()` a few lines above each use, and `outerValue` from `outerValues.get(i)`.
Four places read a value the JDK is entitled to leave null. The Checker Framework knew about the first two from `InvocationHandler.astub`, which this migration deleted; NullAway's own JDK models say the same thing. * `InvocationHandler.invoke` receives a null `args` array when the proxied method declares no parameters. `Compatible` indexed it, and `FunctionExpression` forwarded it to a varargs call that requires an array. * `Primitive.asList` adapts a primitive array, whose elements box to a non-null value, so the `Array.get` result is cast rather than checked. * `EnumerableDefaults.takeTopN` looked up the key it had just read from `lastKey()`, and suppressed the finding on the declaration while dereferencing the value on the next line. It now uses `requireNonNull`.
`MergeUnionEnumerator.initEnumerators` carried `@RequiresNonNull("inputs")`.
`inputs` is a final field assigned in the constructor; the annotation existed
to get the Checker Framework's initialization checker past a call made from
that constructor. NullAway does its own initialization analysis, so the
annotation only made callers prove something already guaranteed, and the
suppression that silenced it goes with it.
`DefaultEnumerable.aggregate` carried `@Contract("!null, !null -> !null")`,
generated when `@PolyNull` was replaced. The original was polymorphic in the
accumulator function as well, so a non-null seed implied a non-null result. The
function may now return null, which makes the clause claim more than the method
delivers -- and NullAway, running with `CheckContracts`, said so.
`Linq4j.SingletonNullEnumerator` yields exactly one element and that element is null. `Functions.Ignore` implements the function interfaces by returning null from every `apply`. Both are meaningful only when their type argument is instantiated nullable. The Checker Framework could demand that: `<@nullable E>` on a type parameter declaration constrains the lower bound, so E had to be nullable. JSpecify tracks upper bounds only, and `<E extends @nullable Object>` says "may be" rather than "must be". Neither class is public, and both carry a suppression naming the reason.
`MemoryFactory` keeps a fixed-size window of rows for MATCH_RECOGNIZE, backed by a `@Nullable Object[]`. `MemoryEnumerator` pads it with `add(null)` once the input runs out, so that the last rows can still be read with their following context, and a slot that has not been written yet reads back as null either way. `add` takes `@Nullable E`, `Memory.get` returns it, and both type parameters take the nullable bound.
An outer join emits a row where one side is missing, and the merge and correlate joins build that row from a null. The types said otherwise. * `ExtendedEnumerable.correlateJoin` takes `Function2<TSource, ? super @nullable TInner, TResult>`. Its own javadoc already said "for semi/anti join inner argument is always null", and `EnumerableDefaults.correlateJoin` already declared the parameter that way. * `CartesianProductJoinEnumerator` accepts `Enumerator<? extends @nullable TInner>`, which is what a left join hands it as the single null inner row; its result selector already took `@Nullable TInner`. * `Linq4j.enumerator(Collection)` and `Linq4j.IterableEnumerator` take the nullable bound, so that null row can be enumerated at all. * Merge join widens its result selector before handing it to `nestedLoopJoin`, which serves right and full joins too and so requires one that tolerates a null left row. Merge join rejects those join types up front. `IterableEnumerator.moveNext` carries a NullAway suppression: assigning `Iterator<? extends T>.next()` to a field of type `T` is reported as assigning @nullable to @nonnull, the same limitation the seeded `aggregate` hits. `WrapMap.get` and `remove` take a nullable key, as `Map` requires, and wrapping one throws. That is what the map did before it was annotated; `put` already carried the same suppression. With this, NullAway reports nothing in calcite-linq4j.
`RexVisitorImpl`, `RexBiVisitorImpl` and `SqlBasicVisitor` are the traversals that subclasses extend and override where they care; every method they define returns null. That is meaningful only when the result type is instantiated nullable, which JSpecify cannot require: it tracks upper bounds, so `<R extends @nullable Object>` says "may be" rather than "must be". The Checker Framework said it with `<@nullable R>`, which constrains the lower bound. Same treatment as `Linq4j.SingletonNullEnumerator` and `Functions.Ignore`.
…Void A visitor that computes nothing was written `RexVisitorImpl<Void>`. `Void` has null as its only value, so a non-null `Void` is a type nothing inhabits, and the traversal returns exactly the null it rules out. 68 sites across `RexVisitorImpl`, `RexVisitor`, `SqlBasicVisitor`, `SqlVisitor` and `RexBiVisitorImpl` now say `@Nullable Void`, including the parameters and fields that hold such a visitor.
NullAway reports a `castToNonNull` whose argument it can already prove non-null, which is how it flags a cast that has stopped earning its place. 46 of them across 20 files, most in the `FlatLists` and `PairLists` constructors, where the elements arrive as non-null parameters.
…ides `java.util.List` declares `<T extends @nullable Object> T[] toArray(T[] a)`: the type variable carries the nullability and the array argument is required. The six `FlatNList` overrides declared `<T2> @nullable T2[] toArray(T2 @nullable [] a)`, which disagreed on all three counts, and then had to cast the argument back to non-null before reading its length. The six `Object[] toArray()` overrides carried a receiver parameter, `Flat1List<@nullable T> this`, which is how the Checker Framework said "only when T is nullable". JSpecify has no such form, and the annotation is what crashed NullAway when the migration generated a `@Contract` for these methods. `ComparableListImpl.toArray` casts what it delegates to. NullAway's JDK model reports `Collection.toArray()` as returning `@Nullable Object[]` at a call site while requiring an override to return `Object[]`, so a class that both implements `List` and calls `toArray` on another list has no signature that satisfies both.
`PairList` is declared `<T extends @nullable Object, U extends @nullable Object>`, but the classes in `PairLists` that implement it, and the `MapEntry` they hand back, declared plain `<T, U>`. A nullable type argument was therefore rejected at every one of them. `MutablePairList` keeps both halves of every pair in one `List<@nullable Object>`, so the element type cannot carry the nullability of T and of U separately, and reading a slot back produced a `@Nullable` value at 26 call sites. They now go through one `element` helper that explains the packing and carries the suppression, rather than each site arguing the point. `backingList` answers with `Arrays.asList` and `Collections.emptyList` instead of `ImmutableList`, which rejects a `@Nullable` element type. The array-backed implementation already answered that way.
`select` maps each element through a function, and the function is free to return null: `SqlFunctions` does exactly that when it projects a column that may be absent. The `<TResult>` type parameter said otherwise on `ExtendedEnumerable`, `ExtendedQueryable`, `QueryableFactory`, `EnumerableDefaults` and the classes that implement them, so a nullable type argument was rejected at the call. Adds the nullable bound to every `<TResult>` and `<TSource, TResult>` method in linq4j, and to the two overrides of those methods in core.
A SQL array or multiset may hold nulls, and an outer join hands the adapter a null collection outright, so the types these helpers work with have to say so. * `Functions.compareLists` and `compareMaps` accept `? extends @nullable Object` * `Linq4j.product` gives its element type a nullable bound * the two outer-join adapters in `SqlFunctions` take `@Nullable List<@nullable Object>` rather than `List<Object>`, which is what they were already handed * `mapFromEntries` builds a `Map<@nullable Object, @nullable Object>` rather than a raw one * `IS_JDK_8` reads `java.version`, which is always set, through `requireNonNull` `ArrayCartesianProductEnumerable` calls `toArray` through a lambda rather than a method reference, because the JDK model reports one signature at a call site and requires another in an override.
… from Three findings turned out to be NullAway defects rather than Calcite ones, and are now filed upstream. The places that work around them say which: * uber/NullAway#1726, a `@Contract` clause whose antecedent arity does not match the call crashes the analysis. The rule in the contributing guide says so. * uber/NullAway#1727, a call returning `? extends T` reads as `@Nullable` when `T` has a nullable upper bound. Both `EnumerableDefaults.aggregate` overloads. * uber/NullAway#1728, `Collection.toArray()` reports one signature at a call site and requires another in an override. `FlatLists` twice, and the lambda in `SqlFunctions` that replaced a `List::toArray` method reference.
…getOrDefault `getOrDefault(key, new BitSet())` cannot return null: the maps hold non-null `BitSet` values and the default is non-null. NullAway reported all 16 calls as nullable anyway, because the fields are declared `HashMap`, which overrides `getOrDefault`, and the model of `Map.getOrDefault` is not carried over to the override. A field declared `Map` reports nothing, and so does `TreeMap`, which inherits the method rather than overriding it. One `edgesOf` helper reads the map with `get` and answers an empty set, which depends on no model and says at one place what the 16 call sites were each implying. Reported as uber/NullAway#1729, see nullaway-bugs/jdk-model-shadows-inherited-library-model.md.
… elements `ImmutableNullableList` exists to hold nulls, and `Pair` is declared `<T1 extends @nullable Object, T2 extends @nullable Object>`. Their static factories said otherwise: a static method declares type parameters of its own, and these had none, so a nullable element was rejected at every call. * `ImmutableNullableList`, its three `copyOf` overloads and `builder`. The inner `Builder` already had the bound, which is why `builder()` returning `Builder<Double>` could not feed a `Builder<@nullable Double>`. * the 16 remaining statics on `Pair`: `zip`, `toMap`, `forEach`, `forEachIndexed`, `adjacents`, `firstAnd` and the rest. Together these account for 62 of the findings, most of them in `RelMdSize`, which computes a size per column and has no size for some of them.
Same treatment `FlatLists` got. `java.util.List` declares `<T extends @nullable Object> T[] toArray(T[] a)`; the override declared `<T> @nullable T[] toArray(T @nullable [] a)` and then cast the argument back to non-null to read its length. `Object[] toArray()` carried a `ConsList<@nullable E> this` receiver parameter, which is how the Checker Framework said "only when E is nullable". The two delegated `toArray()` calls are cast, and say why: the JDK model reports `@Nullable Object[]` at a call site while the override check accepts only `Object[]` (uber/NullAway#1728).
The runtime works on SQL arrays, multisets and maps, whose elements and values are nullable, and on rows whose fields may be absent. The types said otherwise. * `Linq4j.asEnumerable` gives its element type a nullable bound, at all four overloads: viewing a collection as an `Enumerable` should not lose the fact that it holds nulls * `Functions.compareMaps` accepts a nullable key type as well as a nullable value type, which is what a cast `(Map<?, ?>)` produces * `FlatLists.ofSingle` and `of(List)` take nullable elements. They build a `Flat1List` and a flat list, both of which already admit them * `flatListOuter` takes `@Nullable List<@nullable Object>`, which is what an outer join hands it, and its body already tested for null * `mapFromString` builds `Map<String, @nullable String>`: a pair with no separator has no value. `slice` collects `structAccess` results, which are null for an absent field Three locals that hold a `toArray()` result are declared `@Nullable Object[]`, and say why (uber/NullAway#1728).
Six utilities that everything else calls declared type parameters without a
nullable bound, so a nullable argument was rejected at every call.
* `Ord` holds a nullable element type; its seven statics now say so
* `PairList`'s seven statics, matching the interface they build
* `Util.firstDuplicate`, which compares elements rather than dereferencing them
* `SqlOperator.acceptCall`, both overloads, and
`SqlBasicVisitor.ArgHandler.instance`: a visitor result may be nullable, and
`acceptCall` passes it through
`Util.first` is the Elvis operator: its result is non-null when the fallback
is, which `@Contract("_, !null -> !null")` states and NullAway verifies. Callers
in `RelBuilder`, `SqlValidatorImpl` and `ModelHandler` relied on that.
`Ord.zip(Iterator)` suppresses one finding: `next()` on an
`Iterator<? extends E>` reads as `@Nullable E` when `E` has a nullable upper
bound (uber/NullAway#1727).
91 findings.
Eight files, all saying that a value cannot be null where it can. * `ImmutableIntList.toArray`, both overrides, match the JDK signature `<T extends @nullable Object> T[] toArray(T[] a)` and stop casting the argument back to non-null to read its length. Same as `FlatLists` and `ConsList`. * `RexNode.accept(RexBiVisitor, P)` and the `P` of `RexBiVisitor` and `RexBiVisitorImpl` take a nullable bound. `LogicVisitor` returns a nullable `Logic`, and could not be passed to its own `accept`. * `Util.skip`, `SqlParserUtil.replaceSublist` and `ResultSetEnumerable.of` join the statics that carry the bound of the type they work on. * `ArrayTable` reads a column whose values may be null into a list and an array that say so. `Permutation.isValid` loses `@RequiresNonNull({"sources", "targets"})`: both are `int[]`, and a primitive array is never null, so the precondition the Checker Framework needed during construction states nothing NullAway does not know. `JdbcTypeImpl` is suppressed and says why. Its `XXX_NULLABLE` constants return null for a null column, and cannot say so: the enum implements `JdbcType` raw, because a constant cannot carry its own type argument.
… needed `@RequiresNonNull` states which nullable fields a caller must have checked. `RexLiteral.computeDigest`, `RexLiteral.digestIncludesType` and `Resources.Prop.getDefault` named fields that are `final` and non-null: `RexLiteral.type` and `typeName`, and `Resources.Element.method`. The Checker Framework needed them because the methods run while the object is still under construction, which it tracked and NullAway does not. Under NullAway the annotations state nothing new and reject every caller. Same reason `Permutation.isValid` lost its own in the previous commit. Also here: the 16 overrides of `RexNode.accept(RexBiVisitor, P)` take the bounds the abstract method now has, and `ImmutableNullableList.of`, all eight overloads, joins `copyOf` and `builder` in admitting the nulls the class exists to hold. `JdbcUtils` builds its connection key from a url, user, password and driver class, of which only the url is required. 42 findings.
…ead preconditions
`canBeLong` answers `o instanceof Boolean || ...`, which is false for null.
`@Contract("null -> false")` states that, and NullAway narrows the caller in
`ColumnLoader` accordingly, where a dropped `@EnsuresNonNullIf` used to.
`LoptMultiJoin` loses `@RequiresNonNull` from six methods: `joinStart` and
`nFieldsInJoinFactor` are `int[]`, and a primitive array is never null. Same as
`Permutation`, `RexLiteral` and `Resources`.
`LoptOptimizeJoinRule` reads each table out of `simpleFactors` once, through
`requireNonNull`, rather than looking the same key up three times. The keys come
from that map, which a dropped `@KeyFor` used to say.
`ChunkList` packs the neighbouring chunks into slots 0 and 1 of the element
array, so those two writes are suppressed and say why. The two places that
narrow a chunk reference now assign the result of `castNonNull` instead of
casting the same nullable local at each use.
`CalciteConnectionConfigImpl` suppresses its six plugin accessors: `getPlugin`
comes from Avatica, which is not annotated, so NullAway cannot infer one `T`
that suits both the `Class` argument and a nullable default. The `@Contract` on
each stays, and NullAway verifies it.
Three lambdas in `ColumnLoader` drop their explicit parameter type, which was
non-null where the functional interface says nullable.
…nd two more dead preconditions Nine overrides of `SqlOperator.acceptCall` take the bound the base method got two commits ago. A visitor result may be nullable, and `acceptCall` passes it through. Four `InvocationHandler.invoke` implementations move the annotation from the elements to the array: `@Nullable Object @nullable [] args`. The JDK passes null for `args` when the proxied method declares no parameters, which is what the deleted `InvocationHandler.astub` used to say. `ReflectiveConvertletTable.map` and `SimpleProfiler.Run.columns` are `final` and non-null, so the `@RequiresNonNull` naming them stated nothing and rejected their callers.
…hat @contract cannot `Util.first` is the Elvis operator of the codebase, and its `@Contract("_, !null -> !null")` is honoured everywhere except where it matters most. NullAway consults a contract in its dataflow but not in the JSpecify generics check, which infers the type argument from the declared return type alone and reports inference failure: type variable T constrained to be both @nonnull and @nullable at 20 call sites. The same contract on a non-generic method reports nothing, and an explicit type witness silences the generic one, so the two halves of NullAway disagree rather than the calls being wrong. Reported as uber/NullAway#1730, see nullaway-bugs/nullable-return-type-breaks-generic-inference.md. static <T extends @nullable Object> T firstNonNull(@nullable T v0, T v1) says the same thing without a contract: `T` is inferred non-null from the fallback, `@Nullable T` still accepts a nullable first argument, and the result is non-null. `first` stays for the calls whose fallback is itself nullable. `CalciteSystemProperty` already static-imports Guava's `MoreObjects.firstNonNull` and calls the new one qualified.
…owings Four proxy invocation handlers index `args`, which the JDK leaves null when the proxied method declares no parameters, so they read it through `requireNonNull`. Three places declare a `toArray()` result `@Nullable Object[]`, which is what the JDK model gives them. `Util.LINE_SEPARATOR` and `FILE_SEPARATOR` read properties that are always set, and `toUrl` reads `file.separator` a second time rather than the constant it already has. `deepEquals0` used to narrow its argument through `@EnsuresNonNullIf`, which NullAway supports for fields only, so the three `deepEquals` in `LogicalFilter`, `LogicalJoin` and `LogicalAsofJoin` say `requireNonNull` instead. `SqlBinaryOperator` and `SqlTypeUtil` ask for a charset after `inCharFamily` has established there is one.
`UnboundMetadata.bind` answers null when the provider has no metadata for a node, which three implementations already did and none declared. `LazyReference` holds nothing until its supplier has run, so its `AtomicReference` is `<@nullable T>` and the two `(T) null` casts go away. `jsonObjectAggAdd` and `jsonArrayAggAdd` took a raw `Map` and `List` and put null into them, which is what SQL/JSON asks for under NULL ON NULL. They now take `Map<String, @nullable Object>` and `List<@nullable Object>`. `Utilities.compare` is suppressed: its `Comparator` parameter is raw, so NullAway checks the call against the erased `compare(Object, Object)`, while a SQL comparator is exactly the thing that orders nulls. `Matcher` casts the row at offset 0 of the memory window, which is the one the predicate has just accepted. The window itself is padded with nulls at either end, which is why `Memory.get` is nullable.
A run of one-off corrections, all of the same shape: a value that can be absent was flowing into a type that said it could not. * `Collections.nCopies(n, null)` builds a list of absent values, in `ProfilerImpl` twice and `AggregateReduceFunctionsRule` * `CacheUtil` returns what `toArray()` gives it, `@Nullable Object[]` * `EnumerableTableModify` keys an update by a source row, whose column values may be null * `RexSimplify.evaluate` returns a value or an exception, exactly one of which is set, so both halves of the `Pair` are nullable * `RelToSqlConverter` collects into a list that becomes a `SqlNodeList` * `SerializableCharset.readObject` reads back what `writeObject` wrote, and `DelegatingScope` looks up a key it took from the map's own key set * `EnumerableWindow` asks for an offset after ruling out CURRENT ROW, which is the case that has none `SqlNode.toList` and `RelMdColumnUniqueness` replace a method reference with a lambda. A reference resolves against the JDK model, which disagrees with the functional interface it is being assigned to; a lambda is inferred from the target type instead.
…ract guards
`SqlValidatorNamespace.unwrap` answers null when the namespace is not of the
requested type, which `AbstractNamespace` has always done and neither it nor
`DelegatingNamespace` declared. Six call sites in `SqlValidatorImpl` and
`SqlValidatorUtil` ask through `requireNonNull`; each has already established
the type it is asking for.
`NumberUtil.add` and `RelMdPercentageOriginalRows.quotientForPercentage` write
one `if` per argument rather than `if (a == null || b == null)`, so that no
control-flow merge precedes the `return null`. `CheckContracts` treats a return
reached by a merge as reachable even when every incoming edge is not, and
reports the `@Contract("!null, !null -> !null")` as violated. Reported as
uber/NullAway#1731, see nullaway-bugs/contract-check-misses-merged-guard.md. The
other 108 contracts in the codebase verify without complaint.
Also: `TryThreadLocal.of` and `Functions.generate` take the nullable bound of
the type they build, and `Prepare.THREAD_INSUBQUERY_THRESHOLD` holds a threshold
that may be unset.
…ssion lists hold nulls `getNamespaceOrThrow(node).unwrap(XNamespace.class)` asks for a namespace kind the caller has just built, at five places in `SqlValidatorImpl`, and now says so with `requireNonNull` rather than relying on the declaration `unwrap` no longer makes. `Expressions.list` takes a nullable element bound, at all three overloads: a generated expression list holds a null comparer when the row type needs none. Five `Util.first(physType.comparer(), ...)` calls move to `firstNonNull`, whose fallback is a constant expression. `ArrayTable` permutes a column into an array that keeps the column's nulls. `SqlRowOperator` reads a field name once rather than testing one call and using another, `FormatModels` checks the match it just found, and `CalciteConnectionImpl` reads `user.name`, which is not guaranteed.
Calling `Util.firstNonNull` here drags `Util`'s static initialiser into `CalciteSystemProperty`'s, and `Util` reads `CalciteSystemProperty.DEFAULT_CHARSET`, which is still null at that point. The result is an `ExceptionInInitializerError` from a class-initialisation cycle that takes every test using `CalciteAssert` with it. The call was already `MoreObjects.firstNonNull` and stays that way. The previous commit qualified it as `Util.firstNonNull` only to settle a name clash that the static import had already settled.
… the bytecode wildcard `VolcanoPlanner.normalizePlan` reads a group after `find()` has succeeded, `MatchNode` names the row it reads at end of stream, `DataContexts.MapDataContext` maps a name to a value that may be absent, and `SqlBasicCall.set` copies a list whose elements may be null into an array that says so. `SqlNodeList` and `SubstitutionVisitor` suppress `containsAll`, `removeAll` and `retainAll`. An unbounded wildcard read from a bytecode signature behaves as `? extends Object` and rejects a nullable type argument, while the same `Collection<?>` written in annotated source accepts one. Reported as uber/NullAway#1732, see nullaway-bugs/bytecode-unbounded-wildcard-rejects-nullable.md. `SubstitutionVisitor` puts the suppression on one `removeParents` helper rather than on the constructor that calls it twice.
…alues A profiled row carries a value per column, and a column may have none, so `Collector.add` and its three overrides take `List<@nullable Comparable>` and `CompositeCollector` keeps a `@Nullable Comparable[]`. `FlatLists.of(T, T, T)`, the six statics of `CompositeList` and `SqlBasicCall.set` take the nullable element bound of the lists they build. `LatticeSuggester` keys a node by its parent, and a root has none; `AggregateReduceFunctionsRule` names the extra columns it projects, of which the new ones have no name yet.
…ived its reason `SqlToRelConverter` reads the project it has just cast rather than casting it at each use, and asks for a `DmlNamespace` after `isWrapperFor` has established there is one. `SqlValidatorImpl` collects aliases into a list that admits the null a child of the FROM clause may have, and looks up a column by an index it took from the map it is reading. `JavaRowFormat.copy` returns a list of statements and never null, so the `castNonNull` around it in `EnumUtils` goes away. `FilterProjectTransposeRule` answers `replaceIfs` with null when the input has no distribution, rather than a singleton list holding null. `replaceIfs` takes a supplier that may answer null, and does the same thing with it. `CalciteCatalogReader` falls back on a family constant, which is what `firstNonNull` is for.
`ProfilerImpl` separates the two kinds of row it works with: a scanned row uses `NullSentinel` for a SQL null and so holds no Java null, while the sketch path builds a sparse row filled only at the ordinals of the space it feeds. `Collector.add` takes `List<? extends @nullable Comparable>` so it accepts both, and each collector casts the ordinals its own space owns. Type parameters that carry the nullable bound of what they build: `RexWindowBound.accept` and its override, `Functions.ignore2`, `Util.combine`, `SqlNodeList.toArray`, `HepPlanner.onCopyHook`, `EnumerableTableModify.keyOf` and the maps keyed by it, and `ArrayTable.asList`. `SqlNode.toList` and `RelBuilder` replace a method reference and a Guava call whose wildcard comes from bytecode with a lambda and a direct iterator check. `TableFunctionScanNode` drops its raw `Enumerable` for a typed one. `ArrayTable.permute` is suppressed: an array creation keeps a non-null component type whatever it is assigned to, so writing a nullable element reports even though both arrays are declared `@Nullable Comparable[]`.
|
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.
Preview, not ready to merge. NullAway still reports 576 errors in
calcite-coreand 126 incalcite-linq4j, so the nullness CI job is red on purpose. The point is to show the migration and the shape of what remains. See CALCITE-7736.Why
The Checker Framework needs a Gradle plugin of its own, 48
.astubfiles that patch the nullness of the JDK and of third-party libraries, and two dedicated CI jobs. NullAway is a single Error Prone check: no separate plugin, no stub files, and nullness models for the JDK and for popular libraries out of the box. The annotations come from JSpecify, a specification that several checkers read, rather than from one checker's own package.What
Six commits, each doing one thing:
@Nullableand@NonNullfrom the Checker Framework to JSpecify@NullMarkedon the packages that NullAway verifiesLintTest.testLintNullMarked@PolyNull,@MonotonicNonNull,@Pure, the initialization annotations, and the rest@PolyNullwith@ContractThe rename commit is worth skimming rather than reading. Commits 1 to 3 do not build on their own, because the source still carries Checker Framework annotations after
checker-qualis gone; from commit 4 onward every commit compiles.NullAway is configured in JSpecify mode with the experimental generics support (
JSpecifyExperimental,HandleWildcardGenerics,JSpecifyJDKModels,WarnOnGenericInferenceFailure) and withCheckContracts. It is an error in the projects listed innullawayProjectsand off elsewhere, so a nullness problem fails one CI job rather than every test job.org.apache.calcite.linq4j.annotationsis new and holds@Contract,@MonotonicNonNull,@RequiresNonNull,@EnsuresNonNulland@EnsuresNonNullIf. NullAway matches these by the last component of their name rather than by their package, so Calcite declares its own and takes no dependency on the checker.The part worth reviewing
The two tools default an unwritten type parameter bound in opposite directions. CLIMB-to-top gives implicit bounds the top qualifier, so
<T>under the Checker Framework means<T extends @Nullable Object>; JSpecify fills inObject, which under@NullMarkedis non-null. Every unbounded type parameter therefore changed meaning, and Calcite relied on the Checker Framework reading —SqlShuttle extends SqlBasicVisitor<@Nullable SqlNode>was passed toSqlNode.accept(SqlVisitor<R>)with no suppression, which typechecks only ifRadmits a nullable argument.Writing the bound out at 61 declarations took NullAway from 1126 errors to 576.
Pair.ofalone was worth 132: its class already had the bounds, but a static factory declares type parameters of its own.The erasure is unchanged, so these are binary compatible.
How to verify
Needs JDK 21, which Error Prone 2.43 and later require.
classes,testClasses,checkstyleMain,checkstyleTestandautostyleCheckpass.:core:testand:linq4j:testrun 18866 tests with no failures.Open questions
nullawayProjectslists:linq4jand:core. The Checker Framework jobs also covered:server.calcite-annotationsmodule rather than incalcite-linq4j?IndexOutOfBoundsExceptionwhen a@Contractclause names more arguments than the call site passes:ContractHandler.onDataflowVisitMethodInvocationreads arguments by the antecedent's length, and validates the arity on declarations but not at call sites. Worth reporting upstream. Avoided here by not annotating receiver parameters or varargs methods.