add interface CtBodyHolder #943

pvojtechovsky · 2016-11-07T22:33:53Z

Note! I had to rollback CtExecutable.getBody declaration made by

GerardPaligot [email protected] on 7.8.15 14:20

to make it compilable with new CtBodyHolder. I have not found a way how to declare CtBodyHolder to have it compilable with origin CtExecutable.getBody declaration.

I have no idea whether my changes are correct. It might have some impact to Spoon templates!

monperrus · 2016-11-09T13:37:48Z

Hi,
It's OK to change the return of CtExecutable.getBody().

We also need a method setBody() in CtBodyHolder.

pvojtechovsky · 2016-11-09T19:49:31Z

Note that some children of CtBodyHolder are using
setBody(CtStatement)
and some
setBody(CtBlock)

These which expects setBody(CtBlock) have this new method

    public <T extends CtBodyHolder> T setBody(CtStatement statement) {
        CtBlock<?> block;
        if (statement instanceof CtBlock<?>) {
            block = (CtBlock<?>) statement;
        } else {
            block = getFactory().Code().createCtBlock(statement);
        }
        return setBody(block);
    }

Another solution would be to throw exception that CtBlock is expected. But I vote for automatic creation of CtBlock, because it makes live of clients more comfortable.

monperrus · 2016-11-09T20:28:51Z

But I vote for automatic creation of CtBlock, because it makes live of
clients more comfortable.
Yes, that's fine, that's along the line of
47bd0f2

pvojtechovsky · 2016-11-09T21:08:08Z

ParentContractTest failed, but I do not understand what is that. Could you fix it or explain it?

monperrus · 2016-11-10T08:39:10Z

For different reasons, first because having both setBody(CtStatement) and setBody(CtBlock) relies on correct dispatching on arguments, which is done at compile time in cryptic ways.
If you change:

    @Override
    public <T extends CtBodyHolder> T setBody(CtStatement statement) {
        CtBlock<?> block;
        if (statement instanceof CtBlock<?>) {
            block = (CtBlock<?>) statement;
        } else {
            block = getFactory().Code().createCtBlock(statement);
        }
        return setBody(block);
    }

by

    @Override
    public <E extends CtBodyHolder> E setBody(CtStatement statement) {
        CtBlock<? extends T> block;
        if (statement instanceof CtBlock<?>) {
            block = (CtBlock<? extends T>) statement;
        } else {
            block = (CtBlock<? extends T>) getFactory().Code().createCtBlock(statement);
        }
        return setBody(block);
    }

then it works.

But then, other contracts are violated, and for good reasons. I would go for removing the automatic block creation in setBody(CtStatement statement). This can be done in a helper method for those who need this.

pvojtechovsky · 2016-11-10T12:17:57Z

I would go for removing the automatic block creation in setBody(CtStatement statement).

So it should throw exception if client does not sent CtBody?

The I do not understand how will look the generic code which can call CtBodyHolder.setBody()

A) client does not know whether CtBlock or CtStatement has to be used, because it does not know real instance which is behind. And if it uses the wrong value then we throw an exception?
B) client has to know what it is calling... then we do not need CtBodyHolder and they can directly call e.g. CtCatch.setBody.

monperrus · 2016-11-10T13:09:47Z

if we all agree on #956, it should solve the parent problem.

pvojtechovsky · 2016-11-10T15:12:17Z

So should I change something in that PR? I do not understand the problem of ParentContractTest so I do not know what to do

pvojtechovsky · 2016-11-10T18:06:56Z

Now I see that CtExecutableImpl.setBody(CtStatement) called recursively setBody(CtStatement), but it should call setBody(CtBlock) of course. Fixed

monperrus · 2016-11-10T19:01:07Z

I do not understand the problem of ParentContractTest so I do not know
what to do
I suggest to first rebase.

pvojtechovsky · 2016-11-10T20:23:38Z

Now the spoon.processing.CtGenerationTest fails, but it succeeds in my local workspace. I have no idea what is the problem.

monperrus · 2016-11-12T08:26:59Z

I have no idea what is the problem.

Because since you have a change the signature of a metamodel method, you also have to regenerate the ReplacementVisitor. It's generated by CtGenerationTest in target what you have to do after your local test is:

