Nullable field nondeterministically has null check inserted anyhow

[kennknowles]

Summary of issue: A field that is nullable is nondeterministically having a null check in the generated code, depending on what other build tasks occurred.

See https://lists.apache.org/thread/cdw4y50r3hl37z8y7x470m2mddclkqgv (from here you can gather more info from the thread)

Inline, paraphrasing the thread...

To reproduce:

git clone https://github.com/apache/beam
cd beam
git checkout 4ffeae4d2b800f2df36d2ea2eab549f2204d5691~1
./gradlew :runners:direct-java:compileJava
less
./runners/direct-java/build/generated/sources/annotationProcessor/java/main/org/apache/beam/runners/direct/AutoValue_ImmutableListBundleFactory_CommittedImmutableListBundle.java

git checkout 4ffeae4d2b800f2df36d2ea2eab549f2204d5691
./gradlew :runners:direct-java:compileJava
less
./runners/direct-java/build/generated/sources/annotationProcessor/java/main/org/apache/beam/runners/direct/AutoValue_ImmutableListBundleFactory_CommittedImmutableListBundle.java

./gradlew :runners:direct-java:compileJava --rerun-tasks
less
./runners/direct-java/build/generated/sources/annotationProcessor/java/main/org/apache/beam/runners/direct/AutoValue_ImmutableListBundleFactory_CommittedImmutableListBundle.java

The class at https://github.com/apache/beam/blob/1dff59b4ff26310f88f927edfcf44709ed6ea9c2/runners/direct-java/src/main/java/org/apache/beam/runners/direct/ImmutableListBundleFactory.java#L130

abstract static class CommittedImmutableListBundle<T> implements CommittedBundle<T> {
    public static <T> CommittedImmutableListBundle<T> create(
        @Nullable PCollection<T> pcollection,
        StructuralKey<?> key,
        Iterable<WindowedValue<T>> committedElements,
        Instant minElementTimestamp,
        Instant synchronizedCompletionTime) {
      return new AutoValue_ImmutableListBundleFactory_CommittedImmutableListBundle<>(
          pcollection, key, committedElements, minElementTimestamp, synchronizedCompletionTime);
    }
  ...
}

extends https://github.com/apache/beam/blob/1dff59b4ff26310f88f927edfcf44709ed6ea9c2/runners/direct-java/src/main/java/org/apache/beam/runners/direct/CommittedBundle.java#L35

interface CommittedBundle<T> extends Bundle<T, PCollection<T>> {
  /** Returns the PCollection that the elements of this bundle belong to. */
  @Override
  @Nullable
  PCollection<T> getPCollection();

  ...
}

In all cases the PCollection field should be nullable.

In a "good" build, the AutoValue generated code constructor does not check for null

AutoValue_ImmutableListBundleFactory_CommittedImmutableListBundle(
    @Nullable PCollection<T> PCollection,
    StructuralKey<?> key,
    Iterable<WindowedValue<T>> elements,
    Instant minimumTimestamp,
    Instant synchronizedProcessingOutputWatermark) {
  this.PCollection = PCollection;

in a 'bad' build, the autovalue java is re-generated, but has an added nullness check:

AutoValue_ImmutableListBundleFactory_CommittedImmutableListBundle(
    PCollection<T> PCollection,
    StructuralKey<?> key,
    Iterable<WindowedValue<T>> elements,
    Instant minimumTimestamp,
    Instant synchronizedProcessingOutputWatermark) {
  if (PCollection == null) {
    throw new NullPointerException("Null PCollection");
  }
  this.PCollection = PCollection;

and then with --rerun-tasks it gets re-re-generated without the nullness check.

[eamonnmcmanus]

The Java compiler has historically had a number of bugs with type-use annotations, like org.checkerframework.checker.nullness.qual.Nullable. Specifically, it reports them correctly to annotation processors like AutoValue if the code in question is being compiled at the same time, but it doesn't always report them correctly if they are in already-compiled code. To see whether this is indeed the problem, you could try using javax.annotation.Nullable instead. That's not a type-use annotation, and I see you already have javax.annotation.Nonnull in the same source file so presumably this should work.

If this is indeed the problem, I'm afraid I don't have a good solution other than that.

@cushon may have further input.

[cushon]

I agree this sounds like something that could happen if the incremental compilation was sometimes compiling both classes as source, and sometimes compiling just one of them and reading the other as a .class file. If that's what's happening, it's an interaction between gradle's incremental compilation behaviour and this javac issue: https://bugs.openjdk.org/browse/JDK-8225377

[kennknowles]

Oh interesting! Seems likely

[kennknowles]

Ah just because this came into my inbox today, I should say that we are "standardized" on the checkerframework annotation because it is more robustly applicable in different contexts - but as an open source project, other annotations get slipped in from time to time and can last somewhat indefinitely. We haven't added checkstyle rules to forbid them (yet). If there were one best way to have type qualifier annotations that are able to be used in all applicable contexts, we'd go for it.

[cushon]

Note that if JDK-8225377 was part of the problem here, that bug was fixed in JDK 22 and newer, and will be backported to the next JDK 21 update release (21.0.8, which will be released in July).

[kennknowles]

I'm not certain if JDK-8225377 is impacting this issue. Beam is nearly at the point of being able to fully decouple our Java version so we can build with the latest while targeting the oldest. I will try to reproduce after that.