Rewrite `Joiner.on(", ").join("a", "b")` to `String.join(", ", "a", "b")` #389

tobli · 2024-01-10T22:50:42Z

What's changed?

Adds a recipe to rewrite calls to Guava Joiner#join with String.join in compatible cases. A call can be re-written if the delimiter is a String, and the arguments are an array, a list or an iterable of items that can be cast to a CharSequence.

What's your motivation?

Addresses #318

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

How to correctly replace the method invocation.

Have you considered any alternatives or workarounds?

I considered Refaster templates, but didn't see a way to narrow down to only the supported call patterns.

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

…b")` Adds a recipe to rewrite calls to Guava Joiner#join with String.join in compatible cases. A call can be re-written if the delimiter is a String, and the arguments are an array, a list or an iterable of items that can be cast to a CharSequence. Addresses openrewrite#318

tobli · 2024-01-10T22:51:10Z

NOTE: test cases still fail, posting here to get feedback

Stephan202 · 2024-01-11T06:50:25Z

While rewrite-templating doesn't support this particular variant yet, a recipe such as this one could likely also be defined using Refaster.

timtebeek · 2024-01-11T08:29:45Z

Sharing the test failures here to save others a click, before I have time to dive in

PreferJavaStringJoinTest > joinStrings() FAILED
    org.opentest4j.AssertionFailedError: [Unexpected result in "Test.java":
    diff --git a/Test.java b/Test.java
    index b57ba61..2f7b9b4 100644
    --- a/Test.java
    +++ b/Test.java
    @@ -1,3 +1,5 @@ 
    +import com.google.common.base.Joiner;
    +
     class Test {
    -    String s = String.join(", ", "a", "b");
    +    String s = Joiner.on(", ").join(", ", "a", "b");
     }
    \ No newline at end of file
    ] 
    expected: 
      "class Test {
          String s = String.join(", ", "a", "b");
      }"
     but was: 
      "import com.google.common.base.Joiner;
  
      class Test {
          String s = Joiner.on(", ").join(", ", "a", "b");
      }"
        at [email protected]/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
        at [email protected]/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:77)
        at [email protected]/jdk.internal.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
        at [email protected]/java.lang.reflect.Constructor.newInstanceWithCaller(Constructor.java:499)
        at app//org.openrewrite.test.RewriteTest.assertContentEquals(RewriteTest.java:591)
        at app//org.openrewrite.test.RewriteTest.rewriteRun(RewriteTest.java:488)
        at app//org.openrewrite.test.RewriteTest.rewriteRun(RewriteTest.java:132)
        at app//org.openrewrite.test.RewriteTest.rewriteRun(RewriteTest.java:127)
        at app//org.openrewrite.java.migrate.guava.PreferJavaStringJoinTest.joinStrings(PreferJavaStringJoinTest.java:36)

PreferJavaStringJoinTest > joinMixedCharSequences() FAILED
    org.opentest4j.AssertionFailedError: [Unexpected result in "Test.java":
    diff --git a/Test.java b/Test.java
    index 31ecb7c..9a692ac 100644
    --- a/Test.java
    +++ b/Test.java
    @@ -1,3 +1,5 @@ 
    +import com.google.common.base.Joiner;
    +
     class Test {
    -     String s = String.join(", ", "a", new StringBuilder("b"));
    - }
    \ No newline at end of file
    +    String s = Joiner.on(", ").join(", ", "a", new StringBuilder("b"));
    +}
    \ No newline at end of file
    ] 
    expected: 
      "class Test {
           String s = String.join(", ", "a", new StringBuilder("b"));
       }"
     but was: 
      "import com.google.common.base.Joiner;
  
      class Test {
          String s = Joiner.on(", ").join(", ", "a", new StringBuilder("b"));
      }"
        at [email protected]/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
        at [email protected]/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:77)
        at [email protected]/jdk.internal.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
        at [email protected]/java.lang.reflect.Constructor.newInstanceWithCaller(Constructor.java:499)
        at app//org.openrewrite.test.RewriteTest.assertContentEquals(RewriteTest.java:591)
        at app//org.openrewrite.test.RewriteTest.rewriteRun(RewriteTest.java:488)
        at app//org.openrewrite.test.RewriteTest.rewriteRun(RewriteTest.java:132)
        at app//org.openrewrite.test.RewriteTest.rewriteRun(RewriteTest.java:127)
        at app//org.openrewrite.java.migrate.guava.PreferJavaStringJoinTest.joinMixedCharSequences(PreferJavaStringJoinTest.java:102)