cp ./target/generated/spoon/support/visitor/replace/ReplacementVisitor.java ./src/main/java/spoon/support/visitor/replace/ReplacementVisitor.java

and to add it in the commit.

pvojtechovsky · 2016-11-12T10:53:47Z

What about changing all setBody methods to have the same signature setBody(statement)? And convert it block if needed internaly.
Client can send both statement or block and internally we would convert it or throw exception if input is mot compatible with target element

monperrus · 2016-11-12T11:47:08Z

It would be less intuitive for clients who just rely on the type system for documentation. I prefer to keep it this way.

monperrus · 2016-11-13T09:25:07Z

the tests pass hurray!

could you push ReplacementVisitor with Unix newlines (the diff will be much smaller)?

also could you add tests for the new setBody() methods?

monperrus · 2016-11-13T16:39:33Z

Afterthought about this. Instead of adding 3x setBody(CtStatement). What about using setBody(CtBlock) as follows and only adding setBody(CtStatement) in CtLoop for backward-compatibity?

public interface CtBodyHolder extends CtElement {
    /**
     * Gets the body of this element
     */
    CtBlock getBody();

    /*** Sets the body of this element.
     */
    <T extends CtBodyHolder> T setBody(CtStatement CtBlock);
}

pvojtechovsky · 2016-11-13T19:41:35Z

the tests pass hurray!

;-) ... no, this PR is never ending story :(
After you merge #968, the tests will fail again, because actually this API contract tests simply checks nothing.

What about using setBody(CtBlock) as follows and only adding setBody(CtStatement) in CtLoop for backward-compatibity?

I would prefer opposite. In suggest to use setBody(CtStatement) in all 4 cases. The getBody would stay like it is -some have CtStatement and some have CtBlock.
pros

client of spoon has to write less code when calling setBody
- in case s/he wants to have only single statement in the body, then s/he simply calls setBody(CtStatement). S/he does not have to create wrapping CtBlock. The wrappnig CtBlock would be made automatically internally for CtElements which expects body of type CtBlock
- in case s/he wants to have block of statements, then s/he simply calls the same setBody(CtStatement), but this time with CtBlock instance. It will pass too.
it is the case with minimum code for the maintenance

cons

It would be less intuitive for clients who just rely on the type system for documentation. You are right, that if a developer starts to work with spoon first day, then s/he will need longer time to found that s/he has to create CtBlock to be able to add more statements into CtExecutable. Because s/he does not know spoon model and has no idea that there is something like CtBlock. But everybody who works with spoon longer will understand that CtBlock is a set of CtSatements and will like that they do not have to create CtBlock manually if they just want to add one statement somewhere.

I understand, that from technical point of view (java specification), the CtExecutable.body must be a block and compiler would fail if there would be just a statement.
But we all love spoon, because it abstracts from technical details.
So again an argument why CtStatement is OK, because from abstract point of view it is correct when CtExecutable has only one statement.

And if setBody(CtBlock) is always used, then everybody has to always create CtBlock even if s/he wants to add one statement only. What is advantage in that?

I see, ... hard choice. ;-)
I am using spoon just for short time, and I have never called setBody... yet (I have not wrote tests for that PR yet :-))). So it is quite good chance that Your opinion is the correct one. But I actually do not understand why.

monperrus · 2016-11-14T09:07:44Z

src/main/java/spoon/support/reflect/code/CtLoopImpl.java

 	@Override
-	public <T extends CtLoop> T setBody(CtStatement body) {
+	public <T extends CtBodyHolder> T setBody(CtStatement body) {
 		if (body != null) {


to be consistent, we should also create a block if the argument is not a block

I do not understand why. CtLoop accepts statement, so we should not create CtBlock. If you speak about change of spoon model, then I would prefer to do it in another PR ... PLEASE!!! :-)))

for two reasons:

because we want a 100% consistent behavior over all subinterfaces of CtBodyHolder

because it's 100% along the line of the new AlwaysInBlock contract introduced in new feature: Statement always in a block #683

monperrus · 2016-11-14T09:08:43Z

@tdurieux agrees with you, and your reasoning makes sense.

however, we should also modify CtLoopImpl.setBody to be consistent.

pvojtechovsky · 2016-11-14T17:52:32Z

@tdurieux agree with you, and your reasoning makes sense.

