Skip to content

Commit

Permalink
Parser failures with unnecessary semicolons (#5063)
Browse files Browse the repository at this point in the history 
* Parser failures with semicolons in new class

* Fixing handling of unnecessary semicolons in between statements

* Fixing handling of unnecessary semicolons after last member of a class

* Refactoring, extracting addPossibleEmptyStatementsBeforeClosingBrace

* Extra tests

* Fixing handling of comments before first statement in block when semicolon is present

* leadingSemicolons test case

* Improve test

* Handling also semicolons in comments

* Improvement

* Minor, formatting

---------

Co-authored-by: Greg Oledzki <[email protected]>
Co-authored-by: lingenj <[email protected]>
  • Loading branch information
@timtebeek @greg-at-moderne @jevanlingen
3 people authored Feb 21, 2025
1 parent 0dc4b54 commit 49855ca
Show file tree
Hide file tree
Showing 6 changed files with 251 additions and 11 deletions.
35 changes: 33 additions & 2 deletions ...te-java-11/src/main/java/org/openrewrite/java/isolated/ReloadableJava11ParserVisitor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -478,6 +478,7 @@ public J visitClass(ClassTree node, Space fmt) {
members.add(enumSet);
}
members.addAll(convertStatements(membersMultiVariablesSeparated));
addPossibleEmptyStatementsBeforeClosingBrace(members);

J.Block body = new J.Block(randomId(), bodyPrefix, Markers.EMPTY, new JRightPadded<>(false, EMPTY, Markers.EMPTY),
members, sourceBefore("}"));
Expand Down Expand Up @@ -1081,8 +1082,11 @@ public J visitNewClass(NewClassTree node, Space fmt) {
}
}

List<JRightPadded<Statement>> converted = convertStatements(members);
addPossibleEmptyStatementsBeforeClosingBrace(converted);

body = new J.Block(randomId(), bodyPrefix, Markers.EMPTY, new JRightPadded<>(false, EMPTY, Markers.EMPTY),
convertStatements(members), sourceBefore("}"));
converted, sourceBefore("}"));
}

JCNewClass jcNewClass = (JCNewClass) node;
Expand Down Expand Up @@ -1857,7 +1861,7 @@ private List<JRightPadded<Statement>> convertStatements(@Nullable List<? extends
private List<JRightPadded<Statement>> convertStatements(@Nullable List<? extends Tree> trees,
Function<Tree, Space> suffix) {
if (trees == null || trees.isEmpty()) {
return emptyList();
return new ArrayList<>();
}

Map<Integer, List<Tree>> treesGroupedByStartPosition = new LinkedHashMap<>();
Expand All @@ -1875,6 +1879,19 @@ private List<JRightPadded<Statement>> convertStatements(@Nullable List<? extends
int startPosition = ((JCTree) t).getStartPosition();
if (cursor > startPosition)
continue;
if (!(t instanceof JCSkip)) {
while (cursor < startPosition) {
int nonWhitespaceIndex = indexOfNextNonWhitespace(cursor, source);
int semicolonIndex = source.charAt(nonWhitespaceIndex) == ';' ? nonWhitespaceIndex : -1;
if (semicolonIndex > -1) {
Space prefix = Space.format(source, cursor, semicolonIndex);
converted.add(new JRightPadded<>(new J.Empty(randomId(), prefix, Markers.EMPTY), Space.EMPTY, Markers.EMPTY));
cursor = semicolonIndex + 1;
} else {
break;
}
}
}
converted.add(convert(treeGroup.get(0), suffix));
} else {
// multi-variable declarations are split into independent overlapping JCVariableDecl's by the OpenJDK AST
Expand Down Expand Up @@ -2288,4 +2305,18 @@ Space formatWithCommentTree(String prefix, JCTree tree, DCTree.@Nullable DCDocCo

return fmt;
}

private void addPossibleEmptyStatementsBeforeClosingBrace(List<JRightPadded<Statement>> converted) {
while (true) {
int closingBracePosition = positionOfNext("}", null);
int semicolonPosition = positionOfNext(";", null);

if (semicolonPosition > -1 && semicolonPosition < closingBracePosition) {
converted.add(new JRightPadded<>(new J.Empty(randomId(), sourceBefore(";"), Markers.EMPTY), Space.EMPTY, Markers.EMPTY));
cursor = semicolonPosition + 1;
} else {
break;
}
}
}
}
38 changes: 34 additions & 4 deletions ...te-java-17/src/main/java/org/openrewrite/java/isolated/ReloadableJava17ParserVisitor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,6 @@
import javax.lang.model.element.Modifier;
import javax.lang.model.element.Name;
import java.lang.reflect.Field;
import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Method;
import java.nio.charset.Charset;
import java.nio.file.Path;
import java.util.*;
Expand All @@ -64,6 +62,7 @@
import static java.util.stream.Collectors.toList;
import static java.util.stream.StreamSupport.stream;
import static org.openrewrite.Tree.randomId;
import static org.openrewrite.internal.StringUtils.indexOf;
import static org.openrewrite.internal.StringUtils.indexOfNextNonWhitespace;
import static org.openrewrite.java.tree.Space.EMPTY;
import static org.openrewrite.java.tree.Space.format;
Expand Down Expand Up @@ -539,6 +538,7 @@ public J visitClass(ClassTree node, Space fmt) {
members.add(enumSet);
}
members.addAll(convertStatements(membersMultiVariablesSeparated));
addPossibleEmptyStatementsBeforeClosingBrace(members);

J.Block body = new J.Block(randomId(), bodyPrefix, Markers.EMPTY, new JRightPadded<>(false, EMPTY, Markers.EMPTY),
members, sourceBefore("}"));
Expand Down Expand Up @@ -1154,8 +1154,11 @@ public J visitNewClass(NewClassTree node, Space fmt) {
}
}

List<JRightPadded<Statement>> converted = convertStatements(members);
addPossibleEmptyStatementsBeforeClosingBrace(converted);

body = new J.Block(randomId(), bodyPrefix, Markers.EMPTY, new JRightPadded<>(false, EMPTY, Markers.EMPTY),
convertStatements(members), sourceBefore("}"));
converted, sourceBefore("}"));
}

