Groovy support for multiple assignment/tuple unpacking

[sudouser777]

1. Updated the groovy parser to support tuple unpacking.
2. closes #5283 .

[sudouser777]

I have tried to hanlde the spaces situation, but it's tricky spaces are not being handled properly

    @Test
    void destructuringWithSpaces() {
        rewriteRun(
          groovy(
            """
              def ( key, value ) = "a1:b2".split(":")
              println(key)
              println(value)
              """
          )
        );
    }

Also static type also not working

  @Test
  void destructuringWithType() {
      rewriteRun(
        groovy(
          """
            def (String key, String value) = "a1:b2".split(":")
            println(key)
            println(value)
            """
        )
      );
  }

@greg-at-moderne I'm not sure if my approach is correct—open to any suggestions.

[timtebeek]

Thanks a lot for the continued improvements to the Groovy parser @sudouser777 ! Great to see steady progress being made.

Any reason you'd held off on a merge here @greg-at-moderne ?

[greg-at-moderne]

Any reason you'd held off on a merge here @greg-at-moderne ?

No. I am good. I think this is good to be merged now. Your comment on the PR is the only outstanding (minor) thing in my perspective.

[jevanlingen]

Merging it seems good to me, but I would create a new "Groovy parser does not support multiple assignment/tuple unpacking with whitespace" issue though.

[sudouser777]

Ok Shannon makes a valid point here, in that we're better off using a new specific structural type here to match the others

• Groovy support for multiple assignment/tuple unpacking #5293 (comment)

No need to pull that up into a generalized version just yet, but we should revise the approach taken here to remove the markers and introduce a new element.

Would fully understand if at this point you have other priorities @sudouser777 ; just let us know if you'd prefer someone takes over.

Sounds good. I’m happy to take a stab at the changes, though I’m not too familiar with the structural type part. If the changes are a bit involved, totally fine if someone else wants to take over—just let me know!

[jevanlingen]

