[
https://issues.apache.org/jira/browse/CALCITE-7736?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18109325#comment-18109325
]
Julian Hyde commented on CALCITE-7736:
--------------------------------------
I think you should do it, as a single PR. Ping me on the ML when it's ready and
I will approve.
It's a little difficult to read your PR (it's based off of 67e5bfcdc0, which is
ten months behind origin/main). But I get the following stats:
{code:java}
% git diff 5d7f75d5d8 vlsi/CALCITE-7736 --stat
1509 files changed, 5822 insertions(+), 5832 deletions(-) {code}
The changes look modest, and in one or two places we were able to remove
{{{}Nullable{}}}:
{code:java}
// The windows of each key are kept sorted by start time; the merge
// below only compares a window with the one that precedes it.
- Map<@Nullable Object, NavigableMap<Pair<Long, Long>, List<@Nullable
Object[]>>>
+ Map<Object, NavigableMap<Pair<Long, Long>, List<Object[]>>>
sessionKeyMap = new HashMap<>(); {code}
Replacing {{PolyNull}} with {{Contract}} is an improvement:
{code:java}
- <T> @PolyNull T fun(Class<T> operatorTableClass,
- @PolyNull T defaultOperatorTable);
+ @Contract("_, !null -> !null")
+ <T> @Nullable T fun(Class<T> operatorTableClass,
+ @Nullable T defaultOperatorTable); {code}
This is the only regression I noticed:
{code:java}
- private static List<Object> keyOf(Object[] rowValues) {
- return Arrays.asList(Arrays.copyOf(rowValues, rowValues.length));
+ private static List<@Nullable Object> keyOf(Object[] rowValues) {
+ return Arrays.<@Nullable Object>asList(
+ Arrays.copyOf(rowValues, rowValues.length)); {code}
Adding {{firstNonNull}} is a smart move:
{code:java}
public static final Version AVATICA_VERSION =
- Version.of(first(System.getProperty("calcite.avatica.version"), "0"));
+ Version.of(
+ firstNonNull(System.getProperty("calcite.avatica.version"), "0"));
{code}
> 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)