JCNewClass jcNewClass = (JCNewClass) node;
Expand Down Expand Up @@ -1937,7 +1940,7 @@ private List<JRightPadded<Statement>> convertStatements(@Nullable List<? extends
private List<JRightPadded<Statement>> convertStatements(@Nullable List<? extends Tree> trees,
Function<Tree, Space> suffix) {
if (trees == null || trees.isEmpty()) {
return emptyList();
return new ArrayList<>();
}

Map<Integer, List<Tree>> treesGroupedByStartPosition = new LinkedHashMap<>();
Expand All @@ -1955,6 +1958,19 @@ private List<JRightPadded<Statement>> convertStatements(@Nullable List<? extends
int startPosition = ((JCTree) t).getStartPosition();
if (cursor > startPosition)
continue;
if (!(t instanceof JCSkip)) {
while (cursor < startPosition) {
int nonWhitespaceIndex = indexOfNextNonWhitespace(cursor, source);
int semicolonIndex = source.charAt(nonWhitespaceIndex) == ';' ? nonWhitespaceIndex : -1;
if (semicolonIndex > -1) {
Space prefix = Space.format(source, cursor, semicolonIndex);
converted.add(new JRightPadded<>(new J.Empty(randomId(), prefix, Markers.EMPTY), Space.EMPTY, Markers.EMPTY));
cursor = semicolonIndex + 1;
} else {
break;
}
}
}
converted.add(convert(treeGroup.get(0), suffix));
} else {
// multi-variable declarations are split into independent overlapping JCVariableDecl's by the OpenJDK AST
Expand Down Expand Up @@ -2369,4 +2385,18 @@ Space formatWithCommentTree(String prefix, JCTree tree, @Nullable DocCommentTree

return fmt;
}

private void addPossibleEmptyStatementsBeforeClosingBrace(List<JRightPadded<Statement>> converted) {
while (true) {
int closingBracePosition = positionOfNext("}", null);
int semicolonPosition = positionOfNext(";", null);

if (semicolonPosition > -1 && semicolonPosition < closingBracePosition) {
converted.add(new JRightPadded<>(new J.Empty(randomId(), sourceBefore(";"), Markers.EMPTY), Space.EMPTY, Markers.EMPTY));
cursor = semicolonPosition + 1;
} else {
break;
}
}
}
}
34 changes: 32 additions & 2 deletions ...te-java-21/src/main/java/org/openrewrite/java/isolated/ReloadableJava21ParserVisitor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -540,6 +540,7 @@ public J visitClass(ClassTree node, Space fmt) {
members.add(enumSet);
}
members.addAll(convertStatements(membersMultiVariablesSeparated));
addPossibleEmptyStatementsBeforeClosingBrace(members);

J.Block body = new J.Block(randomId(), bodyPrefix, Markers.EMPTY, new JRightPadded<>(false, EMPTY, Markers.EMPTY),
members, sourceBefore("}"));
Expand Down Expand Up @@ -1181,9 +1182,11 @@ public J visitNewClass(NewClassTree node, Space fmt) {
members.add(m);
}
}
List<JRightPadded<Statement>> converted = convertStatements(members);
addPossibleEmptyStatementsBeforeClosingBrace(converted);

body = new J.Block(randomId(), bodyPrefix, Markers.EMPTY, new JRightPadded<>(false, EMPTY, Markers.EMPTY),
convertStatements(members), sourceBefore("}"));
converted, sourceBefore("}"));
}