PreferJavaStringJoinTest > joinIterables() FAILED
    org.opentest4j.AssertionFailedError: [Unexpected result in "Test.java":
    diff --git a/Test.java b/Test.java
    index cba7193..923be78 100644
    --- a/Test.java
    +++ b/Test.java
    @@ -1,5 +1,6 @@ 
    +import com.google.common.base.Joiner;
     import java.util.Set;
 
     class Test {
    -    String s = String.join(", ", Set.of("a"));
    +    String s = Joiner.on(", ").join(", ", Set.of("a"));
     }
    \ No newline at end of file
    ] 
    expected: 
      "import java.util.Set;
  
      class Test {
          String s = String.join(", ", Set.of("a"));
      }"
     but was: 
      "import com.google.common.base.Joiner;
      import java.util.Set;
  
      class Test {
          String s = Joiner.on(", ").join(", ", Set.of("a"));
      }"
        at [email protected]/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
        at [email protected]/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:77)
        at [email protected]/jdk.internal.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
        at [email protected]/java.lang.reflect.Constructor.newInstanceWithCaller(Constructor.java:499)
        at app//org.openrewrite.test.RewriteTest.assertContentEquals(RewriteTest.java:591)
        at app//org.openrewrite.test.RewriteTest.rewriteRun(RewriteTest.java:488)
        at app//org.openrewrite.test.RewriteTest.rewriteRun(RewriteTest.java:132)
        at app//org.openrewrite.test.RewriteTest.rewriteRun(RewriteTest.java:127)
        at app//org.openrewrite.java.migrate.guava.PreferJavaStringJoinTest.joinIterables(PreferJavaStringJoinTest.java:78)

PreferJavaStringJoinTest > joinEmptyIterables() FAILED
    java.lang.AssertionError: Recipe was expected to make a change but made no changes.
        at org.openrewrite.test.LargeSourceSetCheckingExpectedCycles.afterCycle(LargeSourceSetCheckingExpectedCycles.java:80)
        at org.openrewrite.RecipeScheduler.runRecipeCycles(RecipeScheduler.java:97)
        at org.openrewrite.RecipeScheduler.scheduleRun(RecipeScheduler.java:41)
        at org.openrewrite.Recipe.run(Recipe.java:344)
        at org.openrewrite.test.RewriteTest.rewriteRun(RewriteTest.java:363)
        at org.openrewrite.test.RewriteTest.rewriteRun(RewriteTest.java:132)
        at org.openrewrite.test.RewriteTest.rewriteRun(RewriteTest.java:127)
        at org.openrewrite.java.migrate.guava.PreferJavaStringJoinTest.joinEmptyIterables(PreferJavaStringJoinTest.java:123)

PreferJavaStringJoinTest > joinMethodOnSeparateLine() FAILED
    org.opentest4j.AssertionFailedError: [Unexpected result in "Test.java":
    diff --git a/Test.java b/Test.java
    index b57ba61..ad0cecb 100644
    --- a/Test.java
    +++ b/Test.java
    @@ -1,3 +1,6 @@ 
    +import com.google.common.base.Joiner;
    +
     class Test {
    -    String s = String.join(", ", "a", "b");
    +    String s = Joiner.on(", ")
    +                     .join(", ", "a", "b");
     }
    \ No newline at end of file
    ] 
    expected: 
      "class Test {
          String s = String.join(", ", "a", "b");
      }"
     but was: 
      "import com.google.common.base.Joiner;
  
      class Test {
          String s = Joiner.on(", ")
                           .join(", ", "a", "b");
      }"
        at [email protected]/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
        at [email protected]/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:77)
        at [email protected]/jdk.internal.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
        at [email protected]/java.lang.reflect.Constructor.newInstanceWithCaller(Constructor.java:499)
        at app//org.openrewrite.test.RewriteTest.assertContentEquals(RewriteTest.java:591)
        at app//org.openrewrite.test.RewriteTest.rewriteRun(RewriteTest.java:488)
        at app//org.openrewrite.test.RewriteTest.rewriteRun(RewriteTest.java:132)
        at app//org.openrewrite.test.RewriteTest.rewriteRun(RewriteTest.java:127)
        at app//org.openrewrite.java.migrate.guava.PreferJavaStringJoinTest.joinMethodOnSeparateLine(PreferJavaStringJoinTest.java:147)

