avaje / avaje-record-builder Goto Github PK

Or, propagating TYPE_USE annotations (#19) is insufficient.

Or, builders are really validators.

I've been fooling around with nullability analysis to get a sense of the state of the ecosystem primo 2024. For this experiment I'm using

• Maven, because JetBrains claims 60% of Java projects use Maven and because I hate it less than Gradle.
• Checker Framework's nullness checker, because it's well established, can run without a third-party host, and NullAway depends on it anyway.
• JSpecify, because it's ~the only general TYPE_USE nullability annotation library and it specifically lacks certain tool specific support.
• avaje-record-builder, because avaje-record-builder has some recognition of TYPE_USE annotations, and anyway, because I suspect any dialogue about the state of third-party nullability analysis in combination with a record builder generator has a better chance of being fruitful here no matter the outcome.

I'm filing this issue for posterity to record that this combination of tools presently is not workable. The essence of why is the same as the essence of uber/NullAway#121, the outcome of which is not clear to me. I am aware of the extensive history represented by #19 and Randgalt/record-builder#111.

There are variations I have not experimented with much or at all and so cannot really comment on:

• Checker Framework standalone execution, which offers flexible control of what to analyse when and possibly trivially allows for ignoring problematic elements like generated code.
• Checker Framework via Gradle, but I get the sense that Checker Framework pushes Gradle hard and that the Gradle integration is richer than the Maven integration.
• NullAway, which itself is much laxer than Checker Framework, and via Error Prone offers flexible analysis inclusion and exclusion rules even in Maven integration.
• io.soabase.record-builder:record-builder:38, which is unable to recognize TYPE_USE annotations.

Given a record

package example; // avaje/avaje-record-builder#41

@io.avaje.recordbuilder.RecordBuilder
public record Nullity(
        @org.jspecify.annotations.NonNull String a,
        @org.jspecify.annotations.Nullable String b) {}

avaje-record-builder-1.0 generates the moral equivalent of

package example;

import io.avaje.recordbuilder.Generated;
import java.util.function.Consumer;
import org.jspecify.annotations.NonNull;
import org.jspecify.annotations.Nullable;

@Generated("avaje-record-builder")
public class NullityBuilder {
  private String a;
  private String b;

  private NullityBuilder() {} // initialization.fields.uninitialized

  private NullityBuilder(@NonNull String a, @Nullable String b) {
    this.a = a;
    this.b = b; // assignment
  }

  public static NullityBuilder builder() { return new NullityBuilder(); }

  public static NullityBuilder builder(Nullity from) { return new NullityBuilder(from.a(), from.b()); }

  public Nullity build() { return new Nullity(a, b); }

  public NullityBuilder a(@NonNull String a) {
      this.a = a;
      return this;
  }

  public NullityBuilder b(@Nullable String b) {
      this.b = b; // assignment
      return this;
  }
}

When compiled (POM below), Checker Framework produces three errors:

[ERROR] /.../src/avaje-record-builder-nullity/target/generated-sources/annotations/example/NullityBuilder.java:[14,10] error: [initialization.fields.uninitialized] the constructor does not initialize fields: a, b
[ERROR] /.../src/avaje-record-builder-nullity/target/generated-sources/annotations/example/NullityBuilder.java:[19,13] error: [assignment] incompatible types in assignment.
  found   : @Initialized @Nullable String
  required: @Initialized @NonNull String
[ERROR] /.../src/avaje-record-builder-nullity/target/generated-sources/annotations/example/NullityBuilder.java:[51,15] error: [assignment] incompatible types in assignment.
  found   : @Initialized @Nullable String
  required: @Initialized @NonNull String

The line numbers refer to the canonical generated lines, not the compressed version shown above. The compressed version is annotated with the error codes.

There are two errors in the generated code, one "obvious" and one subtle. Other limitations in Checker Framework, Checker Framework's Maven integration, and avaje-record-builder combine to make those two errors somewhere between difficult and impossible to work around (I have not found a way).

1. assignment: Although the NullityBuilder b(@Nullable String b) receiver correctly had org.jspecify.annotations.Nullable propagated from the Nullity.b component the annotation was not propagated to the NullityBuilder.b field, which Checker Framework consequently resolves to @NonNull. @Nullable should have been propagated all the way to the field.
   • This is the "obvious" error as far as Checker Framework's rules go.
2. initialization.fields.uninitialized: The builder() factory instantiates a builder in its default state according to the JLS, i.e. with all fields uninitialized and implicitly null. This is valid Java but Checker Framework effectively resolves those fields to @NonNull, which is a contradiction and therefore rejected. This behaviour is consistent with the various rules in play but the rules that are in play are not representative of a builder's role. The trouble is that NullityBuilder.[@o.j.a.NonNull] String a is an incorrect declaration: NullityBuilder.a must actually be the moral equivalent of @o.j.a.Nullable @javax.validation.constraints.NotNull String, comparable to Checker Framework's @org.checkerframework.checker.nullness.qual.MonotonicNonNull. This is because the builder is stateful and cannot start out in a state of NullityBuilder.a being non-null. A knock-on consequence is that the builder must validate that the values passed to the record's @NonNull components are in fact non-null. Checker Framework makes this extra difficult by rejecting Objects.requireNonNull.
   • This is the subtle error.
   • It is reasonable, and perhaps technically necessary, but not truly important, that the NullityBuilder a(@NonNull String a) receiver had the @NonNull annotation propagated. It is reasonable because passing a @Nullable value is clearly nonsensical but it is unimportant because NullityBuilder.a cannot practically be @NonNull.
   • Arguably NullityBuilder.a and all other @NonNull record components could be promoted to parameters on the builder() method. This would undermine the utility of the resulting builder, however.
   • See also https://checkerframework.org/manual/#initialization-warnings-constructor

POM:

<?xml version="1.0" encoding="UTF-8"?>
<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
  <modelVersion>4.0.0</modelVersion>
  <groupId>example</groupId>
  <artifactId>avaje-record-builder-nullity</artifactId>
  <version>1.0-SNAPSHOT</version>
  <name>avaje-record-builder-nullity</name>

  <properties>
    <checker.version>3.42.0</checker.version>
    <maven-compiler-plugin.version>3.11.0</maven-compiler-plugin.version>
    <maven.compiler.release>17</maven.compiler.release>
    <record.builder.version>1.0</record.builder.version>
    <project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
    <maven.compiler.source>17</maven.compiler.source>
    <maven.compiler.target>17</maven.compiler.target>
  </properties>

  <dependencies>
    <dependency>
      <groupId>io.avaje</groupId>
      <artifactId>avaje-record-builder</artifactId>
      <version>${record.builder.version}</version>
      <scope>provided</scope>
    </dependency>
    <dependency>
      <groupId>org.jspecify</groupId>
      <artifactId>jspecify</artifactId>
      <version>0.3.0</version>
    </dependency>
    <dependency>
      <groupId>org.checkerframework</groupId>
      <artifactId>checker</artifactId>
      <version>${checker.version}</version>
    </dependency>
    <dependency>
      <groupId>org.checkerframework</groupId>
      <artifactId>checker-qual</artifactId>
      <version>${checker.version}</version>
    </dependency>
    <dependency>
      <groupId>org.checkerframework</groupId>
      <artifactId>checker-util</artifactId>
      <version>${checker.version}</version>
    </dependency>
  </dependencies>

  <build>
    <plugins>
      <plugin>
        <groupId>org.apache.maven.plugins</groupId>
        <artifactId>maven-compiler-plugin</artifactId>
        <version>${maven-compiler-plugin.version}</version>
        <configuration>
          <compilerArgs>
          </compilerArgs>
          <annotationProcessorPaths>
            <path>
              <groupId>io.avaje</groupId>
              <artifactId>avaje-record-builder</artifactId>
              <version>${record.builder.version}</version>
            </path>
          </annotationProcessorPaths>
          <annotationProcessors>
            <annotationProcessor>io.avaje.recordbuilder.internal.RecordProcessor</annotationProcessor>
          </annotationProcessors>
        </configuration>
      </plugin>
    </plugins>
  </build>

  <profiles>
    <profile>
      <id>checkerframework</id>
      <activation>
        <jdk>[9,)</jdk>
      </activation>
      <build>
        <plugins>
          <plugin>
            <artifactId>maven-compiler-plugin</artifactId>
            <executions>
              <execution>
                <id>default-compile</id>
                <configuration>
                  <fork>true</fork>
                  <annotationProcessorPaths combine.children="append">
                    <path>
                      <groupId>org.checkerframework</groupId>
                      <artifactId>checker</artifactId>
                      <version>${checker.version}</version>
                    </path>
                  </annotationProcessorPaths>
                  <annotationProcessors combine.children="append">
                    <annotationProcessor>org.checkerframework.checker.nullness.NullnessChecker</annotationProcessor>
                  </annotationProcessors>
                  <compilerArgs combine.children="append">
                    <arg>-Xmaxerrs</arg>
                    <arg>10000</arg>
                    <arg>-Xmaxwarns</arg>
                    <arg>10000</arg>
                    <arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED</arg>
                    <arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.code=ALL-UNNAMED</arg>
                    <arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.file=ALL-UNNAMED</arg>
                    <arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.main=ALL-UNNAMED</arg>
                    <arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.model=ALL-UNNAMED</arg>
                    <arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.processing=ALL-UNNAMED</arg>
                    <arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.tree=ALL-UNNAMED</arg>
                    <arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED</arg>
                    <arg>-J--add-opens=jdk.compiler/com.sun.tools.javac.comp=ALL-UNNAMED</arg>
                  </compilerArgs>
                </configuration>
              </execution>
            </executions>
          </plugin>
        </plugins>
      </build>
    </profile>
  </profiles>

</project>

I'm thinking that "by default" collection types should be wrapped via e.g. Collections.unmodifiableList() and the like at build time.

The thinking is that a "record graph" should be as unmodifiable as possible. I'm actually thinking this should be the default with perhaps the option to turn it off.

handle cases like:

record Tuple2<T1, T2>(T1 t1, T2 t2) {}

My preference is to actually NOT have Javadoc in the generated class.

That sounds a bit weird I know, my thinking is that the builder pattern is SOOO predictable that the Javadoc does not actually add anything or help the person reading the code. In the "less is more" thinking it is actually better to not have the Javadoc at all.

Something like:

  @Test
  void transform() {
    var original = Order.builder()
      .id(42)
      .customer(Customer.builder().id(99).name("fred").build())
      .addLines(new OrderLine(42, product93, 1034))
      .addLines(new OrderLine(42, product93, 1034))
      .build();

    var transformedOrder =
      Order
        .from(original) // <!-- The Original Order that is being transformed
        .transform(orderBuilder -> { // <!-- transformer ....

          if (orderBuilder.status() == Order.Status.NEW) {
            orderBuilder.status(Order.Status.COMPLETE);

            // transform a nested collection
            List<OrderLine> newLines = orderBuilder.lines().stream()
              .filter(line -> line.id() < 43)
              .toList();

            orderBuilder.lines(newLines);
          }
        })
        .build();

  }

Thoughts: I feel like there are 2 main cases:

• Simple builder case .... new builder + set .. + set ... .build()
• Looking to transform an existing record ... in this case we generally need the getter methods on the builder as generally there is business logic that needs the existing state

In avaje-record-builder-1.0 a @RecordBuilder record in the unnamed package generates a builder FQN that is invalid:

@io.avaje.recordbuilder.RecordBuilder
public record UnnamedPackageRecord(int i) {}

Compilation failure
java.io.UncheckedIOException: javax.annotation.processing.FilerException: Illegal name .UnnamedPackageRecordBuilder
	at io.avaje.recordbuilder.internal.RecordProcessor.readElement(RecordProcessor.java:112)
	at io.avaje.recordbuilder.internal.RecordProcessor.readElement(RecordProcessor.java:89)
	at io.avaje.recordbuilder.internal.RecordProcessor.process(RecordProcessor.java:67)
	at jdk.compiler/com.sun.tools.javac.processing.JavacProcessingEnvironment.callProcessor(JavacProcessingEnvironment.java:1023)
	at jdk.compiler/com.sun.tools.javac.processing.JavacProcessingEnvironment.discoverAndRunProcs(JavacProcessingEnvironment.java:939)
	at jdk.compiler/com.sun.tools.javac.processing.JavacProcessingEnvironment$Round.run(JavacProcessingEnvironment.java:1267)
	at jdk.compiler/com.sun.tools.javac.processing.JavacProcessingEnvironment.doProcessing(JavacProcessingEnvironment.java:1382)
	at jdk.compiler/com.sun.tools.javac.main.JavaCompiler.processAnnotations(JavaCompiler.java:1234)
	at jdk.compiler/com.sun.tools.javac.main.JavaCompiler.compile(JavaCompiler.java:916)
	at jdk.compiler/com.sun.tools.javac.main.Main.compile(Main.java:317)
	at jdk.compiler/com.sun.tools.javac.main.Main.compile(Main.java:176)
	at jdk.compiler/com.sun.tools.javac.Main.compile(Main.java:64)
	at jdk.compiler/com.sun.tools.javac.Main.main(Main.java:50)
Caused by: javax.annotation.processing.FilerException: Illegal name .UnnamedPackageRecordBuilder
	at jdk.compiler/com.sun.tools.javac.processing.JavacFiler.checkName(JavacFiler.java:703)
	at jdk.compiler/com.sun.tools.javac.processing.JavacFiler.checkNameAndExistence(JavacFiler.java:723)
	at jdk.compiler/com.sun.tools.javac.processing.JavacFiler.createSourceOrClassFile(JavacFiler.java:498)
	at jdk.compiler/com.sun.tools.javac.processing.JavacFiler.createSourceFile(JavacFiler.java:435)
	at io.avaje.recordbuilder.internal.APContext.createSourceFile(APContext.java:207)
	at io.avaje.recordbuilder.internal.RecordProcessor.readElement(RecordProcessor.java:101)
	... 12 more

@SentryMan I'm not sure if you stumbled on my rant with the original record builder author on the need to propagate TYPE_USE annotations but I recommend you do that.

The basic problem is you cannot not just toString on the TypeMirror to get the correct representation. I believe @bowbahdoe (who has a similar library) filed a JDK bug on it.

Luckily I have a solution you can just copy from JStachio: https://github.com/jstachio/jstachio/blob/main/compiler/apt/src/main/java/io/jstach/apt/internal/util/ToStringTypeVisitor.java

The top static method is what you want to call. Yes that class is internal so doc sucks.

So just for clarity:

@GenerateBuilder // or whatever the annotation is
record Blah(@Nullable String foo){}

Let us assume we are using JSpecify.

When you produce all the setters for the builder it should be:

public Builder foo(java.lang. @org.jspecify.annotations.Nullable String) {
...
}

If you call ToStringTypeVistor.toCodeSafeString(someTypeMirrorFromRecordAccessor)

BTW the above is one reason I do not manage imports on code generation (e.g. FQN everything). It is nontrivial to recreate the correct TYPE_USE annotation declaration and is probably why even the JDK has a bug.

You will have to modify my code heavily if you plan on managing imports.

If I have time later this week I can put in a PR for you.

On another tangent I think I remember you or @rbygrave using Eclipse or perhaps the compiler. I have a great deal of the annotation processing API as well as the JDK API nullness annotated aka Eclipse External Annotations aka EEA. You just copy this directory: https://github.com/jstachio/jstachio/tree/main/etc/eea

Then just copy the checkerframework, errorprone (optional), and eclipse profiles from this pom:
https://github.com/jstachio/jstachio/blob/main/api/jstachio/pom.xml

You will probably have to twiddle some config but that will get you headless null analysis and the Eclipse version is mostly equivalent to Checker (IMO better than Checker because Checker just assumes the entire JDK is nonnull where as eclipse is the opposite).

JLS allows a record with an empty component list -- although such a record has... limited utility, it is legal.

Creating a record builder for a record with an empty component list is fairly useless and inane but not technically prohibited either.

If an avaje-record-build-1.0 @RecordBuilder is applied to a record with an empty component list the annotation processor generates an error and aborts, refusing to produce the requested builder. The record

@io.avaje.recordbuilder.RecordBuilder
public record EmptyComponentList() {}

yields the error

error: Builders can only be generated for record classes

This error message is misleading. The JLS does not define a record as a class with a non-empty component list so the failure reason the annotation processor identifies is incorrect. Class.isRecord() informs whether a type is a record, and specifically raises the possibility that the component list may be empty.

I think refusing to produce the requested builder is an acceptable outcome per the above, if that's what avaje-record-builder wishes to do, but in that case with a dedicated error message. However, it is also technically a special casing, and even though a builder for an empty record appears at first useless I would not rule out use cases e.g. for consistent introspection. In any case, while I am convinced the current implementation is an accidental limitation (not a design decision), personally I would not bother to pursue such a design decision.

My thinking is that I didn't actually expect or want getters on the builder. That is, the builder when used the majority of the time just to build fairly immediately and is not passed around.

If the occasion occurs where getters are needed because the builder is going to be "passed around" then I'm thinking more of the "opt in - I want getters" to be set.

e.g. @RecordBuilder(getters=true)