JCNewClass jcNewClass = (JCNewClass) node;
Expand Down Expand Up @@ -1970,7 +1973,7 @@ private List<JRightPadded<Statement>> convertStatements(@Nullable List<? extends
private List<JRightPadded<Statement>> convertStatements(@Nullable List<? extends Tree> trees,
Function<Tree, Space> suffix) {
if (trees == null || trees.isEmpty()) {
return emptyList();
return new ArrayList<>();
}

Map<Integer, List<Tree>> treesGroupedByStartPosition = new LinkedHashMap<>();
Expand All @@ -1988,6 +1991,19 @@ private List<JRightPadded<Statement>> convertStatements(@Nullable List<? extends
int startPosition = ((JCTree) t).getStartPosition();
if (cursor > startPosition)
continue;
if (!(t instanceof JCSkip)) {
while (cursor < startPosition) {
int nonWhitespaceIndex = indexOfNextNonWhitespace(cursor, source);
int semicolonIndex = source.charAt(nonWhitespaceIndex) == ';' ? nonWhitespaceIndex : -1;
if (semicolonIndex > -1) {
Space prefix = Space.format(source, cursor, semicolonIndex);
converted.add(new JRightPadded<>(new J.Empty(randomId(), prefix, Markers.EMPTY), Space.EMPTY, Markers.EMPTY));
cursor = semicolonIndex + 1;
} else {
break;
}
}
}
converted.add(convert(treeGroup.get(0), suffix));
} else {
// multi-variable declarations are split into independent overlapping JCVariableDecl's by the OpenJDK AST
Expand Down Expand Up @@ -2402,4 +2418,18 @@ Space formatWithCommentTree(String prefix, JCTree tree, @Nullable DocCommentTree

return fmt;
}

private void addPossibleEmptyStatementsBeforeClosingBrace(List<JRightPadded<Statement>> converted) {
while (true) {
int closingBracePosition = positionOfNext("}", null);
int semicolonPosition = positionOfNext(";", null);

if (semicolonPosition > -1 && semicolonPosition < closingBracePosition) {
converted.add(new JRightPadded<>(new J.Empty(randomId(), sourceBefore(";"), Markers.EMPTY), Space.EMPTY, Markers.EMPTY));
cursor = semicolonPosition + 1;
} else {
break;
}
}
}
}
35 changes: 33 additions & 2 deletions rewrite-java-8/src/main/java/org/openrewrite/java/ReloadableJava8ParserVisitor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -476,6 +476,7 @@ public J visitClass(ClassTree node, Space fmt) {
members.add(enumSet);
}
members.addAll(convertStatements(membersMultiVariablesSeparated));
addPossibleEmptyStatementsBeforeClosingBrace(members);

J.Block body = new J.Block(randomId(), bodyPrefix, Markers.EMPTY, new JRightPadded<>(false, EMPTY, Markers.EMPTY),
members, sourceBefore("}"));
Expand Down Expand Up @@ -1079,8 +1080,11 @@ public J visitNewClass(NewClassTree node, Space fmt) {
}
}

List<JRightPadded<Statement>> converted = convertStatements(members);
addPossibleEmptyStatementsBeforeClosingBrace(converted);

body = new J.Block(randomId(), bodyPrefix, Markers.EMPTY, new JRightPadded<>(false, EMPTY, Markers.EMPTY),
convertStatements(members), sourceBefore("}"));
converted, sourceBefore("}"));
}

