refactor MethodTyingContext#isOverriding

[monperrus]

public method MethodTyingContext#isOverriding is really hard to understand. Intuitively, a context cannot override a method (which is what Context#isOverriding suggests) and one would expect a second parameter

isOverriding(CtMethod, CtMethod)

[monperrus]

the same applies to public ClassTypingContextisSubtypeOf

[pvojtechovsky]

I understand, you are right. Let's discuss first refactoring of MethodTypingContext.

MethodTypingContext is light weight class with these simple members:

CtExecutable<?> scopeMethod;
List<CtTypeReference<?>> actualTypeArguments;

which represents an method and it's actual type arguments...
... and there is also ClassTypingContext classTypingContext, which represents the typing context of the method's declaring class, which is always needed too.

It is easy and fast operation to create scopeMethod and actualTypeArguments from any executable or invocation.

But ClassTypingContext is more heavy (it's building needs much more operations), so it makes sense to design the API by way the ClassTypingContext can be reused.

I1) So the idea is to move methods isOverriding, isSubSignature, isSameSignature and adaptMethod of MethodTypingContext somewhere else ... But where?

I2) I guess one is clear: CtMethod ClassTypingContext#adaptMethod(CtMethod). WDYT?

P1) The problem of remaining 3 methods is also with arguments ...

• their first argument can be actually all these types: CtMethod, CtConstructor, CtInvocation, CtExecutableReference
• their second argument is actually: CtMethod

But does these arguments makes sense?

I3) I would say that only this makes sense:

• isOverriding(CtExecutable, CtExecutable)
• isSubSignature(CtExecutable, CtExecutable)
• isSameSignature(CtExecutable, CtExecutable)

I4) And may be CtExecutable, might be reduced to CtMethod or CtLambda, because for others (CtConstructor and CtAnonymousExecutable) it makes no sense to ask?
... but I do not like this idea. I would prefer using of CtExecutable and to return false in case of CtConstructor and CtAnonymousExecutable.

P2) It is actually bad idea/nonsense to create MethodTypingContext using CtInvocation and then call method isOverriding ...

I5) MethodTypingContext created using CtInvocation and CtExecutableReference makes sense only for method #adaptType(CtTypeInformation)

I suggest following changes:
C1) to remove isOverriding, isSubSignature, isSameSignature and adaptMethod from MethodTypingContext and to keep there only public adaptType implemented by protected MethodTypingContext#adaptTypeParameter.

C2) to move adaptMethod into ClassTypingContext

C3) to implement new package protected AbstractSignatureMethodFilter. The most of the code of MethodTypingContext would be moved there.

C4) to implement (refactor existing) OverriddenMethodFilter, by extending AbstractSignatureMethodFilter

C5) to implement SubSignatureMethodFilter, by extending AbstractSignatureMethodFilter

C6) to implement SameSignatureMethodFilter, by extending AbstractSignatureMethodFilter

WDYT?

[monperrus]

Awesome. Let's split the 6 changes C1..C6 into smaller PRs. We start by C1+C3?

[pvojtechovsky]

We start by C1+C3?
not possible, because there is spoon code, which uses the to be removed code and that code, so minimum PR is C1+C3+C4
... but I understand, I will try smallest PR as possible. You know me ;-p

[monperrus]

Thanks :-)

[pvojtechovsky]

I tried to move method signature related methods from MethodTypingContext to AbstractSignatureMethodFilter and it is ugly... because

