review1: feature: CtTypeParameter#isSubtypeOf

[pvojtechovsky]

Implements #1160.

The implementation of isSubtypeOf on type parameters is quite complex task, which needs all the basic steps below, which fits to commits of this PR.

There are:
BS1) new interface GenericTypeAdapter
BS2) CtSuperTypeHierarchy - adapts generic types to Type scope
BS3) CtMethodSuperTypeHierarchy - adapts generic types to Method/Constructor scope
BS4) CtFormalTypeDeclarer#getGenericTypeAdapter(), which creates instance of CtSuperTypeHierarchy or CtMethodSuperTypeHierarchy for the scope of this declarer
BS5) CtTypeParameterImpl#isSubtypeOf - finally the implementation of isSubtypeOf - the target of this PR
BS6) simpler and more correct implementation of CtTypeParameterImpl#isSubtypeOf(CtTypeReference), based on new algorithms

• first and incomplete tests ... the testing of this java generics stuff is quite tricky ... too many combinations.

[monperrus]

Hi Pavel,

This PR contains #1216 and #1217, so we should first discuss them.

You're so prolific with PRs :-) that it would help if you add the review/merge priority in the PR title itself

[pvojtechovsky]

that it would help if you add the review/merge priority in the PR title itself

I will use these prefixes:

• review 1: ... ready for review, without dependency to others. Please check it first
• review 2: ... ready for review, with an dependency to other PRs. I am done with that too, so it is possible to discuss that solution too. But priority is lower then "review 1: "
• WiP - work in progress - contains an unfinished feature. I am not publishing everything what I do, just things where early feedback would be welcome, to influence the final solution as soon as possible.

[spoon-bot]

Revapi Analysis results

Old API: fr.inria.gforge.spoon:spoon-core:jar:5.6.0-20170313.234505-78

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

Detected changes: 4.

Change 1

Name	Element
Old	none
New	method spoon.reflect.reference.CtTypeReference<?> spoon.reflect.declaration.CtTypeParameter::getTypeAdaptedTo(spoon.reflect.declaration.CtFormalTypeDeclarer)
Code	java.method.addedToInterface
Description	Method was added to an interface.
Breaking	source: breaking, semantic: potentially_breaking, binary: non_breaking

Change 2

Name	Element
Old	none
New	method spoon.reflect.reference.CtTypeReference<?> spoon.reflect.declaration.CtTypeParameter::getTypeErasure()
Code	java.method.addedToInterface
Description	Method was added to an interface.
Breaking	source: breaking, semantic: potentially_breaking, binary: non_breaking

Change 3

Name	Element
Old	none
New	method spoon.reflect.declaration.CtFormalTypeDeclarer spoon.reflect.declaration.CtTypeParameter::getTypeParameterDeclarer()
Code	java.method.addedToInterface
Description	Method was added to an interface.
Breaking	source: breaking, semantic: potentially_breaking, binary: non_breaking

Change 4

Name	Element
Old	none
New	method boolean spoon.reflect.declaration.CtMethod::isSameSignature(spoon.reflect.declaration.CtMethod<?>, boolean)
Code	java.method.addedToInterface
Description	Method was added to an interface.
Breaking	source: breaking, semantic: potentially_breaking, binary: non_breaking

[pvojtechovsky]

I updated first comment of this PR to better fit current content.

I suggest following steps:

1. I would like to ask you to have a short look at each commit separately - to get overview about all these topics.
2. let's discuss the class and method names. Then I can rename them. It is much easier for me if there is one PR/branch only!
3. Do you see some problems?
4. Then I would like to discuss with You how many PR's we will make out of this and what will be their content.

Tomorrow I will improve code to pass all other Spoon tests.

[pvojtechovsky]

//model
<T extends spoon.reflect.code.CtStatement> T getLastStatement();
//test code
CtTypeParameterReference paramT = ...//'T' from example above
assertTrue(paramT.isSubtypeOf(factory.Code().createCtTypeReference(CtElement.class)));

