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.

Sign up for GitHub

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

fix: Don't alter the AST node positions of original code. #3800

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

fix: Don't alter the AST node positions of original code. #3800

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b313df5
fix: Don't alter the AST node positions of original code.
amishra-u Dec 21, 2024
fab7ffd
Add unit test and cover missing cases
amishra-u Dec 27, 2024
d32de67
Syntax as pre Pre JDK 8
amishra-u Dec 28, 2024
9536c29
Remove annotation skip for duplicate check
amishra-u Jan 3, 2025
ba3c922
Nit
amishra-u Jan 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions AUTHORS
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ Adam Juraszek <[email protected]>
Aleksandr Zhelezniak <[email protected]>
Amine Touzani <[email protected]>
Andre Brait <[email protected]>
Anshuman Mishra <[email protected]>
Bulgakov Alexander <[email protected]>
Caleb Brinkman <[email protected]>
Christian Nüssgens <[email protected]>
Expand Down
2 changes: 1 addition & 1 deletion src/core/lombok/javac/handlers/HandleConstructor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -364,7 +364,7 @@ public static void addConstructorProperties(JCModifiers mods, JavacNode node, Li
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("<init>"),
null, List.<JCTypeParameter>nil(), params.toList(), List.<JCExpression>nil(),
maker.Block(0L, nullChecks.appendList(assigns).toList()), null), source);
Expand Down
2 changes: 1 addition & 1 deletion src/core/lombok/javac/handlers/HandleEqualsAndHashCode.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,7 @@ public void generateMethods(JavacNode typeNode, JavacNode source, java.util.List
injectMethod(typeNode, equalsMethod);

if (needsCanEqual && canEqualExists == MemberExistsResult.NOT_EXISTS) {
JCMethodDecl canEqualMethod = createCanEqual(typeNode, source, copyAnnotations(onParam));
JCMethodDecl canEqualMethod = createCanEqual(typeNode, source, copyAnnotations(onParam, typeNode.getTreeMaker()));
injectMethod(typeNode, canEqualMethod);
}

Expand Down
2 changes: 1 addition & 1 deletion src/core/lombok/javac/handlers/HandleGetter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -250,7 +250,7 @@ public JCMethodDecl createGetter(long access, JavacNode field, JavacTreeMaker tr

List<JCAnnotation> copyableAnnotations = findCopyableAnnotations(field);
List<JCAnnotation> delegates = findDelegatesAndRemoveFromField(field);
List<JCAnnotation> annsOnMethod = copyAnnotations(onMethod).appendList(copyableAnnotations);
List<JCAnnotation> annsOnMethod = copyAnnotations(onMethod, treeMaker).appendList(copyableAnnotations);
if (field.isFinal()) {
if (getCheckerFrameworkVersion(field).generatePure()) annsOnMethod = annsOnMethod.prepend(treeMaker.Annotation(genTypeRef(field, CheckerFrameworkVersion.NAME__PURE), List.<JCExpression>nil()));
} else {
Expand Down
2 changes: 1 addition & 1 deletion src/core/lombok/javac/handlers/HandleJacksonized.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ public class HandleJacksonized extends JavacAnnotationHandler<Jacksonized> {

// Copy annotations from the class to the builder class.
List<JCAnnotation> copyableAnnotations = findJacksonAnnotationsOnClass(tdNode);
List<JCAnnotation> copiedAnnotations = copyAnnotations(copyableAnnotations);
List<JCAnnotation> copiedAnnotations = copyAnnotations(copyableAnnotations, maker);
for (JCAnnotation anno : copiedAnnotations) {
recursiveSetGeneratedBy(anno, annotationNode);
}
Expand Down
4 changes: 2 additions & 2 deletions src/core/lombok/javac/handlers/HandleSetter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,7 +241,7 @@ public static JCMethodDecl createSetterWithRecv(long access, boolean deprecate,
List<JCAnnotation> copyableAnnotations = findCopyableAnnotations(field);

Name methodName = field.toName(setterName);
List<JCAnnotation> annsOnParam = copyAnnotations(onParam).appendList(copyableAnnotations);
List<JCAnnotation> annsOnParam = copyAnnotations(onParam, treeMaker).appendList(copyableAnnotations);

long flags = JavacHandlerUtil.addFinalIfNeeded(Flags.PARAMETER, field.getContext());
JCExpression pType = cloneType(treeMaker, fieldDecl.vartype, source);
Expand Down Expand Up @@ -274,7 +274,7 @@ public static JCMethodDecl createSetterWithRecv(long access, boolean deprecate,
List<JCExpression> throwsClauses = List.nil();
JCExpression annotationMethodDefaultValue = null;

List<JCAnnotation> annsOnMethod = mergeAnnotations(copyAnnotations(onMethod), findCopyableToSetterAnnotations(field));
List<JCAnnotation> annsOnMethod = mergeAnnotations(copyAnnotations(onMethod, treeMaker), findCopyableToSetterAnnotations(field));
if (isFieldDeprecated(field) || deprecate) {
annsOnMethod = annsOnMethod.prepend(treeMaker.Annotation(genJavaLangTypeRef(field, "Deprecated"), List.<JCExpression>nil()));
}
Expand Down
4 changes: 2 additions & 2 deletions src/core/lombok/javac/handlers/HandleWith.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,7 +229,7 @@ public JCMethodDecl createWith(long access, JavacNode field, JavacTreeMaker make

JCBlock methodBody = null;
long flags = JavacHandlerUtil.addFinalIfNeeded(Flags.PARAMETER, field.getContext());
List<JCAnnotation> annsOnParam = copyAnnotations(onParam).appendList(copyableAnnotations);
List<JCAnnotation> 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);
Expand Down Expand Up @@ -278,7 +278,7 @@ public JCMethodDecl createWith(long access, JavacNode field, JavacTreeMaker make
List<JCExpression> throwsClauses = List.nil();
JCExpression annotationMethodDefaultValue = null;

List<JCAnnotation> annsOnMethod = copyAnnotations(onMethod);
List<JCAnnotation> annsOnMethod = copyAnnotations(onMethod, maker);
CheckerFrameworkVersion checkerFramework = getCheckerFrameworkVersion(source);
if (checkerFramework.generateSideEffectFree()) annsOnMethod = annsOnMethod.prepend(maker.Annotation(genTypeRef(source, CheckerFrameworkVersion.NAME__SIDE_EFFECT_FREE), List.<JCExpression>nil()));

Expand Down
2 changes: 1 addition & 1 deletion src/core/lombok/javac/handlers/HandleWithBy.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -321,7 +321,7 @@ public JCMethodDecl createWithBy(long access, JavacNode field, JavacTreeMaker ma
List<JCExpression> throwsClauses = List.nil();
JCExpression annotationMethodDefaultValue = null;

List<JCAnnotation> annsOnMethod = copyAnnotations(onMethod);
List<JCAnnotation> annsOnMethod = copyAnnotations(onMethod, maker);
CheckerFrameworkVersion checkerFramework = getCheckerFrameworkVersion(source);
if (checkerFramework.generateSideEffectFree()) annsOnMethod = annsOnMethod.prepend(maker.Annotation(genTypeRef(source, CheckerFrameworkVersion.NAME__SIDE_EFFECT_FREE), List.<JCExpression>nil()));

Expand Down
20 changes: 14 additions & 6 deletions src/core/lombok/javac/handlers/JavacHandlerUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
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;
Expand Down Expand Up @@ -75,6 +76,7 @@
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;
Expand Down Expand Up @@ -1746,7 +1748,8 @@ public static List<JCAnnotation> findCopyableAnnotations(JavacNode node) {
}
}
}
return copyAnnotations(result.toList());

return copyAnnotations(result.toList(), node.getTreeMaker());
}

/**
Expand Down Expand Up @@ -1784,7 +1787,7 @@ private static List<JCAnnotation> findAnnotationsInList(JavacNode node, java.uti
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<JCAnnotation> result = new ListBuffer<JCAnnotation>();
Expand All @@ -1799,7 +1802,7 @@ private static List<JCAnnotation> findAnnotationsInList(JavacNode node, java.uti
}
}
}
return copyAnnotations(result.toList());
return copyAnnotations(result.toList(), node.getTreeMaker());
}

/**
Expand Down Expand Up @@ -2108,15 +2111,20 @@ public static void sanityCheckForMethodGeneratingAnnotationsOnBuilderClass(Javac
errorNode.addError(out.append(" are not allowed on builder classes.").toString());
}

static List<JCAnnotation> copyAnnotations(List<? extends JCExpression> in) {
static List<JCAnnotation> copyAnnotations(List<? extends JCExpression> in, JavacTreeMaker maker) {
ListBuffer<JCAnnotation> out = new ListBuffer<JCAnnotation>();
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) {
TreeVisitor<JCTree, Void> visitor = new TreeCopier<Void>(maker.getUnderlyingTreeMaker());
return (JCAnnotation) visitor.visitAnnotation(annotation, null);
}

static List<JCAnnotation> mergeAnnotations(List<JCAnnotation> a, List<JCAnnotation> b) {
if (a == null || a.isEmpty()) return b;
if (b == null || b.isEmpty()) return a;
Expand Down Expand Up @@ -2266,7 +2274,7 @@ private static JCExpression cloneType0(JavacTreeMaker maker, JCTree in) {

if (JCAnnotatedTypeReflect.is(in)) {
JCExpression underlyingType = cloneType0(maker, JCAnnotatedTypeReflect.getUnderlyingType(in));
List<JCAnnotation> anns = copyAnnotations(JCAnnotatedTypeReflect.getAnnotations(in));
List<JCAnnotation> anns = copyAnnotations(JCAnnotatedTypeReflect.getAnnotations(in), maker);
return JCAnnotatedTypeReflect.create(anns, underlyingType);
}

Expand Down
4 changes: 2 additions & 2 deletions src/core/lombok/javac/handlers/JavacSingularsRecipes.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -315,7 +315,7 @@ private void generateSingularMethod(CheckerFrameworkVersion cfv, boolean depreca
if (!setterPrefix.isEmpty()) name = builderType.toName(HandlerUtil.buildAccessorName(source, setterPrefix, name.toString()));

statements.prepend(createConstructBuilderVarIfNeeded(maker, data, builderType, source));
List<JCAnnotation> methodAnnotations = copyAnnotations(findCopyableToBuilderSingularSetterAnnotations(data.annotation.up()));
List<JCAnnotation> methodAnnotations = copyAnnotations(findCopyableToBuilderSingularSetterAnnotations(data.annotation.up()), maker);
finishAndInjectMethod(cfv, maker, returnType, returnStatement, data, builderType, source, deprecate, statements, name, params, methodAnnotations, access, null);
}

Expand Down Expand Up @@ -362,7 +362,7 @@ private void generatePluralMethod(CheckerFrameworkVersion cfv, boolean deprecate
statements.prepend(JavacHandlerUtil.generateNullCheck(maker, null, data.getPluralName(), builderType, "%s cannot be null"));
}

List<JCAnnotation> methodAnnotations = copyAnnotations(findCopyableToSetterAnnotations(data.annotation.up()));
List<JCAnnotation> methodAnnotations = copyAnnotations(findCopyableToSetterAnnotations(data.annotation.up()), maker);

finishAndInjectMethod(cfv, maker, returnType, returnStatement, data, builderType, source, deprecate, statements, name, List.of(param), methodAnnotations, access, ignoreNullCollections);
}
Expand Down
6 changes: 6 additions & 0 deletions src/delombok/lombok/delombok/Delombok.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@ public void setWriter(Writer writer) {
private LinkedHashMap<File, File> fileToBase = new LinkedHashMap<File, File>();
private List<File> filesToParse = new ArrayList<File>();
private Map<String, String> formatPrefs = new HashMap<String, String>();
private List<AbstractProcessor> preLombokProcessors = new ArrayList<AbstractProcessor>();
private List<AbstractProcessor> additionalAnnotationProcessors = new ArrayList<AbstractProcessor>();

/** If null, output to standard out. */
Expand Down Expand Up @@ -653,6 +654,10 @@ public void addFile(File base, String fileName) throws IOException {
fileToBase.put(f, base);
}

public void addPreLombokProcessors(AbstractProcessor processor) {
preLombokProcessors.add(processor);
}

public void addAdditionalAnnotationProcessor(AbstractProcessor processor) {
additionalAnnotationProcessors.add(processor);
}
Expand Down Expand Up @@ -735,6 +740,7 @@ public boolean delombok() throws IOException {
Map<JCCompilationUnit, File> baseMap = new IdentityHashMap<JCCompilationUnit, File>();

Set<AbstractProcessor> processors = new LinkedHashSet<AbstractProcessor>();
processors.addAll(preLombokProcessors);
processors.add(new lombok.javac.apt.LombokProcessor());
processors.addAll(additionalAnnotationProcessors);

Expand Down
73 changes: 59 additions & 14 deletions test/core/src/lombok/RunTestsViaDelombok.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
import java.util.Collection;
import java.util.Collections;
import java.util.Deque;
import java.util.HashMap;
import java.util.IdentityHashMap;
import java.util.List;
import java.util.Locale;
Expand Down Expand Up @@ -83,7 +84,11 @@ public TransformationResult transformCode(final File file, TestParameters parame

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());

Expand All @@ -105,15 +110,35 @@ public TransformationResult transformCode(final File file, TestParameters parame
}
}

public static class NodePositionMapper extends TreeProcessor {
Map<JCTree, Integer> nodePositions = new HashMap<JCTree, Integer>();

@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<JCTree> 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();
Expand All @@ -125,12 +150,30 @@ private String craftFailMsg(String problematicNode, Deque<JCTree> astContext) {
return msg.append("\n-------").toString();
}

private boolean isLombokGenerated(JCTree tree) {
List<JCAnnotation> 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<JCTree> astContext = new ArrayDeque<JCTree>();
final Deque<JCTree> lombokGeneratedNodes = new ArrayDeque<JCTree>();
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;
Expand All @@ -153,16 +196,26 @@ private String craftFailMsg(String problematicNode, Deque<JCTree> astContext) {

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()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I was curious about the necessity of this line, as lombok generated code should not be present in the nodePositions map and, therefore, should not cause a failure.

After removing it locally, I observed additional, unrelated issues. I plan to address these issues in a separate task. If there are no other reasons for this check that I may have overlooked, I will proceed to remove the exclusion of lombok generated nodes after fixing the problems.

Copy link
Author

@amishra-u amishra-u Jan 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

it is due to this code, when a NonNull annotation is applied to a Record field, the NonNull handler modifies the compiler-generated constructor. (I’ve also included these details in the PR summary).

Another one where cloneType method fails to make copy of annotation argument.
https://github.com/projectlombok/lombok/blob/master/src/core/lombok/javac/handlers/HandleLog.java#L162

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();
}
}
}
}
Expand Down Expand Up @@ -289,14 +342,6 @@ public void scan(JCTree tree) {
super.scan(tree);
parents.pop();
}

/**
* We always generate shallow copies for annotations
*/
@Override
public void visitAnnotation(JCAnnotation tree) {
return;
}
});
}

Expand Down