These are chat archives for JnRouvignac/AutoRefactor

21st
Sep 2016
Luis Cruz
@luiscruz
Sep 21 2016 10:33
the problem persists
Even while using only r.remove(node); or r.insertAfter(b.copy(node), method);
Jean-Noël Rouvignac
@JnRouvignac
Sep 21 2016 12:00
Can you push your code in a branch on github?
Jean-Noël Rouvignac
@JnRouvignac
Sep 21 2016 12:20
@luiscruz it is not clear what refactoring you are trying to do
I did not read properly yesterday (context switching)
however, I can see the code tries to insert a MethodInvocation after a MethodDeclaration
But you cannot do that
You need to output valid java code
It is not possible to write this

    @Override
    protected void onPause() {
        super.onPause();
    }

    wl.acquire();

    @Override
    public void onDestroy(){
        wl.release();
        super.onDestroy();
    }
This is what the code you posted above is trying to do
So I think you have to come up with a more realistic refactoring
I think I know what you want to do
let me prototype it
Jean-Noël Rouvignac
@JnRouvignac
Sep 21 2016 12:31
                for(MethodDeclaration method : typeDecl.getMethods() ) {
                    if("onPause".equals(method.resolveBinding().getName())
                            && node.getParent().getNodeType() == ASTNode.EXPRESSION_STATEMENT) {
                        r.insertAt(b.move(node.getParent()),
                                method.getBody().statements().size(),
                                Block.STATEMENTS_PROPERTY,
                                method.getBody());
                        return DO_NOT_VISIT_SUBTREE;
                    }
                }
this is what you want
so a few explanations:
the previous code was copying a Methodnvocation which is an Expression. This is not a Statement, so it results in a compile error if you try to insert it as a Statement.
the previous was removing then inserting a copy
you just need to use b.move(node) which will move the code :)
finally what you wanted was to insert at the end of the onPause() method body
so first you need to get the body (as you did with method.getBody()), and then use insertAt() to say at which index to insert
Please let me know if anything is unclear
Also I suggest you do this in RefactoringRulesTest.java:
Jean-Noël Rouvignac
@JnRouvignac
Sep 21 2016 12:37
     /** If not empty, then only run the refactorings present in this collection. */
     private static final Collection<Class<?>> WHITELIST = Arrays.<Class<?>> asList(
            WakeLockRefactoring.class
     );
This will speed up your feddback cycle tremedously :)
Luis Cruz
@luiscruz
Sep 21 2016 13:22
do you know where can I get more information about this StructuralPropertyDescriptor Block.STATEMENTS_PROPERTY?
Jean-Noël Rouvignac
@JnRouvignac
Sep 21 2016 13:26
what kind of information are you looking for?
Luis Cruz
@luiscruz
Sep 21 2016 13:26
I just want to understand why do we need it
Jean-Noël Rouvignac
@JnRouvignac
Sep 21 2016 13:27
hmm no I do not know
Luis Cruz
@luiscruz
Sep 21 2016 13:27
without having to bug you every time :D
Jean-Noël Rouvignac
@JnRouvignac
Sep 21 2016 13:27
Let me know if you find something I would be interested too
there is the useful links section though
Luis Cruz
@luiscruz
Sep 21 2016 13:27
ok ;)
the list is long :)
When modifying a List, you need to go through this StructuralPropertyDescriptor
I do not really see a better way to do it
But if you do, then please speak up, I'd like to make this API better too
Luis Cruz
@luiscruz
Sep 21 2016 13:32
First I need to understand the “mechanics” :D If I come up with useful suggestions I’ll share them right away ;)
Luis Cruz
@luiscruz
Sep 21 2016 14:47
I still get infinite loop. So, to make sure iterating over the list of methods is not related with the error, I’ve simplified the transformation and I am just trying to move release() to the end of onDestroy().
Jean-Noël Rouvignac
@JnRouvignac
Sep 21 2016 14:49
Even with the code I suggested?
Luis Cruz
@luiscruz
Sep 21 2016 14:49
yap, I believe I’ve copied it right
I’ve seen this in the base code b.copy(node.getName())
Luis Cruz
@luiscruz
Sep 21 2016 14:55
copy is using a SimpleName instead of a node. Is it somehow related?
i’m not making sense, forget
Jean-Noël Rouvignac
@JnRouvignac
Sep 21 2016 14:56
This worked for me:
    @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);
            if(isMethod(enclosingMethod.resolveBinding(), "android.app.Activity", "onDestroy")){
                TypeDeclaration typeDecl= (TypeDeclaration)ASTNodes.getParent(enclosingMethod, TypeDeclaration.class);
                MethodDeclaration[] methods = typeDecl.getMethods();
                for(MethodDeclaration method : methods ) {
                    if("onPause".equals(method.resolveBinding().getName())
                            && node.getParent().getNodeType() == ASTNode.EXPRESSION_STATEMENT) {
                        r.insertAt(b.move(node.getParent()),
                                method.getBody().statements().size(),
                                Block.STATEMENTS_PROPERTY,
                                method.getBody());
                        return DO_NOT_VISIT_SUBTREE;
                    }
                }
            }
        }
        return VISIT_SUBTREE;
    }
