review: feat: CloneMaker can be used to listen on cloning process

[pvojtechovsky]

This PR allows client to track, which clone source node is mapped to which clone target node.

One CloneMaker instance is used by cloning of one CtElement and all it's children.

By overriding this CloneMaker and creating own instance for cloning, the client can listen on cloning process and client's implementation is called with each source and target cloned node.

Example:

class MappingCloneMaker extends CloneMaker {
   Map<CtElement, CtElement> sourceToTarget = new HashMap();
   public <T extends CtElement> T clone(T source) {
       T target = super.clone(source);
       sourceToTarget .put(source, target);
       return target;
   }
}
...
MappingCloneMaker cloner = new MappingCloneMaker ();
cloner.clone(someElement);
CtElement cloneOfChildElementOfSomeElement = cloner.sourceToTarget.get(someChildElementOfSomeElement);

@monperrus will you accept such feature into Spoon? If yes, I will make some test too.
What about name CloneMaker? Do you have better one?

[monperrus]

Hi Pavel,
On the principle, I find interesting that client code can subclass some classes to tailor the cloning process. This is useful and very open-closed principle.

On the implementation, it seems that CloneHelper is already the right abstraction for this, requiring some modification instead of adding a new class.

About tests: it's good when tests come right at the first commit of a feature PR, because they enable the creader to quickly grasp the goal and usage.

[pvojtechovsky]

On the implementation, it seems that CloneHelper is already the right abstraction for this, requiring some modification instead of adding a new class.

I agree, but I did new class because of backward compatibility and easier maintenance. The problem is that CloneHelper contains static clone methods, while new CloneMaker, needs the same methods non static. It is not easily possible to put all into one class. It would be a mess.

So I see two ways
A) to be backward compatible - CloneHelper is API which might be used by somebody. Then I suggest to take this PR as it is
B) to skip compatibility of CloneHelper. Then I delete CloneHelper and rename CloneMaker to CloneHelper and we are done too (there will be only little changes, because CloneMaker was made by copying of CloneHelper)
C) ?

WDYT?

[monperrus]

Although CloneHelper is public, it's not meant to be part of the API. It's OK if you redesign it in a non-backward compatible way.

Yet, it's good to keep its name for sake of history and discoverability.

[pvojtechovsky]

I refactored CloneHelper and removed CloneMaker. Working on test ...

[pvojtechovsky]

Is it correct that some elements of source AST are copied more times into target AST?

It happens e.g. when CtExecutableReference, which points to constructor is sharing one CtTypeReference instance with two fields:

• CtExecutableReference.declaringType
• CtExecutableReference.type
  both points to same instance of CtTypeReference.

In such case origin AST shares CtTypeReference , while cloned instance has 2 clones of one source CtTypeReference.

[monperrus]

Is it correct that some elements of source AST are copied more times into target AST?

Yes. That's not an issue. In Spoon, the AST is meant to be a tree and not a lattice.

[pvojtechovsky]

In Spoon, the AST is meant to be a tree and not a lattice.

I added #1592 which tests that model is a tree and fixes few problems. This fix is needed before we can merge this one (part of the fix is here too in commit fix: Spoon AST is tree not a latice).

[spoon-bot]

Revapi Analysis results

Old API: fr.inria.gforge.spoon:spoon-core:jar:6.0.0-20171012.192256-75

New API: fr.inria.gforge.spoon:spoon-core:jar:6.0.0-SNAPSHOT

Detected changes: 6.

Change 1

Name	Element
Old	method void spoon.support.visitor.clone.CloneVisitor::()
New	method void spoon.support.visitor.clone.CloneVisitor::(spoon.support.visitor.equals.CloneHelper)
Code	java.method.numberOfParametersChanged
Description	The number of parameters of the method have changed.
Breaking	binary: breaking

Change 2

Name	Element
Old	method java.util.Collection spoon.support.visitor.equals.CloneHelper::clone(java.util.Collection)
New	method java.util.Collection spoon.support.visitor.equals.CloneHelper::clone(java.util.Collection)
Code	java.method.noLongerStatic
Description	method no longer static
Breaking	binary: breaking

Change 3

Name	Element
Old	method java.util.List spoon.support.visitor.equals.CloneHelper::clone(java.util.List)
New	method java.util.List spoon.support.visitor.equals.CloneHelper::clone(java.util.List)
Code	java.method.noLongerStatic
Description	method no longer static
Breaking	binary: breaking

Change 4

Name	Element
Old	method java.util.Map<java.lang.String, T> spoon.support.visitor.equals.CloneHelper::clone(java.util.Map<java.lang.String, T>)
New	method java.util.Map<java.lang.String, T> spoon.support.visitor.equals.CloneHelper::clone(java.util.Map<java.lang.String, T>)
Code	java.method.noLongerStatic
Description	method no longer static
Breaking	binary: breaking

Change 5

Name	Element
Old	method java.util.Set spoon.support.visitor.equals.CloneHelper::clone(java.util.Set)
New	method java.util.Set spoon.support.visitor.equals.CloneHelper::clone(java.util.Set)
Code	java.method.noLongerStatic
Description	method no longer static
Breaking	binary: breaking

Change 6

Name	Element
Old	method T spoon.support.visitor.equals.CloneHelper::clone(T)
New	method T spoon.support.visitor.equals.CloneHelper::clone(T)
Code	java.method.noLongerStatic
Description	method no longer static
Breaking	binary: breaking

[pvojtechovsky]

I am done. It is ready for merge

[spoon-bot]

Revapi Analysis results

Old API: fr.inria.gforge.spoon:spoon-core:jar:6.0.0-20171012.192256-75

New API: fr.inria.gforge.spoon:spoon-core:jar:6.0.0-SNAPSHOT

Detected changes: 6.

Change 1

Name	Element
Old	method void spoon.support.visitor.clone.CloneVisitor::()
New	method void spoon.support.visitor.clone.CloneVisitor::(spoon.support.visitor.equals.CloneHelper)
Code	java.method.numberOfParametersChanged
Description	The number of parameters of the method have changed.
Breaking	binary: breaking,

Change 2

Name	Element
Old	method java.util.Collection spoon.support.visitor.equals.CloneHelper::clone(java.util.Collection)
New	method java.util.Collection spoon.support.visitor.equals.CloneHelper::clone(java.util.Collection)
Code	java.method.noLongerStatic
Description	method no longer static
Breaking	binary: breaking,

Change 3

Name	Element
Old	method java.util.List spoon.support.visitor.equals.CloneHelper::clone(java.util.List)
New	method java.util.List spoon.support.visitor.equals.CloneHelper::clone(java.util.List)
Code	java.method.noLongerStatic
Description	method no longer static
Breaking	binary: breaking,

Change 4

Name	Element
Old	method java.util.Map<java.lang.String, T> spoon.support.visitor.equals.CloneHelper::clone(java.util.Map<java.lang.String, T>)
New	method java.util.Map<java.lang.String, T> spoon.support.visitor.equals.CloneHelper::clone(java.util.Map<java.lang.String, T>)
Code	java.method.noLongerStatic
Description	method no longer static
Breaking	binary: breaking,

Change 5

Name	Element
Old	method java.util.Set spoon.support.visitor.equals.CloneHelper::clone(java.util.Set)
New	method java.util.Set spoon.support.visitor.equals.CloneHelper::clone(java.util.Set)
Code	java.method.noLongerStatic
Description	method no longer static
Breaking	binary: breaking,

Change 6

Name	Element
Old	method T spoon.support.visitor.equals.CloneHelper::clone(T)
New	method T spoon.support.visitor.equals.CloneHelper::clone(T)
Code	java.method.noLongerStatic
Description	method no longer static
Breaking	binary: breaking,

[pvojtechovsky]

Thanks for comments and merge fixing ... looks like I still do not understand how git works if I did not merged another branch well :-(