review: feat(sniper): detailed tree of source fragments of compilation unit #2283

pvojtechovsky · 2018-07-26T09:49:51Z

this PR introduces new types:

SourceFragment
ElementSourceFragment implements SourceFragment
TokenSourceFragment implements SourceFragment
CollectionSourceFragment implements SourceFragment

ElementSourceFragment is a wrapper around CtElement and it's SourcePosition. It also builds hierarchy of ElementSourceFragments, which mirrors hierarchy of fragments of source code of compilation unit.

It provides 3 views at tree of source fragments

Tree of ElementSourceFragments. It consists of fragments defined by SourcePosition.start/end of CtElements only. It is basic structure, which must be build after Spoon model is created and before it is modified.
Each ElementSourceFragment can be optionally expanded to detailed tree which contains TokenSourceFragments next to child ElementSourceFragments. See ElementSourceFragment#getChildrenFragments().
Each detailed tree can be reorganized into so called grouped tree, which contains same information like detailed tree, but additionally SourceFragments, which belongs to same parent CtRole are grouped together and represented by CollectionSourceFragment

Notes:

The detailed tree consists of small parser tokens, which are equal to tokens produced by DJPP and TokenWriter.
The detailed tree is based on simple SourcePositionImpl only. It doesn't need any details provided e.g. by DeclarationSourcePosition
The detailed tree uses simple parser to detect spaces, separators, keywords and identifiers.

surli · 2018-08-27T10:10:44Z

src/main/java/spoon/support/reflect/cu/CompilationUnitImpl.java

