Skip to content

Commit

Permalink
Groovy parser does not support multiple closure arguments without par…
Browse files Browse the repository at this point in the history 
…entheses (#4772)

* Add tests

* Working implementation

* Improvement

* Add `copySpec` to gradle test

* Rename test function methods

* improvement

* improvement

* improvement

* `hasParentheses` improvement

* better comment

* improvement

* Remove deprecated groovy `OmitParentheses`

* Revert "Remove deprecated groovy `OmitParentheses`"

This reverts commit 4c21910.

---------

Co-authored-by: Tim te Beek <[email protected]>
  • Loading branch information
@jevanlingen @timtebeek
jevanlingen and timtebeek authored Dec 11, 2024
1 parent ecc2d62 commit a473f26
Show file tree
Hide file tree
Showing 3 changed files with 132 additions and 29 deletions.
38 changes: 29 additions & 9 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 @@ -805,11 +805,11 @@ public void visitArgumentlistExpression(ArgumentListExpression expression) {
int saveCursor = cursor;
Space beforeOpenParen = whitespace();

OmitParentheses omitParentheses = null;
boolean hasParentheses = true;
if (source.charAt(cursor) == '(') {
cursor++;
} else {
omitParentheses = new OmitParentheses(randomId());
hasParentheses = false;
beforeOpenParen = EMPTY;
cursor = saveCursor;
}
Expand Down Expand Up @@ -853,25 +853,26 @@ public void visitArgumentlistExpression(ArgumentListExpression expression) {

if (unparsedArgs.isEmpty()) {
args.add(JRightPadded.build((Expression) new J.Empty(randomId(), whitespace(), Markers.EMPTY))
.withAfter(omitParentheses == null ? sourceBefore(")") : EMPTY));
.withAfter(hasParentheses ? sourceBefore(")") : EMPTY));
} else {
boolean lastArgumentsAreAllClosures = endsWithClosures(expression.getExpressions());
for (int i = 0; i < unparsedArgs.size(); i++) {
org.codehaus.groovy.ast.expr.Expression rawArg = unparsedArgs.get(i);
Expression arg = visit(rawArg);
if (omitParentheses != null) {
arg = arg.withMarkers(arg.getMarkers().add(omitParentheses));
if (!hasParentheses) {
arg = arg.withMarkers(arg.getMarkers().add(new OmitParentheses(randomId())));
}

Space after = EMPTY;
if (i == unparsedArgs.size() - 1) {
if (omitParentheses == null) {
if (hasParentheses) {
after = sourceBefore(")");
}
} else {
} else if (!(arg instanceof J.Lambda && lastArgumentsAreAllClosures && !hasParentheses)) {
after = whitespace();
if (source.charAt(cursor) == ')') {
// the next argument will have an OmitParentheses marker
omitParentheses = new OmitParentheses(randomId());
// next argument(s), if they exists, are trailing closures and will have an OmitParentheses marker
hasParentheses = false;
}
cursor++;
}
Expand All @@ -883,6 +884,25 @@ public void visitArgumentlistExpression(ArgumentListExpression expression) {
queue.add(JContainer.build(beforeOpenParen, args, Markers.EMPTY));
}

public boolean endsWithClosures(List<org.codehaus.groovy.ast.expr.Expression> list) {
if (!(list.get(list.size() - 1) instanceof ClosureExpression)) {
return false;
}

boolean foundNonClosure = false;
for (int i = list.size() - 2; i >= 0; i--) {
if (list.get(i) instanceof ClosureExpression) {
if (foundNonClosure) {
return false;
}
} else {
foundNonClosure = true;
}
}

return true;
}

@Override
public void visitClassExpression(ClassExpression clazz) {
String unresolvedName = clazz.getType().getUnresolvedName().replace('$', '.');
Expand Down
44 changes: 24 additions & 20 deletions rewrite-groovy/src/main/java/org/openrewrite/groovy/GroovyPrinter.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -295,16 +295,16 @@ public J visitLambda(J.Lambda lambda, PrintOutputCapture<P> p) {
LambdaStyle ls = lambda.getMarkers().findFirst(LambdaStyle.class)
.orElse(new LambdaStyle(null, false, !lambda.getParameters().getParameters().isEmpty()));
boolean parenthesized = lambda.getParameters().isParenthesized();
if(!ls.isJavaStyle()) {
if (!ls.isJavaStyle()) {
p.append('{');
}
visitMarkers(lambda.getParameters().getMarkers(), p);
visitSpace(lambda.getParameters().getPrefix(), Space.Location.LAMBDA_PARAMETERS_PREFIX, p);
if(parenthesized) {
if (parenthesized) {
p.append('(');
}
visitRightPadded(lambda.getParameters().getPadding().getParameters(), JRightPadded.Location.LAMBDA_PARAM, ",", p);
if(parenthesized) {
if (parenthesized) {
p.append(')');
}
if (ls.isArrow()) {
Expand All @@ -318,7 +318,7 @@ public J visitLambda(J.Lambda lambda, PrintOutputCapture<P> p) {
} else {
visit(lambda.getBody(), p);
}
if(!ls.isJavaStyle()) {
if (!ls.isJavaStyle()) {
p.append('}');
}
afterSyntax(lambda, p);
Expand Down Expand Up @@ -358,6 +358,7 @@ public J visitForEachLoop(J.ForEachLoop forEachLoop, PrintOutputCapture<P> p) {
afterSyntax(forEachLoop, p);
return forEachLoop;
}

@Override
public J visitMethodDeclaration(J.MethodDeclaration method, PrintOutputCapture<P> p) {
beforeSyntax(method, Space.Location.METHOD_DECLARATION_PREFIX, p);
Expand Down Expand Up @@ -417,29 +418,32 @@ public J visitMethodInvocation(J.MethodInvocation method, PrintOutputCapture<P>

visitSpace(argContainer.getBefore(), Space.Location.METHOD_INVOCATION_ARGUMENTS, p);
List<JRightPadded<Expression>> args = argContainer.getPadding().getElements();
boolean argsAreAllClosures = args.stream().allMatch(it -> it.getElement() instanceof J.Lambda);
boolean hasParentheses = true;
boolean applyTrailingLambdaParenthese = true;
for (int i = 0; i < args.size(); i++) {
JRightPadded<Expression> arg = args.get(i);
boolean omitParens = arg.getElement().getMarkers()
.findFirst(OmitParentheses.class)
.isPresent() ||
arg.getElement().getMarkers()
.findFirst(org.openrewrite.java.marker.OmitParentheses.class)
.isPresent();

if (i == 0 && !omitParens) {
p.append('(');
} else if (i > 0 && omitParens && (
!args.get(0).getElement().getMarkers().findFirst(OmitParentheses.class).isPresent() &&
!args.get(0).getElement().getMarkers().findFirst(org.openrewrite.java.marker.OmitParentheses.class).isPresent()
)) {
p.append(')');
} else if (i > 0) {
boolean omitParensCurrElem = arg.getElement().getMarkers().findFirst(OmitParentheses.class).isPresent() ||
arg.getElement().getMarkers().findFirst(org.openrewrite.java.marker.OmitParentheses.class).isPresent();

if (i == 0) {
if (omitParensCurrElem) {
hasParentheses = false;
} else {
p.append('(');
}
} else if (hasParentheses && omitParensCurrElem) { // first trailing lambda, eg: `stage('Build..') {}`, should close the method
if (applyTrailingLambdaParenthese) { // apply once, to support multiple closures: `foo("baz") {} {}
p.append(')');
applyTrailingLambdaParenthese = false;
}
} else if (hasParentheses || !argsAreAllClosures) {
p.append(',');
}

visitRightPadded(arg, JRightPadded.Location.METHOD_INVOCATION_ARGUMENT, p);

if (i == args.size() - 1 && !omitParens) {
if (i == args.size() - 1 && !omitParensCurrElem) {
p.append(')');
}
}
Expand Down
79 changes: 79 additions & 0 deletions rewrite-groovy/src/test/java/org/openrewrite/groovy/tree/MethodInvocationTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,19 @@ void emptyArgsWithParens() {
);
}

@Test
void noParentheses() {
rewriteRun(
groovy(
"""
class SomeObject {}
def foo(String a, int b, SomeObject c, String d) {}
foo "a", 3, new SomeObject(), "d"
"""
)
);
}

@Test
@SuppressWarnings("GroovyVariableNotAssigned")
void nullSafeDereference() {
Expand Down Expand Up @@ -180,6 +193,72 @@ def acceptsClosure(Closure cl) {}
);
}

@Issue("https://github.com/openrewrite/rewrite/issues/4766")
@Test
void gradleFileWithMultipleClosuresWithoutParentheses() {
rewriteRun(
groovy(
"""
copySpec {
from { 'src/main/webapp' } { exclude "**/*.jpg" }
rename '(.+)-staging(.+)', '$1$2'
}
"""
)
);
}

@Test
void multipleClosureArgumentsWithoutParentheses() {
rewriteRun(
groovy(
"""
def foo(Closure a, Closure b, Closure c) {}
foo { } { } {
}
"""
)
);
}

@Test
void multipleClosureArgumentsWithParentheses() {
rewriteRun(
groovy(
"""
def foo(Closure a, Closure b, Closure c) {}
foo({ }, { }, {
})
"""
)
);
}

@Test
void multipleArgumentsWithClosuresAndNonClosuresWithoutParentheses() {
rewriteRun(
groovy(
"""
def foo(String a, Closure b, Closure c, String d) {}
foo "a", { }, {
}, "d"
"""
)
);
}

@Test
void trailingClosures() {
rewriteRun(
groovy(
"""
def foo(String a, int b, String c, Closure d, Closure e, Closure f) {}
foo("bar", 3, "baz") { } { } {}
"""
)
);
}

@Issue("https://github.com/openrewrite/rewrite/issues/1236")
@Test
@SuppressWarnings("GroovyAssignabilityCheck")
Expand Down

0 comments on commit a473f26

Please sign in to comment.