AutoBuilder with a generic parameter should be parametric in underlying type's nullability

[kennknowles]

Consider this use of AutoBuilder:

@AutoBuilder(callMethod = "of")
public abstract static class Builder<T> {

  @Override
  public abstract Builder<T> setValue(T value);

  ...
} 

(distilled from apache/beam#34902 but I will likely have changed it with a workaround by the time you click)

The type parameter T can legitimately be instantiated with any nullability: @NonNull String, @Nullable String, etc. But the generated builder class throws NullPointerException in response to setValue(null). Instead, the builder class should be fully agnostic as to the nullability (or other qualifiers) for type T.

[eamonnmcmanus]

AutoBuilder follows the JSpecify convention whereby a type variable can be instantiated with a @Nullable type only if it has a @Nullable bound. If you declare your builder as

@AutoBuilder(callMethod = "of")
public abstract static class Builder<T extends @Nullable Object> {...}

then it should omit null checks for parameters of type T.

Let me know if this doesn't work.

[kennknowles]

This makes sense. That is the default bound for checkerframework (since it encompasses all types). I did not realize JSpecify had a different position (which is also a defensible default, making less-well-behaved typed opt-in, though this propagates through a code base and will cause issues at the membrane with other libraries)

But when I changed it to Builder<T extends @Nullable Object> the null checks were still in the generated code.

I pushed up a repro maybe you can see what the issue is: https://github.com/kennknowles/beam/blob/2b5b799f8e9ffd20483f715b585b10981706769f/sdks/java/core/src/test/java/org/apache/beam/sdk/util/WindowedValueTest.java#L58

You can run the test with:

./gradlew :sdks:java:core:test --tests WindowedValueTest.testNullBuilder

I get the result:

java.lang.NullPointerException: Null value
	at org.apache.beam.sdk.values.AutoBuilder_WindowedValues_Builder.setValue(AutoBuilder_WindowedValues_Builder.java:27)
	at org.apache.beam.sdk.util.WindowedValueTest.testNullBuilder(WindowedValueTest.java:58)
   ...

[eamonnmcmanus]

Thanks for the repro! I see the same thing. I'm not sure exactly what's happening. This test is supposed to cover something very similar.

[kennknowles]

Yea it does seem like the same construct. One thing I toyed with is that there are layers of inheritance - couple levels of interface. In the exact commit I linked you to, we have WindowedValue<T> extended by OutputBuilder<T extends @Nullable Object>. I guess I would expect that to cause the null check to be inserted based on the lowest class in the hierarchy. But I would have sworn that even adding the upper bound to both I got the null check generated...

[kennknowles]

Confirmed that with the same upper bound on WindowedValue and OutputBuilder the exception is the same.

[kennknowles]

• Also added <T extends @Nullable Object> to the type parameter for the of method that is called and got the same failure.
• Next attempt: added the annotation to the overload of of that is not called, but no luck.

[cpovirk]

I confirm that nothing seems to help. I even tried a simpler Builder type without luck.

I also wondered if https://bugs.openjdk.org/browse/JDK-8225377 might be involved. However, I see the problem even when I run Gradle with an internal testing version of JDK 21 that contains the fix for that bug. Now, I don't understand Gradle well enough to be confident that that I'm actually using that compiler for the javac invocations. Still, my hope would be that my simpler Builder type would have sidestepped JDK-8225377 entirely. I supposed it's still possible that Gradle is doing enough clever things to trigger it, but that's not my immediate suspicion.

Another idea that seems not to be panning out: The test mentioned above notes a problem with builds that use the javac from JDK 8. Could that be happening here because of some Gradle configuration? I notice that, if I add a reference to Objects.requireNonNullElse, which was added in Java 9, then the build fails with a "cannot find symbol" error. That could mean that it's using JDK 8, or it could mean that the build merely passes --release 8 (which is generally what I'd recommend). If I pass --debug to Gradle, I see a command that includes --release, which I think implies a JDK greater than 8. Hmm.

I guess that the next thing to do would be to extract the code from the project and try compiling that more directly. I am probably going to set this down, though.