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

Review: feat(position): add the position of the modifier in the ast #1959

Merged
merged 14 commits into from
Apr 13, 2018
Merged

Review: feat(position): add the position of the modifier in the ast #1959

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
b669990
fix(implicit): set the constructor implicit directly in jtdtreebuilder
tdurieux Apr 12, 2018
9f78689
add position to CtExtendedModifier
tdurieux Apr 12, 2018
6b4e368
feat(position): add the positin of the modifier in the ast
tdurieux Apr 12, 2018
c64acf7
add generic test
tdurieux Apr 12, 2018
342c870
improve test
tdurieux Apr 12, 2018
0b743a6
fix modifier position
tdurieux Apr 12, 2018
9acf736
remove log from test
tdurieux Apr 12, 2018
cc00222
never return a null position
tdurieux Apr 12, 2018
4f5cdb4
fix test
tdurieux Apr 12, 2018
0e8c755
fix test
tdurieux Apr 12, 2018
fcf983b
fix test
tdurieux Apr 12, 2018
e9030a5
remove log
tdurieux Apr 13, 2018
712689f
improve performance
tdurieux Apr 13, 2018
a852c54
fix index of string
tdurieux Apr 13, 2018
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
7 changes: 5 additions & 2 deletions src/main/java/spoon/support/compiler/jdt/JDTTreeBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -980,8 +980,11 @@ public boolean visit(ConstructorDeclaration constructorDeclaration, ClassScope s
if (constructorDeclaration.binding != null) {
c.setExtendedModifiers(getModifiers(constructorDeclaration.binding.modifiers, true, true));
}
for (CtExtendedModifier extendedModifier : getModifiers(constructorDeclaration.modifiers, false, true)) {
c.addModifier(extendedModifier.getKind()); // avoid to keep implicit AND explicit modifier of the same kind.
// avoid to add explicit modifier to implicit constructor
if (!c.isImplicit()) {
for (CtExtendedModifier extendedModifier : getModifiers(constructorDeclaration.modifiers, false, true)) {
c.addModifier(extendedModifier.getKind()); // avoid to keep implicit AND explicit modifier of the same kind.
}
}
context.enter(c, constructorDeclaration);

Expand Down
7 changes: 4 additions & 3 deletions src/main/java/spoon/support/compiler/jdt/JDTTreeBuilderHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -633,6 +633,10 @@ CtType<?> createType(TypeDeclaration typeDeclaration) {
} else {
type = jdtTreeBuilder.getFactory().Core().createClass();
}

// Setting modifiers
type.setExtendedModifiers(getModifiers(typeDeclaration.modifiers, false, false));

jdtTreeBuilder.getContextBuilder().enter(type, typeDeclaration);

if (typeDeclaration.superInterfaces != null) {
Expand All @@ -655,9 +659,6 @@ CtType<?> createType(TypeDeclaration typeDeclaration) {
type.setSimpleName(new String(typeDeclaration.name));
}

// Setting modifiers
type.setExtendedModifiers(getModifiers(typeDeclaration.modifiers, false, false));

return type;
}

Expand Down
81 changes: 60 additions & 21 deletions src/main/java/spoon/support/compiler/jdt/PositionBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,18 +23,26 @@
import org.eclipse.jdt.internal.compiler.ast.Annotation;
import org.eclipse.jdt.internal.compiler.ast.AnnotationMethodDeclaration;
import org.eclipse.jdt.internal.compiler.ast.Expression;
import org.eclipse.jdt.internal.compiler.ast.Initializer;
import org.eclipse.jdt.internal.compiler.ast.Javadoc;
import org.eclipse.jdt.internal.compiler.ast.MethodDeclaration;
import org.eclipse.jdt.internal.compiler.ast.TypeDeclaration;
import org.eclipse.jdt.internal.compiler.ast.TypeParameter;
import org.eclipse.jdt.internal.compiler.ast.TypeReference;
import spoon.SpoonException;
import spoon.reflect.code.CtCatchVariable;
import spoon.reflect.code.CtStatementList;
import spoon.reflect.cu.CompilationUnit;
import spoon.reflect.cu.SourcePosition;
import spoon.reflect.declaration.CtElement;
import spoon.reflect.declaration.CtPackage;
import spoon.reflect.declaration.CtModifiable;
import spoon.reflect.factory.CoreFactory;
import spoon.support.reflect.CtExtendedModifier;

import java.util.Arrays;
import java.util.Iterator;
import java.util.Set;

import static spoon.support.compiler.jdt.JDTTreeBuilderQuery.getModifiers;

Expand Down Expand Up @@ -94,18 +102,14 @@ SourcePosition buildPositionCtElement(CtElement e, ASTNode node) {
int declarationSourceStart = variableDeclaration.declarationSourceStart;
int declarationSourceEnd = variableDeclaration.declarationSourceEnd;

Annotation[] annotations = variableDeclaration.annotations;
if (annotations != null && annotations.length > 0) {
if (annotations[0].sourceStart() == sourceStart) {
modifiersSourceStart = annotations[annotations.length - 1].sourceEnd() + 2;
}
}
if (modifiersSourceStart == 0) {
if (modifiersSourceStart <= 0) {
modifiersSourceStart = declarationSourceStart;
}
int modifiersSourceEnd;
if (variableDeclaration.type != null) {
modifiersSourceEnd = variableDeclaration.type.sourceStart() - 2;
} else if (variableDeclaration instanceof Initializer) {
modifiersSourceEnd = ((Initializer) variableDeclaration).block.sourceStart;
} else {
// variable that has no type such as TypeParameter
modifiersSourceEnd = declarationSourceStart - 1;
Expand All @@ -114,6 +118,8 @@ SourcePosition buildPositionCtElement(CtElement e, ASTNode node) {
// when no modifier
if (modifiersSourceStart > modifiersSourceEnd) {
modifiersSourceEnd = modifiersSourceStart - 1;
} else if (e instanceof CtModifiable) {
setModifiersPosition((CtModifiable) e, modifiersSourceStart, modifiersSourceEnd);
}

return cf.createDeclarationSourcePosition(cu,
Expand All @@ -133,19 +139,16 @@ SourcePosition buildPositionCtElement(CtElement e, ASTNode node) {
int bodyStart = typeDeclaration.bodyStart;
int bodyEnd = typeDeclaration.bodyEnd;

Annotation[] annotations = typeDeclaration.annotations;
if (annotations != null && annotations.length > 0) {
if (annotations[0].sourceStart() == declarationSourceStart) {
modifiersSourceStart = findNextNonWhitespace(contents, declarationSourceEnd, annotations[annotations.length - 1].declarationSourceEnd + 1);
}
}
if (modifiersSourceStart == 0) {
if (modifiersSourceStart <= 0) {
modifiersSourceStart = declarationSourceStart;
}
//look for start of first keyword before the type keyword e.g. "class". `sourceStart` points at first char of type name
int modifiersSourceEnd = findPrevNonWhitespace(contents, modifiersSourceStart - 1,
findPrevWhitespace(contents, modifiersSourceStart - 1,
findPrevNonWhitespace(contents, modifiersSourceStart - 1, sourceStart - 1)));
if (e instanceof CtModifiable) {
setModifiersPosition((CtModifiable) e, modifiersSourceStart, bodyStart);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I have not read whole code yet, so may be it is correct, but usage of bodyStart here seems to be wrong.

Copy link
Collaborator

Choose a reason for hiding this comment

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

ok, the searched string is little bit longer then necessary, but I cannot imagine code where it would cause problem, so it is OK for me.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We are here in a TypeDeclaration the bodyStart is the first position that I'm sure will contain the modifier.
I'm pretty sure that the way to perform modifiersSourceEnd in this case does not support annotation and thus can be incorrect in some clases

}
if (modifiersSourceEnd < modifiersSourceStart) {
//there is no modifier
modifiersSourceEnd = modifiersSourceStart - 1;
Expand All @@ -172,7 +175,7 @@ SourcePosition buildPositionCtElement(CtElement e, ASTNode node) {
int declarationSourceEnd = methodDeclaration.declarationSourceEnd;
int modifiersSourceStart = methodDeclaration.modifiersSourceStart;

if (modifiersSourceStart == 0) {
if (modifiersSourceStart <= 0) {
modifiersSourceStart = declarationSourceStart;
}

Expand All @@ -186,15 +189,13 @@ SourcePosition buildPositionCtElement(CtElement e, ASTNode node) {
if (javadoc != null && javadoc.sourceEnd() > declarationSourceStart) {
modifiersSourceStart = javadoc.sourceEnd() + 1;
}
Annotation[] annotations = methodDeclaration.annotations;
if (annotations != null && annotations.length > 0) {
if (annotations[0].sourceStart() == declarationSourceStart) {
modifiersSourceStart = annotations[annotations.length - 1].sourceEnd() + 2;
}
}

int modifiersSourceEnd = sourceStart - 1;

if (e instanceof CtModifiable) {
setModifiersPosition((CtModifiable) e, modifiersSourceStart, declarationSourceEnd);
}

if (methodDeclaration instanceof MethodDeclaration && ((MethodDeclaration) methodDeclaration).returnType != null) {
modifiersSourceEnd = ((MethodDeclaration) methodDeclaration).returnType.sourceStart() - 2;
}
Expand Down Expand Up @@ -234,13 +235,51 @@ SourcePosition buildPositionCtElement(CtElement e, ASTNode node) {
bodyStart, bodyEnd,
lineSeparatorPositions);
}
} else if (e instanceof CtCatchVariable) {
Iterator<ASTPair> iterator = this.jdtTreeBuilder.getContextBuilder().stack.iterator();
iterator.next();
ASTPair next = iterator.next();
buildPositionCtElement(e, next.node);
sourceEnd = getSourceEndOfTypeReference(contents, (TypeReference) node, sourceEnd);
return cf.createSourcePosition(cu, sourceStart, sourceEnd, lineSeparatorPositions);
} else if (node instanceof TypeReference) {
sourceEnd = getSourceEndOfTypeReference(contents, (TypeReference) node, sourceEnd);
}

if (e instanceof CtModifiable) {
setModifiersPosition((CtModifiable) e, sourceStart, sourceEnd);
}
return cf.createSourcePosition(cu, sourceStart, sourceEnd, lineSeparatorPositions);
}


private void setModifiersPosition(CtModifiable e, int start, int end) {
CoreFactory cf = this.jdtTreeBuilder.getFactory().Core();
CompilationUnit cu = this.jdtTreeBuilder.getFactory().CompilationUnit().getOrCreate(new String(this.jdtTreeBuilder.getContextBuilder().compilationunitdeclaration.getFileName()));
CompilationResult cr = this.jdtTreeBuilder.getContextBuilder().compilationunitdeclaration.compilationResult;
char[] contents = cr.compilationUnit.getContents();

Set<CtExtendedModifier> modifiers = e.getExtendedModifiers();
if (start < 0 || end + 1 > contents.length) {
System.out.println(e);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it really wanted?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sorry, debug point that I forget to remove

}
String modifierContent = String.valueOf(Arrays.copyOfRange(contents, start, end + 1));
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess there is faster new String(contents, start, end+1) or something like that. Or why do we copy that array?

for (CtExtendedModifier modifier: modifiers) {
if (modifier.isImplicit()) {
modifier.setPosition(SourcePosition.NOPOSITION);
continue;
}
int index = modifierContent.indexOf(modifier.getKind().toString());
if (index == -1) {
throw new SpoonException("Explicit modifier not found");
}
int indexStart = index + start;
int indexEnd = indexStart + modifier.getKind().toString().length() - 1;

modifier.setPosition(cf.createSourcePosition(cu, indexStart, indexEnd, cr.lineSeparatorPositions));
}
}

private int getSourceEndOfTypeReference(char[] contents, TypeReference node, int sourceEnd) {
//e.g. SomeType<String,T>
TypeReference[][] typeArgs = ((TypeReference) node).getTypeArguments();
Expand Down
13 changes: 13 additions & 0 deletions src/main/java/spoon/support/reflect/CtExtendedModifier.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
*/
package spoon.support.reflect;

import spoon.reflect.cu.SourcePosition;
import spoon.reflect.declaration.ModifierKind;

import java.io.Serializable;
Expand All @@ -27,6 +28,7 @@
public class CtExtendedModifier implements Serializable {
private boolean implicit;
private ModifierKind kind;
private SourcePosition position;

public CtExtendedModifier(ModifierKind kind) {
this.kind = kind;
Expand All @@ -53,6 +55,17 @@ public void setKind(ModifierKind kind) {
this.kind = kind;
}

public SourcePosition getPosition() {
if (position == null) {
return SourcePosition.NOPOSITION;
}
return position;
Copy link
Collaborator

Choose a reason for hiding this comment

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

if (position == null) return NoSourcePosition;
WDYT? CtElement already never returns null.

}

public void setPosition(SourcePosition position) {
this.position = position;
}

@Override
public boolean equals(Object o) {
if (this == o) {
Expand Down
4 changes: 3 additions & 1 deletion src/test/java/spoon/test/interfaces/TestInterfaceWithoutSetup.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import org.apache.commons.lang3.StringUtils;
import org.junit.Test;
import spoon.Launcher;
import spoon.reflect.cu.position.NoSourcePosition;
import spoon.reflect.declaration.CtField;
import spoon.reflect.declaration.CtMethod;
import spoon.reflect.declaration.CtType;
Expand All @@ -12,7 +13,6 @@
import java.io.File;
import java.io.IOException;
import java.nio.file.Files;
import java.util.List;
import java.util.Set;

import static org.junit.Assert.assertEquals;
Expand Down Expand Up @@ -64,6 +64,8 @@ public void testModifierFromInterfaceFieldAndMethod() {
counter++;
} else {
assertFalse(extendedModifier.isImplicit());
assertFalse(extendedModifier.getPosition() instanceof NoSourcePosition);
assertEquals(extendedModifier.getKind().toString(), extendedModifier.getPosition().getCompilationUnit().getOriginalSourceCode().substring(extendedModifier.getPosition().getSourceStart(),extendedModifier.getPosition().getSourceEnd() + 1));
counter++;
}
}
Expand Down
34 changes: 25 additions & 9 deletions src/test/java/spoon/test/main/MainTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,14 @@
import spoon.reflect.code.CtFieldWrite;
import spoon.reflect.code.CtLambda;
import spoon.reflect.code.CtVariableWrite;
import spoon.reflect.cu.CompilationUnit;
import spoon.reflect.cu.SourcePosition;
import spoon.reflect.declaration.CtConstructor;
import spoon.reflect.declaration.CtElement;
import spoon.reflect.declaration.CtExecutable;
import spoon.reflect.declaration.CtField;
import spoon.reflect.declaration.CtMethod;
import spoon.reflect.declaration.CtModifiable;
import spoon.reflect.declaration.CtPackage;
import spoon.reflect.declaration.CtShadowable;
import spoon.reflect.declaration.CtType;
Expand All @@ -36,24 +39,21 @@
import spoon.reflect.visitor.CtScanner;
import spoon.reflect.visitor.PrinterHelper;
import spoon.reflect.visitor.filter.TypeFilter;
import spoon.support.reflect.CtExtendedModifier;
import spoon.test.parent.ParentTest;

import java.io.ByteArrayOutputStream;
import java.io.File;
import java.io.PrintStream;
import java.io.PrintWriter;
import java.io.StringWriter;

import java.util.Arrays;
import java.util.List;
import java.util.LinkedList;
import java.util.Set;
import java.util.HashSet;
import java.util.Deque;
import java.util.ArrayDeque;
import java.util.Map;
import java.util.IdentityHashMap;
import java.util.Collection;
import java.util.Deque;
import java.util.HashSet;
import java.util.IdentityHashMap;
import java.util.Map;
import java.util.Set;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
Expand Down Expand Up @@ -114,6 +114,22 @@ public void testMain_checkParentConsistency() {
checkParentConsistency(rootPackage);
}

@Test
public void testMain_checkModifiers() {
// the explicit modifier should be present in the original source code
for (CtModifiable modifiable: rootPackage.getElements(new TypeFilter<>(CtModifiable.class))) {
for (CtExtendedModifier modifier: modifiable.getExtendedModifiers()) {
if (modifier.isImplicit()) {
continue;
}
SourcePosition position = modifier.getPosition();
CompilationUnit compilationUnit = position.getCompilationUnit();
String originalSourceCode = compilationUnit.getOriginalSourceCode();
assertEquals(modifier.getKind().toString(), originalSourceCode.substring(position.getSourceStart(), position.getSourceEnd() + 1));
}
}
}

public void checkGenericContracts(CtPackage pack) {
// parent
ParentTest.checkParentContract(pack);
Expand Down
7 changes: 4 additions & 3 deletions src/test/java/spoon/test/position/PositionTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ public void testPositionClass() throws Exception {
assertEquals(4, foo2.getPosition().getEndLine());

assertEquals("FooClazz", contentAtPosition(classContent, position.getNameStart(), position.getNameEnd()));
assertEquals("public", contentAtPosition(classContent, position.getModifierSourceStart(), position.getModifierSourceEnd()));
assertEquals("@Deprecated\npublic", contentAtPosition(classContent, position.getModifierSourceStart(), position.getModifierSourceEnd()));
}


Expand Down Expand Up @@ -166,7 +166,7 @@ public void testPositionInterface() throws Exception {
assertEquals("{\n\n}", contentAtPosition(classContent, position.getBodyStart(), position.getBodyEnd()));

assertEquals("FooInterface", contentAtPosition(classContent, position.getNameStart(), position.getNameEnd()));
assertEquals("public", contentAtPosition(classContent, position.getModifierSourceStart(), position.getModifierSourceEnd()));
assertEquals("@Deprecated\n@InnerAnnot(value=\"machin\")\npublic", contentAtPosition(classContent, position.getModifierSourceStart(), position.getModifierSourceEnd()));

{
SourcePosition annPosition = foo.getAnnotations().get(0).getPosition();
Expand Down Expand Up @@ -202,7 +202,8 @@ public void testPositionAnnotation() throws Exception {
+ "}", contentAtPosition(classContent, position.getBodyStart(), position.getBodyEnd()));

assertEquals("FooAnnotation", contentAtPosition(classContent, position.getNameStart(), position.getNameEnd()));
assertEquals("public abstract", contentAtPosition(classContent, position.getModifierSourceStart(), position.getModifierSourceEnd()));
assertEquals("@Target(value={})\n"
+ "@Retention(RetentionPolicy.RUNTIME) \npublic abstract", contentAtPosition(classContent, position.getModifierSourceStart(), position.getModifierSourceEnd()));

CtMethod<?> method1 = foo.getMethodsByName("value").get(0);
BodyHolderSourcePosition position1 = (BodyHolderSourcePosition) method1.getPosition();
Expand Down