Skip to content

Commit

Permalink
Make Groovy Parser correctly handle nested parenthesis (#4801)
Browse files Browse the repository at this point in the history 
* WIP

* Format

* Format

* Move grabbing of whitespace and resetting cursor to where it is actually required

* Extra check is not required

* Use toString

* Add `emptyListLiteralWithParentheses` test

* Add `insideFourParenthesesAndEnters` test

* Move list tests all to ListTest

* Add `emptyMapLiteralWithParentheses`

* Review feedback and fix new testcases

* Add `attributeWithParentheses`

* Improve AttributeTest

* Improve AttributeTest

* Improve AttributeTest

* Improve AttributeTest

* Improve AttributeTest

* Review fix new testcases

* Revert edit to testcase

* Add and fix testcase with newline

* Add JavaDoc and move logic regarding whitespace and resetting cursor

---------

Co-authored-by: lingenj <[email protected]>
  • Loading branch information
@Laurens-W @jevanlingen
Laurens-W and jevanlingen authored Dec 19, 2024
1 parent e5d9337 commit 91a031a
Show file tree
Hide file tree
Showing 10 changed files with 255 additions and 97 deletions.
10 changes: 10 additions & 0 deletions rewrite-core/src/main/java/org/openrewrite/internal/StringUtils.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -720,4 +720,14 @@ public static String formatUriForPropertiesFile(String uri) {
public static boolean hasLineBreak(@Nullable String s) {
return s != null && LINE_BREAK.matcher(s).find();
}

public static boolean containsWhitespace(String s) {
for (int i = 0; i < s.length(); ++i) {
if (Character.isWhitespace(s.charAt(i))) {
return true;
}
}

return false;
}
}
142 changes: 91 additions & 51 deletions rewrite-groovy/src/main/java/org/openrewrite/groovy/GroovyParserVisitor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
import org.openrewrite.groovy.tree.G;
import org.openrewrite.internal.EncodingDetectingInputStream;
import org.openrewrite.internal.ListUtils;
import org.openrewrite.internal.StringUtils;
import org.openrewrite.java.internal.JavaTypeCache;
import org.openrewrite.java.marker.ImplicitReturn;
import org.openrewrite.java.marker.OmitParentheses;
Expand Down Expand Up @@ -1350,20 +1351,20 @@ public void visitConstantExpression(ConstantExpression expression) {
jType = JavaType.Primitive.Short;
} else if (type == ClassHelper.STRING_TYPE) {
jType = JavaType.Primitive.String;
// String literals value returned by getValue()/getText() has already processed sequences like "\\" -> "\"
int length = sourceLengthOfString(expression);
// this is an attribute selector
if (source.startsWith("@" + value, cursor)) {
length += 1;
boolean attributeSelector = source.startsWith("@" + value, cursor);
int length = lengthAccordingToAst(expression);
Integer insideParenthesesLevel = getInsideParenthesesLevel(expression);
if (insideParenthesesLevel != null) {
length = length - insideParenthesesLevel * 2;
}
text = source.substring(cursor, cursor + length);
int delimiterLength = 0;
if (text.startsWith("$/")) {
delimiterLength = 2;
} else if (text.startsWith("\"\"\"") || text.startsWith("'''")) {
delimiterLength = 3;
} else if (text.startsWith("/") || text.startsWith("\"") || text.startsWith("'")) {
delimiterLength = 1;
String valueAccordingToAST = source.substring(cursor, cursor + length + (attributeSelector ? 1 : 0));
int delimiterLength = getDelimiterLength();
if (StringUtils.containsWhitespace(valueAccordingToAST)) {
length = delimiterLength + expression.getValue().toString().length() + delimiterLength;
text = source.substring(cursor, cursor + length + (attributeSelector ? 1 : 0));
} else {
text = valueAccordingToAST;
}
value = text.substring(delimiterLength, text.length() - delimiterLength);
} else if (expression.isNullExpression()) {
Expand Down Expand Up @@ -1690,15 +1691,18 @@ public void visitGStringExpression(GStringExpression gstring) {

@Override
public void visitListExpression(ListExpression list) {
if (list.getExpressions().isEmpty()) {
queue.add(new G.ListLiteral(randomId(), sourceBefore("["), Markers.EMPTY,
JContainer.build(singletonList(new JRightPadded<>(new J.Empty(randomId(), EMPTY, Markers.EMPTY), sourceBefore("]"), Markers.EMPTY))),
typeMapping.type(list.getType())));
} else {
queue.add(new G.ListLiteral(randomId(), sourceBefore("["), Markers.EMPTY,
JContainer.build(visitRightPadded(list.getExpressions().toArray(new ASTNode[0]), "]")),
typeMapping.type(list.getType())));
}
queue.add(insideParentheses(list, fmt -> {
skip("[");
if (list.getExpressions().isEmpty()) {
return new G.ListLiteral(randomId(), fmt, Markers.EMPTY,
JContainer.build(singletonList(new JRightPadded<>(new J.Empty(randomId(), EMPTY, Markers.EMPTY), sourceBefore("]"), Markers.EMPTY))),
typeMapping.type(list.getType()));
} else {
return new G.ListLiteral(randomId(), fmt, Markers.EMPTY,
JContainer.build(visitRightPadded(list.getExpressions().toArray(new ASTNode[0]), "]")),
typeMapping.type(list.getType()));
}
}));
}

@Override
Expand All @@ -1713,17 +1717,19 @@ public void visitMapEntryExpression(MapEntryExpression expression) {

@Override
public void visitMapExpression(MapExpression map) {
Space prefix = sourceBefore("[");
JContainer<G.MapEntry> entries;
if (map.getMapEntryExpressions().isEmpty()) {
entries = JContainer.build(Collections.singletonList(JRightPadded.build(
new G.MapEntry(randomId(), whitespace(), Markers.EMPTY,
JRightPadded.build(new J.Empty(randomId(), sourceBefore(":"), Markers.EMPTY)),
new J.Empty(randomId(), sourceBefore("]"), Markers.EMPTY), null))));
} else {
entries = JContainer.build(visitRightPadded(map.getMapEntryExpressions().toArray(new ASTNode[0]), "]"));
}
queue.add(new G.MapLiteral(randomId(), prefix, Markers.EMPTY, entries, typeMapping.type(map.getType())));
queue.add(insideParentheses(map, fmt -> {
skip("[");
JContainer<G.MapEntry> entries;
if (map.getMapEntryExpressions().isEmpty()) {
entries = JContainer.build(Collections.singletonList(JRightPadded.build(
new G.MapEntry(randomId(), whitespace(), Markers.EMPTY,
JRightPadded.build(new J.Empty(randomId(), sourceBefore(":"), Markers.EMPTY)),
new J.Empty(randomId(), sourceBefore("]"), Markers.EMPTY), null))));
} else {
entries = JContainer.build(visitRightPadded(map.getMapEntryExpressions().toArray(new ASTNode[0]), "]"));
}
return new G.MapLiteral(randomId(), fmt, Markers.EMPTY, entries, typeMapping.type(map.getType()));
}));
}

@Override
Expand Down Expand Up @@ -1901,23 +1907,24 @@ public void visitStaticMethodCallExpression(StaticMethodCallExpression call) {

@Override
public void visitAttributeExpression(AttributeExpression attr) {
Space fmt = whitespace();
Expression target = visit(attr.getObjectExpression());
Space beforeDot = attr.isSafe() ? sourceBefore("?.") :
sourceBefore(attr.isSpreadSafe() ? "*." : ".");
J name = visit(attr.getProperty());
if (name instanceof J.Literal) {
String nameStr = ((J.Literal) name).getValueSource();
assert nameStr != null;
name = new J.Identifier(randomId(), name.getPrefix(), Markers.EMPTY, emptyList(), nameStr, null, null);
}
if (attr.isSpreadSafe()) {
name = name.withMarkers(name.getMarkers().add(new StarDot(randomId())));
}
if (attr.isSafe()) {
name = name.withMarkers(name.getMarkers().add(new NullSafe(randomId())));
}
queue.add(new J.FieldAccess(randomId(), fmt, Markers.EMPTY, target, padLeft(beforeDot, (J.Identifier) name), null));
queue.add(insideParentheses(attr, fmt -> {
Expression target = visit(attr.getObjectExpression());
Space beforeDot = attr.isSafe() ? sourceBefore("?.") :
sourceBefore(attr.isSpreadSafe() ? "*." : ".");
J name = visit(attr.getProperty());
if (name instanceof J.Literal) {
String nameStr = ((J.Literal) name).getValueSource();
assert nameStr != null;
name = new J.Identifier(randomId(), name.getPrefix(), Markers.EMPTY, emptyList(), nameStr, null, null);
}
if (attr.isSpreadSafe()) {
name = name.withMarkers(name.getMarkers().add(new StarDot(randomId())));
}
if (attr.isSafe()) {
name = name.withMarkers(name.getMarkers().add(new NullSafe(randomId())));
}
return new J.FieldAccess(randomId(), fmt, Markers.EMPTY, target, padLeft(beforeDot, (J.Identifier) name), null);
}));
}

@Override
Expand Down Expand Up @@ -2522,15 +2529,48 @@ private int sourceLengthOfString(ConstantExpression expr) {
return lengthAccordingToAst;
}

private static @Nullable Integer getInsideParenthesesLevel(ASTNode node) {
private @Nullable Integer getInsideParenthesesLevel(ASTNode node) {
Object rawIpl = node.getNodeMetaData("_INSIDE_PARENTHESES_LEVEL");
if (rawIpl instanceof AtomicInteger) {
// On Java 11 and newer _INSIDE_PARENTHESES_LEVEL is an AtomicInteger
return ((AtomicInteger) rawIpl).get();
} else {
} else if (rawIpl instanceof Integer) {
// On Java 8 _INSIDE_PARENTHESES_LEVEL is a regular Integer
return (Integer) rawIpl;
} else if (node instanceof MethodCallExpression) {
MethodCallExpression expr = (MethodCallExpression) node;
return determineParenthesisLevel(expr.getObjectExpression().getLineNumber(), expr.getLineNumber(), expr.getObjectExpression().getColumnNumber(), expr.getColumnNumber());
}
return null;
}

/**
* @param childLineNumber the beginning line number of the first sub node
* @param parentLineNumber the beginning line number of the parent node
* @param childColumn the column on the {@code childLineNumber} line where the sub node starts
* @param parentColumn the column on the {@code parentLineNumber} line where the parent node starts
* @return the level of parenthesis parsed from the source
*/
private int determineParenthesisLevel(int childLineNumber, int parentLineNumber, int childColumn, int parentColumn) {
int saveCursor = cursor;
whitespace();
int childBeginCursor = cursor;
if (childLineNumber > parentLineNumber) {
for (int i = 0; i < (childColumn - parentLineNumber); i++) {
childBeginCursor = source.indexOf('\n', childBeginCursor);
}
childBeginCursor += childColumn;
} else {
childBeginCursor += childColumn - parentColumn;
}
int count = 0;
for (int i = cursor; i < childBeginCursor; i++) {
if (source.charAt(i) == '(') {
count++;
}
}
cursor = saveCursor;
return count;
}

private int getDelimiterLength() {
Expand Down
25 changes: 16 additions & 9 deletions rewrite-groovy/src/test/java/org/openrewrite/groovy/tree/AttributeTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,19 +20,26 @@

import static org.openrewrite.groovy.Assertions.groovy;

@SuppressWarnings({"GroovyUnusedAssignment", "GrUnnecessarySemicolon"})
class AttributeTest implements RewriteTest {

@Test
void usingGroovyNode() {
void attribute() {
rewriteRun(
groovy(
"""
def xml = new Node(null, "ivy")
def n = xml.dependencies.dependency.find { it.@name == 'test-module' }
n.@conf = 'runtime->default;docs->docs;sources->sources'
"""
)
groovy("new User('Bob').@name")
);
}

@Test
void attributeInClosure() {
rewriteRun(
groovy("[new User('Bob')].collect { it.@name }")
);
}

@Test
void attributeWithParentheses() {
rewriteRun(
groovy("(new User('Bob').@name)")
);
}
}
31 changes: 30 additions & 1 deletion rewrite-groovy/src/test/java/org/openrewrite/groovy/tree/BinaryTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
*/
package org.openrewrite.groovy.tree;

import org.junit.jupiter.api.Disabled;
import org.junit.jupiter.api.Test;
import org.openrewrite.Issue;
import org.openrewrite.test.RewriteTest;
Expand All @@ -34,6 +33,7 @@ void insideParentheses() {

// NOT inside parentheses, but verifies the parser's
// test for "inside parentheses" condition
groovy("( 1 ) + 1"),
groovy("(1) + 1"),
// And combine the two cases
groovy("((1) + 1)")
Expand Down Expand Up @@ -216,6 +216,35 @@ void stringMultipliedInParentheses() {
);
}

@Issue("https://github.com/openrewrite/rewrite/issues/4703")
@Test
void cxds() {
rewriteRun(
groovy(
"""
def differenceInDays(int time) {
return (int) ((time)/(1000*60*60*24))
}
"""
)
);
}

@Issue("https://github.com/openrewrite/rewrite/issues/4703")
@Test
void extraParensAroundInfixOxxxperator() {
rewriteRun(
groovy(
"""
def timestamp(int hours, int minutes, int seconds) {
30 * (hours)
(((((hours))))) * 30
}
"""
)
);
}

@Issue("https://github.com/openrewrite/rewrite/issues/4703")
@Test
void extraParensAroundInfixOperator() {
Expand Down
17 changes: 13 additions & 4 deletions rewrite-groovy/src/test/java/org/openrewrite/groovy/tree/CastTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@
package org.openrewrite.groovy.tree;

import org.junit.jupiter.api.Test;
import org.junitpioneer.jupiter.ExpectedToFail;
import org.openrewrite.Issue;
import org.openrewrite.test.RewriteTest;

Expand All @@ -25,6 +24,18 @@
@SuppressWarnings({"UnnecessaryQualifiedReference", "GroovyUnusedAssignment", "GrUnnecessarySemicolon"})
class CastTest implements RewriteTest {

@Test
void cast() {
rewriteRun(
groovy(
"""
String foo = ( String ) "hallo"
String bar = "hallo" as String
"""
)
);
}

@Test
void javaStyleCast() {
rewriteRun(
Expand Down Expand Up @@ -74,14 +85,13 @@ void groovyCastAndInvokeMethod() {
rewriteRun(
groovy(
"""
( "" as String ).toString()
( "x" as String ).toString()
"""
)
);
}

@Test
@ExpectedToFail("Parentheses with method invocation is not yet supported")
void groovyCastAndInvokeMethodWithParentheses() {
rewriteRun(
groovy(
Expand All @@ -103,7 +113,6 @@ void javaCastAndInvokeMethod() {
);
}

@ExpectedToFail("Parentheses with method invocation is not yet supported")
@Test
void javaCastAndInvokeMethodWithParentheses() {
rewriteRun(
Expand Down
34 changes: 34 additions & 0 deletions rewrite-groovy/src/test/java/org/openrewrite/groovy/tree/ListTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,29 @@
@SuppressWarnings("GroovyUnusedAssignment")
class ListTest implements RewriteTest {

@Test
void emptyListLiteral() {
rewriteRun(
groovy(
"""
def a = []
def b = [ ]
"""
)
);
}

@Test
void emptyListLiteralWithParentheses() {
rewriteRun(
groovy(
"""
def y = ([])
"""
)
);
}

@Test
void listLiteral() {
rewriteRun(
Expand All @@ -33,4 +56,15 @@ void listLiteral() {
)
);
}

@Test
void listLiteralTrailingComma() {
rewriteRun(
groovy(
"""
def a = [ "foo" /* "foo" suffix */ , /* "]" prefix */ ]
"""
)
);
}
}
Loading

0 comments on commit 91a031a

Please sign in to comment.