JCNewClass jcNewClass = (JCNewClass) node;
Expand Down Expand Up @@ -1850,7 +1854,7 @@ private List<JRightPadded<Statement>> convertStatements(@Nullable List<? extends
private List<JRightPadded<Statement>> convertStatements(@Nullable List<? extends Tree> trees,
Function<Tree, Space> suffix) {
if (trees == null || trees.isEmpty()) {
return emptyList();
return new ArrayList<>();
}

Map<Integer, List<Tree>> treesGroupedByStartPosition = new LinkedHashMap<>();
Expand All @@ -1868,6 +1872,19 @@ private List<JRightPadded<Statement>> convertStatements(@Nullable List<? extends
int startPosition = ((JCTree) t).getStartPosition();
if (cursor > startPosition)
continue;
if (!(t instanceof JCSkip)) {
while (cursor < startPosition) {
int nonWhitespaceIndex = indexOfNextNonWhitespace(cursor, source);
int semicolonIndex = source.charAt(nonWhitespaceIndex) == ';' ? nonWhitespaceIndex : -1;
if (semicolonIndex > -1) {
Space prefix = Space.format(source, cursor, semicolonIndex);
converted.add(new JRightPadded<>(new J.Empty(randomId(), prefix, Markers.EMPTY), Space.EMPTY, Markers.EMPTY));
cursor = semicolonIndex + 1;
} else {
break;
}
}
}
converted.add(convert(treeGroup.get(0), suffix));
} else {
// multi-variable declarations are split into independent overlapping JCVariableDecl's by the OpenJDK AST
Expand Down Expand Up @@ -2284,4 +2301,18 @@ Space formatWithCommentTree(String prefix, JCTree tree, DCTree.@Nullable DCDocCo

return fmt;
}

private void addPossibleEmptyStatementsBeforeClosingBrace(List<JRightPadded<Statement>> converted) {
while (true) {
int closingBracePosition = positionOfNext("}", null);
int semicolonPosition = positionOfNext(";", null);

if (semicolonPosition > -1 && semicolonPosition < closingBracePosition) {
converted.add(new JRightPadded<>(new J.Empty(randomId(), sourceBefore(";"), Markers.EMPTY), Space.EMPTY, Markers.EMPTY));
cursor = semicolonPosition + 1;
} else {
break;
}
}
}
}
42 changes: 42 additions & 0 deletions rewrite-java-tck/src/main/java/org/openrewrite/java/tree/ClassDeclarationTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@

import org.junit.jupiter.api.Disabled;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.ValueSource;
import org.openrewrite.Issue;
import org.openrewrite.java.JavaIsoVisitor;
import org.openrewrite.java.MinimumJava17;
Expand Down Expand Up @@ -234,4 +236,44 @@ public J.ClassDeclaration visitClassDeclaration(J.ClassDeclaration classDecl, Ob
)
);
}

@Test
@SuppressWarnings("UnnecessarySemicolon")
void unnecessarySemicolonInBody() {
rewriteRun(
java(
"""
class A {
int i = 0;;
int j = 0;
}
"""
)
);
}

@ParameterizedTest
@ValueSource(strings = {
";",
";;;",
"; // comment",
"; // comment;with;semicolons",
"; /* comment;with;semicolons */",
"; /* comment\n*/",
"; // comment1\n// comment2\n;",
"static String method() { return null; };"
})
void unnecessaryLeadingOrEndingSemicolons(String suffix) {
rewriteRun(
java(
"""
class A {
/*@@*/
int i = 0;
/*@@*/
}
""".replaceAll("/[*]@@[*]/", suffix)
)
);
}
}
Loading

2 comments on commit 49855ca

@traceyyoshima
Copy link
Contributor

@traceyyoshima traceyyoshima commented on 49855ca Feb 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@timtebeek I don't have the rewrite stack built, so I haven't tested this, but it looks like the changes cover a specific case.

Is there a reason to handle semi-colons after all the statements have been processed instead of during the conversion of statements? The cases are handled.

class Foo {
   /* In Block */
   ; // before first statement.
   void methodA() {...}
   ; // between statements.
   void methodB() {...}
   ; // after last statement.
}

@timtebeek
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. Any thoughts here @greg-at-moderne ?

Please sign in to comment.