ASTRewrite.rewriteAST fails with AssertionFailedException for records when it contains inner class

[subyssurendran666]

What it does

#2835

How to test

Author checklist

• I have thoroughly tested my changes
• The change is following the coding conventions
• I have signed the Eclipse Contributor Agreement (ECA)

[jarthana]

@subyssurendran666 Can you say a word or two about the root cause of the problem and what the fix does? Thanks!

[jarthana]

As I said on the PR, a word or two about why the fix is correct would help speed up the review. For e.g., it's not clear why this is not a problem for regular classes but only a problem for records?

[subyssurendran666]

I have a code snippet

record Test(String name, String address) {

    public static Builder builder() {
    }

    public static final class Builder {
    }
}

and trying to rewrite the inner node using the ASTRewrite, then it throws an exception

org.eclipse.core.runtime.AssertionFailedException: assertion failed: 
	at org.eclipse.core.runtime.Assert.isTrue(Assert.java:113)
	at org.eclipse.core.runtime.Assert.isTrue(Assert.java:99)
	at org.eclipse.text.edits.TextEdit.<init>(TextEdit.java:153)
	at org.eclipse.text.edits.DeleteEdit.<init>(DeleteEdit.java:36)
	at org.eclipse.jdt.internal.core.dom.rewrite.ASTRewriteAnalyzer.doTextRemove(ASTRewriteAnalyzer.java:409)
	at org.eclipse.jdt.internal.core.dom.rewrite.ASTRewriteAnalyzer$ListRewriter.rewriteList(ASTRewriteAnalyzer.java:752)

The exception occurs when the length of DeleteEdit becomes negative. However, when I change the record in the test to a class, the issue resolves.

In the method ASTRewriteAnalyzer.doTextRemove(), it already checks if the len == 0 and returns null accordingly. I applied a similar check to handle cases where the value becomes negative.

[subyssurendran666]

I have a code snippet

record Test(String name, String address) {

    public static Builder builder() {
    }

    public static final class Builder {
    }
}

and trying to rewrite the inner node using the ASTRewrite, then it throws an exception

org.eclipse.core.runtime.AssertionFailedException: assertion failed: 
	at org.eclipse.core.runtime.Assert.isTrue(Assert.java:113)
	at org.eclipse.core.runtime.Assert.isTrue(Assert.java:99)
	at org.eclipse.text.edits.TextEdit.<init>(TextEdit.java:153)
	at org.eclipse.text.edits.DeleteEdit.<init>(DeleteEdit.java:36)
	at org.eclipse.jdt.internal.core.dom.rewrite.ASTRewriteAnalyzer.doTextRemove(ASTRewriteAnalyzer.java:409)
	at org.eclipse.jdt.internal.core.dom.rewrite.ASTRewriteAnalyzer$ListRewriter.rewriteList(ASTRewriteAnalyzer.java:752)

The exception occurs when the length of DeleteEdit becomes negative. However, when I change the record in the test to a class, the issue resolves.

In the method ASTRewriteAnalyzer.doTextRemove(), it already checks if the len == 0 and returns null accordingly. I applied a similar check to handle cases where the value becomes negative.

@jarthana I've noticed another issue related to the AST: the starting position of the MethodDeclaration is invalid. I'm currently debugging this issue.

[subyssurendran666]

The method ASTConvert.convert(TypeDeclaration) is responsible for converting the Compiler AST into a DOM AST, utilising ASTConverter.convertToRecordDeclaration(). During this conversion, it invokes ASTConverter.buildBodyDeclarations() to handle the body declarations. In this process, the typeDeclaration may include a default constructor, even if the test code doesn't explicitly define one. This is due to the automatic constructor generation by the Eclipse Compiler for Java (ECJ).

As the ASTConverter attempts to generate a RecordDeclaration from the Compiler AST, it processes the body in a specific order: fields first, then methods, and finally member types. In the current scenario, there are no fields, so the converter immediately moves on to the methods. The first method encountered is the default constructor generated by the ECJ, which is correctly omitted. The process then moves to the member types.

The issue arises in the subsequent iteration of the loop, which only processes members. This behavior leads to the BAD AST issue, as the ASTConverter incorrectly handles the generated RecordDeclaration. This misprocessing is also the root cause of the reported issue with ASTRewrite. The ASTRewrite process ends up rewriting the wrong inner node inside the record, which results in an invalid AST structure.

The positions of the inner nodes inside the MethodDeclaration in the DOM AST have now been corrected.

[mpalat]

@subyssurendran666 since this is an issue primarily due to conversion, can you please add a junit test case for DOM AST conversion. Once review comments are incorporated please go ahead and merge. Thanks!

[subyssurendran666]

@subyssurendran666 since this is an issue primarily due to conversion, can you please add a junit test case for DOM AST conversion. Once review comments are incorporated please go ahead and merge. Thanks!

Sure, I will add ASTConversion test also.

[subyssurendran666]

As I said on the PR, a word or two about why the fix is correct would help speed up the review. For e.g., it's not clear why this is not a problem for regular classes but only a problem for records?

Done.

[mpalat]

@jarthana: I took the liberty of merging - didn't want to risk another rebase :)