-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
Make constructorExists take in mind customized generic builders
#3938
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
base: master
Are you sure you want to change the base?
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
|
I was able to build the project (missed It would be better to make generated default value providers protected to be able to be used inside the customized builder constructor. |
|
Thanks for your contribution. I'll have a look at the PR next week. Until then, please:
|
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.
Pull request overview
This pull request fixes issue #3935 by improving the constructorExists method in HandleSuperBuilder.java to properly detect manually-written constructors that accept builder parameters with customized generic type parameters.
Key Changes
- Refactored
constructorExiststo use AST-based type checking instead of string matching - Now supports builders with more than 2 type parameters (e.g.,
Builder<T, C, B>) as long as the last two are wildcards - Improved code clarity by using explicit AST node type checks
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/core/lombok/javac/handlers/HandleSuperBuilder.java | Refactored constructorExists method to properly handle customized generic builders with additional type parameters beyond the standard 2 |
| AUTHORS | Added contributor credit for Dymitr "ay0ks" |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| JCExpression argLast = paramArgs.get(paramArgs.size() - 1); | ||
| JCExpression argLast2 = paramArgs.get(paramArgs.size() - 2); | ||
| if (argLast instanceof JCWildcard && argLast2 instanceof JCWildcard) { | ||
| return true; |
Copilot
AI
Dec 3, 2025
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 new logic only checks if the last two type arguments are wildcards, but it doesn't verify that they are unbounded wildcards (i.e., ? without bounds like ? extends T or ? super T). The old implementation checked for the exact pattern <?, ?>. If a user defines a constructor with bounded wildcards like Builder<? extends Something, ? super SomethingElse>, this might incorrectly match.
Consider checking the wildcard kind to ensure they are unbounded:
if (argLast instanceof JCWildcard && argLast2 instanceof JCWildcard) {
JCWildcard wc1 = (JCWildcard) argLast;
JCWildcard wc2 = (JCWildcard) argLast2;
if (wc1.kind == TypeBoundKind.UNBOUND && wc2.kind == TypeBoundKind.UNBOUND) {
return true;
}
}| return true; | |
| JCWildcard wc1 = (JCWildcard) argLast; | |
| JCWildcard wc2 = (JCWildcard) argLast2; | |
| if (wc1.kind == BoundKind.UNBOUND && wc2.kind == BoundKind.UNBOUND) { | |
| return true; | |
| } |
| JCExpression paramType = md.params.get(0).vartype; | ||
| if (paramType instanceof JCTypeApply) { | ||
| JCTypeApply paramType_ = (JCTypeApply) paramType; | ||
| if (paramType_.clazz instanceof JCIdent && ((JCIdent) paramType_.clazz).name.contentEquals(builderClassName)) { |
Copilot
AI
Dec 3, 2025
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 new implementation only handles the case where paramType_.clazz is a JCIdent (simple name), but the old code explicitly handled fully-qualified, partially-qualified, or unqualified names by extracting the last part after the last dot. This means the new code won't match constructors with qualified builder class names like protected Parent(com.example.Parent.ParentBuilder<?, ?> b).
Consider also handling JCFieldAccess for qualified names:
if (paramType_.clazz instanceof JCIdent && ((JCIdent) paramType_.clazz).name.contentEquals(builderClassName)) {
// existing logic
} else if (paramType_.clazz instanceof JCFieldAccess) {
String qualifiedName = paramType_.clazz.toString();
String simpleName = qualifiedName.substring(qualifiedName.lastIndexOf('.') + 1);
if (simpleName.equals(builderClassName)) {
// check wildcards
}
}| if (paramType_.clazz instanceof JCIdent && ((JCIdent) paramType_.clazz).name.contentEquals(builderClassName)) { | |
| boolean matchesBuilderClass = false; | |
| if (paramType_.clazz instanceof JCIdent) { | |
| matchesBuilderClass = ((JCIdent) paramType_.clazz).name.contentEquals(builderClassName); | |
| } else if (paramType_.clazz instanceof JCFieldAccess) { | |
| String qualifiedName = paramType_.clazz.toString(); | |
| String simpleName = qualifiedName.substring(qualifiedName.lastIndexOf('.') + 1); | |
| matchesBuilderClass = simpleName.equals(builderClassName); | |
| } | |
| if (matchesBuilderClass) { |
| } | ||
|
|
||
| /** | ||
| * Checks if there is a manual constructor in the given type with a single parameter (builder). |
Copilot
AI
Dec 3, 2025
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 JavaDoc comment should be updated to reflect that this method now handles builders with customized generic parameters (more than the standard 2 type parameters), not just the standard Builder<?, ?> pattern.
Suggested update:
/**
* Checks if there is a manual constructor in the given type with a single parameter (builder).
* Supports builders with custom type parameters, as long as the last two type parameters are wildcards.
*/| * Checks if there is a manual constructor in the given type with a single parameter (builder). | |
| * Checks if there is a manual constructor in the given type with a single parameter (builder). | |
| * Supports builders with custom type parameters, as long as the last two type parameters are wildcards. |
| } | ||
| private boolean constructorExists(JavacNode type, String builderClassName) { | ||
| if (type != null && type.get() instanceof JCClassDecl) { | ||
| for (JCTree def : ((JCClassDecl) type.get()).defs) { |
Copilot
AI
Dec 3, 2025
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.
| if (paramType instanceof JCTypeApply) { | ||
| JCTypeApply paramType_ = (JCTypeApply) paramType; | ||
| if (paramType_.clazz instanceof JCIdent && ((JCIdent) paramType_.clazz).name.contentEquals(builderClassName)) { | ||
| List<JCExpression> paramArgs = paramType_.arguments; |
Copilot
AI
Dec 3, 2025
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.
Access to unsupported JDK-internal API 'com.sun.tools.javac.util.List'. (Use javax.tools and javax.lang.model @SInCE 1.6)
| List<JCExpression> paramArgs = paramType_.arguments; | |
| java.util.List<JCExpression> paramArgs = new ArrayList<>(paramType_.arguments); |
If I'm not mistaken, `builder()` should accept the generics at line 106. I haven't touched this for a really long time, so excuse me.
Fixes #3935