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

review: feature: MethodTypingContext#adaptMethod #1288

Merged
merged 5 commits into from May 10, 2017

Conversation

Projects
None yet
2 participants
@pvojtechovsky
Collaborator

pvojtechovsky commented May 7, 2017

I am implementing the algorithm, which searches for all methods, which has to be modified when changing method signature (e.g. remove method parameter). This algorithm needs to be able to search in all super types of sub types of declaring type of target methods, for methods with equal sub signature... yes, it is tricky sentence ... so see Example:

class A {
 void method() {}
}
interface IB {
 void method();
}
class B extends A implements IB {
//note that B does not declares `method`. It inherits it from `A`.
}
class C implements IB {
 void method();
}

If A#method() has to be refactored, then that algorithm has to found C#method() too. But note that C#method(), does not overrides A#method(), so it is not enough to use existing overriding filter.
The algorithm first needs to detect that IB#method() is overridden by B#method() (which does not exists, but is inherited from A).
We can do that by new feature implemented in this PR like this:

MethodTypingContext mtc = new MethodTypingContext()
   .setClassTypingContext(new ClassTypingContext(classB));
//create a clone of `A#method` whose generic types are adapted to scope of class `B`
CtMethod classB_method = mtc.adaptMethod(classA_method);
mtc.setMethod(classB_method);
assertTrue(mtc.isOverriding(ifaceB_method));

or shortly like this:

MethodTypingContext mtc = new MethodTypingContext()
   .setClassTypingContext(new ClassTypingContext(classB))
   .setMethod(classA_method); //it detects that method is declared in different scope and adapts it automatically
assertTrue(mtc.isOverriding(ifaceB_method));

The example above is simplified. There are no generic parameters in method, so it might be solved different way. But with generic parameters and longer type hierarchies, the adapting of method parameters is necessary, because A#method and C#method might have different parameter types, but they can be still related, thanks to some generic method(s) in between ...

Note: review only second commit. The first commit comes from another PR. I will rebase after first commit is merged.

@pvojtechovsky pvojtechovsky changed the title from review2: feature: MethodTypingContext#adaptMethod to review: feature: MethodTypingContext#adaptMethod May 8, 2017

monperrus added some commits May 9, 2017

@monperrus

This comment has been minimized.

Show comment
Hide comment
@monperrus

monperrus May 9, 2017

Collaborator

I've updated the tests. The core contract and test of adaptMethod is fine to me.

After analysis, I see one major and one minor problem here.

The major one is that the design and usage of isOverriding(CtMethod<?> thatMethod) is hard to grasp: it seems naturally impossible that a method overrides a context (or the other way around). Do we change this here on in a subsequent PR?

Collaborator

monperrus commented May 9, 2017

I've updated the tests. The core contract and test of adaptMethod is fine to me.

After analysis, I see one major and one minor problem here.

The major one is that the design and usage of isOverriding(CtMethod<?> thatMethod) is hard to grasp: it seems naturally impossible that a method overrides a context (or the other way around). Do we change this here on in a subsequent PR?

@monperrus monperrus merged commit 7b7e6a8 into INRIA:master May 10, 2017

3 of 4 checks passed

coverage/coveralls Coverage pending from Coveralls.io
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
pull_request-INRIA-spoon-master-docker exec in /tmp/tmph3_1xslb
Details
pull_request-INRIA-spoon-master-revapi no API change
Details
@monperrus

This comment has been minimized.

Show comment
Hide comment
@monperrus

monperrus May 10, 2017

Collaborator

The major one is that the design and usage of isOverriding(CtMethod<?> thatMethod) is hard to grasp: it seems naturally impossible that a method overrides a context (or the other way around). Do we change this here on in a subsequent PR?

See #1292

Collaborator

monperrus commented May 10, 2017

The major one is that the design and usage of isOverriding(CtMethod<?> thatMethod) is hard to grasp: it seems naturally impossible that a method overrides a context (or the other way around). Do we change this here on in a subsequent PR?

See #1292

@pvojtechovsky pvojtechovsky deleted the pvojtechovsky:feat_adaptMethod branch May 10, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment