feat: Adds unique serializable path for each CtElement from Root Element. #1874

nharrand · 2018-02-21T17:25:08Z

The goal of this PR is to add support for getting an unique CtPath for each CtElement of a model. In particular this could be used for persistence. Any given CtElement can be access through an unique deterministic serializable path. All the following transformations are supported: CtElement -> CtPath -> String -> CtPath -> CtElement.

Main content:

Method CtPath CtElement.getPath(): Returns the path from root element to this.
Class CtElementPathBuilder: Build a CtPath from a given CtElement
Class CtUniqueRolePathElement : Extends CtRolePathElement with a new behavior and a new syntax.
Method MainTest.testElementToPathToElementEquivalency(): Test CtElement -> CtPath -> String -> CtPath -> CtElement

In order to integrate this to CtPathStringBuilder without conflict, the char '@' is used for this new path element.

The syntax for paths is the following

For unique CtElement -> @role
For elements in a List -> @role[index=ID]
For elements in a Set of CtElement for which name exists and is unique (Either CtNamedElement or CtReference) -> @role[name=element.getSimpleName()]
For elements in a Map<String, CtElement> -> @role[key=String]

@subPackage[name=spoon]@subPackage[name=compiler]@containedType[name=Environment]@typeMember[index=0] -> spoon.compiler.Environment 's first type member ie: int getComplianceLevel();

Currently the test fails (related to #1872).

What do you think?

monperrus · 2018-02-21T19:20:44Z

src/main/java/spoon/support/reflect/declaration/CtElementImpl.java

@@ -536,4 +540,12 @@ public CtElement clone() {
 		rh.setValue(this, value);
 		return (E) this;
 	}
+
+	public CtPath getPath() {
+		try {


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

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.

"let it go" then :-)

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

monperrus · 2018-02-21T19:22:17Z

src/test/java/spoon/test/main/MainTest.java

+						assertEquals(returnedElements.size(), 1);
+						CtElement actualElement = (CtElement) returnedElements.toArray()[0];
+						//contract: Element -> Path -> String -> Path -> Element leads to the original element
+						assertEquals(element, actualElement);


assertSame?

Yes, that sounds better. I'll fix it ASAP.

monperrus · 2018-02-21T19:24:25Z

looks super cool, I love the strong generic test.

are you working on fixing #1872?

nharrand · 2018-02-21T20:20:32Z

@monperrus I tried to add:

	static class CtCatchVariable_TYPE_RoleHandler extends ListHandler<CtCatchVariable, CtTypeReference<?>> {
		private CtCatchVariable_TYPE_RoleHandler() {
			super(CtRole.TYPE, CtCatchVariable.class, CtTypeReference.class);
		}

		@SuppressWarnings("unchecked")
		@Override
		public <T, U> U getValue(T element) {
			return ((U) ((Object) (castTarget(element).getMultiTypes())));
		}

		@Override
		public <T, U> void setValue(T element, U value) {
			castTarget(element).setMultiTypes(castValue(value));
		}
}

to ModelRoleHandlers, which fixed the issue but broke another test. I realized that this code is actually generated, if I'm not mistaken. So, this is where I got stuck.

nharrand · 2018-02-23T16:40:03Z

@monperrus I'm not sure why does the Travis CI build fails. It mentions Javadoc errors in files that I haven't changed. Any ideas?

Do you want to keep the current syntax for CtUniqueRolePathElement when transformed to String? Do you want me to merge it to the pre existing syntax?

monperrus · 2018-02-23T17:58:56Z

I propose to replace the previous implementation, to keep the old syntax with slash, and thus to update CtPathTest. It means we would drop the support for wildcard temporarily, but I'm OK with that. (This was an experimental partial feature, with no user to my knowledge).

WDYT?

monperrus · 2018-02-23T18:02:04Z

Fixed the Javadoc error at CtPathStringBuilder.java line 71 (please pull).

(Maven prefixes "warnings" of the Javadoc linter by [Error] which is super confusing, but later on the line it's clear that it is only a warning)

monperrus · 2018-02-23T18:19:44Z

Do you want me to merge it to the pre existing syntax?

Yes that would be great, see #1874 (comment)

…niqueRolePathElement

nharrand · 2018-02-24T17:28:17Z

We can keep the wildcards, they are a part of CtNamedPathElement, and can already be used in combination of the new syntax.
The only change is that blocks cannot be omitted anymore.
Example:

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

becomes

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

monperrus · 2018-02-25T00:32:36Z

the change is OK for me.

it seems we're close to merge!

monperrus · 2018-02-25T09:19:13Z

src/main/java/spoon/reflect/path/impl/CtRolePathElement.java

+					CtElement el = root.getValueByRole(getRole());
+					matchs.add(el);
+				}
+			} catch (SpoonException e) { } //When no element are found for a given role, return empty list.


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

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.

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

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

@pvojtechovsky Thanks for the tip.
Problem solved.

…feature-unique-path

monperrus · 2018-02-25T20:32:07Z

OK for me. Will merge it. That's a really nice contribution to Spoon, thanks!

monperrus · 2018-02-25T21:37:06Z

what about changing the signature of evaluateOn to evaluateOn(CtElement...) (varargs)?. the API would be easier for clients.

see https://coveralls.io/builds/15690018/source?filename=src%2Fmain%2Fjava%2Fspoon%2Freflect%2Fpath%2FCtElementPathBuilder.java#L38

monperrus · 2018-02-25T22:13:50Z

sorry @nharrand , I fixed the test by calling super.scan(element), and it seems it now finds a bug :-)

