Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix groovy multivariable declaration #4757

Merged
merged 26 commits into from
Dec 17, 2024
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
44c138b
Fix parsing of multiple variable assignments in one statement
Laurens-W Dec 9, 2024
e2ac9f4
Minor improvement
Laurens-W Dec 9, 2024
f9f3cd3
Add another testcase and make sure it works
Laurens-W Dec 9, 2024
9274748
Simplify
Laurens-W Dec 9, 2024
eb95dbb
Clarify
Laurens-W Dec 9, 2024
0685d0e
Revert formatting
Laurens-W Dec 9, 2024
06973e1
Apply suggestions from code review
Laurens-W Dec 9, 2024
a0da837
Fixed
Laurens-W Dec 10, 2024
d9856a8
Fixed
Laurens-W Dec 10, 2024
ff0510a
Move tests to AssignmentTest + add `final def` case
jevanlingen Dec 10, 2024
27e95f1
Should have done it this way initially ;)
Laurens-W Dec 10, 2024
4278625
Add testcase for regression
Laurens-W Dec 10, 2024
d22d5ff
Polish
Laurens-W Dec 10, 2024
3553b1f
Extra testcase
Laurens-W Dec 10, 2024
bbdd436
Add `noKeyword` test
jevanlingen Dec 10, 2024
3786a05
Return String instead of passing StringBuilder
Laurens-W Dec 10, 2024
addfd85
Further convert to using String
Laurens-W Dec 10, 2024
422df9e
Simplify
jevanlingen Dec 11, 2024
0a92ffe
Merge branch 'main' into fix-groovy-multivariable-declaration
jevanlingen Dec 11, 2024
f301c34
Merge branch 'main' into fix-groovy-multivariable-declaration
timtebeek Dec 11, 2024
f196837
Different approach using marker
Laurens-W Dec 12, 2024
9b906d2
Remove unused code and FMT
Laurens-W Dec 12, 2024
7cd916b
Merge branch 'main' into fix-groovy-multivariable-declaration
Laurens-W Dec 13, 2024
b371832
Merge branch 'main' into fix-groovy-multivariable-declaration
timtebeek Dec 15, 2024
f4bb9b7
Remove specific exception cases for VariableDeclarations
Laurens-W Dec 16, 2024
7de11c8
Change to startsWith with offset
Laurens-W Dec 17, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,9 @@
import org.openrewrite.java.marker.OmitParentheses;
import org.openrewrite.java.marker.Semicolon;
import org.openrewrite.java.marker.TrailingComma;
import org.openrewrite.java.tree.*;
import org.openrewrite.java.tree.Expression;
import org.openrewrite.java.tree.Statement;
import org.openrewrite.java.tree.*;
import org.openrewrite.marker.Markers;

import java.math.BigDecimal;
Expand Down Expand Up @@ -122,7 +122,7 @@ private static boolean isOlderThanGroovy3() {
private Optional<RedundantDef> maybeRedundantDef(ClassNode type, String name) {
int saveCursor = cursor;
Space defPrefix = whitespace();
if(source.startsWith("def", cursor)) {
if (source.startsWith("def", cursor)) {
skip("def");
// The def is redundant only when it is followed by the method's return type
// I hope no one puts an annotation between "def" and the return type
Expand Down Expand Up @@ -1419,6 +1419,10 @@ public void visitNotExpression(NotExpression expression) {
@Override
public void visitDeclarationExpression(DeclarationExpression expression) {
Optional<RedundantDef> redundantDef = maybeRedundantDef(expression.getVariableExpression().getType(), expression.getVariableExpression().getName());

/* The identifier of this type is one of the following: the name of a type, `final`, `def`, `var` or `,`
The latter because MultiVariable declarations are returned as separate `DeclarationExpression` by the
Groovy AST */
TypeTree typeExpr = visitVariableExpressionType(expression.getVariableExpression());

J.VariableDeclarations.NamedVariable namedVariable;
Expand Down Expand Up @@ -2077,29 +2081,23 @@ public void visitPrefixExpression(PrefixExpression unary) {

public TypeTree visitVariableExpressionType(VariableExpression expression) {
JavaType type = typeMapping.type(staticType(((org.codehaus.groovy.ast.expr.Expression) expression)));
Space prefix = whitespace();
StringBuilder keyword = new StringBuilder();
Laurens-W marked this conversation as resolved.
Show resolved Hide resolved

if (expression.isDynamicTyped()) {
Space prefix = whitespace();
String keyword;
if (source.substring(cursor).startsWith("final")) {
keyword = "final";
} else {
keyword = source.substring(cursor, cursor + 3);
if (expression.isDynamicTyped() || source.charAt(cursor) == ',') {
while (!Character.isWhitespace(source.charAt(cursor))) {
jevanlingen marked this conversation as resolved.
Show resolved Hide resolved
keyword.append(source.charAt(cursor));
cursor++;
}
} else {
keyword.append(expression.getOriginType().getUnresolvedName());
cursor += keyword.length();
return new J.Identifier(randomId(),
prefix,
Markers.EMPTY,
emptyList(),
keyword,
type, null);
}
Space prefix = sourceBefore(expression.getOriginType().getUnresolvedName());
J.Identifier ident = new J.Identifier(randomId(),
EMPTY,
Markers.EMPTY,
emptyList(),
expression.getOriginType().getUnresolvedName(),
keyword.toString(),
type, null);
if (expression.getOriginType().getGenericsTypes() != null) {
return new J.ParameterizedType(randomId(), prefix, Markers.EMPTY, ident, visitTypeParameterizations(
Expand Down
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
Original file line number Diff line number Diff line change
Expand Up @@ -54,4 +54,52 @@ public J visitCompilationUnit(G.CompilationUnit cu, ExecutionContext ctx) {
)
);
}

@Test
void multipleAssignmentsAtOneLine() {
rewriteRun(
groovy(
"""
def startItem = '| ', endItem = ' |'
def repeatLength = startItem.length() + output.length() + endItem.length()
println("\\n" + ("-" * repeatLength) + "\\n" + startItem + output + endItem + "\\n" + ("-" * repeatLength))
"""
)
);
}

@Test
void multipleAssignmentsAtOneLineSimple() {
rewriteRun(
groovy(
"""
def a = '1', b = '2'
"""
)
);
}

@Test
void multipleAssignmentsAtMultipleLineDynamicType() {
rewriteRun(
groovy(
"""
def a = '1' ,
b = '2'
"""
)
);
}

@Test
void multipleAssignmentsAtMultipleLineStaticType() {
rewriteRun(
groovy(
"""
String a = '1' ,
b = '2'
"""
)
);
}
}
Loading