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.

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: Adds unique serializable path for each CtElement from Root Element. #1874

Merged
merged 28 commits into from
Feb 26, 2018
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
24e6e0b
add unique path to element getter
nharrand Feb 21, 2018
5163384
refactor test unique path
nharrand Feb 21, 2018
d3bd277
refactor: remove unecessary exception for CtElement.getPath()
nharrand Feb 21, 2018
b1b806f
refactor: remove unecessary exception for CtElement.getPath()
nharrand Feb 21, 2018
64a2758
fix(CtPathStringBuilder): Now supports CtUniqueRolePath
nharrand Feb 21, 2018
eda3e7c
doc(MainTest.testElementToPathToElementEquivalency): adds contract info
nharrand Feb 21, 2018
afc30dd
refactor(MainTest): cosmetic
nharrand Feb 22, 2018
e3e66c4
merge
nharrand Feb 23, 2018
3c69d39
doc(CtElementPathBuilder): add missing javadoc
nharrand Feb 23, 2018
be56d34
refactor: fix coding style
nharrand Feb 23, 2018
1e604a7
fix: MainTest merging error
nharrand Feb 23, 2018
14e0c2c
Update CtPathStringBuilder.java
monperrus Feb 23, 2018
aa826b8
refactor(CtRolePathElement): Replace old behavior by new one from CtU…
nharrand Feb 24, 2018
7865217
refactor(CtRolePathElement): fix coding style
nharrand Feb 24, 2018
45d6820
doc(CtPathStringBuilder): fix javadoc
nharrand Feb 24, 2018
8bc9df3
fix(CtElementImpl.getPath()): unsilence exception.
nharrand Feb 25, 2018
e2b309c
Merge branch 'feature-unique-path' of github.com:nharrand/spoon into …
nharrand Feb 25, 2018
a7290f4
refactor(CtRolePathElement,CtElementPathBuilder): uses RoleHandlerHelper
nharrand Feb 25, 2018
b2266ae
refactor(CtRolePathElement): remove old commented implem
nharrand Feb 25, 2018
1aaef21
Update path.md
monperrus Feb 25, 2018
d846e6f
Update CtElement.java
monperrus Feb 25, 2018
047f914
adds super.scan because the diff coverage is strange
monperrus Feb 25, 2018
b1f2276
fix(CtElementPathBuilder): List.indexOf rely on equality not identity
nharrand Feb 25, 2018
786d411
refactor(CtElementPathBuilder): fix coding style
nharrand Feb 25, 2018
8f82a94
simplify api of evaluateOn
monperrus Feb 26, 2018
8cfc1ad
fix(CtRolePathElement): the evaluation of a path that leads nowhere r…
nharrand Feb 26, 2018
67ec78b
fix(CtRolePathElement): extends unspecified index behavior to sets an…
nharrand Feb 26, 2018
7ee9921
fix(CtRolePathElement): replace exception throwing with return null w…
nharrand Feb 26, 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
2 changes: 1 addition & 1 deletion doc/path.md
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ For instance, if we want the first statement in the body of method `foo`, declar
in the class `spoon.test.path.Foo`.

```java
new CtPathStringBuilder().fromString(".spoon.test.path.Foo.foo#body[index=0]");
new CtPathStringBuilder().fromString(".spoon.test.path.Foo.foo#body#statement[index=0]");
```

## CtPathBuilder
Expand Down
7 changes: 7 additions & 0 deletions src/main/java/spoon/reflect/declaration/CtElement.java
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import spoon.processing.FactoryAccessor;
import spoon.reflect.code.CtComment;
import spoon.reflect.cu.SourcePosition;
import spoon.reflect.path.CtPath;
import spoon.reflect.path.CtRole;
import spoon.reflect.reference.CtTypeReference;
import spoon.reflect.visitor.CtVisitable;
Expand Down Expand Up @@ -349,4 +350,10 @@ <E extends CtElement> List<E> getAnnotatedChildren(
* @param value to be assigned to this field.
*/
<E extends CtElement, T> E setValueByRole(CtRole role, T value);

/**
* Gets a unique path from models root to the CtElement.
*/
CtPath getPath();

}
94 changes: 94 additions & 0 deletions src/main/java/spoon/reflect/path/CtElementPathBuilder.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,94 @@
package spoon.reflect.path;