If you want to give it a try @sudouser777, that would be awesome! Don't worry, if after some time you decide you don't want to work on it anymore (because it's either too hard,, will take you too much time or whatever reason) that's totally fine. Just put a comment out that you would like someone else to take over.

The structural type part sounds a little more advanced, but it is "just" another LST element. I think reading the Kotlin counterpart will be a great starter: https://github.com/openrewrite/rewrite/blob/1112346c651847e633e773c8d51769db52c2de9a/rewrite-kotlin/src/main/java/org/openrewrite/kotlin/internal/KotlinTreeParserVisitor.java#L2343-L2452

In short, Kotlin has the KtDestructuringDeclaration AST object, where the Groovy parser uses a TupleExpression object.

[sudouser777]

Thanks! @jevanlingen . I’ll check out the Kotlin example and let you know if I can take on the changes. If it’s urgent, I’m totally fine with someone else taking over.

[shanman190]

Looks good to me other than the one missed method rename. Thanks @sudouser777 !

[timtebeek]

Copying the failure we're seeing here for easier reference:

DestructuringTest > destructuringWithType() FAILED
    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. 
    def (~~(non-whitespace)~~>String <~~key,~~(non-whitespace)~~> String <~~value) = "a1:b2".split(":")
    println("${key} ${value}")
        at org.openrewrite.test.RewriteTest.rewriteRun(RewriteTest.java:324)
        at org.openrewrite.test.RewriteTest.rewriteRun(RewriteTest.java:130)
        at org.openrewrite.test.RewriteTest.rewriteRun(RewriteTest.java:125)
        at org.openrewrite.groovy.tree.DestructuringTest.destructuringWithType(DestructuringTest.java:67)

[timtebeek]

Asking @jkschneider for a final review, as he suggested to introduce the G.DestructuringDeclaration element, and we want to ensure the design aligns with what he had intended, especially as LST changes are hard to change later on due to recipes having been writting using the elements and visitor, and LSTs potentially serialized.

[jevanlingen]

DestructuringTest > destructuringWithType() FAILED
    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. 
    def (~~(non-whitespace)~~>String <~~key,~~(non-whitespace)~~> String <~~value) = "a1:b2".split(":")
    println("${key} ${value}")
        at org.openrewrite.test.RewriteTest.rewriteRun(RewriteTest.java:324)
        at org.openrewrite.test.RewriteTest.rewriteRun(RewriteTest.java:130)
        at org.openrewrite.test.RewriteTest.rewriteRun(RewriteTest.java:125)
        at org.openrewrite.groovy.tree.DestructuringTest.destructuringWithType(DestructuringTest.java:67

This happens because of my change:

public J visitDestructuringDeclaration(G.DestructuringDeclaration destructuringDeclaration, P p) {
  // ..
  d = d.getPadding().withVariables(visitContainer(d.getPadding().getVariables(), GContainer.Location.DESTRUCT_ASSIGNMENTS, p));
  return d;
}

Will call the subparts under the hood. Now when the

public J visitVariable(J.VariableDeclarations.NamedVariable variable, P p) {
  J.VariableDeclarations.NamedVariable v = variable;
  // visit members + build `v`
  return v;
}

is visited, the prefix is filled with the type (thus "~~(non-whitespace)~~>String <~~key" happens). Will spend more time tomorrow to look more into this.

[sudouser777]

I tried to debug the issue. Found something. Somehow type is coming in the prefix @timtebeek @jevanlingen This is what causing above failure. I'm not sure on how to fix it though. I tried few things but they didn't work.

[jevanlingen]

@timtebeek & @sudouser777 Types within the destructuring were not handled, which caused them to be treated as part of the prefix. This only became apparent once we started implementing visitDestructuringDeclaration, since that’s when we traverse the tree again and the check "Source file was parsed into an LST that contains non-whitespace characters in its whitespace" kicks in.

I resolved it by introducing an additional type marker. However, it makes me wonder whether we should introduce a G.VariableDeclarations.NamedVariable, which would explicitly allow attaching the type directly.

[sudouser777]

I resolved it by introducing an additional type marker. However, it makes me wonder whether we should introduce a G.VariableDeclarations.NamedVariable, which would explicitly allow attaching the type directly.

We can also change variables to use JContainer<VariableDeclarations> variables instead of JContainer<VariableDeclarations.NamedVariable> variables But it may complicate space handling.

[jevanlingen]

Ah that's smart, didn't think about that! We modeled it after Kotlin deconstruction model, which does not have type declaration within the destruction¹, thus the model was not really tailored for having types within the destruction parts. I have to do work on other stuff now, but if you would have the time to look a bit into that JContainer<VariableDeclarations> variables direction instead of the custom marker, that would be awesome!

¹ Or at least, not right now. See also Kotlin 2.4 Introduces Name-based Destructuring: Stop Guessing Which Variable Goes Where.

[sudouser777]

@jevanlingen I made the changes to use VariableDeclarations please check and let me know if you see any issues with the new changes.

[jevanlingen]

Very nice @sudouser777, much better than my marker solution! I did make some small final changes, I think it's good now.

[greg-at-moderne]

To be honest I stopped following this PR and now it's hard to catch up with so many comments and history.

@timtebeek, @jevanlingen, @sudouser777, are there any reasons NOT to merge it as is?

[sudouser777]

To be honest I stopped following this PR and now it's hard to catch up with so many comments and history.

@timtebeek, @jevanlingen, @sudouser777, are there any reasons NOT to merge it as is?

@greg-at-moderne I’ve completed all the requested changes — from my side, we’re good to merge. I thought you all might still be waiting on something.

[sudouser777]

hi @sudouser777 ; thanks a lot so far! The reason it's been taking longer is that we're internally looking at the best way to map this across languages; Your work has helped kick off those discussions, so there's value even if we for now plan a slightly different direction. We won't ask you to change things yet again, but we're eyeing the use of https://github.com/openrewrite/rewrite/blob/f5f5a9ac880fcac473535326ef8b4366462566e3/rewrite-java/src/main/java/org/openrewrite/java/tree/VariableDeclarator.java#L23-L31

That would similarly allow for multiple assignments, without the need for the new DestructuringDeclaration introduced here. This has been part of ongoing discovery, so don't feel like you have to keep up or make these changes. Just sharing our thoughts here such that you're looped in. We can take over your PR if that's ok with you, or you're welcome to stay involved; whichever you prefer.

Hi @timtebeek, apologies for the very delayed reply — I got caught up with some personal things and wasn’t able to get back to this sooner. Thanks a lot for the update and for keeping me in the loop. I’m totally fine with you taking over the PR. If you need anything from my side, feel free to reach out!

[timtebeek]

No worries; I hope all is well there. We'll try to work this in, unless you beat us to it. :)