StringFormatted should not wrap first argument by default in Java 17 …

…upgrade (#618) Fixes #616
openrewrite · Dec 4, 2024 · bd2cc9c · bd2cc9c
1 parent 05b8264
commit bd2cc9c
Show file tree

Hide file tree

Showing 3 changed files with 92 additions and 55 deletions.
diff --git a/src/main/java/org/openrewrite/java/migrate/lang/StringFormatted.java b/src/main/java/org/openrewrite/java/migrate/lang/StringFormatted.java
@@ -17,15 +17,16 @@
 
 import lombok.EqualsAndHashCode;
 import lombok.Value;
-import org.openrewrite.ExecutionContext;
-import org.openrewrite.Preconditions;
-import org.openrewrite.Recipe;
-import org.openrewrite.TreeVisitor;
+import org.jspecify.annotations.Nullable;
+import org.openrewrite.*;
 import org.openrewrite.java.JavaVisitor;
 import org.openrewrite.java.MethodMatcher;
 import org.openrewrite.java.search.UsesJavaVersion;
 import org.openrewrite.java.search.UsesMethod;
-import org.openrewrite.java.tree.*;
+import org.openrewrite.java.tree.Expression;
+import org.openrewrite.java.tree.J;
+import org.openrewrite.java.tree.JRightPadded;
+import org.openrewrite.java.tree.Space;
 import org.openrewrite.marker.Markers;
 
 import java.time.Duration;
@@ -40,6 +41,13 @@ public class StringFormatted extends Recipe {
 
     private static final MethodMatcher STRING_FORMAT = new MethodMatcher("java.lang.String format(String, ..)");
 
+    @Option(displayName = "Add parentheses around the first argument",
+            description = "Add parentheses around the first argument if it is not a simple expression. " +
+                          "Default true; if false no change will be made. ",
+            required = false)
+    @Nullable
+    Boolean addParentheses;
+
     @Override
     public String getDisplayName() {
         return "Prefer `String.formatted(Object...)`";
@@ -51,52 +59,56 @@ public String getDescription() {
     }
 
     @Override
-    public TreeVisitor<?, ExecutionContext> getVisitor() {
-        return Preconditions.check(
-                Preconditions.and(new UsesJavaVersion<>(17), new UsesMethod<>(STRING_FORMAT)),
-                new StringFormattedVisitor());
+    public Duration getEstimatedEffortPerOccurrence() {
+        return Duration.ofMinutes(1);
     }
 
-    private static class StringFormattedVisitor extends JavaVisitor<ExecutionContext> {
-        @Override
-        public J visitMethodInvocation(J.MethodInvocation methodInvocation, ExecutionContext ctx) {
-            methodInvocation = (J.MethodInvocation) super.visitMethodInvocation(methodInvocation, ctx);
-            if (!STRING_FORMAT.matches(methodInvocation) || methodInvocation.getMethodType() == null) {
-                return methodInvocation;
-            }
+    @Override
+    public TreeVisitor<?, ExecutionContext> getVisitor() {
+        TreeVisitor<?, ExecutionContext> check = Preconditions.and(new UsesJavaVersion<>(17), new UsesMethod<>(STRING_FORMAT));
+        return Preconditions.check(check, new JavaVisitor<ExecutionContext>() {
+            @Override
+            public J visitMethodInvocation(J.MethodInvocation methodInvocation, ExecutionContext ctx) {
+                methodInvocation = (J.MethodInvocation) super.visitMethodInvocation(methodInvocation, ctx);
+                if (!STRING_FORMAT.matches(methodInvocation) || methodInvocation.getMethodType() == null) {
+                    return methodInvocation;
+                }
 
-            maybeRemoveImport("java.lang.String.format");
-            J.MethodInvocation mi = methodInvocation.withName(methodInvocation.getName().withSimpleName("formatted"));
-            mi = mi.withMethodType(methodInvocation.getMethodType().getDeclaringType().getMethods().stream()
-                    .filter(it -> it.getName().equals("formatted"))
-                    .findAny()
-                    .orElse(null));
-            if (mi.getName().getType() != null) {
-                mi = mi.withName(mi.getName().withType(mi.getMethodType()));
-            }
-            List<Expression> arguments = methodInvocation.getArguments();
-            mi = mi.withSelect(wrapperNotNeeded(arguments.get(0)) ? arguments.get(0).withPrefix(Space.EMPTY) :
-                    new J.Parentheses<>(randomId(), Space.EMPTY, Markers.EMPTY,
-                            JRightPadded.build(arguments.get(0))));
-            mi = mi.withArguments(arguments.subList(1, arguments.size()));
-            if (mi.getArguments().isEmpty()) {
-                // To store spaces between the parenthesis of a method invocation argument list
-                // Ensures formatting recipes chained together with this one will still work as expected
-                mi = mi.withArguments(singletonList(new J.Empty(randomId(), Space.EMPTY, Markers.EMPTY)));
-            }
-            return maybeAutoFormat(methodInvocation, mi, ctx);
-        }
+                // No change when change might be controversial, such as string concatenation
+                List<Expression> arguments = methodInvocation.getArguments();
+                boolean wrapperNeeded = wrapperNeeded(arguments.get(0));
+                if (Boolean.FALSE.equals(addParentheses) && wrapperNeeded) {
+                    return methodInvocation;
+                }
 
-        private static boolean wrapperNotNeeded(Expression expression) {
-            return expression instanceof J.Identifier ||
-                   expression instanceof J.Literal ||
-                   expression instanceof J.MethodInvocation ||
-                   expression instanceof J.FieldAccess;
-        }
-    }
+                maybeRemoveImport("java.lang.String.format");
+                J.MethodInvocation mi = methodInvocation.withName(methodInvocation.getName().withSimpleName("formatted"));
+                mi = mi.withMethodType(methodInvocation.getMethodType().getDeclaringType().getMethods().stream()
+                        .filter(it -> it.getName().equals("formatted"))
+                        .findAny()
+                        .orElse(null));
+                if (mi.getName().getType() != null) {
+                    mi = mi.withName(mi.getName().withType(mi.getMethodType()));
+                }
+                mi = mi.withSelect(wrapperNeeded ?
+                        new J.Parentheses<>(randomId(), Space.EMPTY, Markers.EMPTY,
+                                JRightPadded.build(arguments.get(0))) :
+                        arguments.get(0).withPrefix(Space.EMPTY));
+                mi = mi.withArguments(arguments.subList(1, arguments.size()));
+                if (mi.getArguments().isEmpty()) {
+                    // To store spaces between the parenthesis of a method invocation argument list
+                    // Ensures formatting recipes chained together with this one will still work as expected
+                    mi = mi.withArguments(singletonList(new J.Empty(randomId(), Space.EMPTY, Markers.EMPTY)));
+                }
+                return maybeAutoFormat(methodInvocation, mi, ctx);
+            }
 
-    @Override
-    public Duration getEstimatedEffortPerOccurrence() {
-        return Duration.ofMinutes(1);
+            private boolean wrapperNeeded(Expression expression) {
+                return !(expression instanceof J.Identifier ||
+                         expression instanceof J.Literal ||
+                         expression instanceof J.MethodInvocation ||
+                         expression instanceof J.FieldAccess);
+            }
+        });
     }
 }
diff --git a/src/main/resources/META-INF/rewrite/java-version-17.yml b/src/main/resources/META-INF/rewrite/java-version-17.yml
@@ -28,14 +28,15 @@ tags:
 recipeList:
   - org.openrewrite.java.migrate.Java8toJava11
   - org.openrewrite.java.migrate.UpgradeBuildToJava17
-  - org.openrewrite.java.migrate.lang.StringFormatted
   - org.openrewrite.staticanalysis.InstanceOfPatternMatch
   - org.openrewrite.staticanalysis.AddSerialAnnotationToSerialVersionUID
   - org.openrewrite.java.migrate.RemovedRuntimeTraceMethods
   - org.openrewrite.java.migrate.RemovedToolProviderConstructor
   - org.openrewrite.java.migrate.RemovedModifierAndConstantBootstrapsConstructors
   - org.openrewrite.java.migrate.lang.UseTextBlocks:
       convertStringsWithoutNewlines: false
+  - org.openrewrite.java.migrate.lang.StringFormatted:
+      addParentheses: false
   - org.openrewrite.java.migrate.DeprecatedJavaxSecurityCert
   - org.openrewrite.java.migrate.DeprecatedLogRecordThreadID
   - org.openrewrite.java.migrate.RemovedLegacySunJSSEProviderName

diff --git a/src/test/java/org/openrewrite/java/migrate/lang/StringFormattedTest.java b/src/test/java/org/openrewrite/java/migrate/lang/StringFormattedTest.java
@@ -27,7 +27,7 @@
 class StringFormattedTest implements RewriteTest {
     @Override
     public void defaults(RecipeSpec spec) {
-        spec.recipe(new StringFormatted());
+        spec.recipe(new StringFormatted(null));
     }
 
     @Test
@@ -72,12 +72,36 @@ void concatenatedText() {
                 class A {
                     String str = String.format("foo"
                             + "%s", "a");
-                }""", """
+                }
+                """,
+              """
                 package com.example.app;
                 class A {
                     String str = ("foo"
                             + "%s").formatted("a");
-                }"""
+                }
+                """
+            ),
+            17
+          )
+        );
+    }
+
+    @Issue("https://github.com/openrewrite/rewrite-migrate-java/issues/616")
+    @Test
+    void concatenatedFormatStringNotConvertedIfIndicated() {
+        //language=java
+        rewriteRun(
+          spec -> spec.recipe(new StringFormatted(false)),
+          version(
+            java(
+              """
+                package com.example.app;
+                class A {
+                    String str = String.format("foo"
+                            + "%s", "a");
+                }
+                """
             ),
             17
           )
@@ -92,21 +116,21 @@ void callingFunction() {
             java(
               """
                 package com.example.app;
-                
+
                 class A {
                     String str = String.format(getTemplateString(), "a");
-                
+
                     private String getTemplateString() {
                         return "foo %s";
                     }
                 }
                 """,
               """
                 package com.example.app;
-                
+
                 class A {
                     String str = getTemplateString().formatted("a");
-                
+
                     private String getTemplateString() {
                         return "foo %s";
                     }