• some private methods of MethodTypingContext are needed in AbstractSignatureMethodFilter too, so they have to be copied :( or moved to some helper class with static methods
• usage of OverridenMethodFilter is not more intuitive then MethodTypingContext`.

new OverridenMethodFilter(anMethod1).matches(anMethod2)

is not much better then

new MethodTypingContext().setMethod(anMethod1).isOverriding(anMethod2)

and brings only other problems, like it is hard to provide ClassTypingContext to the OverridenMethodFilter, etc...

Also necessary change of

class OverriddenMethodFilter implements Filter<CtMethod<?>>

to

class OverriddenMethodFilter implements Filter<CtExecutable<?>>

is incompatible and produced many compilation errors in spoon tests and would cause compilation errors by spoon clients too.

So it looks like classes like OverriddenMethodFilter are not good low level substitution for isOverriding, isSubSignature, isSameSignature of MethodTypingContext.

What about another solution for isOverriding, isSubSignature, isSameSignature?
I agree that these methods should not be in MethodTypingContext, because MethodTypingContext can be created from invocation and executable reference, which are not correct starting place for signature comparation algorithms.

What about new class MethodContext in same package like MethodTypingContext?

• it would contain MethodTypingContext internally
• the method which has to be shared would be protected or package protected, so they can be reused
• it would allow creation from CtExecutable only
• it would have methods isOverridenByContextMethod(CtExecutable), isSubSignatureOfContextMethod(CtExecutable), isSameSignatureLikeContextMethod(CtExecutable) ... or whatever better names you accept ;-)

And again (same like MethodTypingContext), this MethodContext would be technical/internal class, which is powerful enough to efficiently solve needs of spoon API methods, but is not expected to be DIRECTLY used by normal spoon clients - so it does not have to be 100% intuitive.
This solution would be quite simple and would fix some problems ... but I do not know if it fixes the problem of this issue from your point of view.

Another solution would be to move all that code into CtExecutableImpl. But then I would need an instance of ClassTypingContext directly in CtTypeImpl to keep good performance - to be able reuse once created ClassTypingContext... but simple solution would eat much more memory ... so it would need a pool of ClassTypingContexts ... and it would be really not KISS.

WDYT?
Do you have any other idea?

[pvojtechovsky]

What about moving these 3 methods into ClassTypingContext? The contract would be: Does method1 overides method2 in context of class X?
The method names on ClassTypingContext would be

• isOverriding(CtExecutable, CtExecutable)
• isSameSignature(CtExecutable, CtExecutable)
• isSubSignature(CtExecutable, CtExecutable)
  WDYT?

[monperrus]

What about moving these 3 methods into ClassTypingContext? The contract would be: Does method1 overides method2 in context of class X? The method names on ClassTypingContext would be * isOverriding(CtExecutable, CtExecutable) * isSameSignature(CtExecutable, CtExecutable) * isSubSignature(CtExecutable, CtExecutable) WDYT?

It looks like a good idea.

[monperrus]

And again (same like MethodTypingContext), this MethodContext would be technical/internal class, which is powerful enough to efficiently solve needs of spoon API methods, but is not expected to be DIRECTLY used by normal spoon clients - so it does not have to be 100% intuitive.

IMHO - Internal classes are important for minimizing the API surface, so it's always a good idea to have them - However, I'd say that good design (intuitive, testable) is as important for internal classes and API classes.

[pvojtechovsky]

the same applies to public ClassTypingContextisSubtypeOf

What about rename of ClassTypingContext#isSubtypeOf(CtTypeReference<?>) to ClassTypingContext#isSubtypeOfScopeType(CtTypeReference<?>) ?

If not, then what do you suggest?

[monperrus]

What about isSubtypeOf(CtTypeReference x1, CtTypeReference x2) with a precondition that x1 or x2 corresponds to what's in this context?

[pvojtechovsky]

with a precondition that x1 or x2 corresponds to what's in this context
it is wrong. The isSubtypeOf can be computed only on type which is equal to scope. So the precondition must be like this:
with a precondition that x1 corresponds to what's in this context

Personally I do not like the idea of adding extra useless parameter x1, which just decreases readability, performance, understanding ...

Your suggestion is based on wrong understanding of contract of ClassTypingContext.

[monperrus]

done per 0743b5a which fixes most of the problems.

since neither Pavel nor me can find a good solution to isSubtypeOf, I suggest to close this one for the moment and move on.