Do you agree that T from the model above is sub type of CtElement? I think it is exactly the request from issue #1160 so it is correct that new implementation returns true there.

But this method SpoonTestHelpers#isMetamodelRelatedType called by CtScannerTest#testScannerCallsAllProperties expects opposite. Do you agree to fix that method?
@monperrus Could you please suggest new implementation of SpoonTestHelpers#isMetamodelRelatedType ?

[pvojtechovsky]

I see now that problem is not in SpoonTestHelpers#isMetamodelRelatedType, which works correctly. But there is problem with missing declaration of Derived property CtBlock#getLastStatement(), which is now correctly detected by fixed test.

[pvojtechovsky]

In legacy spoon code there are problems with CtExecutableReference, which
A) has no parent
B) the invocation scope is the parent
In both cases there is not possible to resolve declaration of CtTypeParameterReference, which is used as type of executable parameters. I have committed here some workarounds W1 and W2, but I am not happy from such "solutions". I would prefer something nicer. But what? It is similar problem like #1237, so I need your help here. It seems to me like there is not possible to use dynamic lookup in this case or ... ?

WDYT?

[spoon-bot]

Revapi Analysis results

Old API: fr.inria.gforge.spoon:spoon-core:jar:5.7.0-20170405.224526-35

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

Detected changes: 1.

Change 1

Name	Element
Old	none
New	method spoon.reflect.visitor.filter.GenericTypeAdapter spoon.reflect.declaration.CtFormalTypeDeclarer::getGenericTypeAdapter()
Code	java.method.addedToInterface
Description	Method was added to an interface.
Breaking	binary: non_breaking, semantic: potentially_breaking, source: breaking

[pvojtechovsky]

Tests finally passed! Jupeee .. now "just" little step ... to pass Martin ;)

[monperrus]

Thanks Pavel.

The first comment is that 22 files changed, it's a lot. I'm not sure they're baby Prs and that they are all necessary here. Maybe we'll have to break this into smaller PRs.

I first concentrate on "adaptType':

• do the changes and comments I've just pushed in the tests correspond to your idea?
• can we rename adaptType to resolveType?

[spoon-bot]

Revapi Analysis results

Old API: fr.inria.gforge.spoon:spoon-core:jar:5.7.0-20170407.224529-37

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

Detected changes: 1.

Change 1

Name	Element
Old	none
New	method spoon.reflect.visitor.filter.GenericTypeAdapter spoon.reflect.declaration.CtFormalTypeDeclarer::getGenericTypeAdapter()
Code	java.method.addedToInterface
Description	Method was added to an interface.
Breaking	source: breaking, semantic: potentially_breaking, binary: non_breaking

[pvojtechovsky]

You are right, they are not baby steps. I am aware of that, so let's discuss what how to break it into smaller useful parts.

I first concentrate on adaptType:

good choice. The adaptType of type parameter declared in scope of CtType is simpler.
Then I suggest to discuss new implementation of isSubtypeOf of CtType, which checks actual type arguments too.
Then adaptType of type parameter declared in scope of CtMethod
Then isOverriding, isSubSignature, isSameSignature of CtMethodSuperTypeHierarchy
And last it makes sense to check isSubTypeOf of type parameter (which needs all previous steps) = the target of this PR.

do the changes and comments I've just pushed in the tests correspond to your idea?

yes, they are OK, Thanks for that!

can we rename adaptType to resolveType?

why? Java lang specification uses term "adapting"

a type mentioned in N can be adapted to the type parameters of M

so why do you look for different name? I think that "adapt" represents that process better then "resolve", because there is source context X and target contexts Y and that process is adapting the type from one context into another context. There can be more target contexts. I think "resolving" is more related to founding of ONE solution, and it is not the case of that adapting process, where you can adapt into more then one target contexts.

But you are the master of spoon, so if you still like "resolve" more, I can rename it and live with that. No problem :-)

[pvojtechovsky]

