GH-37286: [Java] Start adding nullability/nullness annotations #37723

davisusanibar · 2023-09-14T14:42:09Z

Rationale for this change

Closes: #37286

What changes are included in this PR?

Initial support for:

Use the Checker Framework to enhances Java’s type system to make it more powerful and useful. Planning to start with Nullness Checker

Are these changes tested?

These are the activities involved on this PR:

Configure the Checker Framework
Treat checker errors as warnings initially
Applying Nullness Checker annotation as needed: @nonnull / @nullable
Check if building timer increases after this checker is added
Fixes for code review

Are there any user-facing changes?

Yes

Closes: [Java] Start adding JSpecify nullability/nullness annotations #37286

github-actions · 2023-09-14T14:42:36Z

Thanks for opening a pull request!

If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose

Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project.

Then could you also rename the pull request title in the following format?

GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

or

MINOR: [${COMPONENT}] ${SUMMARY}

In the case of PARQUET issues on JIRA the title also supports:

PARQUET-${JIRA_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

See also:

github-actions · 2023-09-14T14:43:17Z

⚠️ GitHub issue #37286 has been automatically assigned in GitHub to PR creator.

davisusanibar · 2023-09-19T18:58:43Z

Hello @lidavidm, could you please provide me with an initial review of how I intend to integrate Nullness evaluation into Java modules (please consider only: arrow-memory-core initially)?

I have these 02 doubts:

There are a lots of warning message if we are planning to include Nullness Checker ... then is needed to define an initial scope that could be:
1. Define what modules need to be analyzed by Nullness Checker, or
2. Define what classes need to be analyzed by Nullness Checker.
The building time of Java modules has doubled since Nullness Checker has been added.

Could you please help me with these two doubts so that I know how to proceed?

Thank you in advance

lidavidm · 2023-09-19T19:01:57Z

For (1), what are some examples of the warnings? Ideally, we'd only enable it one module at a time.

For (2), can we enable it only in a separate profile, and have that be run in CI? That way it does not affect the 'normal' builds.

lidavidm · 2023-09-19T19:02:36Z

java/pom.xml

@@ -44,8 +44,11 @@
    <forkCount>2</forkCount>
    <checkstyle.failOnViolation>true</checkstyle.failOnViolation>
    <errorprone.javac.version>9+181-r4173-1</errorprone.javac.version>
-    <error_prone_core.version>2.16</error_prone_core.version>
-    <maven-compiler-plugin.version>3.10.1</maven-compiler-plugin.version>
+    <pinjdk8.errorprone.core.version>2.10.0</pinjdk8.errorprone.core.version>


More reason to drop JDK8 ASAP.

lidavidm · 2023-09-19T19:02:54Z

java/pom.xml

-           with other annotation processors.
-           See https://github.com/jbosstools/m2e-apt/issues/62 for details
-      -->
+      <id>error-prone-checkerframework-jdk8</id>


IMO, we don't bother enabling this for JDK8. In fact, I'd be OK with dropping error-prone for JDK8 as well, and only enabling these checks only on the latest LTS.

lidavidm · 2023-09-19T19:05:29Z

java/memory/memory-core/src/main/java/org/apache/arrow/memory/ArrowBuf.java

@@ -1033,7 +1034,7 @@ public void getBytes(long index, OutputStream out, int length) throws IOExceptio
      // copy length bytes of data from this ArrowBuf starting at
      // address addr(index) into the tmp byte array starting at index 0
      byte[] tmp = new byte[length];
-      MemoryUtil.UNSAFE.copyMemory(null, addr(index), tmp, MemoryUtil.BYTE_ARRAY_BASE_OFFSET, length);


nit: is it possible to make the UNSAFE constant private to avoid other problems like this? (We can do that in a separate ticket.)

lidavidm · 2023-09-19T19:05:55Z

java/memory/memory-core/src/main/java/org/apache/arrow/memory/BaseAllocator.java

@@ -154,7 +157,7 @@ private static String createErrorMsg(final BufferAllocator allocator, final long
    }
  }

