AstComparator.compare(File, File) only compares the first type of the file

[slarse]

AstComparator.compare(File, File) only compares the first type of the compilation unit. So, given a file with multiple type declarations, such as this:

public class Klass {}

class OtherClass {}

And compare it with this:

public class Klazz {}

class OtherClazz {}

only Klass and Klazz are actually compared. The cause of this is that AstComparator.compare(File, File) uses the getCtType() method, which only fetches a single type.

That's pretty unexpected to me. Is this a bug, or intentional? It seems more appropriate to me to compare the compilation units as a whole.

[slarse]

FYI this usability issue was pointed out to me by @algomaster99, I simply looked at what caused it.

I'd be happy to provide a PR to compare the entire compilation units of the files, if that's desirable for the project.

[martinezmatias]

Hi @slarse, @algomaster99

Thanks for reporting the issue. See here that we only pick the first type. https://github.com/SpoonLabs/gumtree-spoon-ast-diff/blob/master/src/main/java/gumtree/spoon/AstComparator.java#L138

I'd be happy to provide a PR to compare the entire compilation units of the files, if that's desirable for the project.

Great, thanks. PRs are welcome. However, the solution could be a bit tricky.

One idea would be to create the ITree for each type, then to put each tree as child of a new root node. That node is then passed to the diff algorithm.

[slarse]

I'm not sure it has to be all too complicated. Just comparing the packages instead of the types kind of works:

    /**
     * compares two java files
     */
    public Diff compare(File f1, File f2) throws Exception {
        // of course, we need a null check here, this is just to illustrate the concept!
        return this.compare(getCtType(f1).getPackage(), getCtType(f2).getPackage());
    }

Although there is one very odd move operation from one unnamed package to another unnamed package, but I think that's just a matter of tweaking how operations are computed.

In Spork, I build an ITree of the unnamed module using gumtree-spoon and then use a GumTree matcher directly on that, and there are no problems merging files with multiple type declarations. So, I don't think this should be too difficult. Thoughts?

[martinezmatias]

I'm not sure it has to be all too complicated. Just comparing the packages instead of the types kind of works:

Great idea!

[slarse]

I'm not sure it has to be all too complicated. Just comparing the packages instead of the types kind of works:

Great idea!

Cool, I'll start working on a PR when time allows :)

[martinezmatias]

Great!

[algomaster99]

@slarse @martinezmatias

@slarse , you said that we could instead AST diff between packages here. Should we take one more step ahead and show the AST diff between modules? CtModule is the root of the Spoon model so it would make more sense if we diff between entire modules. What do you think?

[monperrus]

Should we take one more step ahead and show the AST diff between modules?

yes, the API should be able to support modules

[slarse]

@slarse , you said that we could instead AST diff between packages here. Should we take one more step ahead and show the AST diff between modules? CtModule is the root of the Spoon model so it would make more sense if we diff between entire modules. What do you think?

@algomaster99

When the input is a file, it doesn't matter, the module will always be the unnamed module.

If the input is a directory, then it would potentially matter. But as far as I can tell, the method we're talking about here is not designed to take a directory as input, only a regular file.

So, to summarize, if you want to support directories then it might be worthwile. If you only want to support regular files then comparing the root packages is sufficient.