You should use b.move()
Luis Cruz
@luiscruz
Sep 21 2016 14:59
yes, it’s working
let me see what was I doing wrong
Jean-Noël Rouvignac
@JnRouvignac
Sep 21 2016 15:01
I am wondering what was wrong too
Luis Cruz
@luiscruz
Sep 21 2016 15:01
yea copy defintely gives infinite loop, but I’ve tried move and the error persisted
The code tries to move a statement, but that statement is already part of the list it is being moved to
I am not sure what this means for the index
pretty hard to figure out what should happen in this case :)
it is not clear to me either when copying
dealing with list is definitely not the easiest
Oh yes I know
using b.copy() keeps adding and adding and adding the statement at the end
Luis Cruz
@luiscruz
Sep 21 2016 15:06
really? why?
AutoRefactor automatically refactors code until it cannot apply new changes anymore
With copy(), the code is always matching, so the refactoring is always applied
indefinitely
this is definitely a use case I did not expect ;)
Luis Cruz
@luiscruz
Sep 21 2016 15:08
hmm, ok.. that makes sense :)
Jean-Noël Rouvignac
@JnRouvignac
Sep 21 2016 15:08
Anyway I think you are setup to go a bit further now
Luis Cruz
@luiscruz
Sep 21 2016 15:08
now I get why this is also infinite loop:
    @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);
            if(isMethod(enclosingMethod.resolveBinding(), "android.app.Activity", "onDestroy")){
                MethodDeclaration method;
                method = enclosingMethod;
                r.insertAt(
                        b.move(node.getParent()),
                        method.getBody().statements().size(),
                        Block.STATEMENTS_PROPERTY,
                        method.getBody()
                );
                return DO_NOT_VISIT_SUBTREE;
            }
            return VISIT_SUBTREE;
        }
    }
now I see.. the only reason we have this use case is because I’m making experiments. Otherwise I don’t see a reason why we would need it.
Jean-Noël Rouvignac
@JnRouvignac
Sep 21 2016 15:10
exactly ;)
Luis Cruz
@luiscruz
Sep 21 2016 15:10
Anyway, I really thought transformations would only apply in the end
Jean-Noël Rouvignac
@JnRouvignac
Sep 21 2016 15:11
it does, after visiting
Luis Cruz
@luiscruz
Sep 21 2016 15:11
oh ok, right!
Jean-Noël Rouvignac
@JnRouvignac
Sep 21 2016 15:11
then it applies the changes and parse the Java code again and try to apply another time to see if new opportunities exist
Luis Cruz
@luiscruz
Sep 21 2016 15:13
that’s why we have while (true) { loop
until it does not find new refactorings at !refactorings.hasRefactorings()
alright, thanks! moving on
Luis Cruz
@luiscruz
Sep 21 2016 15:23
Now I’ll be working on more designing more complex samples
Jean-Noël Rouvignac
@JnRouvignac
Sep 21 2016 15:29
Start easy :)
First come up with a realistic example that does exactly what you want
Then tackle more complex scenarios after
As you have seen there are quite a few things to master
Luis Cruz
@luiscruz
Sep 21 2016 15:30
sounds like good advice
Jean-Noël Rouvignac
@JnRouvignac
Sep 21 2016 15:30
so no need to rush into complex scenarios first
Luis Cruz
@luiscruz
Sep 21 2016 15:30
True! :)
Jean-Noël Rouvignac
@JnRouvignac
Sep 21 2016 15:30
Write the rule for simple cases, and apply it to real code bases
fix all NPEs etc. (there will be some)
and do all this by keeping the test samples accurate with what you noticed in real code
then if you notice it does not catch some cases it should, then it is time to add more test cases to fix
just work gradually or you will be overwhelmed
Luis Cruz
@luiscruz
Sep 21 2016 15:33
yea, i’ll keep that in mind
Jean-Noël Rouvignac
@JnRouvignac
Sep 21 2016 15:33
Luis Cruz
@luiscruz
Sep 21 2016 15:38
it is quite big now xD
Jean-Noël Rouvignac
@JnRouvignac
Sep 21 2016 15:46
Yeah but thank God it was not like this from the start