nharrand · 2018-02-25T22:17:04Z

Yes, I see that! I'm looking into that.

nharrand · 2018-02-25T22:54:29Z

Ok this should work now.

pvojtechovsky · 2018-02-26T13:12:08Z

src/main/java/spoon/reflect/path/impl/CtRolePathElement.java

+						if (getArguments().containsKey("index")) {
+							int index = Integer.parseInt(getArguments().get("index"));
+							matchs.add((CtElement) roleHandler.asList(root).get(index));
+						}


else it should add all items of the into matchs. WDYT?

and what about case when index >= size()?

Yes, you are right. I only had in mind my Unique path case in mind here, but yes. I'll fix this

Right, that also means that I should add negative test cases...

else it should add all items of the into matchs.

We should extends that behavior to sets and maps, don't you think?

Yes, I agree. For Set it should add all items of the Set. And for Map it should add all values of the map.

Implemented in 67ec78b

pvojtechovsky · 2018-02-26T13:13:58Z

src/main/java/spoon/reflect/path/impl/CtRolePathElement.java

+					case MAP:
+						if (getArguments().containsKey("key")) {
+							String name = getArguments().get("key");
+							matchs.add((CtElement) roleHandler.asMap(root).get(name));


I guess it is not correct to add null into matchs, when map doesn't contain a key

Indeed. I'll fix this too.

pvojtechovsky · 2018-02-26T13:17:41Z

src/main/java/spoon/support/reflect/declaration/CtElementImpl.java

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


throw new SpoonException(e); ... the orgin exception should be used as cause of the SpoonException

Ok. But, my idea was that CtElementPathBuilder().fromElement(CtElement to, CtElement from) would throw a CtPathException if from was no parent of to which means that the user made a mistake. But in the specific context of this call, as the arguments are an element and root package, no exception should happen unless the spoon implementation is at fault, therefor SpoonException.
WDYT?

Yes, no exception should happen, but if it happens because of some mistake in spoon code, then your current implementation will avoid efficient detection of the cause, because the cause of the problem is lost. So just type in e into brackets and I am OK with that ;-)

just type in e into brackets

What do you mean? Shall I change throw new SpoonException(); into throw new SpoonException(e); ?

Done in 8cfc1ad

…eturns an empty collection. Add test for such events

…d maps. add tests

pvojtechovsky · 2018-02-26T17:09:39Z

src/main/java/spoon/reflect/path/impl/CtRolePathElement.java

+		return STRING + getRole().toString() + getParamString();
+	}
+
+	public CtElement getFromSet(Set set, String name) throws CtPathException {


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.

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

pvojtechovsky · 2018-02-26T17:10:54Z

src/main/java/spoon/reflect/path/impl/CtRolePathElement.java

+				throw new CtPathException();
+			}
+		}
+		throw new CtPathException();


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.

Fixed in 7ee9921

…hen element is not found in set

pvojtechovsky · 2018-02-26T17:24:38Z

It looks good to me. I am looking forward to use this new Spoon feature! Thank You @nharrand .

I guess we can merge. @monperrus WDYT?

spoon-bot · 2018-02-26T17:29:12Z

API changes: 2 (Detected by Revapi)

Old API: fr.inria.gforge.spoon:spoon-core:jar:6.2.0-20180225.235035-111 / New API: fr.inria.gforge.spoon:spoon-core:jar:6.2.0-SNAPSHOT

	Method was added to an interface.
Old	none
New	method CtElement#getPath()
Breaking	binary: non_breaking

	The type of the parameter changed from 'java.util.Collection<? extends spoon.reflect.declaration.CtElement>' to 'spoon.reflect.declaration.CtElement[]'.
Old	parameter CtPath#evaluateOn(===Collection===)
New	parameter CtPath#evaluateOn(===.Array===)
Breaking	binary: breaking

monperrus · 2018-02-26T19:44:51Z

OK to merge! This opens super exciting new usages.

Kudos and thanks to @nharrand

nharrand · 2018-02-26T19:58:32Z

You are welcome. Thanks for the reviews.

I'm a little disappointed that it counts as only one commit though... ;-)

monperrus · 2018-02-27T15:38:50Z

that's the tough law of squashing!

@nharrand

Dear all, I'd like to nominate Nicolas Harrand (@nharrand) to become an integrator in Spoon. Nicolas has done several key contributions over the past year: * he has added support for paths in order to uniquely identify AST nodes (#1874), this feature is unique (to my knowledge, it does not exist in any other source code analysis library for Java), and enables original source code analysis over time and versions. * he has added support for analysis of binary files with decompilation (#2455) * he has laid down the foundations of a modularized Spoon, which will likely be key in the future * he has ported Spoon to JDK 11 (#2789). This kind of infrastructure contribution is also very important in the long term. In addition, Nicolas has proven his ability to communicate constructively and respectfully over issues and pull-requests. What do you think?

nharrand added 6 commits February 21, 2018 14:34

add unique path to element getter

24e6e0b

refactor test unique path

5163384

refactor: remove unecessary exception for CtElement.getPath()

d3bd277

refactor: remove unecessary exception for CtElement.getPath()

b1b806f

fix(CtPathStringBuilder): Now supports CtUniqueRolePath

64a2758

doc(MainTest.testElementToPathToElementEquivalency): adds contract info

eda3e7c

monperrus reviewed Feb 21, 2018

View reviewed changes

nharrand added 4 commits February 22, 2018 11:48

refactor(MainTest): cosmetic

afc30dd

merge

e3e66c4

doc(CtElementPathBuilder): add missing javadoc

3c69d39

refactor: fix coding style

be56d34

fix: MainTest merging error

1e604a7

Update CtPathStringBuilder.java

14e0c2c

nharrand added 2 commits February 24, 2018 18:15

refactor(CtRolePathElement): Replace old behavior by new one from CtU…

aa826b8

…niqueRolePathElement

refactor(CtRolePathElement): fix coding style

7865217

doc(CtPathStringBuilder): fix javadoc

45d6820

monperrus reviewed Feb 25, 2018

