Vladimir Sitnikov created CALCITE-7736:
------------------------------------------
Summary: Replace the Checker Framework with NullAway and JSpecify
Key: CALCITE-7736
URL: https://issues.apache.org/jira/browse/CALCITE-7736
Project: Calcite
Issue Type: Improvement
Components: core, linq4j
Reporter: Vladimir Sitnikov
Assignee: Vladimir Sitnikov
Calcite verifies nullness with the Checker Framework. That setup carries a
Gradle plugin of its own, 48 {{.astub}} files that patch the nullness of the
JDK and of third-party libraries, and two dedicated CI jobs.
NullAway runs as an Error Prone check, so it needs no separate plugin and no
stub files: it ships nullness models for the JDK and for popular libraries. The
annotations come from JSpecify, a specification that several checkers read,
rather than from one checker's own package.
h3. Plan
* replace {{org.checkerframework:checker-qual}} with {{org.jspecify:jspecify}}
* delete the 48 stub files and the two CheckerFramework CI jobs, and fold
nullness verification into the existing {{errorprone}} job
* configure NullAway in JSpecify mode, with the experimental generics options
({{JSpecifyExperimental}}, {{HandleWildcardGenerics}}, {{JSpecifyJDKModels}},
{{WarnOnGenericInferenceFailure}}) and {{CheckContracts}}
* declare {{@NullMarked}} on the main packages of {{calcite-linq4j}} and
{{calcite-core}}, the two modules NullAway verifies, and add a lint test that
fails when a package there has no {{package-info.java}}
h3. Annotations that JSpecify does not define
JSpecify defines {{@Nullable}}, {{@NonNull}} and {{@NullMarked}}. Calcite also
uses {{@PolyNull}}, {{@MonotonicNonNull}}, {{@RequiresNonNull}},
{{@EnsuresNonNull}}, {{@EnsuresNonNullIf}}, {{@Pure}}, {{@KeyFor}} and the
initialization annotations.
{{@PolyNull}} becomes {{@Contract("!null, _ -> !null")}}, which NullAway
verifies against the method body. The field pre- and postconditions and
{{@MonotonicNonNull}} move to a new {{org.apache.calcite.linq4j.annotations}}
package: 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 rest have no equivalent and go away.
h3. Type parameter bounds
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.
Every unbounded type parameter therefore changes meaning, and Calcite relied on
the Checker Framework reading. For example {{SqlShuttle extends
SqlBasicVisitor<@Nullable SqlNode>}} was passed to
{{SqlNode.accept(SqlVisitor<R>)}} with no suppression, which typechecks only if
{{R}} admits a nullable argument. Writing the bound out at 61 declarations
accounts for most of what NullAway reports.
h3. Why NullAway rather than another checker
The Checker Framework is thorough but slow, which is why Calcite runs it in two
CI jobs of its own rather than as part of an ordinary build. NullAway is a
single Error Prone check and costs a fraction of that, so nullness verification
can move into a normal compile and eventually run by default rather than in a
job that only a maintainer looks at. A nullness mistake then surfaces in the
build that introduced it.
NullAway also reports the whole set of problems in one pass — the run below
produces 576 in calcite-core — which is what makes a migration of this size
tractable: you fix a class of errors across the codebase and re-run, instead of
discovering them one at a time.
h3. Status
A first pass leaves 576 errors in {{calcite-core}} and 126 in
{{calcite-linq4j}}, so the nullness CI job is red. The pull request is a
preview: it shows the migration and the shape of what remains, rather than a
finished conversion.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
