-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
base: master
Are you sure you want to change the base?
feat: allow generics with SuperBuilder customization #3646
Conversation
074f236
to
cf064e3
Compare
Let me know if you need any support, e.g. a intermediate review etc. |
@@ -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("?, ?>")) |
There was a problem hiding this comment.
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;
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
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:Lombok fails with the error
error: constructor Parent(ParentBuilder<T,?,?>) is already defined in class Parent