These are chat archives for JnRouvignac/AutoRefactor

20th
Sep 2016
Luis Cruz
@luiscruz
Sep 20 2016 12:16
I am trying to create the WakeLock rule, so I have created the following files:
plugin/src/main/java/org/autorefactor/refactoring/rules/WakeLockRefactoring.java
samples/src/test/java/org/autorefactor/refactoring/rules/samples_in/WakeLockSample.java
samples/src/test/java/org/autorefactor/refactoring/rules/samples_out/WakeLockSample.java

However, when I run tests I get the following message:

java.lang.AssertionError: WakeLock: refactoring class WakeLockRefactoring should exist

Am I missing something? Is that the right place to put my refactoring class?
brb
Jean-Noël Rouvignac
@JnRouvignac
Sep 20 2016 13:29
Hi Luis
Luis Cruz
@luiscruz
Sep 20 2016 13:29
Hi
I think I should enhance documentation on the wiki with this
Luis Cruz
@luiscruz
Sep 20 2016 13:31
Oh ok! I missed that part, but it makes sense!
If you want I can write something based on my experience
Jean-Noël Rouvignac
@JnRouvignac
Sep 20 2016 13:32
Yes this is a good place
Don't worry about this
I'll do it
Also I can maybe change the test error message to suggest adding the rule to this class
Luis Cruz
@luiscruz
Sep 20 2016 13:34
yea that would be great
Luis Cruz
@luiscruz
Sep 20 2016 13:45
check ;)
Jean-Noël Rouvignac
@JnRouvignac
Sep 20 2016 13:54
error message improved too: JnRouvignac/AutoRefactor@1c7a3a9
Luis Cruz
@luiscruz
Sep 20 2016 13:55
I’ve just finished configuring the project in a different machine. The procedure stands, but I had to change jre version.
I’ll give a try to the new error message

java.lang.AssertionError: WakeLock: refactoring class WakeLockRefactoring should exist.
Make sure you added it to the method getAllRefactoringRules() of the class ...

cool!

Jean-Noël Rouvignac
@JnRouvignac
Sep 20 2016 14:03
yay!
Luis Cruz
@luiscruz
Sep 20 2016 14:12

Now I get this:

org.junit.ComparisonFailure: WakeLock: wrong output; expected:<...R_SERVICE); […]

Which means it’s working :) thanks!