View reviewed changes

nharrand added 4 commits February 25, 2018 10:24

fix(CtElementImpl.getPath()): unsilence exception.

8bc9df3

Merge branch 'feature-unique-path' of github.com:nharrand/spoon into …

e2b309c

…feature-unique-path

refactor(CtRolePathElement,CtElementPathBuilder): uses RoleHandlerHelper

a7290f4

refactor(CtRolePathElement): remove old commented implem

b2266ae

Update path.md

1aaef21

monperrus added 2 commits February 25, 2018 22:38

Update CtElement.java

d846e6f

adds super.scan because the diff coverage is strange

047f914

see https://coveralls.io/builds/15690018/source?filename=src%2Fmain%2Fjava%2Fspoon%2Freflect%2Fpath%2FCtElementPathBuilder.java#L38

fix(CtElementPathBuilder): List.indexOf rely on equality not identity

b1f2276

nharrand and others added 2 commits February 26, 2018 00:30

refactor(CtElementPathBuilder): fix coding style

786d411

simplify api of evaluateOn

8f82a94

pvojtechovsky reviewed Feb 26, 2018

View reviewed changes

nharrand added 2 commits February 26, 2018 15:49

fix(CtRolePathElement): the evaluation of a path that leads nowhere r…

8cfc1ad

…eturns an empty collection. Add test for such events

fix(CtRolePathElement): extends unspecified index behavior to sets an…

67ec78b

…d maps. add tests

pvojtechovsky reviewed Feb 26, 2018

View reviewed changes

fix(CtRolePathElement): replace exception throwing with return null w…

7ee9921

…hen element is not found in set

monperrus merged commit fd9d6c0 into INRIA:master Feb 26, 2018

nharrand deleted the feature-unique-path branch February 26, 2018 20:18

sbihel mentioned this pull request Feb 28, 2018

add support for minimization of amplified tests STAMP-project/dspot#54

Open

surli mentioned this pull request Mar 6, 2018

New release: Spoon 6.2.0 is here #1895

Closed

monperrus mentioned this pull request Dec 4, 2018

community: Nicolas Harrand becomes integrator #2818

Merged

feat: Adds unique serializable path for each CtElement from Root Element. #1874

feat: Adds unique serializable path for each CtElement from Root Element. #1874

Conversation

nharrand commented Feb 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nharrand Feb 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

monperrus commented Feb 21, 2018

nharrand commented Feb 21, 2018 • edited Loading

nharrand commented Feb 23, 2018

monperrus commented Feb 23, 2018

monperrus commented Feb 23, 2018

monperrus commented Feb 23, 2018 • edited Loading

nharrand commented Feb 24, 2018 • edited Loading

monperrus commented Feb 25, 2018

Choose a reason for hiding this comment

nharrand Feb 25, 2018 • edited Loading

Choose a reason for hiding this comment

nharrand Feb 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

monperrus commented Feb 25, 2018

monperrus commented Feb 25, 2018

monperrus commented Feb 25, 2018

nharrand commented Feb 25, 2018

nharrand commented Feb 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nharrand Feb 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nharrand Feb 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nharrand Feb 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pvojtechovsky commented Feb 26, 2018

spoon-bot commented Feb 26, 2018

monperrus commented Feb 26, 2018

nharrand commented Feb 26, 2018

monperrus commented Feb 27, 2018 via email

nharrand Feb 25, 2018 •

edited

Loading

nharrand commented Feb 21, 2018 •

edited

Loading

monperrus commented Feb 23, 2018 •

edited

Loading

nharrand commented Feb 24, 2018 •

edited

Loading

nharrand Feb 25, 2018 •

edited

Loading

nharrand Feb 25, 2018 •

edited

Loading

nharrand Feb 26, 2018 •

edited

Loading

nharrand Feb 26, 2018 •

edited

Loading

nharrand Feb 26, 2018 •

edited

Loading