import spoon.reflect.declaration.CtElement;
import spoon.reflect.declaration.CtNamedElement;
import spoon.reflect.path.impl.CtPathElement;
import spoon.reflect.path.impl.CtPathImpl;
import spoon.reflect.path.impl.CtRolePathElement;
import spoon.reflect.reference.CtReference;

import java.util.List;
import java.util.Map;
import java.util.Set;

/**
* This builder allow to create some CtPath from CtElements
*
* Created by nharrand on 21/02/2018.
*/
public class CtElementPathBuilder {
/**
* Build path to a CtElement el, from one of its parent.
*
* @throws CtPathException is thrown when root is not a parent of el.
*
* @param el : the element to which the CtPath leads to
* @param root : Starting point of the CtPath
* @return CtPath from root to el
*/
public CtPath fromElement(CtElement el, CtElement root) throws CtPathException {
CtPathImpl path = new CtPathImpl();
CtElement cur = el;
while (cur != root) {
CtElement parent = cur.getParent();
CtRole role = cur.getRoleInParent();
if (parent == null || role == null) {
throw new CtPathException();
}
CtPathElement pathElement = new CtRolePathElement(role);
if (parent.getValueByRole(role) instanceof List) {
//Element needs to be differentiated from its brothers
List list = parent.getValueByRole(role);
//Assumes that List's order is deterministic.
int index = 0;
for (Object o : list) {
if (o == cur) {
break;
}
index++;
}
pathElement.addArgument("index", index + "");
} else if (parent.getValueByRole(role) instanceof Set) {
if (!(cur instanceof CtNamedElement) && !(cur instanceof CtReference)) {
throw new CtPathException();
}
//Element needs to be differentiated from its brothers
Set set = parent.getValueByRole(role);
String name = null;
for (Object o : set) {
if (o == cur) {
if (cur instanceof CtNamedElement) {
name = ((CtNamedElement) cur).getSimpleName();
} else {
name = ((CtReference) cur).getSimpleName();
}
break;
}
}
if (name == null) {
throw new CtPathException();
} else {
pathElement.addArgument("name", name);
}

} else if (parent.getValueByRole(role) instanceof Map) {
Map map = parent.getValueByRole(role);
String key = null;
for (Object o : map.keySet()) {
if (map.get(o) == cur) {
key = (String) o;
break;
}
}
if (key == null) {
throw new CtPathException();
} else {
pathElement.addArgument("key", key);
}
}
cur = parent;
path.addFirst(pathElement);
}
return path;
}
}
3 changes: 2 additions & 1 deletion src/main/java/spoon/reflect/path/CtPathStringBuilder.java
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,12 @@
*/
package spoon.reflect.path;


import spoon.reflect.path.impl.CtNamedPathElement;
import spoon.reflect.path.impl.CtPathElement;
import spoon.reflect.path.impl.CtPathImpl;
import spoon.reflect.path.impl.CtRolePathElement;
import spoon.reflect.path.impl.CtTypedNameElement;
import spoon.reflect.path.impl.CtRolePathElement;

import java.util.regex.Matcher;
import java.util.regex.Pattern;
Expand Down
118 changes: 55 additions & 63 deletions src/main/java/spoon/reflect/path/impl/CtRolePathElement.java
Original file line number Diff line number Diff line change
Expand Up @@ -16,15 +16,18 @@
*/
package spoon.reflect.path.impl;

import spoon.reflect.code.CtIf;
import spoon.SpoonException;
import spoon.reflect.declaration.CtElement;
import spoon.reflect.declaration.CtExecutable;
import spoon.reflect.declaration.CtField;
import spoon.reflect.declaration.CtNamedElement;
import spoon.reflect.path.CtPathException;
import spoon.reflect.path.CtRole;
import spoon.reflect.visitor.CtInheritanceScanner;
import spoon.reflect.reference.CtReference;

import java.util.Collection;
import java.util.LinkedList;
import java.util.List;
import java.util.Map;
import java.util.Set;