PreferJavaStringJoinTest > joinStringArray() FAILED
    org.opentest4j.AssertionFailedError: [Unexpected result in "Test.java":
    diff --git a/Test.java b/Test.java
    index 0517daf..2c5f880 100644
    --- a/Test.java
    +++ b/Test.java
    @@ -1,3 +1,5 @@ 
    +import com.google.common.base.Joiner;
    +
     class Test {
    -    String s = String.join(", ", new String[] {"a"});
    +    String s = Joiner.on(", ").join(", ", new String[] {"a"});
     }
    \ No newline at end of file
    ] 
    expected: 
      "class Test {
          String s = String.join(", ", new String[] {"a"});
      }"
     but was: 
      "import com.google.common.base.Joiner;
  
      class Test {
          String s = Joiner.on(", ").join(", ", new String[] {"a"});
      }"
        at [email protected]/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
        at [email protected]/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:77)
        at [email protected]/jdk.internal.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
        at [email protected]/java.lang.reflect.Constructor.newInstanceWithCaller(Constructor.java:499)
        at app//org.openrewrite.test.RewriteTest.assertContentEquals(RewriteTest.java:591)
        at app//org.openrewrite.test.RewriteTest.rewriteRun(RewriteTest.java:488)
        at app//org.openrewrite.test.RewriteTest.rewriteRun(RewriteTest.java:132)
        at app//org.openrewrite.test.RewriteTest.rewriteRun(RewriteTest.java:127)
        at app//org.openrewrite.java.migrate.guava.PreferJavaStringJoinTest.joinStringArray(PreferJavaStringJoinTest.java:57)

knutwannheden · 2024-01-11T09:02:36Z

Yes, unfortunately we haven't added support for Refaster's @Repeated annotation yet.

knutwannheden · 2024-01-11T09:34:21Z

@tobli I added a commit to use JavaTemplate instead. There is still one test failure, where I suspect that our type attribution is not quite right yet for J.NewClass. I will investigate this.

knutwannheden · 2024-01-11T16:29:30Z

This change should close the type attribution gap for constructor calls: openrewrite/rewrite#3902

tobli · 2024-01-11T18:43:19Z

Thanks for helping out, I'm learning a lot =)

knutwannheden · 2024-01-11T18:50:32Z

I just merged openrewrite/rewrite#3902, so when the build is done, I think the tests in here might also pass.

As you probably noticed, I changed the code to use JavaTemplate. That is also what the Refaster support uses under the hood and is typically the easiest way to generate code. Especially, since it takes care of getting all the type attribution correct, which can otherwise be very tricky.

tobli · 2024-01-11T19:10:20Z

I just merged openrewrite/rewrite#3902, so when the build is done, I think the tests in here might also pass.

As you probably noticed, I changed the code to use JavaTemplate. That is also what the Refaster support uses under the hood and is typically the easiest way to generate code. Especially, since it takes care of getting all the type attribution correct, which can otherwise be very tricky.

thanks, I'll study it in more detail. I'll have to dig into the refaster/rewrite code a bit deeper, this is an area where more docs might help. I did see generics in refaster templates weren't supported, would I use something like a @Matches annotation if I'd like to add constraints (e.g. accepting only some iterators) to arguments?

tobli · 2024-01-11T19:13:01Z

If I understand the comment in RepeatableArgumentMatcher correctly, it allows for matching also method calls the return strings as arguments. That's a very neat feature! Would be nice to package that up into some utility that could be used in imperative recipes as well. Or maybe it already exists and I haven't found it yet =)

knutwannheden · 2024-01-11T19:17:41Z

thanks, I'll study it in more detail. I'll have to dig into the refaster/rewrite code a bit deeper, this is an area where more docs might help. I did see generics in refaster templates weren't supported, would I use something like a @Matches annotation if I'd like to add constraints (e.g. accepting only some iterators) to arguments?

I think if your code uses generics you are at the moment out of luck with our Refaster support. Although that is something we would like to address soon.

Refaster can be very convenient for simple things. But as soon as you need to add some variation, this declarative approach starts to fall down. But note that JavaTemplates can also be constructed from lambdas and can both be used to match code and replace code.

Here is an example where a JavaTemplate constructed from a lambda is used to modify code: https://github.com/openrewrite/rewrite-micrometer/blob/7807cea5e939f7f167a6f02e1c4641b24f4d358c/src/main/java/org/openrewrite/micrometer/misk/MigrateEmptyLabelMiskCounter.java#L73-L79

And here is a visitor which uses this to match some input code: https://github.com/openrewrite/rewrite-micrometer/blob/83c4ce67c5c9898dd4ea41018d8a55f307d127e4/src/main/java/org/openrewrite/micrometer/misk/NoExplicitEmptyLabelList.java#L49-L68

