Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG] SuperBuilder not able to customize the Builder class when there is a property with @Singular annotation present #2701

Closed
ahaverdings opened this issue Jan 6, 2021 · 13 comments

Comments

@ahaverdings
Copy link

ahaverdings commented Jan 6, 2021

When trying to extend the ParentBuilder the (de)lombok compiler throws the following error.

java.lang.NullPointerException
	at lombok.javac.handlers.JavacSingularsRecipes$JavacSingularizer.checkForAlreadyExistingNodesAndGenerateError(JavacSingularsRecipes.java:197)
	at lombok.javac.handlers.HandleSuperBuilder.handle(HandleSuperBuilder.java:310)
	at lombok.javac.HandlerLibrary$AnnotationHandlerContainer.handle(HandlerLibrary.java:113)
	at lombok.javac.HandlerLibrary.handleAnnotation(HandlerLibrary.java:256)
	at lombok.javac.JavacTransformer$AnnotationVisitor.visitAnnotationOnType(JavacTransformer.java:79)
	at lombok.javac.JavacNode.traverse(JavacNode.java:132)
	at lombok.javac.JavacAST.traverseChildren(JavacAST.java:138)
	at lombok.javac.JavacNode.traverse(JavacNode.java:95)
	at lombok.javac.JavacAST.traverseChildren(JavacAST.java:138)
	at lombok.javac.JavacNode.traverse(JavacNode.java:90)
	at lombok.javac.JavacAST.traverse(JavacAST.java:134)
	at lombok.javac.JavacTransformer.transform(JavacTransformer.java:63)
	at lombok.javac.apt.LombokProcessor.process(LombokProcessor.java:326)
	at lombok.core.AnnotationProcessor$JavacDescriptor.process(AnnotationProcessor.java:187)
	at lombok.core.AnnotationProcessor.process(AnnotationProcessor.java:237)
	at lombok.launch.AnnotationProcessorHider$AnnotationProcessor.process(AnnotationProcessor.java:90)
	at com.sun.tools.javac.processing.JavacProcessingEnvironment.callProcessor(JavacProcessingEnvironment.java:794)
	at com.sun.tools.javac.processing.JavacProcessingEnvironment.discoverAndRunProcs(JavacProcessingEnvironment.java:705)
	at com.sun.tools.javac.processing.JavacProcessingEnvironment.access$1800(JavacProcessingEnvironment.java:91)
	at com.sun.tools.javac.processing.JavacProcessingEnvironment$Round.run(JavacProcessingEnvironment.java:1035)
	at com.sun.tools.javac.processing.JavacProcessingEnvironment.doProcessing(JavacProcessingEnvironment.java:1176)
	at com.sun.tools.javac.main.JavaCompiler.processAnnotations(JavaCompiler.java:1170)
	at com.sun.tools.javac.main.JavaCompiler.compile(JavaCompiler.java:856)
	at com.sun.tools.javac.main.Main.compile(Main.java:523)
	at com.sun.tools.javac.api.JavacTaskImpl.doCall(JavacTaskImpl.java:129)
	at com.sun.tools.javac.api.JavacTaskImpl.call(JavacTaskImpl.java:138)
	at org.codehaus.plexus.compiler.javac.JavaxToolsCompiler.compileInProcess(JavaxToolsCompiler.java:125)
	at org.codehaus.plexus.compiler.javac.JavacCompiler.performCompile(JavacCompiler.java:171)
	at org.apache.maven.plugin.compiler.AbstractCompilerMojo.execute(AbstractCompilerMojo.java:886)
	at org.apache.maven.plugin.compiler.CompilerMojo.execute(CompilerMojo.java:129)
	at org.apache.maven.plugin.DefaultBuildPluginManager.executeMojo(DefaultBuildPluginManager.java:137)
	at org.apache.maven.lifecycle.internal.MojoExecutor.execute(MojoExecutor.java:210)
	at org.apache.maven.lifecycle.internal.MojoExecutor.execute(MojoExecutor.java:156)
	at org.apache.maven.lifecycle.internal.MojoExecutor.execute(MojoExecutor.java:148)
	at org.apache.maven.lifecycle.internal.LifecycleModuleBuilder.buildProject(LifecycleModuleBuilder.java:117)
	at org.apache.maven.lifecycle.internal.LifecycleModuleBuilder.buildProject(LifecycleModuleBuilder.java:81)
	at org.apache.maven.lifecycle.internal.builder.singlethreaded.SingleThreadedBuilder.build(SingleThreadedBuilder.java:56)
	at org.apache.maven.lifecycle.internal.LifecycleStarter.execute(LifecycleStarter.java:128)
	at org.apache.maven.DefaultMaven.doExecute(DefaultMaven.java:305)
	at org.apache.maven.DefaultMaven.doExecute(DefaultMaven.java:192)
	at org.apache.maven.DefaultMaven.execute(DefaultMaven.java:105)
	at org.apache.maven.cli.MavenCli.execute(MavenCli.java:957)
	at org.apache.maven.cli.MavenCli.doMain(MavenCli.java:289)
	at org.apache.maven.cli.MavenCli.main(MavenCli.java:193)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at org.codehaus.plexus.classworlds.launcher.Launcher.launchEnhanced(Launcher.java:282)
	at org.codehaus.plexus.classworlds.launcher.Launcher.launch(Launcher.java:225)
	at org.codehaus.plexus.classworlds.launcher.Launcher.mainWithExitCode(Launcher.java:406)
	at org.codehaus.plexus.classworlds.launcher.Launcher.main(Launcher.java:347)
	at org.codehaus.classworlds.Launcher.main(Launcher.java:47)

