Fix groovy multivariable declaration

[Laurens-W]

What's changed?

When parsing multivariable declarations now takes into account that no traditional identifier is available for any variable after the first

What's your motivation?

• Fix Groovy parser does not support multiple variable declarations #4748

Anything in particular you'd like reviewers to focus on?

Anyone you would like to review specifically?

@timtebeek @jevanlingen @sambsnyd @knutwannheden

Have you considered any alternatives or workarounds?

Any additional context

The Groovy AST returns multivariable declarations as separate DeclarationExpressions within a block. Therefore we cannot easily map them to J.VariableDeclarations

Checklist

• I've added unit tests to cover both positive and negative cases
• I've read and applied the recipe conventions and best practices
• I've used the IntelliJ IDEA auto-formatter on affected files

[timtebeek]

Saving others a click with the failures seen: https://ge.openrewrite.org/s/nr7naekjegaas/tests/overview?outcome=FAILED

wildcardWithUpperBound()
diamondOperator()
singleVariableDeclarationStaticallyTyped()
wildcardWithLowerBound()
genericMaps()

First one fails with

org.opentest4j.AssertionFailedError: [When parsing and printing the source code back to text without modifications, the printed source didn't match the original source code. This means there is a bug in the parser implementation itself. Please open an issue to report this, providing a sample of the code that generated this error for "file.groovy":	

diff --git a/file.groovy b/file.groovy	
index 3861186..ac7fbf9 100644	
--- a/file.groovy	
+++ b/file.groovy	
@@ -1 +1 @@ 	
-List<? extends String> l	
\ No newline at end of file	
+List<?< extends String> l	
\ No newline at end of file	

expected: "List<? extends String> l"	
 but was: "List<?< extends String> l"	

[Laurens-W]

I simplified a little too far, reverted that partially and now they work (locally)

[timtebeek]

One remaining failure it seems in org.openrewrite.groovy.tree.AssignmentTest/simpleWithFinal()

java.lang.AssertionError: Source file was parsed into an LST that contains non-whitespace characters in its whitespace. This is indicative of a bug in the parser. 	
final~~(non-whitespace)~~> def <~~x = "s"	

[timtebeek]

Perhaps good to call out here that we now model final def and def final as two unique concatenated Identifiers, which seems odd to me:

----G.CompilationUnit | "G.CompilationUnit(typesInUse=org.openrewrite.java.internal.TypesInUse@745c2004, padding=org.openrewrite.groovy.tree.G..."
    |---J.VariableDeclarations | "final def x = "x""
    |   |---J.Identifier | "final def"
    |   \-------J.VariableDeclarations.NamedVariable | "x = "x""
    |           |---J.Identifier | "x"
    |           \-------J.Literal | ""x""
    |---J.VariableDeclarations | "final y = "y""
    |   |---J.Identifier | "final"
    |   \-------J.VariableDeclarations.NamedVariable | "y = "y""
    |           |---J.Identifier | "y"
    |           \-------J.Literal | ""y""
    \---J.VariableDeclarations | "final z = "z""
        |---J.Identifier | "final"
        \-------J.VariableDeclarations.NamedVariable | "z = "z""
                |---J.Identifier | "z"
                \-------J.Literal | ""z""

Note that I don't expect this to occur often, if at all, and it still prints OK; but figured comment it here for a closer look by others reviewing as well.

[knutwannheden]

While this isn't perfect either, I think I would opt for mapping all of final, def, and var to modifiers (J.Modifier) and instead deprecating the RedundantDef marker, while still supporting it in the printer to remain backwards compatible.

[Laurens-W]

Implemented this. I'm not sure if we can actually mark the RedundantDef marker as deprecated though as that may also apply to declared methods, so I left that in.

As we discussed we may want to find a way to combine consecutive DeclarationExpressions into one J.VariableDeclarations. That could be a further improvement and I'm willing to work on that, but want to return focus to the parenthesis issue as well :)

[jevanlingen]

At least marking the RedundantDef marker as @deprecated should not be a problem. If you have the option to remove the maybeRedundantDef method as well, all the better. As long as you keep the RedundantDef marker in printer, you are still backwards compatible.

__
Also notice the RedundantDef marker has ben added lately (05/12/2024). So the sooner we no longer use it, the less serialised LSTs do have this marker.

[knutwannheden]

@sambsnyd Since you recently added that RedundantDef, do you also want to chime in here and tell us what your thoughts are about this?

[knutwannheden]

While this isn't perfect either, I think I would opt for mapping all of final, def, and var to modifiers (J.Modifier) and instead deprecating the RedundantDef marker, while still supporting it in the printer to remain backwards compatible.

[knutwannheden]

It is very unclear to me what these changes here have to do with the parsing issue. Is a comment required here?

[Laurens-W]

Due to the way DeclarationExpressions were parsed into J.VariableDeclarations we needed a specific check here. After my changes the parsing is more in-line with other types and so we no longer need that exceptional case here!

[knutwannheden]

I think we should be able to proceed with this. Is it difficult to solve this without the MultiVariable marker?

[Laurens-W]

I think we should be able to proceed with this. Is it difficult to solve this without the MultiVariable marker?

I haven't looked into it too much yet but I'd imagine somewhere after this block:

rewrite/rewrite-groovy/src/main/java/org/openrewrite/groovy/GroovyParserVisitor.java

Lines 203 to 214 in d449490

	for (ASTNode value : entry.getValue()) {
	if (value instanceof InnerClassNode) {
	// Inner classes will be visited as part of visiting their containing class
	continue;
	}
	JRightPadded<Statement> statement = convertTopLevelStatement(unit, value);
	if (statements.isEmpty() && pkg == null && statement.getElement() instanceof J.Import) {
	prefix = statement.getElement().getPrefix();
	statement = statement.withElement(statement.getElement().withPrefix(EMPTY));
	}
	statements.add(statement);
	}

we have the required context to sweep consecutive VariableDeclarations together