Skip to content

Commit

Permalink
Map trailing commas as TrailingComma Marker, to fix J.Erroneous…
Browse files Browse the repository at this point in the history 
… issues seen (#4869)

* Do not allow J.Erroneous LST nodes by default in tests

* Allow erroneous nodes in FindCompileErrorsTest

* Use `TrailingComma` marker for enum values and array initializers

Add an overload for `ReloadableJava17ParserVisitor#convert()` which allows supplying a `Markers` function, so that we can capture trailing commas using a `TrailingComma` marker and then also avoid the issue with the `J.Erroneous` getting constructed.

* Update whitespace handling for last enum constant

The last enum constant should only be right-padded if it has a trailing comma or semicolon. This is important because the whitespace after it belongs to the prefix of the next statement or the `J.Block#end`.

* Allow errorneous nodes in specific tests

* Update JavaDoc for typeValidation.erroneous after feedback

* Use overloaded method

* Apply to Java 21 parser as well

* Format Java 21 parser

* Apply to Java 11 parser as well

* Apply to Java 8 parser as well

* Fix failing tests

* Minimize diff between versions to make it easier to compare

---------

Co-authored-by: Knut Wannheden <[email protected]>
Co-authored-by: Laurens Westerlaken <[email protected]>
  • Loading branch information
@timtebeek @knutwannheden @Laurens-W
3 people authored Jan 9, 2025
1 parent d11eae9 commit 82b61de
Show file tree
Hide file tree
Showing 14 changed files with 363 additions and 143 deletions.
98 changes: 68 additions & 30 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 @@ -38,6 +38,7 @@
import org.openrewrite.java.JavaPrinter;
import org.openrewrite.java.internal.JavaTypeCache;
import org.openrewrite.java.marker.OmitParentheses;
import org.openrewrite.java.marker.TrailingComma;
import org.openrewrite.java.tree.*;
import org.openrewrite.marker.Markers;
import org.openrewrite.style.NamedStyles;
Expand Down Expand Up @@ -327,7 +328,7 @@ public J visitBreak(BreakTree node, Space fmt) {

J.Identifier label = node.getLabel() == null ? null : new J.Identifier(randomId(),
sourceBefore(node.getLabel().toString()), Markers.EMPTY,
emptyList(), skip(node.getLabel().toString()), null, null);
emptyList(), node.getLabel().toString(), null, null);

return new J.Break(randomId(), fmt, Markers.EMPTY, label);
}
Expand Down Expand Up @@ -420,13 +421,31 @@ public J visitClass(ClassTree node, Space fmt) {

JRightPadded<Statement> enumSet = null;
if (!jcEnums.isEmpty()) {
AtomicBoolean semicolonPresent = new AtomicBoolean(false);

Tree lastConstant = jcEnums.get(jcEnums.size() - 1);
List<JRightPadded<J.EnumValue>> enumValues = convertAll(jcEnums, commaDelim, t -> {
// this semicolon is required when there are non-value members, but can still
// be present when there are not
semicolonPresent.set(positionOfNext(";", '}') > 0);
return semicolonPresent.get() ? sourceBefore(";", '}') : EMPTY;
if (t != lastConstant) {
return whitespace();
}
int savedCursor = cursor;
Space suffix = whitespace();
if (source.charAt(cursor) == ',' || source.charAt(cursor) == ';') {
return suffix;
}
// Whitespace should be assigned to prefix of next statement or `J.Block#end`
cursor = savedCursor;
return EMPTY;
}, t -> {
if (t == lastConstant && skip(",") != null) {
int savedCursor = cursor;
Space suffix = whitespace();
if (source.charAt(cursor) == ';') {
return Markers.build(singletonList(new TrailingComma(randomId(), suffix)));
}
// Whitespace should be assigned to prefix of next statement or `J.Block#end`
cursor = savedCursor;
return Markers.build(singletonList(new TrailingComma(randomId(), EMPTY)));
}
return Markers.EMPTY;
});

enumSet = padRight(
Expand All @@ -435,7 +454,7 @@ public J visitClass(ClassTree node, Space fmt) {
EMPTY,
Markers.EMPTY,
enumValues,
semicolonPresent.get()
skip(";") != null
),
EMPTY
);
Expand Down Expand Up @@ -1010,7 +1029,14 @@ public J visitNewArray(NewArrayTree node, Space fmt) {
JContainer<Expression> initializer = node.getInitializers() == null ? null :
JContainer.build(sourceBefore("{"), node.getInitializers().isEmpty() ?
singletonList(padRight(new J.Empty(randomId(), sourceBefore("}"), Markers.EMPTY), EMPTY)) :
convertAll(node.getInitializers(), commaDelim, t -> sourceBefore("}")), Markers.EMPTY);
convertAll(node.getInitializers(), commaDelim, t -> whitespace(), t -> {
if (t == node.getInitializers().get(node.getInitializers().size() - 1) && source.charAt(cursor) == ',') {
cursor++;
return Markers.build(singletonList(new TrailingComma(randomId(), whitespace())));
}
return Markers.EMPTY;
}), Markers.EMPTY);
skip("}");

return new J.NewArray(randomId(), fmt, Markers.EMPTY, typeExpr, dimensions,
initializer, typeMapping.type(node));
Expand Down Expand Up @@ -1622,7 +1648,6 @@ public J visitWildcard(WildcardTree node, Space fmt) {
* Conversion utilities
* --------------
*/

private <J2 extends J> @Nullable J2 convert(@Nullable Tree t) {
if (t == null) {
return null;
Expand Down Expand Up @@ -1667,20 +1692,24 @@ private static int getActualStartPosition(JCTree t) {
}

private <J2 extends @Nullable J> @Nullable JRightPadded<J2> convert(@Nullable Tree t, Function<Tree, Space> suffix) {
return convert(t, suffix, j -> Markers.EMPTY);
}

private <J2 extends @Nullable J> @Nullable JRightPadded<J2> convert(@Nullable Tree t, Function<Tree, Space> suffix, Function<Tree, Markers> markers) {
if (t == null) {
return null;
}
J2 j = convert(t);
@SuppressWarnings("ConstantConditions") JRightPadded<J2> rightPadded = j == null ? null :
new JRightPadded<>(j, suffix.apply(t), Markers.EMPTY);
new JRightPadded<>(j, suffix.apply(t), markers.apply(t));
int idx = findFirstNonWhitespaceChar(rightPadded.getAfter().getWhitespace());
if (idx >= 0) {
rightPadded = (JRightPadded<J2>) JRightPadded.build(getErroneous(List.of(rightPadded)));
}
// Cursor hasn't been updated but points at the end of erroneous node already
// This means that error node start position == end position
// Therefore ensure that cursor has moved to the end of erroneous node bu adding its length to the cursor
// Example `/pet` results in 2 erroeneous nodes: `/` and `pet`. The `/` node would have start and end position the
// Therefore ensure that cursor has moved to the end of erroneous node but adding its length to the cursor
// Example `/pet` results in 2 erroneous nodes: `/` and `pet`. The `/` node would have start and end position the
// same from the JC compiler.
if (endPos(t) == cursor && rightPadded.getElement() instanceof J.Erroneous) {
cursor += ((J.Erroneous) rightPadded.getElement()).getText().length();
Expand All @@ -1691,8 +1720,8 @@ private static int getActualStartPosition(JCTree t) {
}

private <J2 extends J> J.Erroneous getErroneous(List<JRightPadded<J2>> converted) {
PrintOutputCapture p = new PrintOutputCapture<>(0);
new JavaPrinter<>().visitContainer(JContainer.build(EMPTY, converted, Markers.EMPTY), JContainer.Location.METHOD_INVOCATION_ARGUMENTS, p);
PrintOutputCapture<Integer> p = new PrintOutputCapture<>(0);
new JavaPrinter<Integer>().visitContainer(JContainer.build(EMPTY, converted, Markers.EMPTY), JContainer.Location.METHOD_INVOCATION_ARGUMENTS, p);
return new J.Erroneous(
org.openrewrite.Tree.randomId(),
EMPTY,
Expand Down Expand Up @@ -1725,13 +1754,20 @@ private <J2 extends J> List<J2> convertAll(List<? extends Tree> trees) {
private <J2 extends J> List<JRightPadded<J2>> convertAll(List<? extends Tree> trees,
Function<Tree, Space> innerSuffix,
Function<Tree, Space> suffix) {
return convertAll(trees, innerSuffix, suffix, t -> Markers.EMPTY);
}

private <J2 extends J> List<JRightPadded<J2>> convertAll(List<? extends Tree> trees,
Function<Tree, Space> innerSuffix,
Function<Tree, Space> suffix,
Function<Tree, Markers> markers) {
int size = trees.size();
if (size == 0) {
return emptyList();
}
List<JRightPadded<J2>> converted = new ArrayList<>(size);
for (int i = 0; i < size; i++) {
converted.add(convert(trees.get(i), i == size - 1 ? suffix : innerSuffix));
converted.add(convert(trees.get(i), i == size - 1 ? suffix : innerSuffix, markers));
}
return converted;
}
Expand All @@ -1756,17 +1792,17 @@ private <J2 extends J> List<JRightPadded<J2>> convertAll(List<? extends Tree> tr

private Space statementDelim(@Nullable Tree t) {
if (t instanceof JCAssert ||
t instanceof JCAssign ||
t instanceof JCAssignOp ||
t instanceof JCBreak ||
t instanceof JCContinue ||
t instanceof JCDoWhileLoop ||
t instanceof JCImport ||
t instanceof JCMethodInvocation ||
t instanceof JCNewClass ||
t instanceof JCReturn ||
t instanceof JCThrow ||
t instanceof JCUnary) {
t instanceof JCAssign ||
t instanceof JCAssignOp ||
t instanceof JCBreak ||
t instanceof JCContinue ||
t instanceof JCDoWhileLoop ||
t instanceof JCImport ||
t instanceof JCMethodInvocation ||
t instanceof JCNewClass ||
t instanceof JCReturn ||
t instanceof JCThrow ||
t instanceof JCUnary) {
return sourceBefore(";");
}

Expand Down Expand Up @@ -1933,6 +1969,7 @@ private Space sourceBefore(String untilDelim, @Nullable Character stop) {
cursor += untilDelim.length();
return EMPTY;
}

Space space = format(source, cursor, delimIndex);
cursor = delimIndex + untilDelim.length(); // advance past the delimiter
return space;
Expand Down Expand Up @@ -2011,15 +2048,16 @@ private Space whitespace() {
return space;
}

private String skip(@Nullable String token) {
private @Nullable String skip(@Nullable String token) {
if (token == null) {
//noinspection ConstantConditions
return null;
}
if (source.startsWith(token, cursor)) {
cursor += token.length();
return token;
}
return token;
return null;
}

// Only exists as a function to make it easier to debug unexpected cursor shifts
Expand All @@ -2040,7 +2078,7 @@ private List<String> listFlags(long flags) {
try {
// FIXME instanceof probably not right here...
return field.get(null) instanceof Long &&
field.getName().matches("[A-Z_]+");
field.getName().matches("[A-Z_]+");
} catch (IllegalAccessException e) {
throw new RuntimeException(e);
}
Expand Down
Loading

0 comments on commit 82b61de

Please sign in to comment.