Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

How to resolve CtTypeParameter using Actual type arguments? #1233

Closed
pvojtechovsky opened this issue Mar 18, 2017 · 11 comments
Closed

How to resolve CtTypeParameter using Actual type arguments? #1233

pvojtechovsky opened this issue Mar 18, 2017 · 11 comments

Comments

@pvojtechovsky
Copy link
Collaborator

I would like to fix #1160 and I came to an idea how to improve handling of actual type arguments of CtTypeReference during some operations, like CtTypeReference#getSuperclass(), CtTypeReference#getSuperInterfaces(), ...?

There is a test case, which shows how it might behave in future.

The idea is

CtTypeReference#getSuperclass() called on reference TRa, would return the CtTypeReference TRb to super class with actual type arguments matching the actual type arguments of TRa.

Simple example:

The type reference to ArrayList<String> would return super interface type reference List<String> and not List<T>, like it does it now.

Please think about this and give me feedback if such change of Spooon behavior is acceptable. I would like to base further PRs on that too. Or if it is not acceptable, then I suggest to add an new API method, which will behave like this. Please suggest how it should look like then.

@monperrus
Copy link
Collaborator

The advantages of the current behavior is that:

  1. it's the least surprising, close to the code one reads
  2. the behavior of CtType.getSuperClass() and CtTypeReference.getSuperClass() are aligned

What you propose makes sense, probably in a new method getSuperClassResolved (or getSuperClassBounded) in CtTypeReference.

WDYT?

@pvojtechovsky
Copy link
Collaborator Author

keep current behavior

ok, I agree.

What you propose makes sense,

Yes, spoon needs that support, because it is really not trivial operation to detect, which value is assigned to which type parameter.

probably in a new method getSuperClassResolved (or getSuperClassBounded) in CtTypeReference.

I would say that we need something else. The problem of resolving value of type parameters is not only about super class. Spoon should have a way how to resolve type parameter everywhere ... in superInterfaces, methods, parameters, statements, ... nearly everywhere where is CtType may be a CtTypeParameter, which needs to be resolved.

So the API should solve this use case: Client has a CtTypeReference or CtType and wants to know what would be the "resolved" CtTypeReference or CtType for an defined actual type arguments.
If the input CtType(Reference) is instance of CtTypeParameter(Reference), then we need to resolve it, otherwise we can return origin CtType(Reference).

But there are some open points in this "design". For example:

How to define the actual type arguments?

  • CtTypeReference is good candidate, because it already holds actual type arguments for referenced type and even actual type arguments for optionall hierarchy of declaring types (needed in case we play with inner class). So references to types would be defined by CtTypeReference
  • There are Constructors and Methods, whose invocations might have actual type arguments. Here the CtTypeReference does not fit. Here we need CtExecutableReference
  • But both CtTypeReference and CtExecutableReference seems to be not good in code which does not solves invocation problems, but plays with relations between CtTypes - mainly if method A overrides method B. In this case no references are needed and we need to be able to adapt CtTypeParameters of one CtType (not CtTypeReference) or one CtMethod (not CtExecutableReference) to another sub/super type or another method.

All these three cases might share common structure, which defines actual type arguments of any hierarchy (e.g.: method/inner class1/inner class2/class) of formal type declarers.

IdentityHashMap<CtFormalTypeDeclarer, List<CtTypeReference<?>>>

But that structure is ugly from client's point of view. It needs some wrapper...

May be API like this would be more user friendly:

  • CtActualTypeContainer#resolveTypeParameter(CtTypeInformation) : CtTypeReference
  • CtActualTypeContainer#resolveTypeParameters(CtTypeInformation...) : List
  • CtFormalTypeDeclarer#resolveTypeParameter(CtTypeInformation) : CtTypeReference
  • CtFormalTypeDeclarer#resolveTypeParameters(CtTypeInformation...) : List

WDYT?

@pvojtechovsky pvojtechovsky changed the title Actual type arguments and CtTypeReference.getSuperclass() and CtType.getReference() How to resolve CtTypeParameter using Actual type arguments? Mar 20, 2017
@pvojtechovsky
Copy link
Collaborator Author

May be an API might be implemented as TypeParameterResolvingFunction mapping function like this:

//the holder of actual type arguments. Points for example to `List<String>`
CtTypeReference typeRef = ... 
//to be resolved type parameter. Point for example to 'T' of `interface List<T> {}`
CtType(Parameter)(Reference) anType = ...
CtTypeReference resolvedTypeRef = anType.map(new TypeParameterResolvingFunction(typeRef)).first()
//resolvedTypeRef points to `String'

This solution has one advantage. The type parameter resolving may need a lot of computation (searching in classes hierarchies of sub type to super type) and the result of this computation might be cached by instance of TypeParameterResolvingFunction, so the next call of the same mapping function can be resolved much faster.

@pvojtechovsky
Copy link
Collaborator Author

CtTypeReference trListOfString = ... //refers to List<String>
CtTypeReference trListOfInteger = ... //refers to List<Integer>
//Do you agree that Spoon should return false here?
assertFalse(trListOfString.isSubtypeOf(trListOfInteger))

Would you accept such implementation?
Following this documentation it should return false.

@monperrus
Copy link
Collaborator

Would you accept such implementation?

yes

@monperrus
Copy link
Collaborator

I agree that there are a number of case to consider. I would still recommend baby steps.

What about starting with CtTypeReference#inferType() : CtTypeReference, where inferType refers to Type Inference.

However, before starting, recall that JDT already infers a number of types for us. According to this thread, it seems that they are not resolved and that we need to do this by hand. Correct? Maybe we could simply improve JDTTreeBuilder so that all types are resolved?

@pvojtechovsky
Copy link
Collaborator Author

It looks like a misunderstanding. I have not found any case when I would need "Type inference". The JDT compiler infers types correctly.

All my work related to resolving of CtTypeParameter have two targets:

  1. to be able to detect whether generic type A is subttype of generic type B
  2. to be able to detect whether generic method M1 of generic type A has the same signature like generic method M2 of generic type B, where A.isSubtypeOf(B).

So I need two functions:

  • type erasure
  • adapting type parameters (actual type arguments) of type A to type parameters of type B

I see no relation to Type inference. May be I or you just use wrong term?

I would still recommend baby steps.

I do my best ... but even the smaller BS, seems to be quite big till now. But I am not finished. I will check how it can be split to smaller parts - as usually :-)

@monperrus
Copy link
Collaborator

OK. Maybe we don't even have to add new methods, but just to improve isSubtypeOf and isOverriding

@pvojtechovsky
Copy link
Collaborator Author

It is probably possible. I can put most of the code into an helper model structure, which will be able to adapt type parameters, compute type erasure and then detect if type is subtype of another type and if method overrides another method. Then we can use this model structure instead of origin implementation of isSubtypeOf and IsOverriding

@monperrus
Copy link
Collaborator

monperrus commented Mar 27, 2017 via email

@monperrus
Copy link
Collaborator

closed with 5e7ee76

@surli surli closed this as completed Apr 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants