vlsi opened a new pull request, #5213: URL: https://github.com/apache/calcite/pull/5213
**Preview, not ready to merge.** NullAway still reports 576 errors in `calcite-core` and 126 in `calcite-linq4j`, so the nullness CI job is red on purpose. The point is to show the migration and the shape of what remains. See [CALCITE-7736](https://issues.apache.org/jira/browse/CALCITE-7736). ## Why The Checker Framework needs 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 is a single Error Prone check: no separate plugin, no stub files, and nullness models for the JDK and for popular libraries out of the box. The annotations come from JSpecify, a specification that several checkers read, rather than from one checker's own package. ## What Six commits, each doing one thing: | Commit | | |---|---| | Replace the Checker Framework with NullAway and JSpecify | build and CI only | | Move `@Nullable` and `@NonNull` from the Checker Framework to JSpecify | mechanical rename; 1197 insertions against 1197 deletions, and no changed line touches anything but those two imports | | Declare `@NullMarked` on the packages that NullAway verifies | plus `LintTest.testLintNullMarked` | | Migrate the Checker Framework annotations that JSpecify does not define | `@PolyNull`, `@MonotonicNonNull`, `@Pure`, the initialization annotations, and the rest | | Replace `@PolyNull` with `@Contract` | 108 clauses | | Give type parameters the nullable bounds the Checker Framework inferred | 61 declarations | The rename commit is worth skimming rather than reading. Commits 1 to 3 do not build on their own, because the source still carries Checker Framework annotations after `checker-qual` is gone; from commit 4 onward every commit compiles. NullAway is configured in JSpecify mode with the experimental generics support (`JSpecifyExperimental`, `HandleWildcardGenerics`, `JSpecifyJDKModels`, `WarnOnGenericInferenceFailure`) and with `CheckContracts`. It is an error in the projects listed in `nullawayProjects` and off elsewhere, so a nullness problem fails one CI job rather than every test job. `org.apache.calcite.linq4j.annotations` is new and holds `@Contract`, `@MonotonicNonNull`, `@RequiresNonNull`, `@EnsuresNonNull` and `@EnsuresNonNullIf`. 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 part worth reviewing The two tools default an unwritten type parameter bound in opposite directions. CLIMB-to-top gives implicit bounds the top qualifier, so `<T>` under the Checker Framework means `<T extends @Nullable Object>`; JSpecify fills in `Object`, which under `@NullMarked` is non-null. Every unbounded type parameter therefore changed meaning, and Calcite relied on the Checker Framework reading — `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 took NullAway from 1126 errors to 576. `Pair.of` alone was worth 132: its class already had the bounds, but a static factory declares type parameters of its own. The erasure is unchanged, so these are binary compatible. ## How to verify ```bash ./gradlew -PenableErrorprone :linq4j:classes :core:classes ``` Needs JDK 21, which Error Prone 2.43 and later require. `classes`, `testClasses`, `checkstyleMain`, `checkstyleTest` and `autostyleCheck` pass. `:core:test` and `:linq4j:test` run 18866 tests with no failures. ## Open questions - `nullawayProjects` lists `:linq4j` and `:core`. The Checker Framework jobs also covered `:server`. - Should the annotations live in a separate `calcite-annotations` module rather than in `calcite-linq4j`? - NullAway crashes with an `IndexOutOfBoundsException` when a `@Contract` clause names more arguments than the call site passes: `ContractHandler.onDataflowVisitMethodInvocation` reads arguments by the antecedent's length, and validates the arity on declarations but not at call sites. Worth reporting upstream. Avoided here by not annotating receiver parameters or varargs methods. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
