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

feat: allow generics with SuperBuilder customization #3646

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ntroutman
Copy link

@ntroutman ntroutman commented Apr 1, 2024

Trying to customize @SuperBuilder for classes that contain generics will fail due to a check in javac/handlers/HandleSuperBuilder.constructorExists assuming that the only generics on the builder are the ones introduced by the builder itself. This means attempting to do:

@SuperBuilder
public class Parent<T> {
  int field;

  protected Parent(ParentBuilder<T, ?, ?> b) {
    if (b.field == 0)
      throw new IllegalArgumentException("field must be != 0");
      this.field = b.field;
    }
}

Lombok fails with the error error: constructor Parent(ParentBuilder<T,?,?>) is already defined in class Parent

@janrieke
Copy link
Contributor

janrieke commented Apr 3, 2024

Let me know if you need any support, e.g. a intermediate review etc.

@ntroutman ntroutman marked this pull request as ready for review April 3, 2024 18:59
@@ -1160,7 +1160,7 @@ private boolean constructorExists(JavacNode type, String builderClassName) {
if (lastIndexOfDot >= 0) {
typeName = typeName.substring(lastIndexOfDot+1);
}
if ((builderClassName+"<?, ?>").equals(typeName))
if (typeName.startsWith(builderClassName) && typeName.endsWith("?, ?>"))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code before was not optimal already, as String comparisons tend to break quickly. Imagine something like Java extending the diamond operator also for ?, so that you can write:
protected Parent(ParentBuilder<> b) {...}
Thus, we should use the opportunity to improve it. I suggest doing an instanceof check and only use the generic toString as a fallback:

String typeName;
JCTree paramType = md.params.get(0).getType();
if (paramType instanceof JCTypeApply) {
	typeName = ((JCTypeApply)paramType).clazz.toString();
} else {
	typeName = paramType.toString();
}
int lastIndexOfDot = typeName.lastIndexOf('.');
if (lastIndexOfDot >= 0) {
	typeName = typeName.substring(lastIndexOfDot+1);
}
if (builderClassName.equals(typeName))
	return true;

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I'll work on that. I won't like have a chance until a couple weeks from now.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, using toString() to compare generics is a nono, though I find the idea of falling back on that in case a series of instanceof fails a great idea. @ntroutman still working on it, or?

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

Successfully merging this pull request may close these issues.

3 participants