Unnecessary Numbering in Anonymous Class Tree Representation

[pouryafard75]

Hello,

I have identified a bug that I believe could be problematic in many cases.

When objects are created from anonymous classes, an additional number appears in their representation. I understand that this number originates from the parser and represents the index of the anonymous class, providing a form of relative ordering. However, this number is included in the tree representation, leading to unexpected behavior in the matching algorithm.

For example, please refer to this commit.

In this commit, I added a statement containing an anonymous class. However, due to the additional anonymous class on the right-hand side, the number causes two identical subtrees to differ:

As a result, the diff algorithm fails to match them correctly and the following editscript gets geneated

update-node
---
Class: 1 [559,1073]
replace 1 by 2

And due to the offsets of the modifed subtree, the resulting diff will be like:

We are in the final stages of our experiments and would greatly appreciate it if you could provide a hotfix for this particular issue. This case was inspired by an actual open-source project commit that we encountered in our benchmark dataset.

Thank you in advance for your assistance!

[monperrus]

thanks for the bug report, we'll look into it.

[rishivijayv]

I was looking in to this issue, and will give an update on what I have been able to find so far. Please let me know if this is the right path or if I'm going down the wrong track here 😅 @monperrus @algomaster99

Case Reproduction

I was able to re-create the bug with the following setup (similar to the linked commit, but a little easier).

First.java

public class SpoonInvestigation {
    private void x(){
        sleep(1);
    }
    public void y() {
        startServer(new HttpServlet(){
            @Override
            protected void doGet(){}
        });
    }
}

Second.java

Here I have just added startServer(...) to x()

public class SpoonInvestigation {
    private void x(){
        sleep(1);
        startServer(new HttpServlet(){});
    }
    public void y() {
        startServer(new HttpServlet(){
            @Override
            protected void doGet(){}
        });
    }
}

Test Case

	@Test
	public void exampleRemoveMethod() throws Exception {
		AstComparator diff = new AstComparator();
		File fl = new File("src/test/resources/First.java");
		File fr = new File("src/test/resources/Second.java");

		Diff result = diff.compare(fl, fr);
		System.out.print(result);
	}

This produces the following output (the extraneous update, as in the original)

Update Class at SpoonInvestigation:6
	 {
	    @java.lang.Override
	    protected void doGet() {
	    }
	} to {
	    @java.lang.Override
	    protected void doGet() {
	    }
	}
Insert Invocation at SpoonInvestigation:4
	startServer(new HttpServlet() {})

An unexpected (at least to me) output also seems to be produced when the second startServer() anonymous class is the same as the new anonymous class inserted (in this example, they both had no extra "body"):

Insert Invocation at SpoonInvestigation:7
	startServer(new HttpServlet() {})
Move Invocation from SpoonInvestigation:6 to SpoonInvestigation:4
	startServer(new HttpServlet() {})

Cause

As @pouryafard75 highlights, I believe this is happening because of the numeric labels being assigned to Anonymous Classes. For instance, when stepping through the code and looking at rootSpoonRight at this location, I see that the anonymous class in method x has label 1, and the anonymous class in method y has label 2.

This is in contrast to the starting file (rootSpoonLeft), where the label for the anonymous class in y is 1

So, it could be that the diff algorithm is incorrectly associating these labels with the actual "names" of the classes and reporting the differences in the way that it is?

Fixes

The first thing I wanted to do was try and see what happens if I remove the labels being assigned entirely (or convert them to something ubiquitous like an empty string).

I believe I've found where these labels are being set in Spoon's scanning code. Which in turn seems to be called here and is responsible for populating the tree-rooted structures above

These anonymous entities seem to be getting their "simple names" at this location, which I believe eventually show up as labels.
Unfortunately, commenting this out, or using an empty string ("") as the name at that location results in test failures for the spoon code.
Does this seem to be a plausible location of the cause of this bug @monperrus @algomaster99 ? Or am I braking up the wrong tree here? 🤔 The fact that quite a few spoon's tests seem to rely on these anonymous entities having an automatically generated name makes me feel uneasy about just doing away with these to solve the bug, but I might be misunderstanding something here 😓

[pouryafard75]

Thank you for your detailed answer, @rishivijayv.

I believe what’s needed, is to consistently generate identical trees for identical pieces of code. Currently, the presence of the class labels 1 and 2 causes the trees to differ, which results in the diff algorithm failing. As you suggested, the most logical solution would be to remove the the class labels for the anonymous classes.

[monperrus]

Thanks for the analysis.

It seems that we don't need to change Spoon, but only add a post-processing step in TreeScanner here.

Discovering there is a field nolabel = isNoLabelMode() (source) which may be part of the solution.

[rishivijayv]

@algomaster99 and I looked at the issue and the nolabel property seems to fix the issues!

Setting nolabel property to true before computing the diff only gives us the expected, insertion diff now:

	@Test
	public void exampleRemoveMethod() throws Exception {
		AstComparator diff = new AstComparator();
		File fl = new File("src/test/resources/First.java");
		File fr = new File("src/test/resources/Second.java");

		// New addition: explicitly setting the property to true
                System.setProperty("nolabel", "true");
		Diff result = diff.compare(fl, fr);
		System.out.print(result);
	}