This shows that java class hierarchies of inner classes are different on each inner class level. In this test model there are two INDEPENDENT class hierarchies:
First is hierachy of top level classes:

CelebrationLunch<K,L,M> extends Lunch<M,K>

and second there is hierarchy of inner class

WeddingLunch<X> extends CelebrationLunch<Tacos, Paella, X> extends Lunch<X,Tacos>

Because in this wild example model, both ( 1. top level class CelebrationLunch and 2. inner class WeddingLunch ) extends from Lunch<A,B>, there is necessary to be able to adapt type parameters A and B in both contexts.

So this test line shows how to move from context of inner class WeddingLunch to the parent context of top level class CelebrationLunch and to adapt type parameters there.

May be it is confusing in this example that

1. WeddingLunch is inner class of CelebrationLunch
2. WeddingLunch extends CelebrationLunch too
   The typingContextOfDisgust.getEnclosingGenericTypeAdapter() is move from inner class to declaring class. It is NOT move from sub type to super type.

[pvojtechovsky]

It is test of contract of getEnclosingGenericTypeAdapter method

[monperrus]

To simplify, can we create instead an adapter of the super class such as new CtSuperTypeHierarchy(Lunch).adaptType(ctClassLunch_A).getQualifiedName())?
Can we remove this method?

[pvojtechovsky]

client can of course create new CtSuperTypeHierarchy using correct reference or type and then s/he gets the same result.

Can we remove this method?

There is hierarchy of GenericTypeAdapter (GTA) instances, like GTA of Method -> GTA of Type -> GTA of enclosing type (recursivelly up to top level type). It is necessary because java allows such complex structures, so I think it is correct that GenericTypeAdapter has method getEnclosingGenericTypeAdapter, which gives access to parent adapter, which is already there and whose construction may need some computation ... so it is inefficient to create GenericTypeAdapter again if the needed one is already there.

If you want to remove it just because clients will not understand it, then note that all implementations of GenericTypeAdapter may be understood as internal helper classes. We can create nice facade methods in existing spoon model elements, so the normal client's does not need to learn/see that.

[monperrus]

I think it is correct that GenericTypeAdapter has method getEnclosingGenericTypeAdapter, which gives access to parent adapter,

What does "parent" mean in this context? the AST parent? or the super class? or something else?

[pvojtechovsky]

I am sorry, "parent" was not the best name. I mean "declaring type". See example:

class A1<K1> {
  class B1<L1> {
  <M1> M1 method(K1 k, L1 l){}
  }
}
class A2<K2> extends A1<K2> {
  class B2<L2> extends B1<L2> {
  <M2> M2 method(K2 k, L2 l){}
  }
}

In the model above the

//adapter for method `A2.B2#method`
CtMethodSuperTypeHierarchy methodAdapter = new CtMethodSuperTypeHierarchy(...method("A2.B2#method"));

//the enclosing adapter of `methodAdapter` is adapter of inner class `A2.B2`
CtSuperTypeHierarchy b2Adapter = methodAdapter.getEnclosingGenericTypeAdapter();
//the enclosing adapter of `b2Adapter ` is adapter of top level class `A2`
CtSuperTypeHierarchy a2Adapter = b2Adapter.getEnclosingGenericTypeAdapter();
//the enclosing adapter of `a2Adapter ` is null, because A2 is top level class
assertNull(a2Adapter.getEnclosingGenericTypeAdapter())

Note that methodAdapter internally needs adapters of all enclosing declaring types recursively, because that method can contain typed parameters of

1. method itself - there is type parameter M2
2. declaring type B2 - there is type parameter L2
3. declaring type A2 - there is type parameter K2

[pvojtechovsky]

Same like above. In this simplified model

class Lunch<A,B> {}
class CelebrationLunch<K,L,M> extends Lunch<M,K> {}

it shows that type parameter A of Lunch is adapted to M of CelebrationLunch.

[monperrus]

I have a doubt here, the "Lunch.A" of disgust should be Double?

[pvojtechovsky]

