I think this PR is on good way

It looks like this PR would break a lot of APIs: wouldn't it worth it to put it in the next release, as we already break lot of things?

If I understood well the changes are not that complicated, once we got a proper list of method return type to change, am I right? Is it possible to compute that list ?

I'm trying to understand how to help on this PR but I still have questions regarding how to generalize it.
For example, we have CtTypedElement#setType and CtConstructor#setType, for now they both return E extends CtTypedElement.
Apparently the list you provided indicate that CtConstructor#setType should be refactored. I tried to refactor it to return a CtConstructor in CtConstructor, but then it does not override CtTypedElement anymore: I have to refactor that one too.

So I obtained:

• CtTypedElement.java

CtTypedElement setType(CtTypeReference<T> type);

• CtConstructor.java

@Override
@UnsettableProperty
CtConstructor setType(CtTypeReference<T> type);

But now I got a compilation error because setType() is used in CodeFactory with this code:

CtNewArray<T[]> array = factory.Core().<T[]>createNewArray().setType(arrayReference);

So it seems to me that unless we override the methods in each interface with the proper return type, we won't obtain a fluent API. What am I missing here?

So it seems to me that unless we override the methods in each interface with the proper return type, we won't obtain a fluent API. What am I missing here?

I have not analyzed it deep. Just my experience/feeling tells me that you are right. Yes it needs rewrite of all return types of all/most Spoon API methods. We get rid of useless generics in some places.

Yes it needs rewrite of all return types of all/most Spoon API methods.

Then I propose that we write first a contract to check that all interfaces are overriding their parents interfaces methods if needed (e.g. all set/add methods which return the same type as the current interface), and to automatically generate the proper signature. Then we should be able to to maintain the whole API properly when adding new methods somewhere. WDYT?

Still 159 methods to go (see log).

There is really a tradeoff here between supporting Kotlin and maintaining a lot of methods with the proper return type (even if those methods can be generated).

Well, we can now use the Sniper mode for this.

API changes: 18 (Detected by Revapi)

Old API: fr.inria.gforge.spoon:spoon-core:jar:7.1.0-20180918.224838-177 / New API: fr.inria.gforge.spoon:spoon-core:jar:7.1.0-SNAPSHOT

There is really a tradeoff here between supporting Kotlin and maintaining a lot of methods with the proper return type

This change makes sense even for Java. Current return type is wrong, so I like this PR even if I do not use Kotlin.

Well, we can now use the Sniper mode for this.

So who has courage to try that? ;-)

I have refactored spoon model (by sniper), so now it has correct return types, but now there is visible some incompatibility ... e.g. code like this:

spoon.getFactory().Code().createConstructorCall(spoon.getFactory().Core().createTypeReference().setSimpleName("Bar"));

is not compilable now, because setSimpleName return type was more flexible before

<T extends CtNamedElement> T setSimpleName(String);

then it is now

CtNamedElement setSimpleName(String);

therefore compilation of createConstructorCall fails on unexpected type CtNamedElement.

Solutions:
A) add type casts in these rare cases
B) add setSimpleName method into each class which inherits it with return type = that class
C) to keep it unchanged

I vote for A)

WDYT?

A) add type casts in these rare cases

I'm not sure those cases are so rare, and even so it would break client API, and we want to avoid that as much as possible.

I'm in favor of B: can't we use the sniper to automatically add the method to inherited classes?

I'm in favor of B: can't we use the sniper to automatically add the method to inherited classes?

I like this idea too, but it produces many methods in leaf interfaces, which are normally inherited, but this time they will be directly declared in sources.
May be it is not so good idea...
We should at least mark these methods using some annotation to simplify further maintenance and testing of spoon metamodel.

Why we need these methods? Because we want to use fluent API during creation of spoon model. But that solution is only partial. I have feeling like it cannot be done well this way.
The correct solution is to generate a helper classes - builders (like #741) - which will provide fluent API to configure all AST node properties, including building of children, including final validation of consistency of model, which can be done at time when BuilderXxx.build() method is called.

WDYT?

Should we really dirty current Spoon API by helper methods or we keep API simple, but little bit incompatible for some rare cases?

Note: even if we add nearly each inherited method into each leaf interface, the current (ugly?) code like this:

return factory.Core().<A, T>createAssignment().<CtAssignment<A, T>>setAssignment(expression).setAssigned(vaccess);

will be compilable - OK, ...
but it is still ugly and will produce warnings now that generic type argument is useless. So clients have to change it anyway to

return factory.Core().<A, T>createAssignment().setAssignment(expression).setAssigned(vaccess);

Without helper methods in can be written like this

return ((CtAssignment<A, T>>) factory.Core().<A, T>createAssignment().setAssignment(expression)).setAssigned(vaccess);

or not fluent but readable

CtAssignment<A, T>> assignment = factory.Core().createAssignment();
assignment.setAssignment(expression);
assignment.setAssigned(vaccess);
return assignment;

The nicest would be of course:

return factory.Builder().assignment().assignment(expression).assigned(vaccess).build();

@monperrus @surli I need your feedback here to be able to continue with #2615
It is quite important decision, because it will have either impact on compatibility or will add many helper methods into Spoon api interfaces. WDYT?

Hi @pvojtechovsky, thanks for working on this one!

I'm still standing for solution B:

1. Old clients code will still work, even if they got some warnings
2. It's a huge work to do, but we have to do it only once: the model shouldn't change that much in terms of concept, and in terms of implementation we will still use the inheritance: we override only to provide the good type. So the maintaining cost should be limited.

@surli thanks for your feedback
@monperrus I would like to hear your opinion too ...before I start with that work. I do not want to throw it away later ;-)

I personally prefer client incompatibility over duplicated methods in API, but I can live with both solutions.

ping @monperrus I would like to hear your opinion.

I don't have a strong opinion for now.

On the one hand, we want backward compatibility. On the other hand, to my knowledge, very few clients use the fluent API, partly because it only works partially (see Pavel's example in issue).

Note that we don't have to fix/generate all methods at once, we can do baby PRs one by one. For instance, we can leave the case of setSimpleName for the end.

I propose to close this one (#1852) and continue the discussion in #2615 which is where we want to go.

Can you port the architectural test we have here to #2615?

Can you port the architectural test we have here to #2615?

done. #2615 now contains improved test which detects all wrong methods and ignores correct methods which looks like they return this, but they doesn't.

I agree to close this one