-  public static boolean isDebug() {
+  public static @Nullable boolean isDebug() {


This seems wrong: this is a primitive and should never be null.

lidavidm · 2023-09-19T19:06:04Z

java/memory/memory-core/src/main/java/org/apache/arrow/memory/BaseAllocator.java

@@ -42,7 +43,7 @@ abstract class BaseAllocator extends Accountant implements BufferAllocator {

  public static final String DEBUG_ALLOCATOR = "arrow.memory.debug.allocator";
  public static final int DEBUG_LOG_LENGTH = 6;
-  public static final boolean DEBUG;
+  public static final @Nullable boolean DEBUG;


This also seems wrong.

lidavidm · 2023-09-19T19:06:26Z

java/memory/memory-core/src/main/java/org/apache/arrow/memory/BaseAllocator.java

@@ -64,15 +65,16 @@ abstract class BaseAllocator extends Accountant implements BufferAllocator {
  // Package exposed for sharing between AllocatorManger and BaseAllocator objects
  private final String name;
  private final RootAllocator root;
-  private final Object DEBUG_LOCK = DEBUG ? new Object() : null;
+
+  private final @Nullable Object DEBUG_LOCK = DEBUG ? new Object() : null;


IMO, there is no harm in always initializing this.

lidavidm · 2023-09-19T19:07:36Z

java/memory/memory-core/src/main/java/org/apache/arrow/memory/BaseAllocator.java

@@ -183,6 +186,7 @@ public ArrowBuf getEmpty() {
   * we have a new ledger
   * associated with this allocator.
   */
+  @SuppressWarnings({"nullness:locking.nullable", "nullness:argument"})


What are the warnings here?

lidavidm · 2023-09-19T19:08:06Z

java/memory/memory-core/src/main/java/org/apache/arrow/memory/util/ArrowBufPointer.java

@@ -81,7 +82,8 @@ public ArrowBufPointer(ArrowBuf buf, long offset, long length) {
   * @param length the length off set of the memory region pointed to.
   * @param hasher the hasher used to calculate the hash code.
   */
-  public ArrowBufPointer(ArrowBuf buf, long offset, long length, ArrowBufHasher hasher) {
+  @SuppressWarnings("initialization.fields.uninitialized")


Can we initialize fields instead of suppressing the warnings?

lidavidm · 2023-09-19T19:09:38Z

java/memory/memory-core/src/main/java/org/apache/arrow/memory/util/MemoryUtil.java

@@ -33,7 +35,7 @@
 public class MemoryUtil {
  private static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(MemoryUtil.class);

-  private static final Constructor<?> DIRECT_BUFFER_CONSTRUCTOR;
+  private static final @Nullable Constructor<?> DIRECT_BUFFER_CONSTRUCTOR;


Does Checker Framework have anything for static initializers?

I didn't see anything at first glance besides this https://checkerframework.org/api/org/checkerframework/checker/initialization/InitializationChecker.html

davisusanibar · 2023-09-19T19:55:22Z

For (1), what are some examples of the warnings? Ideally, we'd only enable it one module at a time.

For (2), can we enable it only in a separate profile, and have that be run in CI? That way it does not affect the 'normal' builds.

(1) You could see warning messages here, search for [WARNING] /arrow/java/ inside of {2_AMD64 Debian 9 Java JDK 11 Maven 3.6.2.txt, ...}

(2) Ok, let me add that inside a custom profile only for CI

lidavidm · 2023-09-19T20:43:30Z

(1) let's only run the checker on memory-core to start with

lidavidm · 2023-09-19T20:46:42Z

For https://github.com/apache/arrow/tree/main/java/format/src/main/java/org/apache/arrow/flatbuf

let's add a package-info.java with a @DefaultQualifiers annotation to mark everything in the package as nullable-by-default

danepitkin

Thanks for opening up this PR @davisusanibar!

I see there are a lot of Suppressed warnings. Are they truly all needed? (I haven't looked into all of them). I would like to see zero suppressed warnings if possible. Otherwise, the checker doesn't feel as useful.

danepitkin · 2023-10-05T18:22:21Z

java/pom.xml

@@ -46,6 +46,7 @@
    <errorprone.javac.version>9+181-r4173-1</errorprone.javac.version>
    <error_prone_core.version>2.16</error_prone_core.version>
    <maven-compiler-plugin.version>3.10.1</maven-compiler-plugin.version>
+    <checker.framework.version>3.38.0</checker.framework.version>


v3.39.0 just released a few days ago, which adds additional support for Java 21 compilation

Should we try v3.39.0? It might be worth rebasing so that the Java 21 CI job can run to see if its required.

danepitkin · 2023-10-05T18:28:31Z

java/memory/memory-core/src/main/java/org/apache/arrow/memory/util/StackTrace.java

@@ -29,6 +29,7 @@ public class StackTrace {
  /**
   * Constructor. Captures the current stack trace.
   */
+  @SuppressWarnings("nullness:assignment") //incompatible types in assignment


Can you expand on why this is suppressed? nullness:assignment warning makes me think we can just check the length of the stack trace to make sure we are assigning valid entries.

Can you expand on why this is suppressed? nullness:assignment warning makes me think we can just check the length of the stack trace to make sure we are assigning valid entries.

It is not necessary, I will update it.

danepitkin · 2023-10-05T18:30:50Z

java/memory/memory-core/src/main/java/org/apache/arrow/memory/Accountant.java

@@ -286,7 +288,7 @@ public void setLimit(long newLimit) {
   *
   * @return Currently allocate memory in bytes.
   */
-  public long getAllocatedMemory() {
+  public long getAllocatedMemory(Accountant this) {


Hmm, is this needed? It's not used.

Hmm, is this needed? It's not used.

Not needed, I am going to delete that

davisusanibar · 2023-10-06T18:13:28Z

Thanks for opening up this PR @davisusanibar!

I see there are a lot of Suppressed warnings. Are they truly all needed? (I haven't looked into all of them). I would like to see zero suppressed warnings if possible. Otherwise, the checker doesn't feel as useful.

Hi @danepitkin yes this is possible to suppress some warnings such as @SuppressWarnings("nullness:dereference.of.nullable") adding more if/else logic.

Let me add that logic explicitly.

lidavidm · 2024-01-08T18:03:07Z

java/memory/memory-core/src/main/java/org/apache/arrow/memory/Accountant.java

+      if (parent == null) {
+        throw new IllegalArgumentException(String.valueOf("Accountant(parent) must not be null"));
+      }


Why do you use String.valueOf?

Suggested change

if (parent == null) {

throw new IllegalArgumentException(String.valueOf("Accountant(parent) must not be null"));

}

Preconditions.checkArgument(parent != null, "parent must not be null");

As I mention at #37723 (comment) (1st option) if I added Preconditions.xxx then that finished also with errors, then, is needed to add inline throw validation.

Finished OK:

final BufferLedger oldLedger = map.remove(allocator); if (oldLedger == null) { throw new IllegalArgumentException(String.valueOf("BufferLedger(oldLedger) must not be null")); } BufferAllocator oldAllocator = oldLedger.getAllocator();

Finished with errors:

final BufferLedger oldLedger = map.remove(allocator); Preconditions.checkArgument(oldLedger != null, "BufferLedger(oldLedger) must not be null"); BufferAllocator oldAllocator = oldLedger.getAllocator(); Error message: error: [dereference.of.nullable] dereference of possibly-null reference oldLedger

You need to annotate the Preconditions methods so that Checker Framework recognizes them...

lidavidm · 2024-01-08T18:03:32Z

java/memory/memory-core/src/main/java/org/apache/arrow/memory/AllocationManager.java

@@ -48,8 +49,9 @@ public abstract class AllocationManager {
  // This is mostly a semantic constraint on the API user: if the reference count reaches 0 in the owningLedger, then
  // there are not supposed to be any references through other allocators. In practice, this doesn't do anything
  // as the implementation just forces ownership to be transferred to one of the other extant references.
-  private volatile BufferLedger owningLedger;
+  private volatile @Nullable BufferLedger owningLedger;


@davisusanibar can you review this?

java/memory/memory-core/src/main/java/org/apache/arrow/memory/AllocationManager.java

java/memory/memory-core/src/main/java/org/apache/arrow/memory/BufferLedger.java

lidavidm · 2024-01-08T18:06:21Z

java/memory/memory-core/src/main/java/org/apache/arrow/memory/BaseAllocator.java

  private final RoundingPolicy roundingPolicy;
-  private final AllocationManager.Factory allocationManagerFactory;
+  private final AllocationManager.@NonNull Factory allocationManagerFactory;


@davisusanibar can you follow up here? File an issue?

lidavidm · 2024-01-08T18:06:34Z

java/memory/memory-core/src/main/java/org/apache/arrow/memory/util/CheckerFrameworkUtil.java

+   * @param eval The assertion to be evaluated at compile time
+   */
+  @AssertMethod
+  public static void assertMethod(boolean eval) {


I don't see any change.

java/memory/memory-core/src/main/java/org/apache/arrow/memory/BaseAllocator.java

lidavidm · 2024-01-08T21:17:39Z

@github-actions crossbow submit java

github-actions · 2024-01-08T21:20:03Z

Revision: f92f376

Submitted crossbow builds: ursacomputing/crossbow @ actions-b35c84940e

Task	Status
java-jars
verify-rc-source-java-linux-almalinux-8-amd64
verify-rc-source-java-linux-conda-latest-amd64
verify-rc-source-java-linux-ubuntu-20.04-amd64
verify-rc-source-java-linux-ubuntu-22.04-amd64
verify-rc-source-java-macos-amd64

conbench-apache-arrow · 2024-01-10T21:54:45Z

After merging your PR, Conbench analyzed the 5 benchmarking runs that have been run so far on merge-commit ccc79e9.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 3 possible false positives for unstable benchmarks that are known to sometimes produce them.

…pache#37723) ### Rationale for this change Closes: apache#37286 ### What changes are included in this PR? Initial support for: - Use the Checker Framework to enhances Java’s type system to make it more powerful and useful. Planning to start with [Nullness Checker](https://checkerframework.org/manual/#nullness-checker) ### Are these changes tested? These are the activities involved on this PR: - [x] Configure the Checker Framework - [x] Treat checker errors as warnings initially - [x] Applying Nullness Checker annotation as needed: @ NonNull / @ Nullable - [x] Check if building timer increases after this checker is added - [x] Fixes for code review ### Are there any user-facing changes? Yes * Closes: apache#37286 Lead-authored-by: david dali susanibar arce <davi.sarces@gmail.com> Co-authored-by: David Susanibar Arce <dsusanibar@Voltron.local> Co-authored-by: David Susanibar Arce <davi.sarces@gmail.com> Signed-off-by: David Li <li.davidm96@gmail.com>

davisusanibar added 3 commits September 12, 2023 10:00

feat: initial checker support

428641e

Merge branch 'main' into null-ability-impl

cd4a048

feat: initial checker support

4ac2002

github-actions bot added Component: Java awaiting review Awaiting review labels Sep 14, 2023

davisusanibar changed the title ~~Null ability impl~~ GH-37286: [Java] Start adding nullability/nullness annotations Sep 14, 2023

davisusanibar added 5 commits September 14, 2023 10:05

Merge branch 'main' into null-ability-impl

27f7046

fix: tmp

daaf63d

fix: review weekend

70146d1

fix: error jdk8 + version properties

c10b5d5

fix: error JDK11+, update error prone version

71aef53

lidavidm reviewed Sep 19, 2023

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting review Awaiting review labels Sep 19, 2023

David Susanibar Arce added 3 commits September 27, 2023 09:50

fix: error scope

2d5f864

Merge branch 'main' into clean-j-house

ab54e2d

fix: add nullability checker framework

919b301

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Oct 2, 2023

fix: required nullable mark

0230b91

danepitkin reviewed Oct 5, 2023

View reviewed changes

danepitkin mentioned this pull request Oct 5, 2023

[Java] Remove Java 8 support in Arrow v18 #38051

Open

7 tasks

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Jan 8, 2024

davisusanibar requested a review from lidavidm January 8, 2024 18:00

lidavidm reviewed Jan 8, 2024

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Jan 8, 2024

fix: add checkerframewrok inside custom preconditions

4e011c0

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Jan 8, 2024

davisusanibar mentioned this pull request Jan 8, 2024

[Java][Memory][Core]: Initialize DEBUG variable in BaseAllocator class separately #39513

Open

davisusanibar requested review from lidavidm and vibhatha January 8, 2024 19:46

lidavidm reviewed Jan 8, 2024

View reviewed changes

java/memory/memory-core/src/main/java/org/apache/arrow/memory/BaseAllocator.java Outdated Show resolved Hide resolved

java/memory/memory-core/src/main/java/org/apache/arrow/memory/BaseAllocator.java Outdated Show resolved Hide resolved

github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Jan 8, 2024

fix: addressing PR comments

f92f376

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Jan 8, 2024

davisusanibar requested a review from lidavidm January 8, 2024 20:45

lidavidm approved these changes Jan 8, 2024

View reviewed changes

github-actions bot added awaiting merge Awaiting merge and removed awaiting change review Awaiting change review labels Jan 8, 2024

lidavidm merged commit ccc79e9 into apache:main Jan 9, 2024
17 checks passed

lidavidm removed the awaiting merge Awaiting merge label Jan 9, 2024

GH-37286: [Java] Start adding nullability/nullness annotations #37723

GH-37286: [Java] Start adding nullability/nullness annotations #37723

Conversation

davisusanibar commented Sep 14, 2023 • edited

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

github-actions bot commented Sep 14, 2023

github-actions bot commented Sep 14, 2023

davisusanibar commented Sep 19, 2023

lidavidm commented Sep 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davisusanibar commented Sep 19, 2023

lidavidm commented Sep 19, 2023

lidavidm commented Sep 19, 2023

danepitkin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danepitkin Oct 5, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davisusanibar commented Oct 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lidavidm commented Jan 8, 2024

github-actions bot commented Jan 8, 2024

conbench-apache-arrow bot commented Jan 10, 2024

davisusanibar commented Sep 14, 2023 •

edited

danepitkin Oct 5, 2023 •

edited