In the hierarchy of WeddingLunch<Mole> the Lunch.A is Mole
In the hierarchy of CelebrationLunch<Integer,Long,Double> the Lunch.A is Double

[monperrus]

OK.

[monperrus]

why? Java lang specification uses term "adapting"

OK

[pvojtechovsky]

The java language model is quite complicated if you start to combine inner classes, type parameters, generic methods and legacy methods ... Even after 17 years of daily java programming I had to learn a lot to understand how it works, to be able to implement this PR.

It would be bad if spoon client's would need to understand it too, to be able to use that. Therefore I think that we should create easy and understandable facade methods in the spoon model, which will hide that complexity.
The most of these facade methods are already there:

• CtTypeInformation#isSubtypeOf - already uses GenericTypeAdapter
• CtExecutableReference#isOverriding - will use GenericTypeAdapter after we finish this PR
  Others might be added soon. Something like:
• CtMethod#isOverriding

And it question whether somebody needs these methods:
//adapts type to the scope of this declarer

• CtFormalTypeDeclarer.adaptFrom(CtTypeInformation type)
• CtTypeReference.adaptFrom(CtTypeInformation)
• CtExecutableReference.adaptFrom(CtTypeInformation)
  or
  //adapts this type or reference to the scope
• CtTypeInformation.adaptTo(CtFormalTypeDeclarer scope)
• CtTypeInformation.adaptTo(CtTypeReference scope)
• CtTypeInformation.adaptTo(CtExecutableReference scope)

But it does not mean that we should make GenericTypeAdapter and it's implementations private. We can just put them into some special spoon package so they are less visible for normal clients, but can be still directly used by experienced clients and by spoon core itself.

[monperrus]

I agree with the core semantic and API design of adaptType. Now, the plan is 1) to find good names 3) to identify if all changes in the 22 changed files of this PR are required and/or can be split into smaller separate baby PRs.

Let's start with the naming issue, I propose the following:
CtSuperTypeHierarchy -> ClassTypingContext
CtMethodSuperTypeHierarchy -> MethodTypingContext

[pvojtechovsky]

GenericTypeAdapter, ClassTypingContext and MethodTypingContext moved to spoon/support/visitor/

CtFormalTypeDeclarer#getGenericTypeAdapter removed. Created TypeFactory#createTypeAdapter(CtFormalTypeDeclarer) instead.

What next? ;-)

[spoon-bot]

Revapi Analysis results

Old API: fr.inria.gforge.spoon:spoon-core:jar:5.7.0-20170415.141103-48

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

Detected changes: 1.

Change 1

Name	Element
Old	none
New	method spoon.support.visitor.GenericTypeAdapter spoon.reflect.factory.Factory::createTypeAdapter(spoon.reflect.declaration.CtFormalTypeDeclarer)
Code	java.method.addedToInterface
Description	Method was added to an interface.
Breaking	semantic: potentially_breaking, source: breaking, binary: non_breaking

[spoon-bot]

Revapi Analysis results

Old API: fr.inria.gforge.spoon:spoon-core:jar:5.7.0-20170415.224541-49

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

Detected changes: 1.

Change 1

Name	Element
Old	none
New	method spoon.support.visitor.GenericTypeAdapter spoon.reflect.factory.Factory::createTypeAdapter(spoon.reflect.declaration.CtFormalTypeDeclarer)
Code	java.method.addedToInterface
Description	Method was added to an interface.
Breaking	binary: non_breaking, source: breaking, semantic: potentially_breaking

[monperrus]

never delete existing test code in a feature PR.

when reviewing, if no test code is deleted, with only addition, it's easy to be sure that there is no regression.

this is particularly true for large PRs such as this one.

[pvojtechovsky]

Github comparer confused us. In the better comparer there is visible that red line 91 is not deleted by is located on green line 105, etc.
So I did not deleted test code ;)

[monperrus]

let's close this old PR.

I'm ready to merge so as to move on.

OK to merge?

[pvojtechovsky]

Yes, merge is OK for me.