[
https://issues.apache.org/jira/browse/CALCITE-7736?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18107242#comment-18107242
]
Vladimir Sitnikov commented on CALCITE-7736:
--------------------------------------------
{quote}What coverage would we lose if we ditch Checker Framework?
{quote}
Frankly, I have no idea at this point.
Previously, NullAway did not support generics which was a show-stopper for
Calcite.
Now NullAway state they have "experimental support" for generics. I don't know
what is the actual level of support, and I think only a spike test could show
the real issues.
I hope the coverage won't degrade, and I am sure NullAway would be happy to
accept contributions should Calcite surface a bug in NullAway.
{quote}If we switch to NullAway we should enable it in all modules, not just
main and linq4j.
{quote}
Initially I planned fixing all NullAway issues for all the modules that passed
Checkerframework verification. As far as I know only [linq4j core and server
modules were
machine-verified|https://github.com/apache/calcite/blob/79f3b80b7dfae03a615d7974345bf877d0e9cb39/.github/workflows/main.yml#L382C81-L382C125].
Sure we could annotate the other modules, however, it might take time. I don't
know how good LLMs are with adding nullability annotations as it is effectively
requires public API decisions (e.g. "should we accept null to parameter X?")
which are better to be made by a maintainer.
> 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)
