[
https://issues.apache.org/jira/browse/CALCITE-7736?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Vladimir Sitnikov updated CALCITE-7736:
---------------------------------------
Description:
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. {{@MonotonicNonNull}} and the annotations for
field preconditions and postconditions 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.
was:
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.
> 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
> Priority: Major
>
> 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. {{@MonotonicNonNull}} and the annotations
> for field preconditions and postconditions 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)
