[
https://issues.apache.org/jira/browse/CALCITE-7736?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18108054#comment-18108054
]
Vladimir Sitnikov commented on CALCITE-7736:
--------------------------------------------
I went ahead and added nullability annotations and verifications to virtually
all the other modules. It does help to improve types since some of the modules
like linq4j and core (e.g. mongodb) might call {{core}} with {{null}} which was
not seen previously.
I filed [10 issues for
NullAway|https://github.com/uber/NullAway/issues?q=is%3Aissue%20author%3Avlsi],
and 2 of them have already been resolved with a code change.
The drawback of "blindly" adding `@NullMarked` is that kotlin treats jspecify
annotations as compilation failures, so if we miss {{{}@Nullable{}}}, then
Kotlin clients won't be able to pass {{{}null{}}}. Sure we could fix it in
subsequent releases. I think it might be a reasonable tradeoff.
The merge order would probably be:
1) linq4j+core+server. That is the same set that was verified previously.
2) then file a bunch of PRs (e.g. one per module).
An alternative option to #2 could be a single PR with "all the other modules"
I'm fine either way. Just suggest your options.
----
If you check out the draft and find out the change is not wanted, please say
that aloud. I would just close the PR.
As for me, checkerframework is painfully slow which is especially painful with
agentic programming.
> 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)