Do I understand well, that you will accept the PR, which has only one method setBody in all children of CtBodyHolder and this one method has one parameter CtStatement.

It of course means, that all methods setBody(CtBlock) will be removed.

Yes, I like that solution. But I do not like to rewrite it again and again ... so I would prefer to have your agreement about that concept, before I spent hours of my free time for that. ;-)

monperrus · 2016-11-14T18:31:45Z

But I do not like to rewrite it again and again ... so I would prefer to have your agreement about that concept, before I spent hours of my free time for that. ;-)

I understand :-)

that all methods setBody(CtBlock) will be removed.

Yes but it will be backward compatible both statically (CtStatement is an upper class) and dynamically (same behavior)

And we'll keep the return type of getBody to be CtBlock (we can override the return type in subinterfaces of CtBodyHolder) for those for which it was already the case (backward compatible, dynamically enforced by the setter).

This solution seems to be 1) fully backward compatible and 2) more powerful because it abstracts away details and simplifies the job of client code.

monperrus · 2016-11-14T18:32:09Z

You'll also have to rebase due to the change in ReplacementVisitor in sorry.

monperrus · 2016-11-14T21:23:05Z

src/main/java/spoon/reflect/code/CtCatch.java

 	CtBlock<?> getBody();

 	/**
-	 * Sets the catch's body.
+	 * Sets the catch's body. The instance of CtBlock makes sense too


we don't need to add it again in CtCatch if we don't change the return type or the contract.

monperrus · 2016-11-14T21:24:12Z

src/main/java/spoon/reflect/code/CtBodyHolder.java

+	CtStatement getBody();
+
+	/**
+	 * Sets the body of this element.


here we should document the automated block creation.

"If body is not a block, it is wrapped in a CtBlock which is semantically equivalent and eases transformation afterwards if required."

monperrus · 2016-11-14T21:24:44Z

src/main/java/spoon/reflect/code/CtLoop.java

 	 */
-	<T extends CtLoop> T setBody(CtStatement body);
+	@Override


monperrus · 2016-11-14T21:24:55Z