Example

When extending the Child SuperBuilder the following example is working.

package example;

import lombok.EqualsAndHashCode;
import lombok.Getter;
import lombok.ToString;
import lombok.experimental.SuperBuilder;

@Getter
@SuperBuilder(toBuilder = true)
@EqualsAndHashCode(callSuper = true)
@ToString
public class Child extends Parent {
    private final String property1;
    private final String property2;

    public static abstract class ChildBuilder<C extends Child, B extends Child.ChildBuilder<C, B>> extends ParentBuilder<C, B> {
        public B customChild(final String value) {
            // ...
            System.out.println(value);
            return self(); 
        }
    }
}

But when trying to extend the Parent SuperBuilder and there is a property with @Singular annotation present the NPE is thrown.

package example;

import java.util.List;

import lombok.EqualsAndHashCode;
import lombok.Getter;
import lombok.Singular;
import lombok.ToString;
import lombok.experimental.SuperBuilder;

@Getter
@SuperBuilder(toBuilder = true)
@EqualsAndHashCode
@ToString
public class Parent {
    private final String description;
    @Singular private final List<String> errors;

    public static abstract class ParentBuilder<C extends Parent, B extends Parent.ParentBuilder<C, B>> {
        public B custom(final String value) {
            // ...
            System.out.println(value);
            return self(); 
        }
    }
}

Expected

What I'm trying to reach is that the custom method is available in all child (grandchild) builders. Extending the parent builder using private static final class ParentBuilderImpl extends Parent.ParentBuilder<Parent, Parent.ParentBuilder> { ... } will not throw the error but also will not make the custom method available in child builders (since its only available in ParentBuilderImpl where child builder do not extend from)

  • Lombok version: 1.18.16
  • Platform: IntelliJ, Java 1.8
@janrieke
Copy link
Contributor

janrieke commented Jan 6, 2021

Cannot reproduce in Eclipse and Maven. I suspect this is an IntelliJ plugin bug.
Could you check whether directly compiling via maven/gradle works?

@rzwitserloot
Copy link
Collaborator

Also, I vaguely recall that superbuilder will just intentionally error out on you if you attempt to write the class; that signature is incredibly complicated, asking lombok users to write it out is not something we intend to support. If it does not error out, I'm tempted to add it. (In other words, maybe the problem is that the intellij plugin fails to error out on this).

@janrieke
Copy link
Contributor

janrieke commented Jan 6, 2021

However, HandleSuperBuilder:310 looks a bit fishy to me:
if (singularizer.checkForAlreadyExistingNodesAndGenerateError(job.builderType, sd))

I didn't have a closer look on that BuilderJob refactoring you did, @rzwitserloot, but I think the argument should be job.builderAbstractType there. An uninitialized builderType would explain the NPE, although I couldn't reproduce it in the given example.

@rzwitserloot
Copy link
Collaborator

@janrieke no doubt - that was part of a gigantic refactor and I must have missed a beat. I'll fix this in a few moments. Other than that, I think the next step is to close this issue and tell @ahverdings to file with team intellij, specifically, that manually adding a buildertype that @SuperBuilder would otherwise generate should result in a compiler error, right?

