review1: feature: CtMethod#isOverriding(CtMethod) #1220

pvojtechovsky · 2017-03-12T21:02:16Z

New API CtMethod#isOverriding(CtMethod), which works better on generic methods, then legacy CtExecutableReference#isOverriding(CtExecutableReference)

This PR actually fails on wrong legacy implementation of CtExecutableReference#isOverriding(CtExecutableReference)

I would like to discuss with you: "How to fix that legacy implementation?"

I did not understood:

why isOverriding method was implemented on CtExecutableReference and not on CtMethod since beginning?
how Template engine works with CtExecutableReference#isOverriding ... I do not know how to replace that code with CtMethod#isOverriding ... because I do not understand that code yet...

monperrus · 2017-04-26T19:51:27Z

do we close this one?

pvojtechovsky · 2017-04-27T05:00:53Z

No. I think it makes sense to have CtMethod#isOverriding or something similar. Actually there is needed CtExecutableReference to be able to check isOverriding and I think it is inefficient/wrong. I will make a PR based on current code this or next week.

pvojtechovsky · 2017-04-27T20:09:40Z

First commit comes from #1275. I will rebase after it is merged.
Then each commit can be understood as small PR

implements CtMethod#isOverriding(CtMethod)
use of CtMethod#isOverriding(CtMethod) in existing code - this one might be extracted as special PR. Just let me know if you think it makes sense.
very simple test - extension of existing test
complex test of CtMethod#isOverriding(CtMethod) - this one originally found problems fixed by review1: fix generic type adapting #1275

I think CtMethod#isOverriding(CtMethod) makes more sense then legacy CtExecutableReference#isOverriding(CtExecutableReference). WDYT?

spoon-bot · 2017-04-27T20:27:52Z

Revapi Analysis results

Old API: fr.inria.gforge.spoon:spoon-core:jar:5.7.0-20170427.191244-62

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

Detected changes: 1.

Change 1

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

monperrus · 2017-04-29T06:53:11Z

I think it makes sense to have CtMethod#isOverriding or something similar.

I agree

why isOverriding method was implemented on CtExecutableReference and not on CtMethod since beginning?

To be able to do analyses on calls on library methods (which are not in the source classpath).

However, it's not relevant anymore because now that we create the spoon model of library classes as well through "shadow classes" (see getTypeDeclaration, getExecutableDeclaration).

How to fix that legacy implementation?

By simply delegating execRef#isOverriding to execRef.getExecutableDeclaration().isOverriding?

spoon-bot · 2017-04-30T18:29:06Z

Revapi Analysis results

Old API: fr.inria.gforge.spoon:spoon-core:jar:5.7.0-20170429.224534-66

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

Detected changes: 1.

Change 1

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

pvojtechovsky · 2017-04-30T19:18:15Z

If the tests pass then this PR is done from my point of view

spoon-bot · 2017-05-01T08:44:21Z

Revapi Analysis results

Old API: fr.inria.gforge.spoon:spoon-core:jar:5.7.0-20170430.224618-67

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

Detected changes: 1.

Change 1

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

spoon-bot · 2017-05-01T09:11:13Z

Revapi Analysis results

Old API: fr.inria.gforge.spoon:spoon-core:jar:5.7.0-20170430.224618-67

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

Detected changes: 1.

Change 1

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

pvojtechovsky · 2017-05-01T09:20:49Z

I removed OverriddenMethodFilter and OverridingMethodFilter from this PR to make it simpler.
I made another PR for them #1276.

spoon-bot · 2017-05-01T09:29:45Z

Revapi Analysis results

Old API: fr.inria.gforge.spoon:spoon-core:jar:5.7.0-20170430.224618-67

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

Detected changes: 1.

Change 1

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

spoon-bot · 2017-05-01T09:39:04Z

Revapi Analysis results

Old API: fr.inria.gforge.spoon:spoon-core:jar:5.7.0-20170430.224618-67

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