On the First.java and Second.java files above, the diff produced is now as follows:

Insert Invocation at SpoonInvestigation:4
	startServer(new HttpServlet() {})

This is also what is returned in the other case I described: where the anonymous class in y() originally was the same as the new anonymous class inserted in x() in Second.java.

@pouryafard75 : For this specific case, we think that setting the nolabel property to true as above should output the correct diff. Let us know if doing that works for you and the data set you're working with 😄 !

@monperrus : @algomaster99 and I also had a brief discussion about how anonymous classes (and other anonymous entities) seem to be getting their simple-names generated from a "global pool" in Spoon (here) and were wondering if there was a better way to do this. We had a very rudimentary idea of using some information about the parent/super-class in generating the simple-name (not many details yet). Not entirely sure how important or feasible that is (since it will also likely require changing some test cases: as some of them failed when I tried to replace that name with an empty string), but just FYI in case that is something we'd be interested in exploring further!

[monperrus]

Great!

Could you add this test case in the test suite?

Could you give it a try of setting the default to nolabel and see what breaks in the test suite?

how anonymous classes (and other anonymous entities) seem to be getting their simple-names generated from a "global pool" in Spoon (here) and were wondering if there was a better way to do this

we can probably do better. give it a spin!

[rishivijayv]

@monperrus

Could you add this test case in the test suite?

Created a pull request with a couple of test cases that I've been mentioning in the discussion here: #349 !

Could you give it a try of setting the default to nolabel and see what breaks in the test suite?

I tried to set the nolabel property to always be true globally by making this change in the TreeScanner.java file (setting the default to true, and returning this new value of nolabel in isNoLabelMode method here

With this, the new couple of tests I've added pass, but around 70 test cases in the AstComparaterTest.java file fail. When running the entire suite with the mvn test command, around 82 fails, and there are some errors too:

Tests run: 197, Failures: 82, Errors: 4, Skipped: 3

This made me feel setting it to true globally might not be a good idea so I stopped there, unless this needs more investigating?

we can probably do better. give it a spin!

I've created an issue for that in the Spoon repo here: INRIA/spoon#6244 ! I could try something in the coming weeks or someone else can take up the mantle if I don't get around to it/don't succeed 😅 I've tried and document in the issue the reasoning briefly so it's clear for future contributors 😄

[pouryafard75]

@rishivijayv Thanks a lot for your support!

Your solution fixes the issue, but now the tree representation no longer contains any labels, which is a bigger problem for me. I'm translating from Spoon-GumTree representation to the original GumTree to ensure compatibility across all ASTDiffs, and the labels are crucial for this process.

[pouryafard75]

To give you more context, we use offsets and apply specific rules for translation.

Spoon-GumTree follows an older tree representation where non-leaf (inner) nodes can contain labels. This reduces control over granularity. For example, a method node like Method: y [126,258] spans the entire method's offsets. Consequently, if the method is renamed, the entire method appears as modified due to the offsets.

In contrast, GumTree’s original representation provides finer control by introducing a SimpleName node that isolates the method’s name along with its specific offsets.

Therefore, one of our translation rules is to convert inner nodes that contain labels, as implemented in InnerNodeWithLabelTranslation.

Thanks for your efforts to fix this manner. For the moment, I try to address the issue on my end.

[monperrus]

thanks all for the progress.

I also had a brief discussion about how anonymous classes (and other anonymous entities) seem to be getting their simple-names generated from a "global pool" in Spoon (here)

it seems that the bug must be solved directly in Spoon then, and we now have INRIA/spoon#6244

[rishivijayv]

it seems that the bug must be solved directly in Spoon then, and we now have INRIA/spoon#6244

@monperrus one other discussion I had with @algomaster99 was the possibility that maybe instead of a blanket "ignore all labels" flag, we could conditionally ignore labels for certain entities (assuming something like this is easy to do).

This could either be maintained as a dictionary (where noLabel would be a Map) and then we can look-up whether to maintain a Label or not while processing that entity.

Or, we could hard-code this and in going from Spoon's AST into a structure that gumtree expects. So when processing Anonymous Classes for instance, we could just "bypass" the code that uses the simpleName to get the label (maybe somewhere here ?). This might probably make more sense for now given there's only one instance that we know of where ignoring labels is fine, instead of over-engineering 😅

But both of these sound a bit hacky to me, as having labels or a unique way to identify entities seems like a cleaner and safer approach, and I'd like to think gumtree has these labels for a purpose -- so just ignoring them for certain entities might cause other issues in the long-term 🤔

[monperrus]

I like that idea!

boolean noLabel becomes something like List<CtType> ignoredLabelsFor

[algomaster99]

The root cause of the problem is that the labels should not record positional information. Now there are two ways to remove that information:

1. Come up with a better label for anonymous classes (not 1/2/3...)
2. Having a map like you proposed here

In terms of scalability and generality, solution 1 is clearly better.