[
https://issues.apache.org/jira/browse/CALCITE-7736?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18107773#comment-18107773
]
Vladimir Sitnikov commented on CALCITE-7736:
--------------------------------------------
Neither checkerframework nor nullaway produce bytecode. They verify nullability
(e.g. code passing null value when only non-null expected).
The verification was always present, however, it seem it was just not executed
in the CI: {{./gradlew jandex}} was there exactly to verify if the bytecode is
parseable with ASM. So I guess jandex task should be added to errorprone ci job
(which should be renamed to "code style" or something like that)
> 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
> Labels: pull-request-available
>
> 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)