+	@Override
+	public ElementSourceFragment getRootSourceFragment() {
+		if (rootFragment == null) {
+			String originSourceCode = getOriginalSourceCode();


getOriginalSourceCode() might return null if getFile() returns null. You should check the result to avoid NPE here.

surli · 2018-08-27T10:12:59Z

src/main/java/spoon/support/reflect/cu/CompilationUnitImpl.java

+			for (CtImport imprt : getImports()) {
+				rootFragment.addChild(new ElementSourceFragment(imprt, null /*TODO role for import of CU*/));
+			}
+			for (CtType<?> ctType : declaredTypes) {


The CU can also be used for package declaration or module declaration: in those cases, you don't create the tree?

I forgot that cases. As fast solution: I will check them and throw SpoonException(...not supported...). Later we can check if we can create the tree for them too - I have no idea yet if it is easily possible or needs more effort.

It looks ok for me. Maybe it worth opening an issue to avoid forgetting it.

surli · 2018-08-27T10:14:35Z

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

@@ -474,6 +477,39 @@ public void scan(CtRole role, CtElement element) {
 		});
 	}

+	@Test
+	public void testSourcePositionTreeIsCorrectlyOrdered() {
+		List<CtType> types = rootPackage.filterChildren(new TypeFilter<>(CtType.class)).filterChildren((CtType t) -> t.isTopLevel()).list();


Could you specify the contract?

spoon-bot · 2018-08-27T19:04:37Z

API changes: 2 (Detected by Revapi)

Old API: fr.inria.gforge.spoon:spoon-core:jar:7.1.0-20180827.060031-119 / New API: fr.inria.gforge.spoon:spoon-core:jar:7.1.0-SNAPSHOT

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

	Method was added to an interface.
Old	none
New	method CompilationUnit#getSourceFragment(SourcePositionHolder)
Breaking	binary: non_breaking

monperrus · 2018-08-27T19:30:00Z

It looks interesting. What's the final goal of this new feature?

What do you mean by "ElementSourceFragment is a wrapper around CtElement and its SourcePosition"?

pvojtechovsky · 2018-08-27T19:41:55Z

It looks interesting. What's the final goal of this new feature?

It is core feature need by #1927 - sniper mode.

What do you mean by "ElementSourceFragment is a wrapper around CtElement and its SourcePosition"?

yes,

it connects element and source position
it knows children and sibling fragments, so it builds tree of source fragments

It is not enough to link element only and to use CtElement#getPosition, because

the position might be changed during refactoring and we need origin elements and origin source positions which maps to origin sources (before refactoring)

monperrus · 2018-08-27T19:48:57Z

It is core feature need by #1927 - sniper mode.

Excellent!

we need origin elements and origin source positions which maps to origin sources (before refactoring)

Do you mean something like CtElement#getOriginalPosition and CtElement#getOriginalElement?

pvojtechovsky · 2018-08-28T05:38:26Z

we need origin elements and origin source positions which maps to origin sources (before refactoring)

Do you mean something like CtElement#getOriginalPosition and CtElement#getOriginalElement?

I would say no. But may be I do not understand your idea.

The concept of this PR and core feature for sniper mode is:

The compilation unit has origin source code
The compilation unit has tree of SourceFragments, which mirrors the tokens from origin source code
There are different kinds of SourceFragments: ElementSourceFragment, TokenSourceFragment, ...
ElementSourceFragment represents a fragment which mirrors the CtElement
TokenSourceFragment represents a fragment of primitive tokens, like separators, newlines, etc. It is not mapped to any child CtElement.

So from this concept point of view there is relation from ElementSourceFragment to the CtElement, which was made during building of origin Spoon model. Opposite relation is not needed.

When the spoon model is changed later (some CtElements are cloned, added, modified, moved, removed, ... ) then these new CtElements doesn't have source position, etc. So it makes no sense to ask these elements for origin position or origin element.

monperrus · 2018-08-29T19:43:12Z

The compilation unit has origin source code

Do you mean getRootSourceFragment?

The compilation unit has tree of SourceFragments, which mirrors the tokens from origin source code

I would propose to rename ElementSourceFragment to CtElementMirror. WDYT?

ElementSourceFragment represents a fragment which mirrors the CtElement

I expect a field CtElement mirroredElem there. But there is no such field. What's the reason?

pvojtechovsky · 2018-08-30T08:47:57Z

The compilation unit has origin source code

I meant String getOriginSourceCode() method.

I would propose to rename ElementSourceFragment to CtElementMirror. WDYT?
I expect a field CtElement mirroredElem there. But there is no such field. What's the reason?

It looks like you look at the concept/feature from wrong direction...
The primary concept of this feature is to split source code (represented as one long String) to the small Strings which are semantically equal to TokenWriter tokens.
Then these tokens are organized into tree structure, which is represented by SourceFragments.
Then some source fragments can be mapped to CtElements (see ElementSourceFragment).

There is no reason why EVERY CtElement should know it's ElementSourceFragment, but there is algoritm in CompilationUnit which can search and return ElementSourceFragment for CtElement - if there exist such ElementSourceFragment...

@monperrus Is it more clear now?

monperrus · 2018-08-30T17:52:31Z

It's not clear but it's better. It looks interesting.

As we've done in the past, are you OK if I propose some renaming and refactoring by pushing in your branch?

pvojtechovsky · 2018-08-30T18:26:55Z

pushing in your branch?

Yes sure, I am ok with that. But it would be better for me if you would do the refactoring directly in #1927 - branch refDJPP. It contains code which uses classes from this PR. After you refactor it, I can rebase interactive your changes by way the part will become part of feaSourceFragment branch and another part will become part of refDJPP.

Otherwise you will refactor only feaSourceFragment and I will have to then fix compilation problems in refDJPP manually without refactoring tool :-(

WDYT?

By the way I am open to discuss merging of new SourceFragment and old SourcePosition somehow - but before we can discuss it it needs your understanding of SourceFragment concept and implementation of #1927.

monperrus · 2018-08-30T18:31:57Z

In #2419 this the same code, with a refactoring:

removed getRootSourceFragment and getSourceFragment from CompilationUnit
added getOriginalSourceFragment in SourcePositionHolder

Does it make sense to you?

pvojtechovsky · 2018-08-30T18:47:34Z

I like it a lot!

I just do not know how it will behave it this method is called on the brand new CtElement, which was made by cloning or Factory#createXxx call. We should make a test and define some expected behavior.

monperrus · 2018-08-30T19:06:04Z

I like it a lot!

Super.

We should make a test and define some expected behavior.

Just added a test with contract "on a new element, getOriginalSourceFragment returns the actual element toString"

pvojtechovsky · 2018-08-30T19:12:09Z

Just added a test with contract "on a new element, getOriginalSourceFragment returns the actual
element toString"

May be it would be better to return some constant SourceFragment NO_SOURCE_FRAGMENT ... because

there is no original source fragment in new CtElement
the returned source fragment made by toString, doesn't conform to source fragment concept: source fragment consists of tree of smalled source fragments which represents TokenWriter tokens.

WDYT?

monperrus · 2018-09-01T07:13:07Z

May be it would be better to return some constant SourceFragment NO_SOURCE_FRAGMENT ... because

You're right, it would be better.

monperrus · 2018-09-04T19:59:35Z

do you continue based on the design we discussed in #2419?

pvojtechovsky · 2018-09-05T05:28:10Z

OK, I can integrate these change into main PR today or tomorrow evening.

monperrus · 2018-09-05T07:28:08Z

looking forward to it!

pvojtechovsky · 2018-09-06T18:43:29Z

I do not understand why CI failed here. Is it problem of CI or this PR?

I am finished with my changes here.

monperrus · 2018-09-06T20:43:14Z

Thanks Pavel.

I've put the implementation classes in an internal package.

Let me think about it and add some more documentation.

monperrus · 2018-09-06T20:47:31Z

and I want to have a look at the tests, which look super interesting!!

pvojtechovsky · 2018-09-07T06:14:09Z

Why this change?

ElementSourceFragment getOriginalSourceFragment() {

changed to

SourceFragment getOriginalSourceFragment()

It can never return SourceFragment. It is always ElementSourceFragment. ElementSourceFragment is not just implementation of SourceFragment interface. It means a special Source fragment which is linked to an CtElement. Other SourceFragments might be for example linked to a token.

monperrus · 2018-09-07T11:11:53Z

Because of the design best practice that interface should only depend on other interfaces, and not on concrete implementations. One solution is to extract an interface out of ElementSourceFragment. WDYT?

pvojtechovsky · 2018-09-07T12:49:05Z

One solution is to extract an interface out of ElementSourceFragment

I understand, but If I remember well, you already agreed that in some cases Spoon uses interfaces too much. I do not think that it is helpful to have interface ElementSourceFragment and exactly one class ElementSourceFragmentImpl ... nobody will ever write different implementation for ElementSourceFragment
WDYT? ;-)

monperrus · 2018-09-10T09:48:25Z

Yes, you're right, sometimes there are several design principles that contradict :-) We can go back to |ElementSourceFragment. We need anyhow to put them @experimental. |

monperrus · 2018-09-10T19:50:57Z

Pavel, to your opinion, is ElementSourceFragment meant to be used by clients? or is it more an internal class for us?

For clients, what about a simple method in the interface:

String getOriginalSourceFragment()

WDYT?

pvojtechovsky · 2018-09-11T05:19:08Z

String getOriginalSourceFragment()

short answer: No

long answer: ElementSourceFragment getOriginalSourceFragment() returns root ElementSourceFragment of current element. That root ElementSourceFragment contains subtree of all child fragments, which is the core feature needed to be able to print sources of element using origin sources as much as possible -> sniper mode.

is ElementSourceFragment meant to be used by clients?

I would say, normally clients doesn't need that, same like they doesn't need SourcePosition. But these clients who wants to play with origin sources, will highly appreciate that detailed source tree.

monperrus · 2018-09-11T14:15:10Z

then, I moved SourceFragment to package "internal" and we're ready to merge.

monperrus

Thanks Pavel, for this new advanced feature.

pvojtechovsky force-pushed the feaSourceFragment branch 2 times, most recently from 06f1446 to ff24212 Compare August 21, 2018 17:51

pvojtechovsky changed the title ~~WIP: feat: detailed tree of source fragments of compilation unit~~ review: feat: detailed tree of source fragments of compilation unit Aug 21, 2018

surli reviewed Aug 27, 2018

View reviewed changes

monperrus changed the title ~~review: feat: detailed tree of source fragments of compilation unit~~ review: feat(sniper): detailed tree of source fragments of compilation unit Aug 27, 2018

pvojtechovsky added 6 commits September 6, 2018 17:58

feat: detailed tree of SourceFragments of CompilationUnit

ee410e4

TokenType is public

08efff0

fix license headers

5e0d36d

fail when getRootSourceFragment is called on package or module

4376d2a

contract and improvement of testSourcePositionTreeIsCorrectlyOrdered

e05fbb3

apply INRIA#2283

918aa77

pvojtechovsky force-pushed the feaSourceFragment branch from 9d2116b to 918aa77 Compare September 6, 2018 16:38

pvojtechovsky mentioned this pull request Sep 6, 2018

WIP: brainstorming around #2283 #2419

Closed

pvojtechovsky added 2 commits September 6, 2018 18:47

test source fragments of labeled statement

1609637

fix javadoc

b8b67da

monperrus added 4 commits September 6, 2018 22:27

up

40bfa1b

up

2728369

up

3650f56

up

ec98d9f

pvojtechovsky and others added 2 commits September 10, 2018 21:03

getOriginalSourceFragment returns ElementSourceFragment, @experimental

19acd2c

up

e6071df

monperrus added 2 commits September 11, 2018 15:53

up

7a8c1f4

up

a932398

monperrus approved these changes Sep 11, 2018

View reviewed changes

monperrus merged commit 27f50a2 into INRIA:master Sep 11, 2018

pvojtechovsky deleted the feaSourceFragment branch September 12, 2018 05:06

monperrus mentioned this pull request Sep 20, 2018

Release of Spoon 7.1.0 #2489

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

review: feat(sniper): detailed tree of source fragments of compilation unit #2283

review: feat(sniper): detailed tree of source fragments of compilation unit #2283

pvojtechovsky commented Jul 26, 2018 •

edited

Loading

surli Aug 27, 2018

surli Aug 27, 2018

pvojtechovsky Aug 27, 2018

surli Aug 27, 2018

surli Aug 27, 2018

spoon-bot commented Aug 27, 2018

monperrus commented Aug 27, 2018

pvojtechovsky commented Aug 27, 2018

monperrus commented Aug 27, 2018

pvojtechovsky commented Aug 28, 2018

monperrus commented Aug 29, 2018

pvojtechovsky commented Aug 30, 2018

monperrus commented Aug 30, 2018

pvojtechovsky commented Aug 30, 2018

monperrus commented Aug 30, 2018

pvojtechovsky commented Aug 30, 2018

monperrus commented Aug 30, 2018 via email

pvojtechovsky commented Aug 30, 2018

monperrus commented Sep 1, 2018

monperrus commented Sep 4, 2018

pvojtechovsky commented Sep 5, 2018

monperrus commented Sep 5, 2018 via email

pvojtechovsky commented Sep 6, 2018

monperrus commented Sep 6, 2018

monperrus commented Sep 6, 2018

pvojtechovsky commented Sep 7, 2018

monperrus commented Sep 7, 2018 via email

pvojtechovsky commented Sep 7, 2018

monperrus commented Sep 10, 2018 via email

monperrus commented Sep 10, 2018

pvojtechovsky commented Sep 11, 2018

monperrus commented Sep 11, 2018

monperrus left a comment

review: feat(sniper): detailed tree of source fragments of compilation unit #2283

review: feat(sniper): detailed tree of source fragments of compilation unit #2283

Conversation

pvojtechovsky commented Jul 26, 2018 • edited Loading

surli Aug 27, 2018

Choose a reason for hiding this comment

surli Aug 27, 2018

Choose a reason for hiding this comment

pvojtechovsky Aug 27, 2018

Choose a reason for hiding this comment

surli Aug 27, 2018

Choose a reason for hiding this comment

surli Aug 27, 2018

Choose a reason for hiding this comment

spoon-bot commented Aug 27, 2018

monperrus commented Aug 27, 2018

pvojtechovsky commented Aug 27, 2018

monperrus commented Aug 27, 2018

pvojtechovsky commented Aug 28, 2018

monperrus commented Aug 29, 2018

pvojtechovsky commented Aug 30, 2018

monperrus commented Aug 30, 2018

pvojtechovsky commented Aug 30, 2018

monperrus commented Aug 30, 2018

pvojtechovsky commented Aug 30, 2018

monperrus commented Aug 30, 2018 via email

pvojtechovsky commented Aug 30, 2018

monperrus commented Sep 1, 2018

monperrus commented Sep 4, 2018

pvojtechovsky commented Sep 5, 2018

monperrus commented Sep 5, 2018 via email

pvojtechovsky commented Sep 6, 2018

monperrus commented Sep 6, 2018

monperrus commented Sep 6, 2018

pvojtechovsky commented Sep 7, 2018

monperrus commented Sep 7, 2018 via email

pvojtechovsky commented Sep 7, 2018

monperrus commented Sep 10, 2018 via email

monperrus commented Sep 10, 2018

pvojtechovsky commented Sep 11, 2018

monperrus commented Sep 11, 2018

monperrus left a comment

Choose a reason for hiding this comment

pvojtechovsky commented Jul 26, 2018 •

edited

Loading