Now I want to detect this method android.app.Activity.onDestroy which is probably being overriden in a subclass of Activity. So, in practice I want to detect android.app.AnySubclassOfActivity.onDestroy
(in this case I’m refering to method invocations)
do you have any suggestion?
Jean-Noël Rouvignac
@JnRouvignac
Sep 20 2016 14:17
Sure you want to detect methods named onDestroy() then
visit(MethodDeclaration) {
if (isMethod(node, "android.app.Activity.onDestroy"))
or somethign like this
isMethod() comes from ASTHelper
So you start with public boolean visit(MethodDeclaration node) {
Luis Cruz
@luiscruz
Sep 20 2016 14:20
here’s what I have so far
    @Override
    public boolean visit(MethodInvocation node) {
        if(isMethod(node, "android.os.PowerManager.WakeLock", "release")){
            // check whether it is being called in onDestroy
            final Refactorings r = this.ctx.getRefactorings();
            final ASTBuilder b = this.ctx.getASTBuilder();
            MethodDeclaration enclosingMethod = (MethodDeclaration) ASTNodes.getParent(node, ASTNode.METHOD_DECLARATION);
            String enclosingMethodName = enclosingMethod.getName().getFullyQualifiedName();
            if("android.app.Activity.onDestroy".equals(enclosingMethodName)){
                r.set(node, MethodInvocation.NAME_PROPERTY, "danger");
            }
            // put it on onPause
            return DO_NOT_VISIT_SUBTREE;
        }
        return VISIT_SUBTREE;
    }
So, I am detecting a method invocation of wakelock.release() and then I am checking whether the invocation happens inside the declaration of android.app.Activity.onDestroy ()
Jean-Noël Rouvignac
@JnRouvignac
Sep 20 2016 14:25
looks good so far
you may do this?
if (isMethod(enclosingMethod.resolveMethodBinding(), "android.app.Activity", "onDestroy") ?
instead of getFullyQualifiedName
Luis Cruz
@luiscruz
Sep 20 2016 14:27
ok, cool
ham it does not work with method declaration
btw, what do you think about (MethodDeclaration) ASTNodes.getParent(node, ASTNode.METHOD_DECLARATION). is there a better way of doing this?
Jean-Noël Rouvignac
@JnRouvignac
Sep 20 2016 14:53
I am using this:
            MethodDeclaration enclosingMethod = ASTHelper.getAncestor(node, MethodDeclaration.class);
In ASTHelper, you should be able to extract a method like this:
    public static boolean isMethod(IMethodBinding node, String typeQualifiedName,
            String methodName, String... parameterTypesQualifiedNames) {
and use it from your rule
Luis Cruz
@luiscruz
Sep 20 2016 15:11
I have this method
public static boolean isMethod(MethodInvocation node, String typeQualifiedName,
            String methodName, String... parameterTypesQualifiedNames) {
Jean-Noël Rouvignac
@JnRouvignac
Sep 20 2016 15:12
You can extract this method out of it:
public static boolean isMethod(IMethodBinding node, String typeQualifiedName,
            String methodName, String... parameterTypesQualifiedNames) {
Luis Cruz
@luiscruz
Sep 20 2016 15:13
what do you mean by extracting? creating a new method based on this?
Jean-Noël Rouvignac
@JnRouvignac
Sep 20 2016 15:21
yes it is what I mean
"Extract method" is the name of a refactoring https://sourcemaking.com/refactoring/extract-method
sorry I keep using these assuming people know them
This is the name you can find in Eclipse/IntelliJ too
Luis Cruz
@luiscruz
Sep 20 2016 15:22
keep using them — I should know this :P
Luis Cruz
@luiscruz
Sep 20 2016 15:24
alright, good to know. I did it manually though :disappointed_relieved:
So now I have this problem:
Jean-Noël Rouvignac
@JnRouvignac
Sep 20 2016 15:25
ouch manually. It's been years I did not do it :)
Luis Cruz
@luiscruz
Sep 20 2016 15:26
I am using android.app.Activity as the typeQualifiedName
But
I want to select any typeQualifiedName that is a subclass of android.app.Activity
Jean-Noël Rouvignac
@JnRouvignac
Sep 20 2016 15:39
That should work
Does it not work?
Sorry end of day for me
I think you should first make your base case work (matching the code, then refactoring it)
making the code pretty comes after
aka refactoring
this is why we need samples to cover as much as possible all the cases from the sample
Making the code pretty is just the final step
Luis Cruz
@luiscruz
Sep 20 2016 15:43
alright, makes sense :smile:
it’s working
I was assuming it wasn’t without testing it
I’m slowly getting familiar with this. Now I’ll try transformations.
Jean-Noël Rouvignac
@JnRouvignac
Sep 20 2016 15:45
Good luck!
There are plenty of examples in the code base
Luis Cruz
@luiscruz
Sep 20 2016 15:48
thanks!
Luis Cruz
@luiscruz
Sep 20 2016 17:37
Let’s see if I’m making sense. The next portion of code is trying to look at all the methods of a class to get the onPause method declaration. When found, I remove the wakelock.release() and put it somewhere after onPause():
for(MethodDeclaration method : methods) {
    if("onPause".equals(method.resolveBinding().getName())){
        r.remove(node);
        r.insertAfter(node, method);
        break;
    }
}

I know r.insertAfter won’t work as expected but I can’t understand why I’m getting errors:

org.autorefactor.util.IllegalStateException: WakeLockSample.java:1:1:An infinite loop has been detected for file WakeLockSample.java. A possible cause is that code is being incorrectly refactored one way then refactored back to what it was. Fix the code before pursuing. Possible culprit ASTVisitor classes are: org.autorefactor.refactoring.rules.WakeLockRefactoring

If I comment r.remove(node); I get the following:

org.autorefactor.util.IllegalStateException: WakeLockSample.java:1:1:An infinite loop has been detected for file WakeLockSample.java. A possible cause is that code is being incorrectly refactored one way then refactored back to what it was. Fix the code before pursuing. Possible culprit ASTVisitor classes are: org.autorefactor.refactoring.rules.WakeLockRefactoring

done for today. Working on this again tomorrow.
Jean-Noël Rouvignac
@JnRouvignac
Sep 20 2016 18:07
It looks like the refactoring is badly applied or replacement is not working
The refactoring is wrong I think
You should insert a copy of the node
Not the node itself which you asked to remove before
So it should be:
r.insertAfter(b.copy(node), method);
Or something like that