Keep char[] and non-Java sources out of String.valueOf conversion

src/main/java/org/openrewrite/staticanalysis/ReplaceStringConcatenationWithStringValueOf.java

@@ -17,14 +17,18 @@
 
 import lombok.Getter;
 import org.openrewrite.ExecutionContext;
+import org.openrewrite.Preconditions;
 import org.openrewrite.Recipe;
 import org.openrewrite.TreeVisitor;
 import org.openrewrite.java.JavaTemplate;
 import org.openrewrite.java.JavaVisitor;
 import org.openrewrite.java.MethodMatcher;
+import org.openrewrite.java.tree.Expression;
 import org.openrewrite.java.tree.J;
+import org.openrewrite.java.tree.JavaType;
 import org.openrewrite.java.tree.MethodCall;
 import org.openrewrite.java.tree.TypeUtils;
+import org.openrewrite.staticanalysis.java.JavaFileChecker;
 
 import java.time.Duration;
 import java.util.Set;
@@ -42,7 +46,9 @@ public class ReplaceStringConcatenationWithStringValueOf extends Recipe {
     final String description = "Replace inefficient string concatenation patterns like `\"\" + ...` with " +
             "`String.valueOf(...)`. This improves code readability and may have minor performance " +
             "benefits. The empty string prefix `\"\" +` is an indirect way to convert a value to " +
-            "a `String`, while `String.valueOf()` clearly communicates the conversion intent.";
+            "a `String`, while `String.valueOf()` clearly communicates the conversion intent. " +
+            "Concatenation with a `char[]` is left unchanged, since `String.valueOf(char[])` renders " +
+            "the array's contents while concatenation renders the array like any other `Object`.";
 
     @Getter
     final Set<String> tags = singleton("RSPEC-S1153");
@@ -52,7 +58,9 @@ public class ReplaceStringConcatenationWithStringValueOf extends Recipe {
 
     @Override
     public TreeVisitor<?, ExecutionContext> getVisitor() {
-        return new JavaVisitor<ExecutionContext>() {
+        // The equivalence relies on Java's string conversion; Groovy renders an `int[]` as `[1, 2]` through `+`
+        // but as a type-hash string through `String.valueOf`
+        return Preconditions.check(new JavaFileChecker<>(), new JavaVisitor<ExecutionContext>() {
             @Override
             public <T extends J> J visitParentheses(J.Parentheses<T> parens, ExecutionContext ctx) {
                 J p = super.visitParentheses(parens, ctx);
@@ -67,19 +75,29 @@ public <T extends J> J visitParentheses(J.Parentheses<T> parens, ExecutionContex
 
             @Override
             public J visitBinary(J.Binary binary, ExecutionContext ctx) {
+                Expression right = binary.getRight();
+                while (right instanceof J.Parentheses && ((J.Parentheses<?>) right).getTree() instanceof Expression) {
+                    right = (Expression) ((J.Parentheses<?>) right).getTree();
+                }
+                JavaType rightType = right.getType();
+                JavaType.Array arrayType = TypeUtils.asArray(rightType);
                 if (J.Literal.isLiteralValue(binary.getLeft(), "") &&
                         binary.getOperator() == J.Binary.Type.Addition &&
-                        !TypeUtils.isString(binary.getRight().getType()) &&
-                        !J.Literal.isLiteralValue(binary.getRight(), null) &&
+                        rightType != null &&
+                        !TypeUtils.isString(rightType) &&
+                        // `String.valueOf(null)` selects the `char[]` overload and throws, while `"" + null` yields "null"
+                        !J.Literal.isLiteralValue(right, null) &&
+                        // Concatenation renders a `char[]` like any `Object`; `String.valueOf(char[])` renders its
+                        // contents, or throws on null
+                        (arrayType == null || arrayType.getElemType() != JavaType.Primitive.Char) &&
                         // Avoid breaking symmetry in chained String concatenations
                         !(binary.getRight() instanceof J.Binary) &&
                         !(getCursor().getParentTreeCursor().getValue() instanceof J.Binary)) {
-                    return JavaTemplate.apply("String.valueOf(#{any()})", getCursor(), binary.getCoordinates().replace(), binary.getRight() instanceof J.Parentheses ?
-                            ((J.Parentheses<?>) binary.getRight()).getTree() : binary.getRight())
+                    return JavaTemplate.apply("String.valueOf(#{any()})", getCursor(), binary.getCoordinates().replace(), right)
                             .withPrefix(binary.getPrefix());
                 }
                 return super.visitBinary(binary, ctx);
             }
-        };
+        });
     }
 }

src/main/resources/META-INF/rewrite/recipes.csv

@@ -155,7 +155,7 @@ maven,org.openrewrite.recipe:rewrite-static-analysis,org.openrewrite.staticanaly
 `Stack` inherits from `Vector`, which carries unnecessary synchronization overhead in single-threaded contexts and exposes non-stack operations like random index access.",1,,Static analysis and remediation,,Remediations for issues identified by SAST tools.,,
 maven,org.openrewrite.recipe:rewrite-static-analysis,org.openrewrite.staticanalysis.ReplaceStringBufferWithStringBuilder,Replace `java.lang.StringBuffer` with `java.lang.StringBuilder`,"`StringBuffer` synchronizes every operation, which adds overhead in the common single-threaded case. `StringBuilder` exposes the identical API without the synchronization. This recipe replaces a local `StringBuffer` with a `StringBuilder` when data flow analysis can prove the `StringBuffer` never escapes its method (it is not returned, assigned to a field, or passed as an argument), so no other thread can observe it and the synchronization is redundant. Fields and escaping variables are left untouched.",1,,Static analysis and remediation,,Remediations for issues identified by SAST tools.,,"[{""name"":""org.openrewrite.staticanalysis.table.LegacySynchronizedTypesNotMigrated"",""displayName"":""Legacy synchronized types not migrated"",""instanceName"":""Legacy synchronized types not migrated"",""description"":""Instances of a legacy synchronized type (`Hashtable`, `Vector`, `Stack`, `StringBuffer`) that were found but left unchanged because they could not be proven safe to modernize."",""columns"":[{""name"":""sourcePath"",""type"":""String"",""displayName"":""Source path"",""description"":""The path to the source file containing the unmigrated reference.""},{""name"":""enclosingClass"",""type"":""String"",""displayName"":""Class"",""description"":""The fully qualified name of the class containing the reference.""},{""name"":""unmigratedType"",""type"":""String"",""displayName"":""Unmigrated type"",""description"":""The fully qualified name of the legacy synchronized type that was found but not migrated.""},{""name"":""reason"",""type"":""String"",""displayName"":""Reason"",""description"":""Why the instance was left unchanged.""}]}]"
 maven,org.openrewrite.recipe:rewrite-static-analysis,org.openrewrite.staticanalysis.ReplaceStringBuilderWithString,Replace `StringBuilder#append` with `String`,"Replace `StringBuilder.append()` with String if you are only concatenating a small number of strings and the code is simple and easy to read, as the compiler can optimize simple string concatenation expressions into a single String object, which can be more efficient than using StringBuilder.",1,,Static analysis and remediation,,Remediations for issues identified by SAST tools.,,
-maven,org.openrewrite.recipe:rewrite-static-analysis,org.openrewrite.staticanalysis.ReplaceStringConcatenationWithStringValueOf,Replace String concatenation with `String.valueOf()`,"Replace inefficient string concatenation patterns like `"""" + ...` with `String.valueOf(...)`. This improves code readability and may have minor performance benefits. The empty string prefix `"""" +` is an indirect way to convert a value to a `String`, while `String.valueOf()` clearly communicates the conversion intent.",1,,Static analysis and remediation,,Remediations for issues identified by SAST tools.,,
+maven,org.openrewrite.recipe:rewrite-static-analysis,org.openrewrite.staticanalysis.ReplaceStringConcatenationWithStringValueOf,Replace String concatenation with `String.valueOf()`,"Replace inefficient string concatenation patterns like `"""" + ...` with `String.valueOf(...)`. This improves code readability and may have minor performance benefits. The empty string prefix `"""" +` is an indirect way to convert a value to a `String`, while `String.valueOf()` clearly communicates the conversion intent. Concatenation with a `char[]` is left unchanged, since `String.valueOf(char[])` renders the array's contents while concatenation renders the array like any other `Object`.",1,,Static analysis and remediation,,Remediations for issues identified by SAST tools.,,
 maven,org.openrewrite.recipe:rewrite-static-analysis,org.openrewrite.staticanalysis.ReplaceTextBlockWithString,Replace text block with regular string,Replace text block with a regular multi-line string. Text blocks that fit on a single line without concatenation or escaped newlines gain no readability benefit from the triple-quote syntax and are clearer as plain string literals.,1,,Static analysis and remediation,,Remediations for issues identified by SAST tools.,,
 maven,org.openrewrite.recipe:rewrite-static-analysis,org.openrewrite.staticanalysis.ReplaceThreadRunWithThreadStart,Replace calls to `Thread.run()` with `Thread.start()`,`Thread.run()` should not be called directly.,2,,Static analysis and remediation,,Remediations for issues identified by SAST tools.,,
 maven,org.openrewrite.recipe:rewrite-static-analysis,org.openrewrite.staticanalysis.ReplaceValidateNotNullHavingSingleArgWithObjectsRequireNonNull,Replace `org.apache.commons.lang3.Validate#notNull` with `Objects#requireNonNull`,Replace `org.apache.commons.lang3.Validate.notNull(Object)` with `Objects.requireNonNull(Object)`.,3,,Static analysis and remediation,,Remediations for issues identified by SAST tools.,,

src/test/java/org/openrewrite/staticanalysis/ReplaceStringConcatenationWithStringValueOfTest.java

@@ -20,7 +20,9 @@
 import org.openrewrite.DocumentExample;
 import org.openrewrite.test.RecipeSpec;
 import org.openrewrite.test.RewriteTest;
+import org.openrewrite.test.TypeValidation;
 
+import static org.openrewrite.groovy.Assertions.groovy;
 import static org.openrewrite.java.Assertions.java;
 
 @SuppressWarnings("StringConcatenationMissingWhitespace")
@@ -218,6 +220,35 @@ void method() {
         );
     }
 
+    @Test
+    void replaceOtherArrayConcatenations() {
+        rewriteRun(
+          //language=java
+          java(
+            """
+              class Test {
+                  void method(char[][] grid, int[] ints, String[] strings, Object[] objects) {
+                      String a = "" + grid;
+                      String b = "" + ints;
+                      String c = "" + strings;
+                      String d = "" + objects;
+                  }
+              }
+              """,
+            """
+              class Test {
+                  void method(char[][] grid, int[] ints, String[] strings, Object[] objects) {
+                      String a = String.valueOf(grid);
+                      String b = String.valueOf(ints);
+                      String c = String.valueOf(strings);
+                      String d = String.valueOf(objects);
+                  }
+              }
+              """
+          )
+        );
+    }
+
     @Test
     void preserveComments() {
         rewriteRun(
@@ -292,6 +323,22 @@ void method() {
             );
         }
 
+        @Test
+        void doNotChangeParenthesizedNullConcatenation() {
+            rewriteRun(
+              //language=java
+              java(
+                """
+                  class Test {
+                      void method() {
+                          String s = "" + (null);
+                      }
+                  }
+                  """
+              )
+            );
+        }
+
         @Test
         void doNotChangeNonEmptyStringConcatenation() {
             rewriteRun(
@@ -308,6 +355,81 @@ void method() {
             );
         }
 
+        @Test
+        void doNotChangeCharArrayConcatenation() {
+            rewriteRun(
+              //language=java
+              java(
+                """
+                  class Test {
+                      String render(char[] chars) {
+                          return "" + chars;
+                      }
+                  }
+                  """
+              )
+            );
+        }
+
+        @Test
+        void doNotChangeCharArrayConcatenationForAnyOperandShape() {
+            rewriteRun(
+              //language=java
+              java(
+                """
+                  class Test {
+                      char[] field;
+
+                      char[] chars() {
+                          return field;
+                      }
+
+                      void method(Object o, boolean b, char[] other) {
+                          String a = "" + field;
+                          String c = "" + chars();
+                          String d = "" + (char[]) o;
+                          String e = "" + (b ? field : other);
+                          String f = "" + (other);
+                      }
+                  }
+                  """
+              )
+            );
+        }
+
+        @Test
+        void doNotChangeWhenOperandTypeIsMissing() {
+            rewriteRun(
+              spec -> spec.typeValidationOptions(TypeValidation.none()),
+              //language=java
+              java(
+                """
+                  class Test {
+                      String method(Unresolved holder) {
+                          return "" + holder.chars();
+                      }
+                  }
+                  """
+              )
+            );
+        }
+
+        @Test
+        void doNotChangeGroovySources() {
+            rewriteRun(
+              groovy(
+                //language=groovy
+                """
+                  class Test {
+                      String render(int[] ints) {
+                          return "" + ints
+                      }
+                  }
+                  """
+              )
+            );
+        }
+
         @Test
         void doNotChangeWhenEmptyStringIsOnRight() {
             rewriteRun(