@janrieke
Copy link
Contributor

janrieke commented Jan 6, 2021

No, the example is perfectly fine. @SuperBuilder should be able to deal with preexisting builder classes like the one given in the example.

@rzwitserloot
Copy link
Collaborator

@janrieke You want to sign up for the notion that users end up manually writing this gigantic signature?

public static abstract class ParentBuilder<C extends Parent, B extends Parent.ParentBuilder<C, B>>

That's... unexpected. Okay.

@janrieke
Copy link
Contributor

janrieke commented Jan 6, 2021

I implemented that primarily (before @Jacksonized existed) to allow users to configure the interaction with Jackson. Now the usecases are more limited, but there are still some (as this issue shows).

@flomin
Copy link

flomin commented Jan 6, 2021

We are using lombok in a custom library used by 6 different projects.
The update to the 1.18.16 version is breaking due to this issue. The compilation using maven ends with the same error shown in the issue.

We are using custom methods in the parent class using @SuperBuilder for several cases:

  • to hide/ignore properties from the builder by changing accessor method public to private (more elegant and concise than defining a specific constructor with @SuperBuilder as the class has a LOT of properties)
  • to define complex definition behavior with several arguments (like complex tree structure with node initialization), could be refactored using temporary excluded variables to store the values and do all the work in the build() method.

These two breaking cases could be solved using another way around with heavy refactoring... But it's a breaking feature for us as the documentation of @SuperBuilder states these changes should be possible here:
You can customize most of the code generated by @SuperBuilder, except for internal methods (e.g. self()). You have to make sure that the builder class declaration headers match those that would have been generated by lombok. Due to the heavy generics usage, we strongly advice to take the uncustomized delomboked code as a reference when customizing @SuperBuilder.

@janrieke
Copy link
Contributor

janrieke commented Jan 6, 2021

Thinking about it, manually writing this builder class header is indeed far from ideal (even if you'd c&p it from delombok). Maybe Lombok could insert the necessary type parameters automatically if it detects a builder class without them.

@ahaverdings
Copy link
Author

@janrieke Used vscode and gradle but error is the same: Lombok annotation handler class lombok.javac.handlers.HandleSuperBuilder failed on Parent.java: java.lang.NullPointerException (stacktrace is probably somewhere but im guessing its the same)

Also tried using older versions and somewhere between version 1.18.12 and 1.18.14 a change was made that causes this NPE

@janrieke
Copy link
Contributor

janrieke commented Jan 6, 2021

I'm pretty sure the line mentioned above causes the problem. However, I still cannot reproduce it.

Could you check if your simple example really causes this issue? At least it's not compiling as is, because the @EqualsAndHashCode(callSuper = true) on the Parent is wrong.

A crashing example would be great so that we can derive a unit test from it.

@ahaverdings
Copy link
Author

You're correct, the example (with corrected EqualsAndHashCode) is working. Which seemed weird to me so I did some further digging and found what causes the issue here.

In my full piece of code I have many properties and for sake of the example left them out, thinking they would not cause the issue. My Parent class has a property annotated with @Singular and that was the reason it failed. I have tested and even customizing the child builder will fail when you have a property with the @Singular annotation.

updated Parent class example code (which fails):

package example;

import java.util.List;

import lombok.EqualsAndHashCode;
import lombok.Getter;
import lombok.Singular;
import lombok.ToString;
import lombok.experimental.SuperBuilder;

@Getter
@SuperBuilder(toBuilder = true)
@EqualsAndHashCode
@ToString
public class Parent {
    private final String description;
    @Singular private final List<String> errors;

    public static abstract class ParentBuilder<C extends Parent, B extends Parent.ParentBuilder<C, B>> {
        public B custom(final String value) {
            // ...
            return self(); 
        }
    }
}

None the less, is version 1.18.12 it's succesfull, from version 1.18.14 and up it fails.

I will update the bug report.

@ahaverdings ahaverdings changed the title [BUG] SuperBuilder not able to customize ParentBuilder class [BUG] SuperBuilder not able to customize the Builder class when there is a property with @Singular annotation present Jan 6, 2021
@henrykuijpers
Copy link

Is there any update on this, @rzwitserloot @janrieke ? Would love to see this fixed. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants