fix(CtDeprecatedRef): removes only unused deprecated methods

[MartinWitt]

Fix #3195 for details about the problem.
Edit 21.12 Small update:
Added loop detection for deprecated methods only calling deprecated methods e.g. A->B->C->A
Added detection for recursive calling methods e.g. B(){B()}.
Still need to write tests and catch bugs.

[MartinWitt]

Removed the cycle detection because the case A->B->C->A does not seem too realistic and produces many problems.
The bug mentioned in #3195 should be fixed(?) now.
Maybe someone finds some case i forgot to check.

[monperrus]

Thanks a lot, that's super useful.

In Spoon, we try to have readable PRs by minimizing formatting changes not related to the core change. Could you set up your editor to not reformat source code and have a nice minimized diff?

In Spoon, we try to have high level, large test classes, could you move the test to RefactorTest?

Thanks!

[MartinWitt]

This isn´t really part of the bug but we could give the refactoring method some signature like removeDeprecatedMethods(String input, String output, Class annotationClazz) and allow removing for all annotations.
Maybe someone wants to remove all unused methods with @beta annotation.
We could go even further and allow removing of all unused methods?
And removal of classes with annotations could be worth looking in too.
wdyt?

[digulla]

I would prefer a method which collects unused methods and returns a list or set. The other methods (remove or add annotation) should accept a list. That way, the solution would be easy to reuse for all kinds of scenarios (find all unused methods and print them, find the methods + filter the result before processing them, etc).

[MartinWitt]

After @digulla comment I removed the review state for the issue because there is still some more work needed. Splitted the invocationstate analysis for methods in a own class for reusability.

[MartinWitt]

Added javadoc and fixed typo. Now the deprecated removal should work.
I would add the invocation package as subpackage in refactoring, to allow the extension from methods to types. Removing unused top level types could be some feature, if some1 is brav enough to try to implement it.

wdyt about the new package?

[monperrus]

write in target/deprecated-refactoring instead

[monperrus]

Thanks a lot for the effort. I'm not sure we need a new package for a single specific feature, it fits nicely in the refactoring package already.

[monperrus]

Thanks.

Overall coverage decreases by -0.2%. I'm looking at the coverage report, section "New Missed Lines in Diff"
https://coveralls.io/builds/28324991

New class MethodInvocationSearch.java has a coverage of 19% which is low.

Could you add test cases to cover MethodInvocationSearch better (say 80%)?

[MartinWitt]

seems like there are some strange java8 issues. Looking into it.

[monperrus]

thanks @MartinWitt for adding tests to your great PR.

Do you need help to make CI pass?

[MartinWitt]

Yeah i still can't find the bug in the CI. Testing with jdk8 is a bit difficult with windows.
Would be nice to get some help @monperrus .

[MartinWitt]

it looks like i lose method "test4" in jdk8.

[MartinWitt]

After testing with jdk8, i found the problem.

The chained methodcall new Foo(v).test4() does not return CtInvocation test4(). Is there some way to get the call and not the target for a CtInvocation?

Edit: (Clarification) in jdk9+ it's working as intended

[monperrus]

Is there some way to get the call and not the target for a CtInvocation?

what do you mean by "the call"?

[MartinWitt]

I have created a testcase showcasing the unexpected behavior. In RefactoringTest.showBug() you get different results for the invoked methods depending on the jdk version.
model.getElements(new TypeFilter<>(CtInvocation.class)) gives the same result on all versions, but the getExecutable().getExecutableDeclaration() creates different results, depending if you use jdk8 or jdk9+.

[monperrus]

the getExecutable().getExecutableDeclaration() creates different results, depending if you use jdk8 or jdk9+.

Interesting.

This is not the first time we see behavioral differences across Java versions:

• We have tests with looser assertions to handle that (search for JDK/jdk in the test folder)
• We even have an if in the app code to check for the Java version, see https://github.com/INRIA/spoon/blob/master/src/main/java/spoon/reflect/visitor/chain/CtQueryImpl.java#L593

[MartinWitt]

Opened an issue #3240, added check in test for jdk version and add doc comment for clarification.
I wouldn't stop a user, using this refactoring with old jdk but instead tell him in the javadoc.

[monperrus]

Thanks a lot for the big and valuable pull-request!