src/main/java/spoon/reflect/code/CtTry.java

 	CtBlock<?> getBody();

 	/**
-	 * Sets the tried body.
+	 * Sets the tried body. The instance of CtBlock makes sense too


monperrus · 2016-11-14T21:28:30Z

src/main/java/spoon/support/reflect/code/CtLoopImpl.java

 	@Override
-	public <T extends CtLoop> T setBody(CtStatement body) {
+	public <T extends CtBodyHolder> T setBody(CtStatement body) {
 		if (body != null) {


for two reasons:

because we want a 100% consistent behavior over all subinterfaces of CtBodyHolder

because it's 100% along the line of the new AlwaysInBlock contract introduced in new feature: Statement always in a block #683

monperrus · 2016-11-14T21:28:47Z

src/main/java/spoon/reflect/factory/CodeFactory.java

+	 * @param element
+	 * @return CtBlock instance
+	 */
+	public <T extends CtStatement> CtBlock<?> getOrCreateCtBlock(T element) {


excellent to factorize the code!

Thanks!
What about setImplicit? it is mentioned in #683

return this.createCtBlock(element).setImplicit(true);

pvojtechovsky · 2016-11-14T22:05:37Z

I have forgot to add tests. I will do it tommorrow

monperrus · 2016-11-14T22:08:40Z

No, setImplicit is only for JDTTReeBuilder. As soon as you use an
intercession method, all modified and created blocks must be explicit.

monperrus · 2016-11-14T22:09:46Z

src/main/java/spoon/reflect/code/CtTry.java

@@ -53,16 +53,12 @@
 	boolean removeCatcher(CtCatch catcher);

 	/**
-	 * Sets the tried body.
+	 * Gets the tried body.


Thanks for the first type, one remains "try body" (and not tried)

monperrus · 2016-11-14T22:11:16Z

looks like we're almost done :-)

pvojtechovsky · 2016-11-14T22:15:16Z

I had to fix some failing tests too. They were creating CtElement instances without factory. I guess it was a bug. Or should spoon work well when CtElement.factory == null ??

pvojtechovsky · 2016-11-14T22:16:47Z

looks like we're almost done :-)

It will be like a X-mass for me :-) But tomorrow. The test is missing

monperrus · 2016-11-14T22:17:41Z

Or should spoon work well when CtElement.factory == null ??
No. Setting the factory is always necessary.

throw Exception which contains details about real problem

monperrus · 2016-11-16T16:05:59Z

many thanks Pavel.

pvojtechovsky force-pushed the interface-CtBodyHolder branch from 1b32d6e to 927fc95 Compare November 9, 2016 19:46

monperrus mentioned this pull request Nov 10, 2016

test(ParentContractTest): improves contract about setting parents #956

Merged

pvojtechovsky force-pushed the interface-CtBodyHolder branch from 927fc95 to bf4db3f Compare November 10, 2016 17:57

pvojtechovsky force-pushed the interface-CtBodyHolder branch from 6726695 to ceceb1e Compare November 10, 2016 18:24

pvojtechovsky force-pushed the interface-CtBodyHolder branch 2 times, most recently from 9fcd54c to 9923a95 Compare November 10, 2016 20:07

monperrus changed the title ~~added interface CtBodyHolder~~ add interface CtBodyHolder Nov 12, 2016

monperrus reviewed Nov 14, 2016

View reviewed changes

pvojtechovsky force-pushed the interface-CtBodyHolder branch from 96b995c to 39b3d1a Compare November 14, 2016 21:13

pvojtechovsky force-pushed the interface-CtBodyHolder branch from 39b3d1a to 15c825f Compare November 14, 2016 21:16

monperrus requested changes Nov 14, 2016

View reviewed changes

monperrus reviewed Nov 14, 2016

View reviewed changes

pvojtechovsky force-pushed the interface-CtBodyHolder branch from cdf2c67 to ec992f7 Compare November 14, 2016 22:11

pvojtechovsky added 8 commits November 15, 2016 20:58

added interface CtBodyHolder

12f4985

refactor: CtBodyHolder.setBody(body) implemented

0c87036

fix FactoryTest.testFactoryOverriding

9c04c0f

element needs a Factory

54827f7

throw Exception which contains details about real problem

implement review requests

ce85ae2

CtTry.getBody comment fixed

a0903cb

fix check style

17ad1af

test CtBodyHolder

4060e6e

pvojtechovsky force-pushed the interface-CtBodyHolder branch from 17e1a21 to 4060e6e Compare November 15, 2016 20:00

update ReplacementVisitor

b18238b

monperrus approved these changes Nov 16, 2016

View reviewed changes

monperrus merged commit b4fbe3e into INRIA:master Nov 16, 2016

pvojtechovsky deleted the interface-CtBodyHolder branch November 16, 2016 18:24

tdurieux mentioned this pull request Jan 4, 2017

Release 5.5.0 planned for Wednesday Jan 11th 2017 #1081

Closed

add interface CtBodyHolder #943

add interface CtBodyHolder #943

Conversation

pvojtechovsky commented Nov 7, 2016

monperrus commented Nov 9, 2016

pvojtechovsky commented Nov 9, 2016

monperrus commented Nov 9, 2016

pvojtechovsky commented Nov 9, 2016

monperrus commented Nov 10, 2016

pvojtechovsky commented Nov 10, 2016

monperrus commented Nov 10, 2016

pvojtechovsky commented Nov 10, 2016

pvojtechovsky commented Nov 10, 2016

monperrus commented Nov 10, 2016

pvojtechovsky commented Nov 10, 2016

monperrus commented Nov 12, 2016

pvojtechovsky commented Nov 12, 2016

monperrus commented Nov 12, 2016

monperrus commented Nov 13, 2016

monperrus commented Nov 13, 2016

pvojtechovsky commented Nov 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

monperrus commented Nov 14, 2016 • edited Loading

pvojtechovsky commented Nov 14, 2016

monperrus commented Nov 14, 2016

monperrus commented Nov 14, 2016

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 Nov 14, 2016

monperrus commented Nov 14, 2016

Choose a reason for hiding this comment

monperrus commented Nov 14, 2016

pvojtechovsky commented Nov 14, 2016

pvojtechovsky commented Nov 14, 2016

monperrus commented Nov 14, 2016

monperrus commented Nov 16, 2016

monperrus commented Nov 14, 2016 •

edited

Loading