Skip to content

Commit

Permalink
Fix groovy multivariable declaration (#4757)
Browse files Browse the repository at this point in the history 
* Fix parsing of multiple variable assignments in one statement

* Minor improvement

* Add another testcase and make sure it works

* Simplify

* Clarify

* Revert formatting

* Apply suggestions from code review

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

* Fixed

* Fixed

* Move tests to AssignmentTest + add `final def` case

* Should have done it this way initially ;)

* Add testcase for regression

* Polish

* Extra testcase

* Add `noKeyword` test

* Return String instead of passing StringBuilder
Include subpart of remainder to give an indication where the issue occurred

* Further convert to using String

* Simplify

* Different approach using marker

* Remove unused code and FMT

* Remove specific exception cases for VariableDeclarations

* Change to startsWith with offset

---------

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: lingenj <[email protected]>
Co-authored-by: Tim te Beek <[email protected]>
  • Loading branch information
@Laurens-W @github-actions
@jevanlingen @timtebeek
4 people authored Dec 17, 2024
1 parent d449490 commit 5f2d11c
Show file tree
Hide file tree
Showing 5 changed files with 189 additions and 51 deletions.
16 changes: 2 additions & 14 deletions rewrite-gradle/src/main/java/org/openrewrite/gradle/AddDependencyVisitor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -290,22 +290,10 @@ public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, Execu
}
} else {
Space originalPrefix = addDependencyInvocation.getPrefix();
if (currentStatement instanceof J.VariableDeclarations) {
J.VariableDeclarations variableDeclarations = (J.VariableDeclarations) currentStatement;
if (variableDeclarations.getTypeExpression() != null) {
addDependencyInvocation = addDependencyInvocation.withPrefix(variableDeclarations.getTypeExpression().getPrefix());
}
} else {
addDependencyInvocation = addDependencyInvocation.withPrefix(currentStatement.getPrefix());
}
addDependencyInvocation = addDependencyInvocation.withPrefix(currentStatement.getPrefix());

if (addDependencyInvocation.getSimpleName().equals(beforeDependency.getSimpleName())) {
if (currentStatement instanceof J.VariableDeclarations) {
J.VariableDeclarations variableDeclarations = (J.VariableDeclarations) currentStatement;
if (variableDeclarations.getTypeExpression() != null && !variableDeclarations.getTypeExpression().getPrefix().equals(originalPrefix)) {
statements.set(i, variableDeclarations.withTypeExpression(variableDeclarations.getTypeExpression().withPrefix(originalPrefix)));
}
} else if (!currentStatement.getPrefix().equals(originalPrefix)) {
if (!currentStatement.getPrefix().equals(originalPrefix)) {
statements.set(i, currentStatement.withPrefix(originalPrefix));
}
}
Expand Down
101 changes: 67 additions & 34 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 @@ -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 @@ -113,16 +113,16 @@ private static boolean isOlderThanGroovy3() {
}

/**
* Groovy methods can be declared with "def" AND a return type
* In these cases the "def" is semantically meaningless but needs to be preserved for source code accuracy
* If there is both a def and a return type, this method returns a RedundantDef object and advances the cursor
* position past the "def" keyword, leaving the return type to be parsed as normal.
* In any other situation an empty Optional is returned and the cursor is not advanced.
* Groovy methods can be declared with "def" AND a return type
* In these cases the "def" is semantically meaningless but needs to be preserved for source code accuracy
* If there is both a def and a return type, this method returns a RedundantDef object and advances the cursor
* position past the "def" keyword, leaving the return type to be parsed as normal.
* In any other situation an empty Optional is returned and the cursor is not advanced.
*/
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 @@ -182,8 +182,8 @@ public G.CompilationUnit visit(SourceUnit unit, ModuleNode ast) throws GroovyPar
for (ClassNode aClass : ast.getClasses()) {
if (aClass.getSuperClass() == null ||
!("groovy.lang.Script".equals(aClass.getSuperClass().getName()) ||
"RewriteGradleProject".equals(aClass.getSuperClass().getName()) ||
"RewriteSettings".equals(aClass.getSuperClass().getName()))) {
"RewriteGradleProject".equals(aClass.getSuperClass().getName()) ||
"RewriteSettings".equals(aClass.getSuperClass().getName()))) {
sortedByPosition.computeIfAbsent(pos(aClass), i -> new ArrayList<>()).add(aClass);
}
}
Expand Down Expand Up @@ -822,8 +822,8 @@ public void visitArgumentlistExpression(ArgumentListExpression expression) {
// When named parameters are in use they may appear before, after, or intermixed with any positional arguments
if (unparsedArgs.size() > 1 && unparsedArgs.get(0) instanceof MapExpression &&
(unparsedArgs.get(0).getLastLineNumber() > unparsedArgs.get(1).getLastLineNumber() ||
(unparsedArgs.get(0).getLastLineNumber() == unparsedArgs.get(1).getLastLineNumber() &&
unparsedArgs.get(0).getLastColumnNumber() > unparsedArgs.get(1).getLastColumnNumber()))) {
(unparsedArgs.get(0).getLastLineNumber() == unparsedArgs.get(1).getLastLineNumber() &&
unparsedArgs.get(0).getLastColumnNumber() > unparsedArgs.get(1).getLastColumnNumber()))) {

// Figure out the source-code ordering of the expressions
MapExpression namedArgExpressions = (MapExpression) unparsedArgs.get(0);
Expand Down Expand Up @@ -1099,7 +1099,7 @@ public void visitBlockStatement(BlockStatement block) {
J expr = visit(statement);
if (i == blockStatements.size() - 1 && (expr instanceof Expression)) {
if (parent instanceof ClosureExpression || (parent instanceof MethodNode &&
!JavaType.Primitive.Void.equals(typeMapping.type(((MethodNode) parent).getReturnType())))) {
!JavaType.Primitive.Void.equals(typeMapping.type(((MethodNode) parent).getReturnType())))) {
expr = new J.Return(randomId(), expr.getPrefix(), Markers.EMPTY,
expr.withPrefix(EMPTY));
expr = expr.withMarkers(expr.getMarkers().add(new ImplicitReturn(randomId())));
Expand Down Expand Up @@ -1438,7 +1438,10 @@ public void visitNotExpression(NotExpression expression) {

@Override
public void visitDeclarationExpression(DeclarationExpression expression) {
Space prefix = whitespace();
Optional<RedundantDef> redundantDef = maybeRedundantDef(expression.getVariableExpression().getType(), expression.getVariableExpression().getName());
Optional<MultiVariable> multiVariable = maybeMultiVariable();
List<J.Modifier> modifiers = getModifiers();
TypeTree typeExpr = visitVariableExpressionType(expression.getVariableExpression());

J.VariableDeclarations.NamedVariable namedVariable;
Expand Down Expand Up @@ -1466,19 +1469,60 @@ public void visitDeclarationExpression(DeclarationExpression expression) {

J.VariableDeclarations variableDeclarations = new J.VariableDeclarations(
randomId(),
EMPTY,
redundantDef.map(Markers.EMPTY::add).orElse(Markers.EMPTY),
emptyList(),
prefix,
Markers.EMPTY,
emptyList(),
modifiers,
typeExpr,
null,
emptyList(),
singletonList(JRightPadded.build(namedVariable))
);
if (redundantDef.isPresent()) {
variableDeclarations = variableDeclarations.withMarkers(variableDeclarations.getMarkers().add(redundantDef.get()));
}
if (multiVariable.isPresent()) {
variableDeclarations = variableDeclarations.withMarkers(variableDeclarations.getMarkers().add(multiVariable.get()));
}

queue.add(variableDeclarations);
}

private Optional<MultiVariable> maybeMultiVariable() {
int saveCursor = cursor;
Space commaPrefix = whitespace();
if (source.startsWith(",", cursor)) {
skip(",");
return Optional.of(new MultiVariable(randomId(), commaPrefix));
}
cursor = saveCursor;
return Optional.empty();
}

private List<J.Modifier> getModifiers() {
List<J.Modifier> modifiers = new ArrayList<>();
int saveCursor = cursor;
Space prefix = whitespace();
while (source.startsWith("def", cursor) || source.startsWith("var", cursor) || source.startsWith("final", cursor)) {
if (source.startsWith("var", cursor)) {
modifiers.add(new J.Modifier(randomId(), prefix, Markers.EMPTY, "var", J.Modifier.Type.LanguageExtension, emptyList()));
cursor += 3;
} else if (source.startsWith("def", cursor)) {
modifiers.add(new J.Modifier(randomId(), prefix, Markers.EMPTY, "def", J.Modifier.Type.LanguageExtension, emptyList()));
cursor += 3;
} else if (source.startsWith("final", cursor)) {
modifiers.add(new J.Modifier(randomId(), prefix, Markers.EMPTY, "final", J.Modifier.Type.LanguageExtension, emptyList()));
cursor += 5;
} else {
break;
}
saveCursor = cursor;
prefix = whitespace();
}
cursor = saveCursor;
return modifiers;
}

@Override
public void visitEmptyExpression(EmptyExpression expression) {
queue.add(new J.Empty(randomId(), EMPTY, Markers.EMPTY));
Expand Down Expand Up @@ -1766,7 +1810,7 @@ public void visitMethodCallExpression(MethodCallExpression call) {
MethodNode methodNode = (MethodNode) call.getNodeMetaData().get(StaticTypesMarker.DIRECT_METHOD_CALL_TARGET);
JavaType.Method methodType = null;
if (methodNode == null && call.getObjectExpression() instanceof VariableExpression &&
((VariableExpression) call.getObjectExpression()).getAccessedVariable() != null) {
((VariableExpression) call.getObjectExpression()).getAccessedVariable() != null) {
// Groovy doesn't know what kind of object this method is being invoked on
// But if this invocation is inside a Closure we may have already enriched its parameters with types from the static type checker
// Use any such type information to attempt to find a matching method
Expand Down Expand Up @@ -2097,29 +2141,18 @@ 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();
String typeName = "";

if (expression.isDynamicTyped()) {
Space prefix = whitespace();
String keyword;
if (source.substring(cursor).startsWith("final")) {
keyword = "final";
} else {
keyword = source.substring(cursor, cursor + 3);
}
cursor += keyword.length();
return new J.Identifier(randomId(),
prefix,
Markers.EMPTY,
emptyList(),
keyword,
type, null);
if (!expression.isDynamicTyped() && source.startsWith(expression.getOriginType().getUnresolvedName(), cursor)) {
typeName = expression.getOriginType().getUnresolvedName();
cursor += typeName.length();
}
Space prefix = sourceBefore(expression.getOriginType().getUnresolvedName());
J.Identifier ident = new J.Identifier(randomId(),
EMPTY,
Markers.EMPTY,
emptyList(),
expression.getOriginType().getUnresolvedName(),
typeName,
type, null);
if (expression.getOriginType().getGenericsTypes() != null) {
return new J.ParameterizedType(randomId(), prefix, Markers.EMPTY, ident, visitTypeParameterizations(
Expand Down Expand Up @@ -2671,7 +2704,7 @@ private String name() {
Can contain a J.FieldAccess, as in a variable declaration with fully qualified type parameterization:
List<java.lang.String>
*/
private JContainer<Expression> visitTypeParameterizations(GenericsType @Nullable[] genericsTypes) {
private JContainer<Expression> visitTypeParameterizations(GenericsType @Nullable [] genericsTypes) {
Space prefix = sourceBefore("<");
List<JRightPadded<Expression>> parameters;

Expand Down
10 changes: 7 additions & 3 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 @@ -265,13 +265,17 @@ public J visitVariableDeclarations(J.VariableDeclarations multiVariable, PrintOu
beforeSyntax(multiVariable, Space.Location.VARIABLE_DECLARATIONS_PREFIX, p);
visitSpace(Space.EMPTY, Space.Location.ANNOTATIONS, p);
visit(multiVariable.getLeadingAnnotations(), p);
for (J.Modifier m : multiVariable.getModifiers()) {
visitModifier(m, p);
}
multiVariable.getMarkers().findFirst(RedundantDef.class).ifPresent(def -> {
visitSpace(def.getPrefix(), Space.Location.LANGUAGE_EXTENSION, p);
p.append("def");
});
for (J.Modifier m : multiVariable.getModifiers()) {
visitModifier(m, p);
}
multiVariable.getMarkers().findFirst(MultiVariable.class).ifPresent(multiVar -> {
visitSpace(multiVar.getPrefix(), Space.Location.NAMED_VARIABLE_SUFFIX, p);
p.append(",");
});
visit(multiVariable.getTypeExpression(), p);
// For backwards compatibility.
for (JLeftPadded<Space> dim : multiVariable.getDimensionsBeforeName()) {
Expand Down
30 changes: 30 additions & 0 deletions rewrite-groovy/src/main/java/org/openrewrite/groovy/marker/MultiVariable.java
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
/*
* Copyright 2024 the original author or authors.
* <p>
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
* <p>
* https://www.apache.org/licenses/LICENSE-2.0
* <p>
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package org.openrewrite.groovy.marker;

import lombok.Value;
import lombok.With;
import org.openrewrite.java.tree.Space;
import org.openrewrite.marker.Marker;

import java.util.UUID;

@Value
@With
public class MultiVariable implements Marker {
UUID id;
Space prefix;
}
83 changes: 83 additions & 0 deletions rewrite-groovy/src/test/java/org/openrewrite/groovy/tree/AssignmentTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,41 @@
@SuppressWarnings({"GroovyUnusedAssignment", "GrUnnecessarySemicolon"})
class AssignmentTest implements RewriteTest {

@Test
void noKeyword() {
rewriteRun(
groovy(
"""
x = "s"
"""
)
);
}

@Test
void simple() {
rewriteRun(
groovy(
"""
def x = "s"
"""
)
);
}

@Test
void simpleWithFinal() {
rewriteRun(
groovy(
"""
final def x = "x"
def final y = "y"
final z = "z"
"""
)
);
}

@Test
void concat() {
rewriteRun(
Expand Down Expand Up @@ -91,4 +126,52 @@ void baseNConversions() {
)
);
}

@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'
"""
)
);
}
}

0 comments on commit 5f2d11c

Please sign in to comment.