knutwannheden · 2024-01-11T19:40:31Z

If I understand the comment in RepeatableArgumentMatcher correctly, it allows for matching also method calls the return strings as arguments. That's a very neat feature! Would be nice to package that up into some utility that could be used in imperative recipes as well. Or maybe it already exists and I haven't found it yet =)

Just like #{any(String)} can be used in JavaTemplate code to match any expression of a given type (String in the example), a parameter to a Refaster template also matches any expression of the given type.

The idea of RepeatableArgumentMatcher is to restrict this to only match expressions which can be regarded as not having any side effects. Instead of doing a full data flow analysis, this matcher restricts the matches to literals, variables, and a few things more (including "getters"). This may be necessary when the Refaster after template includes that parameter multiple times in its body, which would likely be problematic when it has side effects.

knutwannheden · 2024-01-11T19:42:51Z

@tobli Thanks a lot for the contribution!

timtebeek · 2024-01-11T19:51:29Z

Yes thanks! Once deployed on https://app.moderne.io we might want to try it against a couple project there, and if it works well include it in https://github.com/openrewrite/rewrite-migrate-java/blob/main/src/main/resources/META-INF/rewrite/no-guava.yml

knutwannheden · 2024-01-11T20:06:27Z

It is doing something: https://app.moderne.io/results/o2zCnjMuC

timtebeek · 2024-01-11T20:36:44Z

Two results look ok at first glance

knutwannheden · 2024-01-11T20:59:46Z

I added it to no-guava.yml.

tobli marked this pull request as draft January 10, 2024 22:51

timtebeek self-requested a review January 11, 2024 08:37

Use JavaTemplate instead

25e9c2d

Merge branch 'main' into prefer-java-string-join-over-guava

e3745cf

timtebeek marked this pull request as ready for review January 11, 2024 19:03

knutwannheden merged commit a198cd6 into openrewrite:main Jan 11, 2024
1 check passed

timtebeek mentioned this pull request Jan 11, 2024

Guava Joiner.on(delimiter).join(list) to String.join(delimiter, list) #318

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite `Joiner.on(", ").join("a", "b")` to `String.join(", ", "a", "b")` #389

Rewrite `Joiner.on(", ").join("a", "b")` to `String.join(", ", "a", "b")` #389

tobli commented Jan 10, 2024

tobli commented Jan 10, 2024

Stephan202 commented Jan 11, 2024

timtebeek commented Jan 11, 2024

knutwannheden commented Jan 11, 2024

knutwannheden commented Jan 11, 2024

knutwannheden commented Jan 11, 2024

tobli commented Jan 11, 2024

knutwannheden commented Jan 11, 2024

tobli commented Jan 11, 2024

tobli commented Jan 11, 2024

knutwannheden commented Jan 11, 2024

knutwannheden commented Jan 11, 2024 •

edited

Loading

knutwannheden commented Jan 11, 2024

timtebeek commented Jan 11, 2024

knutwannheden commented Jan 11, 2024

timtebeek commented Jan 11, 2024

knutwannheden commented Jan 11, 2024

Rewrite Joiner.on(", ").join("a", "b") to String.join(", ", "a", "b") #389

Rewrite Joiner.on(", ").join("a", "b") to String.join(", ", "a", "b") #389

Conversation

tobli commented Jan 10, 2024

What's changed?

What's your motivation?

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

Have you considered any alternatives or workarounds?

Checklist

tobli commented Jan 10, 2024

Stephan202 commented Jan 11, 2024

timtebeek commented Jan 11, 2024

knutwannheden commented Jan 11, 2024

knutwannheden commented Jan 11, 2024

knutwannheden commented Jan 11, 2024

tobli commented Jan 11, 2024

knutwannheden commented Jan 11, 2024

tobli commented Jan 11, 2024

tobli commented Jan 11, 2024

knutwannheden commented Jan 11, 2024

knutwannheden commented Jan 11, 2024 • edited Loading

knutwannheden commented Jan 11, 2024

timtebeek commented Jan 11, 2024

knutwannheden commented Jan 11, 2024

timtebeek commented Jan 11, 2024

knutwannheden commented Jan 11, 2024

Rewrite `Joiner.on(", ").join("a", "b")` to `String.join(", ", "a", "b")` #389

Rewrite `Joiner.on(", ").join("a", "b")` to `String.join(", ", "a", "b")` #389

knutwannheden commented Jan 11, 2024 •

edited

Loading