From b313df556d9f04b343769ba336e7a66d94e6c419 Mon Sep 17 00:00:00 2001 From: Anshuman Mishra Date: Fri, 20 Dec 2024 23:50:39 -0800 Subject: [PATCH 1/5] fix: Don't alter the AST node positions of original code. --- .../javac/handlers/HandleConstructor.java | 2 +- .../javac/handlers/HandleEqualsAndHashCode.java | 2 +- .../lombok/javac/handlers/HandleGetter.java | 2 +- .../javac/handlers/HandleJacksonized.java | 2 +- .../lombok/javac/handlers/HandleSetter.java | 4 ++-- src/core/lombok/javac/handlers/HandleWith.java | 4 ++-- .../lombok/javac/handlers/HandleWithBy.java | 2 +- .../lombok/javac/handlers/JavacHandlerUtil.java | 17 +++++++++++------ .../javac/handlers/JavacSingularsRecipes.java | 4 ++-- 9 files changed, 22 insertions(+), 17 deletions(-) diff --git a/src/core/lombok/javac/handlers/HandleConstructor.java b/src/core/lombok/javac/handlers/HandleConstructor.java index 31d15445..ca885cd6 100644 --- a/src/core/lombok/javac/handlers/HandleConstructor.java +++ b/src/core/lombok/javac/handlers/HandleConstructor.java @@ -364,7 +364,7 @@ public class HandleConstructor { if (addConstructorProperties && !isLocalType(typeNode) && LombokOptionsFactory.getDelombokOptions(typeNode.getContext()).getFormatPreferences().generateConstructorProperties()) { addConstructorProperties(mods, typeNode, fieldsToParam); } - if (onConstructor != null) mods.annotations = mods.annotations.appendList(copyAnnotations(onConstructor)); + if (onConstructor != null) mods.annotations = mods.annotations.appendList(copyAnnotations(onConstructor, maker)); return recursiveSetGeneratedBy(maker.MethodDef(mods, typeNode.toName(""), null, List.nil(), params.toList(), List.nil(), maker.Block(0L, nullChecks.appendList(assigns).toList()), null), source); diff --git a/src/core/lombok/javac/handlers/HandleEqualsAndHashCode.java b/src/core/lombok/javac/handlers/HandleEqualsAndHashCode.java index 259880fa..40854a34 100644 --- a/src/core/lombok/javac/handlers/HandleEqualsAndHashCode.java +++ b/src/core/lombok/javac/handlers/HandleEqualsAndHashCode.java @@ -190,7 +190,7 @@ public class HandleEqualsAndHashCode extends JavacAnnotationHandler { List copyableAnnotations = findCopyableAnnotations(field); List delegates = findDelegatesAndRemoveFromField(field); - List annsOnMethod = copyAnnotations(onMethod).appendList(copyableAnnotations); + List annsOnMethod = copyAnnotations(onMethod, field.getTreeMaker()).appendList(copyableAnnotations); if (field.isFinal()) { if (getCheckerFrameworkVersion(field).generatePure()) annsOnMethod = annsOnMethod.prepend(treeMaker.Annotation(genTypeRef(field, CheckerFrameworkVersion.NAME__PURE), List.nil())); } else { diff --git a/src/core/lombok/javac/handlers/HandleJacksonized.java b/src/core/lombok/javac/handlers/HandleJacksonized.java index 3ee0eec4..bec2d771 100644 --- a/src/core/lombok/javac/handlers/HandleJacksonized.java +++ b/src/core/lombok/javac/handlers/HandleJacksonized.java @@ -136,7 +136,7 @@ public class HandleJacksonized extends JavacAnnotationHandler { // Copy annotations from the class to the builder class. List copyableAnnotations = findJacksonAnnotationsOnClass(tdNode); - List copiedAnnotations = copyAnnotations(copyableAnnotations); + List copiedAnnotations = copyAnnotations(copyableAnnotations, maker); for (JCAnnotation anno : copiedAnnotations) { recursiveSetGeneratedBy(anno, annotationNode); } diff --git a/src/core/lombok/javac/handlers/HandleSetter.java b/src/core/lombok/javac/handlers/HandleSetter.java index 380f71ca..bdb5b1ac 100644 --- a/src/core/lombok/javac/handlers/HandleSetter.java +++ b/src/core/lombok/javac/handlers/HandleSetter.java @@ -241,7 +241,7 @@ public class HandleSetter extends JavacAnnotationHandler { List copyableAnnotations = findCopyableAnnotations(field); Name methodName = field.toName(setterName); - List annsOnParam = copyAnnotations(onParam).appendList(copyableAnnotations); + List annsOnParam = copyAnnotations(onParam, treeMaker).appendList(copyableAnnotations); long flags = JavacHandlerUtil.addFinalIfNeeded(Flags.PARAMETER, field.getContext()); JCExpression pType = cloneType(treeMaker, fieldDecl.vartype, source); @@ -274,7 +274,7 @@ public class HandleSetter extends JavacAnnotationHandler { List throwsClauses = List.nil(); JCExpression annotationMethodDefaultValue = null; - List annsOnMethod = mergeAnnotations(copyAnnotations(onMethod), findCopyableToSetterAnnotations(field)); + List annsOnMethod = mergeAnnotations(copyAnnotations(onMethod, treeMaker), findCopyableToSetterAnnotations(field)); if (isFieldDeprecated(field) || deprecate) { annsOnMethod = annsOnMethod.prepend(treeMaker.Annotation(genJavaLangTypeRef(field, "Deprecated"), List.nil())); } diff --git a/src/core/lombok/javac/handlers/HandleWith.java b/src/core/lombok/javac/handlers/HandleWith.java index c7fa0531..52f2c192 100644 --- a/src/core/lombok/javac/handlers/HandleWith.java +++ b/src/core/lombok/javac/handlers/HandleWith.java @@ -229,7 +229,7 @@ public class HandleWith extends JavacAnnotationHandler { JCBlock methodBody = null; long flags = JavacHandlerUtil.addFinalIfNeeded(Flags.PARAMETER, field.getContext()); - List annsOnParam = copyAnnotations(onParam).appendList(copyableAnnotations); + List annsOnParam = copyAnnotations(onParam, maker).appendList(copyableAnnotations); JCExpression pType = cloneType(maker, fieldDecl.vartype, source); JCVariableDecl param = maker.VarDef(maker.Modifiers(flags, annsOnParam), fieldDecl.name, pType, null); @@ -278,7 +278,7 @@ public class HandleWith extends JavacAnnotationHandler { List throwsClauses = List.nil(); JCExpression annotationMethodDefaultValue = null; - List annsOnMethod = copyAnnotations(onMethod); + List annsOnMethod = copyAnnotations(onMethod, maker); CheckerFrameworkVersion checkerFramework = getCheckerFrameworkVersion(source); if (checkerFramework.generateSideEffectFree()) annsOnMethod = annsOnMethod.prepend(maker.Annotation(genTypeRef(source, CheckerFrameworkVersion.NAME__SIDE_EFFECT_FREE), List.nil())); diff --git a/src/core/lombok/javac/handlers/HandleWithBy.java b/src/core/lombok/javac/handlers/HandleWithBy.java index ff67fd9f..7978928f 100644 --- a/src/core/lombok/javac/handlers/HandleWithBy.java +++ b/src/core/lombok/javac/handlers/HandleWithBy.java @@ -321,7 +321,7 @@ public class HandleWithBy extends JavacAnnotationHandler { List throwsClauses = List.nil(); JCExpression annotationMethodDefaultValue = null; - List annsOnMethod = copyAnnotations(onMethod); + List annsOnMethod = copyAnnotations(onMethod, maker); CheckerFrameworkVersion checkerFramework = getCheckerFrameworkVersion(source); if (checkerFramework.generateSideEffectFree()) annsOnMethod = annsOnMethod.prepend(maker.Annotation(genTypeRef(source, CheckerFrameworkVersion.NAME__SIDE_EFFECT_FREE), List.nil())); diff --git a/src/core/lombok/javac/handlers/JavacHandlerUtil.java b/src/core/lombok/javac/handlers/JavacHandlerUtil.java index 38658bb6..15c47a93 100644 --- a/src/core/lombok/javac/handlers/JavacHandlerUtil.java +++ b/src/core/lombok/javac/handlers/JavacHandlerUtil.java @@ -1746,7 +1746,8 @@ public class JavacHandlerUtil { } } } - return copyAnnotations(result.toList()); + + return copyAnnotations(result.toList(), node.getTreeMaker()); } /** @@ -1784,7 +1785,7 @@ public class JavacHandlerUtil { if (annoName == null) return List.nil(); if (!annoName.isEmpty()) { - for (String bn : annotationsToFind) if (typeMatches(bn, node, annoName)) return List.of(anno); + for (String bn : annotationsToFind) if (typeMatches(bn, node, annoName)) return copyAnnotations(List.of(anno), node.getTreeMaker()); } ListBuffer result = new ListBuffer(); @@ -1799,7 +1800,7 @@ public class JavacHandlerUtil { } } } - return copyAnnotations(result.toList()); + return copyAnnotations(result.toList(), node.getTreeMaker()); } /** @@ -2108,14 +2109,18 @@ public class JavacHandlerUtil { errorNode.addError(out.append(" are not allowed on builder classes.").toString()); } - static List copyAnnotations(List in) { + static List copyAnnotations(List in, JavacTreeMaker maker) { ListBuffer out = new ListBuffer(); for (JCExpression expr : in) { if (!(expr instanceof JCAnnotation)) continue; - out.append((JCAnnotation) expr.clone()); + out.append(copyAnnotation((JCAnnotation) expr, maker)); } return out.toList(); } + + static JCAnnotation copyAnnotation(JCAnnotation annotation, JavacTreeMaker maker) { + return maker.Annotation(annotation.annotationType, annotation.args); + } static List mergeAnnotations(List a, List b) { if (a == null || a.isEmpty()) return b; @@ -2266,7 +2271,7 @@ public class JavacHandlerUtil { if (JCAnnotatedTypeReflect.is(in)) { JCExpression underlyingType = cloneType0(maker, JCAnnotatedTypeReflect.getUnderlyingType(in)); - List anns = copyAnnotations(JCAnnotatedTypeReflect.getAnnotations(in)); + List anns = copyAnnotations(JCAnnotatedTypeReflect.getAnnotations(in), maker); return JCAnnotatedTypeReflect.create(anns, underlyingType); } diff --git a/src/core/lombok/javac/handlers/JavacSingularsRecipes.java b/src/core/lombok/javac/handlers/JavacSingularsRecipes.java index 4954a610..938d6e8f 100644 --- a/src/core/lombok/javac/handlers/JavacSingularsRecipes.java +++ b/src/core/lombok/javac/handlers/JavacSingularsRecipes.java @@ -315,7 +315,7 @@ public class JavacSingularsRecipes { if (!setterPrefix.isEmpty()) name = builderType.toName(HandlerUtil.buildAccessorName(source, setterPrefix, name.toString())); statements.prepend(createConstructBuilderVarIfNeeded(maker, data, builderType, source)); - List methodAnnotations = copyAnnotations(findCopyableToBuilderSingularSetterAnnotations(data.annotation.up())); + List methodAnnotations = copyAnnotations(findCopyableToBuilderSingularSetterAnnotations(data.annotation.up()), maker); finishAndInjectMethod(cfv, maker, returnType, returnStatement, data, builderType, source, deprecate, statements, name, params, methodAnnotations, access, null); } @@ -362,7 +362,7 @@ public class JavacSingularsRecipes { statements.prepend(JavacHandlerUtil.generateNullCheck(maker, null, data.getPluralName(), builderType, "%s cannot be null")); } - List methodAnnotations = copyAnnotations(findCopyableToSetterAnnotations(data.annotation.up())); + List methodAnnotations = copyAnnotations(findCopyableToSetterAnnotations(data.annotation.up()), maker); finishAndInjectMethod(cfv, maker, returnType, returnStatement, data, builderType, source, deprecate, statements, name, List.of(param), methodAnnotations, access, ignoreNullCollections); } From fab7ffdc2b5b04358ffd28e38c8e071817a4decb Mon Sep 17 00:00:00 2001 From: Anshuman Mishra Date: Fri, 27 Dec 2024 14:34:00 -0800 Subject: [PATCH 2/5] Add unit test and cover missing cases --- .../javac/handlers/JavacHandlerUtil.java | 7 +- src/delombok/lombok/delombok/Delombok.java | 6 ++ test/core/src/lombok/RunTestsViaDelombok.java | 66 +++++++++++++++++-- 3 files changed, 71 insertions(+), 8 deletions(-) diff --git a/src/core/lombok/javac/handlers/JavacHandlerUtil.java b/src/core/lombok/javac/handlers/JavacHandlerUtil.java index 15c47a93..5c9f80c6 100644 --- a/src/core/lombok/javac/handlers/JavacHandlerUtil.java +++ b/src/core/lombok/javac/handlers/JavacHandlerUtil.java @@ -38,6 +38,7 @@ import java.util.HashMap; import java.util.Map; import java.util.regex.Pattern; +import com.sun.source.tree.TreeVisitor; import com.sun.tools.javac.code.Attribute; import com.sun.tools.javac.code.BoundKind; import com.sun.tools.javac.code.Flags; @@ -75,6 +76,7 @@ import com.sun.tools.javac.tree.JCTree.JCTypeParameter; import com.sun.tools.javac.tree.JCTree.JCVariableDecl; import com.sun.tools.javac.tree.JCTree.JCWildcard; import com.sun.tools.javac.tree.JCTree.TypeBoundKind; +import com.sun.tools.javac.tree.TreeCopier; import com.sun.tools.javac.tree.TreeMaker; import com.sun.tools.javac.tree.TreeScanner; import com.sun.tools.javac.util.Context; @@ -2117,9 +2119,10 @@ public class JavacHandlerUtil { } return out.toList(); } - + static JCAnnotation copyAnnotation(JCAnnotation annotation, JavacTreeMaker maker) { - return maker.Annotation(annotation.annotationType, annotation.args); + TreeVisitor visitor = new TreeCopier(maker.getUnderlyingTreeMaker()); + return (JCAnnotation) visitor.visitAnnotation(annotation, null); } static List mergeAnnotations(List a, List b) { diff --git a/src/delombok/lombok/delombok/Delombok.java b/src/delombok/lombok/delombok/Delombok.java index e5fb440e..a0510f6c 100755 --- a/src/delombok/lombok/delombok/Delombok.java +++ b/src/delombok/lombok/delombok/Delombok.java @@ -103,6 +103,7 @@ public class Delombok { private LinkedHashMap fileToBase = new LinkedHashMap(); private List filesToParse = new ArrayList(); private Map formatPrefs = new HashMap(); + private List preLombokProcessors = new ArrayList(); private List additionalAnnotationProcessors = new ArrayList(); /** If null, output to standard out. */ @@ -653,6 +654,10 @@ public class Delombok { fileToBase.put(f, base); } + public void addPreLombokProcessors(AbstractProcessor processor) { + preLombokProcessors.add(processor); + } + public void addAdditionalAnnotationProcessor(AbstractProcessor processor) { additionalAnnotationProcessors.add(processor); } @@ -735,6 +740,7 @@ public class Delombok { Map baseMap = new IdentityHashMap(); Set processors = new LinkedHashSet(); + processors.addAll(preLombokProcessors); processors.add(new lombok.javac.apt.LombokProcessor()); processors.addAll(additionalAnnotationProcessors); diff --git a/test/core/src/lombok/RunTestsViaDelombok.java b/test/core/src/lombok/RunTestsViaDelombok.java index 918a0de4..1051cec8 100644 --- a/test/core/src/lombok/RunTestsViaDelombok.java +++ b/test/core/src/lombok/RunTestsViaDelombok.java @@ -33,6 +33,8 @@ import java.util.ArrayList; import java.util.Collection; import java.util.Collections; import java.util.Deque; +import java.util.HashMap; +import java.util.HashSet; import java.util.IdentityHashMap; import java.util.List; import java.util.Locale; @@ -83,7 +85,11 @@ public class RunTestsViaDelombok extends AbstractRunTests { delombok.setDiagnosticsListener(new CapturingDiagnosticListener(file, result.getMessages())); - if (parameters.isCheckPositions()) delombok.addAdditionalAnnotationProcessor(new ValidatePositionProcessor(parameters.getMinVersion())); + if (parameters.isCheckPositions()) { + NodePositionMapper nodePositionMapper = new NodePositionMapper(); + delombok.addPreLombokProcessors(nodePositionMapper); + delombok.addAdditionalAnnotationProcessor(new ValidatePositionProcessor(parameters.getMinVersion(), nodePositionMapper)); + } delombok.addAdditionalAnnotationProcessor(new ValidateTypesProcessor()); delombok.addAdditionalAnnotationProcessor(new ValidateNoDuplicateTreeNodeProcessor()); @@ -105,15 +111,35 @@ public class RunTestsViaDelombok extends AbstractRunTests { } } + public static class NodePositionMapper extends TreeProcessor { + Map nodePositions = new HashMap<>(); + + @Override void processCompilationUnit(final JCCompilationUnit unit) { + unit.accept(new TreeScanner() { + @Override public void scan(JCTree tree) { + if (tree == null) return; + if (tree.pos >= 0) { + nodePositions.put(tree, tree.pos); + } + super.scan(tree); + + } + }); + } + + } + public static class ValidatePositionProcessor extends TreeProcessor { private final int version; + private final NodePositionMapper nodePositionMapper; - public ValidatePositionProcessor(int version) { + public ValidatePositionProcessor(int version, NodePositionMapper nodePositionMapper) { this.version = version; + this.nodePositionMapper = nodePositionMapper; } private String craftFailMsg(String problematicNode, Deque astContext) { - StringBuilder msg = new StringBuilder(problematicNode).append(" position of node not set: "); + StringBuilder msg = new StringBuilder(problematicNode); for (JCTree t : astContext) { msg.append("\n ").append(t.getClass().getSimpleName()); String asStr = t.toString(); @@ -125,12 +151,30 @@ public class RunTestsViaDelombok extends AbstractRunTests { return msg.append("\n-------").toString(); } + private boolean isLombokGenerated(JCTree tree) { + List annotations = com.sun.tools.javac.util.List.nil(); + if (tree instanceof JCMethodDecl) { + annotations = ((JCMethodDecl) tree).mods.annotations; + } + if (tree instanceof JCVariableDecl) { + annotations = ((JCVariableDecl) tree).mods.annotations; + } + for (JCAnnotation annotation: annotations) { + if ("lombok.Generated".equals(annotation.getAnnotationType().toString())) { + return true; + } + } + return false; + } + @Override void processCompilationUnit(final JCCompilationUnit unit) { final Deque astContext = new ArrayDeque(); + final Deque lombokGeneratedNodes = new ArrayDeque(); unit.accept(new TreeScanner() { @Override public void scan(JCTree tree) { if (tree == null) return; if (tree instanceof JCMethodDecl && (((JCMethodDecl) tree).mods.flags & Flags.GENERATEDCONSTR) != 0) return; + if (isLombokGenerated(tree)) lombokGeneratedNodes.add(tree); astContext.push(tree); try { if (tree instanceof JCModifiers) return; @@ -153,16 +197,26 @@ public class RunTestsViaDelombok extends AbstractRunTests { if (tree instanceof JCVariableDecl && (((JCVariableDecl) tree).mods.flags & Javac.GENERATED_MEMBER) != 0) return; - if (check && tree.pos == -1) fail(craftFailMsg("Start", astContext)); + if (check && tree.pos == -1) fail(craftFailMsg("Start position of node not set: ", astContext)); + // Ignore ast position validation on lombok generated nodes. + if (lombokGeneratedNodes.isEmpty()) { + Integer expectedPos = nodePositionMapper.nodePositions.get(tree); + if (expectedPos != null && !expectedPos.equals(tree.pos)) { + fail(craftFailMsg(String.format("Expected node position %d, actual node position %d: ", expectedPos, tree.pos), astContext)); + } + } if (check && Javac.getEndPosition(tree, unit) == -1) { - fail(craftFailMsg("End", astContext)); + fail(craftFailMsg("End position of node not set: ", astContext)); } } finally { try { super.scan(tree); } finally { - astContext.pop(); + JCTree _tree = astContext.pop(); + if (!lombokGeneratedNodes.isEmpty() && lombokGeneratedNodes.peek().equals(_tree)) { + lombokGeneratedNodes.pop(); + } } } } From d32de67f16c762c0ca56502c55670e31ab9b295d Mon Sep 17 00:00:00 2001 From: Anshuman Mishra Date: Sat, 28 Dec 2024 09:34:28 -0800 Subject: [PATCH 3/5] Syntax as pre Pre JDK 8 --- test/core/src/lombok/RunTestsViaDelombok.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/core/src/lombok/RunTestsViaDelombok.java b/test/core/src/lombok/RunTestsViaDelombok.java index 1051cec8..8b3efc17 100644 --- a/test/core/src/lombok/RunTestsViaDelombok.java +++ b/test/core/src/lombok/RunTestsViaDelombok.java @@ -112,7 +112,7 @@ public class RunTestsViaDelombok extends AbstractRunTests { } public static class NodePositionMapper extends TreeProcessor { - Map nodePositions = new HashMap<>(); + Map nodePositions = new HashMap(); @Override void processCompilationUnit(final JCCompilationUnit unit) { unit.accept(new TreeScanner() { From 9536c297eac49753792dcc20297e0e043661f209 Mon Sep 17 00:00:00 2001 From: Anshuman Mishra Date: Fri, 3 Jan 2025 13:04:26 -0800 Subject: [PATCH 4/5] Remove annotation skip for duplicate check --- AUTHORS | 1 + test/core/src/lombok/RunTestsViaDelombok.java | 9 --------- 2 files changed, 1 insertion(+), 9 deletions(-) diff --git a/AUTHORS b/AUTHORS index a8c9bcc4..ebbc5b37 100755 --- a/AUTHORS +++ b/AUTHORS @@ -4,6 +4,7 @@ Adam Juraszek Aleksandr Zhelezniak Amine Touzani Andre Brait +Anshuman Mishra Bulgakov Alexander Caleb Brinkman Christian Nüssgens diff --git a/test/core/src/lombok/RunTestsViaDelombok.java b/test/core/src/lombok/RunTestsViaDelombok.java index 8b3efc17..80eb24be 100644 --- a/test/core/src/lombok/RunTestsViaDelombok.java +++ b/test/core/src/lombok/RunTestsViaDelombok.java @@ -34,7 +34,6 @@ import java.util.Collection; import java.util.Collections; import java.util.Deque; import java.util.HashMap; -import java.util.HashSet; import java.util.IdentityHashMap; import java.util.List; import java.util.Locale; @@ -343,14 +342,6 @@ public class RunTestsViaDelombok extends AbstractRunTests { super.scan(tree); parents.pop(); } - - /** - * We always generate shallow copies for annotations - */ - @Override - public void visitAnnotation(JCAnnotation tree) { - return; - } }); } From ba3c92236096be9896d4d37404b62a2193ce9607 Mon Sep 17 00:00:00 2001 From: Anshuman Mishra Date: Fri, 3 Jan 2025 13:08:23 -0800 Subject: [PATCH 5/5] Nit --- src/core/lombok/javac/handlers/HandleGetter.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/lombok/javac/handlers/HandleGetter.java b/src/core/lombok/javac/handlers/HandleGetter.java index 41d4e092..03dd566c 100644 --- a/src/core/lombok/javac/handlers/HandleGetter.java +++ b/src/core/lombok/javac/handlers/HandleGetter.java @@ -250,7 +250,7 @@ public class HandleGetter extends JavacAnnotationHandler { List copyableAnnotations = findCopyableAnnotations(field); List delegates = findDelegatesAndRemoveFromField(field); - List annsOnMethod = copyAnnotations(onMethod, field.getTreeMaker()).appendList(copyableAnnotations); + List annsOnMethod = copyAnnotations(onMethod, treeMaker).appendList(copyableAnnotations); if (field.isFinal()) { if (getCheckerFrameworkVersion(field).generatePure()) annsOnMethod = annsOnMethod.prepend(treeMaker.Annotation(genTypeRef(field, CheckerFrameworkVersion.NAME__PURE), List.nil())); } else {