/**
* A CtPathElement that define some roles for matching.
Expand All @@ -40,60 +43,6 @@ public class CtRolePathElement extends AbstractPathElement<CtElement, CtElement>

public static final String STRING = "#";

private class RoleVisitor extends CtInheritanceScanner {
private Collection<CtElement> matchs = new LinkedList<>();

private RoleVisitor() {
}

@Override
public <R> void scanCtExecutable(CtExecutable<R> e) {
super.scanCtExecutable(e);

switch (role) {
case BODY:
if (e.getBody() != null) {
if (getArguments().containsKey("index")
&& e.getBody()
.getStatements()
.size() > Integer.parseInt(
getArguments().get("index"))) {
matchs.add(e.getBody().getStatements().get(Integer
.parseInt(getArguments().get("index"))));
} else {
matchs.addAll(e.getBody().getStatements());
}
}
}

}

@Override
public <T> void visitCtField(CtField<T> e) {
super.visitCtField(e);

if (role == CtRole.DEFAULT_EXPRESSION && e.getDefaultExpression() != null) {
matchs.add(e.getDefaultExpression());
}
}

@Override
public void visitCtIf(CtIf e) {
super.visitCtIf(e);

switch (role) {
case THEN:
if (e.getThenStatement() != null) {
matchs.add(e.getThenStatement());
}
case ELSE:
if (e.getElseStatement() != null) {
matchs.add(e.getElseStatement());
}
}
}
}

private final CtRole role;

public CtRolePathElement(CtRole role) {
Expand All @@ -106,14 +55,57 @@ public CtRole getRole() {

@Override
public String toString() {
return STRING + role.toString() + getParamString();
return STRING + getRole().toString() + getParamString();
}

public CtElement getFromSet(Set set, String name) throws CtPathException {
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we need it public? Note: In Spoon we try to make public API as small as possible. It is easier to maintain and refactor then.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, we don't. I'll change it.

for (Object o: set) {
if (o instanceof CtNamedElement) {
if (((CtNamedElement) o).getSimpleName().equals(name)) {
return (CtElement) o;
}
} else if (o instanceof CtReference) {
if (((CtReference) o).getSimpleName().equals(name)) {
return (CtElement) o;
}
} else {
throw new CtPathException();
}
}
throw new CtPathException();
Copy link
Collaborator

Choose a reason for hiding this comment

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

throwing of exception as part of normal program flow is not a good idea. It has a performance impact. So it is much faster (main line debug mode), when you return null and check it in caller.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in 7ee9921

}

@Override
public Collection<CtElement> getElements(Collection<CtElement> roots) {
RoleVisitor visitor = new RoleVisitor();
visitor.scan(roots);
return visitor.matchs;
Collection<CtElement> matchs = new LinkedList<>();
for (CtElement root : roots) {
try {
if (root.getValueByRole(getRole()) instanceof List) {
if (getArguments().containsKey("index")) {
int index = Integer.parseInt(getArguments().get("index"));
matchs.add((CtElement) ((List) root.getValueByRole(getRole())).get(index));
}
} else if (root.getValueByRole(getRole()) instanceof Set) {
if (getArguments().containsKey("name")) {
String name = getArguments().get("name");
try {
matchs.add(getFromSet(root.getValueByRole(getRole()), name));
} catch (CtPathException e) {
//System.err.println("[ERROR] Element not found for name: " + name);
//No element found for name.
}
}
} else if (root.getValueByRole(getRole()) instanceof Map) {
if (getArguments().containsKey("key")) {
String name = getArguments().get("key");
matchs.add((CtElement) ((Map) root.getValueByRole(getRole())).get(name));
}
} else {
CtElement el = root.getValueByRole(getRole());
matchs.add(el);
}
} catch (SpoonException e) { } //When no element are found for a given role, return empty list.
Copy link
Collaborator

Choose a reason for hiding this comment

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

same thing here, do we want this exception to be silent?

Copy link
Collaborator Author

@nharrand nharrand Feb 25, 2018

Choose a reason for hiding this comment

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

Yes, imo, because as it is, if we do not silence this exception, wildcards are broken.
It is thrown when we query a node for children with a given role that does not exist among its children.

Copy link
Collaborator Author

@nharrand nharrand Feb 25, 2018

Choose a reason for hiding this comment

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

I guess one alternative would be adding a method boolean hasChildWith(CtRole role) to CtElement.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It is already there. See RoleHandlerHelper#getOptionalRoleHandler
Have a look also at RoleHandler#getContainerKind, which might be used instead of root.getValueByRole(getRole()) instanceof Set and similar

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@pvojtechovsky Thanks for the tip.
Problem solved.

}
return matchs;
}

}
12 changes: 12 additions & 0 deletions src/main/java/spoon/support/reflect/declaration/CtElementImpl.java
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
package spoon.support.reflect.declaration;

import org.apache.log4j.Logger;
import spoon.reflect.CtModelImpl;
import spoon.reflect.annotations.MetamodelPropertyField;
import spoon.reflect.code.CtComment;
import spoon.reflect.code.CtJavaDoc;
Expand All @@ -31,6 +32,9 @@
import spoon.reflect.factory.FactoryImpl;
import spoon.reflect.meta.RoleHandler;
import spoon.reflect.meta.impl.RoleHandlerHelper;
import spoon.reflect.path.CtElementPathBuilder;
import spoon.reflect.path.CtPath;
import spoon.reflect.path.CtPathException;
import spoon.reflect.path.CtRole;
import spoon.reflect.declaration.CtImport;
import spoon.reflect.reference.CtReference;
Expand Down Expand Up @@ -536,4 +540,12 @@ public <E extends CtElement, T> E setValueByRole(CtRole role, T value) {
rh.setValue(this, value);
return (E) this;
}

public CtPath getPath() {
try {
Copy link
Collaborator

Choose a reason for hiding this comment

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

why not removing the try/catch and let the exception go?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The exception is supposed to be risen from CtElementPathBuilder().fromElement(CtElement el, CtElement from) when from is not a parent of el. So in this case, I think it can't happen and I didn't want to annoy the user with the handling of an exception which is not supposed to happen.
But still, we can let it go.

Copy link
Collaborator

Choose a reason for hiding this comment

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

"let it go" then :-)

Copy link
Collaborator Author

@nharrand nharrand Feb 25, 2018

Choose a reason for hiding this comment

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

I have a counter proposal: Why not catch CtPathException (which should not happen in that context) and throw a SpoonException to show that it's an spoon implementation error. Further more it would avoid that the user has to handle it.

try {
	return new CtElementPathBuilder().fromElement(this, getParent(CtModelImpl.CtRootPackage.class));
} catch (CtPathException e) {
	throw new SpoonException();
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK for me.

return new CtElementPathBuilder().fromElement(this, getParent(CtModelImpl.CtRootPackage.class));
} catch (CtPathException e) {
}
return null;
}
}
43 changes: 38 additions & 5 deletions src/test/java/spoon/test/main/MainTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,9 @@
import spoon.reflect.declaration.CtType;
import spoon.reflect.declaration.CtTypeParameter;
import spoon.reflect.declaration.ParentNotInitializedException;
import spoon.reflect.path.CtPath;
import spoon.reflect.path.CtPathException;
import spoon.reflect.path.CtPathStringBuilder;
import spoon.reflect.path.CtRole;
import spoon.reflect.reference.CtArrayTypeReference;
import spoon.reflect.reference.CtExecutableReference;
Expand All @@ -40,13 +43,17 @@
import java.io.PrintStream;
import java.io.PrintWriter;
import java.io.StringWriter;
import java.util.ArrayDeque;
import java.util.Collection;
import java.util.Deque;

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

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
Expand Down Expand Up @@ -441,6 +448,32 @@ public void scan(CtRole role, CtElement element) {
});
}

@Test
public void testElementToPathToElementEquivalency() {

List<CtElement> list = Arrays.asList(rootPackage);
rootPackage.accept(new CtScanner() {
@Override
public void scan(CtElement element) {
if (element != null) {
CtPath path = element.getPath();
String pathStr = path.toString();
try {
CtPath pathRead = new CtPathStringBuilder().fromString(pathStr);
Collection<CtElement> returnedElements = pathRead.evaluateOn(list);
//contract: CtUniqueRolePathElement.evaluateOn() returns a unique elements if provided only a list of one inputs
assertEquals(returnedElements.size(), 1);
CtElement actualElement = (CtElement) returnedElements.toArray()[0];
//contract: Element -> Path -> String -> Path -> Element leads to the original element
assertSame(element, actualElement);
} catch (CtPathException e) {
fail("Path is either incorrectly generated or incorrectly read");
}
}
}
});
}

@Test
public void testElementIsContainedInAttributeOfItsParent() {
rootPackage.accept(new CtScanner() {
Expand Down
Loading