Detected changes: 1.

Change 1

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

monperrus · 2017-05-02T07:45:50Z

src/main/java/spoon/support/reflect/reference/CtExecutableReferenceImpl.java

@@ -234,7 +234,8 @@ public boolean isConstructor() {
 	@Override
 	public boolean isOverriding(CtExecutableReference<?> executable) {
 		CtExecutable<?> exec = executable.getDeclaration();
-		if (exec == null) {
+		CtExecutable<?> thisExec = getDeclaration();


why not getExecutableDeclaration? (better, incl shadow classes)

why? Because I used same approach like old code.
Anyway you are right. So I have replaced that on 3 places. Now it uses better getExecutableDeclaration

monperrus · 2017-05-02T07:46:39Z

src/test/java/spoon/test/method_overriding/MethodOverriddingTest.java

+	private void checkMethodOverride(BiFunction<CtMethod<?>, CtMethod<?>, Boolean> isOverriding) {
+		Factory factory = ModelUtils.build(new File("src/test/java/spoon/test/method_overriding/testclasses").listFiles());
+		Map<String, List<CtMethod>> methodsByName = new HashMap<>();
+		factory.getModel().getRootPackage().filterChildren(new TypeFilter<>(CtMethod.class)).forEach((CtMethod m)->{


need one assert to check that we go at least once through the loop

spoon-bot · 2017-05-03T19:50:28Z

Revapi Analysis results

Old API: fr.inria.gforge.spoon:spoon-core:jar:5.7.0-20170503.161928-72

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

Detected changes: 1.

Change 1

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

monperrus · 2017-05-04T08:48:53Z

rebase?

spoon-bot · 2017-05-04T08:58:57Z

Revapi Analysis results

Old API: fr.inria.gforge.spoon:spoon-core:jar:5.7.0-20170504.085207-75

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

Detected changes: 1.

Change 1

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

monperrus · 2017-05-04T09:11:04Z

Thanks Pavel.

By the way, test method checkMethodOverride(BiFunction<CtMethod<?>, CtMethod<?>, Boolean> isOverriding) is really hard to understand.

A simpler checkMethodOverride(CtMethod , CtMethod) and checkMethod**NOT**Override(CtMethod , CtMethod) would be easier to understand and maintain.

pvojtechovsky force-pushed the TestOfIsSubTypeOfTypeParameter branch 3 times, most recently from ed17f5e to 5d5fc3b Compare March 14, 2017 20:33

pvojtechovsky force-pushed the TestOfIsSubTypeOfTypeParameter branch from 5d5fc3b to 47af98d Compare April 27, 2017 20:03

pvojtechovsky changed the title ~~WiP: feature: CtMethod#isOverriding(CtMethod)~~ review2: feature: CtMethod#isOverriding(CtMethod) Apr 27, 2017

pvojtechovsky force-pushed the TestOfIsSubTypeOfTypeParameter branch from 47af98d to 23439b0 Compare April 30, 2017 18:19

pvojtechovsky force-pushed the TestOfIsSubTypeOfTypeParameter branch from 7e30b1c to 4c201ba Compare May 1, 2017 08:34

pvojtechovsky force-pushed the TestOfIsSubTypeOfTypeParameter branch from 4c201ba to da41bd3 Compare May 1, 2017 09:01

pvojtechovsky force-pushed the TestOfIsSubTypeOfTypeParameter branch from c2d18af to 33d568b Compare May 1, 2017 09:15

pvojtechovsky changed the title ~~review2: feature: CtMethod#isOverriding(CtMethod)~~ review1: feature: CtMethod#isOverriding(CtMethod) May 1, 2017

monperrus reviewed May 2, 2017

View reviewed changes

pvojtechovsky mentioned this pull request May 3, 2017

Fix SuperInheritanceHierarchyFunction in noClassPath mode #1281

Merged

pvojtechovsky force-pushed the TestOfIsSubTypeOfTypeParameter branch from 8bf9f64 to 6a6cda1 Compare May 3, 2017 19:39

pvojtechovsky force-pushed the TestOfIsSubTypeOfTypeParameter branch from 6a6cda1 to 62599d9 Compare May 3, 2017 19:53

keep contract: Method overrides itself

076b8aa

pvojtechovsky added 7 commits May 4, 2017 10:51

feature: CtMethod#isOverriding(CtMethod)

9c8b90c

fix NPE when method declaration is not in model

2160dfc

test 1

13103db

test method overriding

9572b7e

improve comment. Describe legacy contract this.isOverriding(this)

e26e81f

use CtExecutableReference#getExecutableDeclaration

8781578

assert that test is checking some methods

f3a5d55

pvojtechovsky force-pushed the TestOfIsSubTypeOfTypeParameter branch from 62599d9 to f3a5d55 Compare May 4, 2017 08:51

monperrus merged commit 9e5e8a8 into INRIA:master May 4, 2017

pvojtechovsky deleted the TestOfIsSubTypeOfTypeParameter branch May 5, 2017 16:52

surli mentioned this pull request Jun 1, 2017

Release: Spoon-5.7.0 incoming! #1328

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

review1: feature: CtMethod#isOverriding(CtMethod) #1220

review1: feature: CtMethod#isOverriding(CtMethod) #1220

pvojtechovsky commented Mar 12, 2017

monperrus commented Apr 26, 2017

pvojtechovsky commented Apr 27, 2017

pvojtechovsky commented Apr 27, 2017 •

edited

Loading

spoon-bot commented Apr 27, 2017

monperrus commented Apr 29, 2017

spoon-bot commented Apr 30, 2017

pvojtechovsky commented Apr 30, 2017

spoon-bot commented May 1, 2017

spoon-bot commented May 1, 2017

pvojtechovsky commented May 1, 2017

spoon-bot commented May 1, 2017

spoon-bot commented May 1, 2017

monperrus May 2, 2017

pvojtechovsky May 2, 2017

monperrus May 2, 2017

pvojtechovsky May 2, 2017

spoon-bot commented May 3, 2017

monperrus commented May 4, 2017

spoon-bot commented May 4, 2017

monperrus commented May 4, 2017

review1: feature: CtMethod#isOverriding(CtMethod) #1220

review1: feature: CtMethod#isOverriding(CtMethod) #1220

Conversation

pvojtechovsky commented Mar 12, 2017

monperrus commented Apr 26, 2017

pvojtechovsky commented Apr 27, 2017

pvojtechovsky commented Apr 27, 2017 • edited Loading

spoon-bot commented Apr 27, 2017

Revapi Analysis results

Change 1

monperrus commented Apr 29, 2017

spoon-bot commented Apr 30, 2017

Revapi Analysis results

Change 1

pvojtechovsky commented Apr 30, 2017

spoon-bot commented May 1, 2017

Revapi Analysis results

Change 1

spoon-bot commented May 1, 2017

Revapi Analysis results

Change 1

pvojtechovsky commented May 1, 2017

spoon-bot commented May 1, 2017

Revapi Analysis results

Change 1

spoon-bot commented May 1, 2017

Revapi Analysis results

Change 1

monperrus May 2, 2017

Choose a reason for hiding this comment

pvojtechovsky May 2, 2017

Choose a reason for hiding this comment

monperrus May 2, 2017

Choose a reason for hiding this comment

pvojtechovsky May 2, 2017

Choose a reason for hiding this comment

spoon-bot commented May 3, 2017

Revapi Analysis results

Change 1

monperrus commented May 4, 2017

spoon-bot commented May 4, 2017

Revapi Analysis results

Change 1

monperrus commented May 4, 2017

pvojtechovsky commented Apr 27, 2017 •

edited

Loading