Generated types not correctly imported in generated builders when in separate packages

[brandondahler]

Problem

When an annotation processor generated type (RecordBuilder or otherwise) is referenced by a record configured to generate a builder, the builder code generated by @RecordBuilder does not generate the required import statement for that referenced type when the referenced type is in a different package.

Reproduction

For example, if we have a simple Parent class configured to generate ParentBuilder in a root package:

package io.soabase.recordbuilder.test.generated;

import io.soabase.recordbuilder.core.RecordBuilder;

@RecordBuilder
public record Parent(String foo) {

}

and a Child class configured to generate a ChildBuilder in a child package:

package io.soabase.recordbuilder.test.generated.nested;

import io.soabase.recordbuilder.core.RecordBuilder;
import io.soabase.recordbuilder.test.generated.ParentBuilder;

@RecordBuilder
public record Child(ParentBuilder parentBuilder) {

}

Expected behavior

The generated ChildBuilder java file contains an import io.soabase.recordbuilder.test.generated.ParentBuilder; line in its source and therefore compiles successfully.

Actual results

The generated ChildBuilder code will incorrectly accidentally omit the import statement required for ParentBuilder:

// Auto generated by io.soabase.recordbuilder.core.RecordBuilder: https://github.com/Randgalt/record-builder
package io.soabase.recordbuilder.test.generated.nested;

import java.util.AbstractMap;
import java.util.Map;
import java.util.Objects;
import java.util.function.Consumer;
import java.util.stream.Stream;
import javax.annotation.processing.Generated;

@Generated("io.soabase.recordbuilder.core.RecordBuilder")
public class ChildBuilder {
    private ParentBuilder parentBuilder;

    @Generated("io.soabase.recordbuilder.core.RecordBuilder")
    private ChildBuilder() {
    }

    <SNIP>
}

Compilation returns an error on only on the generated ChildBuilder code:

[ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.14.0:compile (default-compile) on project record-builder-test: Compilation failure: Compilation failure: 
[ERROR] <SNIP>/record-builder/record-builder-test/target/generated-sources/annotations/io/soabase/recordbuilder/test/generated/nested/ChildBuilder.java:[13,13] cannot find symbol
[ERROR]   symbol:   class ParentBuilder
[ERROR]   location: class io.soabase.recordbuilder.test.generated.nested.ChildBuilder
[ERROR] <SNIP>/record-builder/record-builder-test/target/generated-sources/annotations/io/soabase/recordbuilder/test/generated/nested/ChildBuilder.java:[20,26] cannot find symbol
[ERROR]   symbol:   class ParentBuilder
[ERROR]   location: class io.soabase.recordbuilder.test.generated.nested.ChildBuilder
[ERROR] <SNIP>/record-builder/record-builder-test/target/generated-sources/annotations/io/soabase/recordbuilder/test/generated/nested/ChildBuilder.java:[28,31] cannot find symbol
[ERROR]   symbol:   class ParentBuilder
[ERROR]   location: class io.soabase.recordbuilder.test.generated.nested.ChildBuilder
[ERROR] <SNIP>/record-builder/record-builder-test/target/generated-sources/annotations/io/soabase/recordbuilder/test/generated/nested/ChildBuilder.java:[95,39] cannot find symbol
[ERROR]   symbol:   class ParentBuilder
[ERROR]   location: class io.soabase.recordbuilder.test.generated.nested.ChildBuilder
[ERROR] <SNIP>/record-builder/record-builder-test/target/generated-sources/annotations/io/soabase/recordbuilder/test/generated/nested/ChildBuilder.java:[104,12] cannot find symbol
[ERROR]   symbol:   class ParentBuilder
[ERROR]   location: class io.soabase.recordbuilder.test.generated.nested.ChildBuilder
[ERROR] <SNIP>/record-builder/record-builder-test/target/generated-sources/annotations/io/soabase/recordbuilder/test/generated/nested/ChildBuilder.java:[117,9] cannot find symbol
[ERROR]   symbol:   class ParentBuilder
[ERROR]   location: interface io.soabase.recordbuilder.test.generated.nested.ChildBuilder.With
[ERROR] <SNIP>/record-builder/record-builder-test/target/generated-sources/annotations/io/soabase/recordbuilder/test/generated/nested/ChildBuilder.java:[141,41] cannot find symbol
[ERROR]   symbol:   class ParentBuilder
[ERROR]   location: interface io.soabase.recordbuilder.test.generated.nested.ChildBuilder.With
[ERROR] <SNIP>/record-builder/record-builder-test/target/generated-sources/annotations/io/soabase/recordbuilder/test/generated/nested/ChildBuilder.java:[157,16] cannot find symbol
[ERROR]   symbol:   class ParentBuilder
[ERROR]   location: class io.soabase.recordbuilder.test.generated.nested.ChildBuilder._FromWith
[ERROR] -> [Help 1]

Additional details

Records which use an annotation-generated types which are in the same package do work correctly due to the import being unnecessary. That is, the following class with the previous example does not experience any compilation issues:

package io.soabase.recordbuilder.test.generated;

import io.soabase.recordbuilder.core.RecordBuilder;

@RecordBuilder
public record Sibling(ParentBuilder parentBuilder) {

}

Full reproduction diff

diff --git a/record-builder-test/src/main/java/io/soabase/recordbuilder/test/generated/Parent.java b/record-builder-test/src/main/java/io/soabase/recordbuilder/test/generated/Parent.java
new file mode 100644
index 0000000..29bb117
--- /dev/null
+++ b/record-builder-test/src/main/java/io/soabase/recordbuilder/test/generated/Parent.java
@@ -0,0 +1,8 @@
+package io.soabase.recordbuilder.test.generated;
+
+import io.soabase.recordbuilder.core.RecordBuilder;
+
+@RecordBuilder
+public record Parent(String foo) {
+
+}
diff --git a/record-builder-test/src/main/java/io/soabase/recordbuilder/test/generated/Sibling.java b/record-builder-test/src/main/java/io/soabase/recordbuilder/test/generated/Sibling.java
new file mode 100644
index 0000000..4d94214
--- /dev/null
+++ b/record-builder-test/src/main/java/io/soabase/recordbuilder/test/generated/Sibling.java
@@ -0,0 +1,8 @@
+package io.soabase.recordbuilder.test.generated;
+
+import io.soabase.recordbuilder.core.RecordBuilder;
+
+@RecordBuilder
+public record Sibling(ParentBuilder parentBuilder) {
+
+}
diff --git a/record-builder-test/src/main/java/io/soabase/recordbuilder/test/generated/nested/Child.java b/record-builder-test/src/main/java/io/soabase/recordbuilder/test/generated/nested/Child.java
new file mode 100644
index 0000000..6d17a1b
--- /dev/null
+++ b/record-builder-test/src/main/java/io/soabase/recordbuilder/test/generated/nested/Child.java
@@ -0,0 +1,9 @@
+package io.soabase.recordbuilder.test.generated.nested;
+
+import io.soabase.recordbuilder.core.RecordBuilder;
+import io.soabase.recordbuilder.test.generated.ParentBuilder;
+
+@RecordBuilder
+public record Child(ParentBuilder parentBuilder) {
+
+}

[brandondahler]

I'm looking into the specific technical details that are causing this.

Fundamentally it is related to the fact annotation processing is performed in rounds and doesn't guarantee that every type referenced by a class is fully resolved prior to scheduling that class for processing. One option is to delay generation of the builder to the next round when this scenario is encountered; however, at least in theory, the referencing class should have a full type name resolved for the target class even if that target class isn't generated yet.

[brandondahler]

Debugging the issue, it seems to be rooted in the usage of TypeName.get(TypeMirror) from JavaPoet inside of ElementUtils::getRecordClassType.

In the reproduction's case, ParentBuilder is resolved as a com.sun.tools.javac.code.Type$ErrorType whose symbol does not contain the package for the name. TypeName.get(TypeMirror) treats ErrorType instances in the same way that it treats normal class types. Most likely the only reasonable way to handle this will be to delay builder generation until there are no components which report that they are ErrorType TypeMirror instances.

[brandondahler]

I was able to fix the direct problem by adding a basic delay mechanism that I've seen in other annotation processors:

diff --git a/record-builder-processor/src/main/java/io/soabase/recordbuilder/processor/RecordBuilderProcessor.java b/record-builder-processor/src/main/java/io/soabase/recordbuilder/processor/RecordBuil
derProcessor.java
index c2cd5c5..cdb00d1 100644
--- a/record-builder-processor/src/main/java/io/soabase/recordbuilder/processor/RecordBuilderProcessor.java
+++ b/record-builder-processor/src/main/java/io/soabase/recordbuilder/processor/RecordBuilderProcessor.java
@@ -20,12 +20,15 @@ import io.soabase.recordbuilder.core.RecordBuilder;
 import io.soabase.recordbuilder.core.RecordBuilderGenerated;
 import io.soabase.recordbuilder.core.RecordInterface;
 
+import java.util.ArrayList;
+import java.util.List;
 import javax.annotation.processing.AbstractProcessor;
 import javax.annotation.processing.Filer;
 import javax.annotation.processing.Generated;
 import javax.annotation.processing.RoundEnvironment;
 import javax.lang.model.SourceVersion;
 import javax.lang.model.element.*;
+import javax.lang.model.type.TypeKind;
 import javax.tools.Diagnostic;
 import javax.tools.JavaFileObject;
 import java.io.IOException;
@@ -57,11 +60,23 @@ public class RecordBuilderProcessor extends AbstractProcessor {
     // will log an error if a duplicate deconstructor record is attempted
     private final Set<TypeName> createdDeconstructors = ConcurrentHashMap.newKeySet();
 
+    // Annotation-Element pairs may be delayed to subsequent processing rounds if child processors
+    // report that they are unable to handle generation in their current round.
+    private final List<DelayedItem> delayedItems = new ArrayList<>();
+
+    private record DelayedItem(TypeElement annotation, Element element) {
+    }
+
     @Override
     public boolean process(Set<? extends TypeElement> annotations, RoundEnvironment roundEnv) {
+        delayedItems.removeIf(delayedItem -> process(delayedItem.annotation(), delayedItem.element()));
+
         annotations.stream().filter(annotation -> annotation.getKind() == ElementKind.ANNOTATION_TYPE)
-                .forEach(annotation -> roundEnv.getElementsAnnotatedWith(annotation)
-                        .forEach(element -> process(annotation, element)));
+                .forEach(annotation -> roundEnv.getElementsAnnotatedWith(annotation).forEach(element -> {
+                    if (!process(annotation, element)) {
+                        delayedItems.add(new DelayedItem(annotation, element));
+                    }
+                }));
         return false;
     }
 
@@ -84,7 +99,7 @@ public class RecordBuilderProcessor extends AbstractProcessor {
         return SourceVersion.latest();
     }
 
-    private void process(TypeElement annotation, Element element) {
+    private boolean process(TypeElement annotation, Element element) {
         String annotationClass = annotation.getQualifiedName().toString();
         RecordBuilder.Template recordBuilderTemplate = annotation.getAnnotation(RecordBuilder.Template.class);
         RecordBuilder.DeconstructorTemplate deconstructorTemplate = annotation
@@ -92,7 +107,7 @@ public class RecordBuilderProcessor extends AbstractProcessor {
 
         if (annotationClass.equals(RECORD_BUILDER)) {
             var typeElement = (TypeElement) element;
-            processRecordBuilder(typeElement, getMetaData(typeElement), Optional.empty());
+            return processRecordBuilder(typeElement, getMetaData(typeElement), Optional.empty());
         } else if (annotationClass.equals(RECORD_INTERFACE)) {
             var typeElement = (TypeElement) element;
             processRecordInterface(typeElement, element.getAnnotation(RecordInterface.class).addRecordBuilder(),
@@ -107,11 +122,13 @@ public class RecordBuilderProcessor extends AbstractProcessor {
                 processRecordInterface((TypeElement) element, true, recordBuilderTemplate.options(), Optional.empty(),
                         true);
             } else {
-                processRecordBuilder((TypeElement) element, recordBuilderTemplate.options(), Optional.empty());
+                return processRecordBuilder((TypeElement) element, recordBuilderTemplate.options(), Optional.empty());
             }
         } else if (deconstructorTemplate != null) {
             processDeconstructor(element, deconstructorTemplate.value(), deconstructorTemplate.options());
         }
+
+        return true;
     }
 
     private RecordBuilder.Options getMetaData(Element element) {
@@ -132,6 +149,7 @@ public class RecordBuilderProcessor extends AbstractProcessor {
             if (includeHelper.isValid()) {
                 var packagePattern = ElementUtils.getStringAttribute(ElementUtils
                         .getAnnotationValue(includeHelper.getAnnotationValues(), "packagePattern").orElse(null), "*");
+
                 for (var typeElement : includeHelper.getClassTypeElements()) {
                     var packageName = buildPackageName(packagePattern, element, typeElement);
                     if (packageName != null) {
@@ -274,7 +292,7 @@ public class RecordBuilderProcessor extends AbstractProcessor {
                 deconstructorBuilder.build(), metaData.fileIndent(), metaData.fileComment());
     }
 
-    private void processRecordBuilder(TypeElement record, RecordBuilder.Options metaData,
+    private boolean processRecordBuilder(TypeElement record, RecordBuilder.Options metaData,
             Optional<String> packageName) {
         // we use string based name comparison for the element kind,
         // as the ElementKind.RECORD enum doesn't exist on JRE releases
@@ -283,15 +301,23 @@ public class RecordBuilderProcessor extends AbstractProcessor {
         if (!"RECORD".equals(record.getKind().name())) {
             processingEnv.getMessager().printMessage(Diagnostic.Kind.ERROR, "RecordBuilder only valid for records.",
                     record);
-            return;
+            return true;
         }
 
         validateMetaData(metaData, record);
 
+        var anyElementsUnresolved = record.getRecordComponents().stream()
+                .anyMatch(recordComponent -> recordComponent.asType().getKind() == TypeKind.ERROR);
+
+        if (anyElementsUnresolved) {
+            return false;
+        }
+
         var recordFacade = RecordFacade.fromTypeElement(processingEnv, record, packageName, metaData);
         var internalProcessor = new InternalRecordBuilderProcessor(processingEnv, recordFacade, metaData);
         internalProcessor.builderType().ifPresent(builderType -> writeJavaFile(record, internalProcessor.packageName(),
                 internalProcessor.builderClassType(), builderType, metaData.fileIndent(), metaData.fileComment()));
+        return true;
     }
 
     private void validateMetaData(RecordBuilder.Options metaData, Element element) {

There's quite a bit more auditing needed though -- at very least the processIncludes method needs to be reworked to allow delaying, but that method can produce multiple files and re-running the entire element will not work as it incrementally outputs files.

I'll get a PR up after I make some more progress.

[brandondahler]

This has turned a bit more interesting since delaying execution when ERROR types are encountered means that if there's a cycle of records which are co-dependent on each other's builder, they won't ever complete.

The simplest example which would be valid before but would no longer generate with this update is:

@RecordBuilder
public record Circular(CircularBuilder builder) {

}

This is obviously contrived but would be a regression. Is this is something we should concerned about?

[Randgalt]

I did some testing myself. Deferring seems complicated and error prone. This is a kind of forward reference. In C, you must declare forward references. What if we require record-builder users to do this. This is because the annotation processor has no reliable way of getting the package of ParentBuilder. We can throw a compiler error and say that you must use the FQPN for forward references like this (which works by the way).

package io.soabase.recordbuilder.test.circular.nested;

import io.soabase.recordbuilder.core.RecordBuilder;

@RecordBuilder
public record Child(io.soabase.recordbuilder.test.circular.ParentBuilder parentBuilder) {
}

I think this is really a javac bug. If I can fix it with the FQPN then the compiler should know this via the import.

[brandondahler]

Interesting and I concur -- in theory the symbol should be resolvable when there's a top-level import in the file telling it what the FQN is for the simple name. I'll also try working around the issue in my real project by using the FQN when I get a chance.

[brandondahler]

Debugging through makes me believe that it is related to this specific line in the JDK - https://github.com/openjdk/jdk/blob/ef0851d8adbb834e1cd5aff5b3b973b953e57e2d/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/TypeEnter.java#L571 . What specifically appears to be happening is that it is resolving the import with its FQN but realizing that said class doesn't actually exist. Because it isn't a valid type, it skips adding the FQN to the imported class name list. Then when it reaches the actual usage of the short name, it can't resolve the short name to the FQN since it isn't in the list.

Unrelated to this issue - there's a similar if statement in importNamedStatic that would also make error symbols not be considered valid imports.

[Randgalt]

So what do we think? I can have record-builder post a compiler error instructing users how to fix this with FQPN.

[brandondahler]

A warning would probably make the most sense, I'm messing around with the JDK and will see if it makes sense upstream to handle this as a bug up there. Working around it in this project presumably going to make as many problems as it solves.

[brandondahler]

I've submitted a bug report upstream via https://bugs.java -- will post back here if it is accepted and is assigned a reference number within the Java Bug System.