SpoonLabs / gumtree-spoon-ast-diff

Computes the AST difference (aka edit script) between two Spoon Java source code abstract syntax trees
http://hal.archives-ouvertes.fr/hal-01054552
Apache License 2.0
161 stars 42 forks source link

Incorrect edit script #165

Open algomaster99 opened 3 years ago

algomaster99 commented 3 years ago

left.java

class Invocation {
    int a = 1;
    void checkForSign() {
        if (a > 0) {
            System.out.println("y");
        } else if (a < 0) {
            System.out.println("z");
        } else {
            System.out.println("x");
        }
    }
}

right.java

class Invocation {
    int a = 1;
    void checkForSign() {
        if (a > 0) {
            System.out.println("x");
        } else if (a < 0) {
            System.out.println("y");
        } else {
            System.out.println("z");
        }
    }
}

EDIT SCRIPT

  1. Delete Block at Invocation:6
    {
    
    }
  2. Insert Block at Invocation:4
    {
    
    }
  3. Move Invocation from Invocation:9 to Invocation:5
    System.out.println("x")
  4. Move Block from Invocation:4 to Invocation:6
    {
        System.out.println("y");
    }
  5. Move Invocation from Invocation:7 to Invocation:9
    System.out.println("z")

Ideally, three move operations were enough but an optimum edit script is not guaranteed but a correct one should be. The edit script produced for this case has one major problem - the second operation inserts a block into an already existing block (if block is already there at 4). This could be a consequence of the absence of CtBlock concept in spoon but I am not sure. However, if this is the case, convertToSpoon needs to be fixed.

Using this edit script in diffmin also throws an error as elaborated here. This could be an issue with the mapping as THEN block in else-if in left.java should have been mapped to THEN block in if in right.java. However, GumTree matchers are based on heuristics so we cannot expect the matcher to give accurate mappings every time as it is an NP-hard problem as argued by @slarse.

tree

algomaster99 commented 2 years ago

@monperrus @khaes-kth srcDiff's output for this diff. Screenshot from 2022-02-25 11-02-34

monperrus commented 2 years ago

cool. and the Gumtree-JDT output?

algomaster99 commented 2 years ago

GumTree-JDT (master version as of the date this comment was created). Screenshot from 2022-02-25 11-18-34 Screenshot from 2022-02-25 11-19-50

It is not really clear which element moved unless you have read the edit script.

khaes-kth commented 2 years ago

@algomaster99 Is it fair to say they all fail on this diff?

algomaster99 commented 2 years ago

Mergely

https://editor.mergely.com/Bmo5md7x/

GitHub diff view

https://github.com/algomaster99/diffmin-examples/pull/7/files

We can claim that all of them fail, if we say the correct way to show the diff was 3 updates.

monperrus commented 2 years ago

"Fail" may be too binary. We rather want an ordering relationship: X is better than Y.

Here, I'd say that scrDiff is better than gumtree-JDT for example.

And that the Github diff is the best.

khaes-kth commented 2 years ago

Agreed. As always, there is no problem, only an "opportunity" for us to improve :)

monperrus commented 2 years ago

You're training very well 👍️

algomaster99 commented 2 years ago

Here, I'd say that scrDiff is better than gumtree-JDT for example. And that the Github diff is the best.

I agree with your opinions.