From a48fed4428e56a57fba3aa5c001ec15e103e0d39 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Martin?= Date: Sun, 9 Aug 2026 16:41:19 +0200 Subject: [PATCH 1/4] ReplaceStringConcatenationWithStringValueOf: skip `char[]` and non-Java sources The recipe rewrote `"" + x` to `String.valueOf(x)` for every non-String right operand, but the two agree only under Java's string conversion. For a `char[]`, overload resolution selects `String.valueOf(char[])`, which renders the array's contents and throws on a null array, while concatenation renders it like any other `Object`. The visitor also ran on every `JavaSourceFile`: in Groovy `"" + x` renders a `Map` as `[a:1]` and an `int[]` as `[1, 2]`, where `String.valueOf(x)` gives `{a=1}` and a type-hash string. In both cases the rewrite changed the String the code produces. Skip a `char[]` right operand, and gate the visitor with `JavaFileChecker`, as other Java-specific recipes here do. `char[]` is skipped rather than routed through `String.valueOf((Object) chars)` because emitting that cast spells a type name whose resolution the recipe cannot verify: a type named `Object` in scope, or `java` for a qualified cast, would turn the output into code that no longer compiles. The recipe therefore loses the `char[]` case; the recipe description and its generated `recipes.csv` row now say so. No existing test expectation changed. --- ...eStringConcatenationWithStringValueOf.java | 18 +++- .../resources/META-INF/rewrite/recipes.csv | 2 +- ...ingConcatenationWithStringValueOfTest.java | 88 +++++++++++++++++++ 3 files changed, 104 insertions(+), 4 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/ReplaceStringConcatenationWithStringValueOf.java b/src/main/java/org/openrewrite/staticanalysis/ReplaceStringConcatenationWithStringValueOf.java index f20f7c7ba..c129b1cc4 100644 --- a/src/main/java/org/openrewrite/staticanalysis/ReplaceStringConcatenationWithStringValueOf.java +++ b/src/main/java/org/openrewrite/staticanalysis/ReplaceStringConcatenationWithStringValueOf.java @@ -17,14 +17,17 @@ 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.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 +45,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 tags = singleton("RSPEC-S1153"); @@ -52,7 +57,10 @@ public class ReplaceStringConcatenationWithStringValueOf extends Recipe { @Override public TreeVisitor getVisitor() { - return new JavaVisitor() { + // Only transform Java sources: the equivalence of `"" + x` and `String.valueOf(x)` relies on + // Java's string conversion. In Groovy, for example, `"" + x` renders a `Map` as `[a:1]` and + // an `int[]` as `[1, 2]`, while `String.valueOf(x)` produces `{a=1}` and a type-hash string. + return Preconditions.check(new JavaFileChecker<>(), new JavaVisitor() { @Override public J visitParentheses(J.Parentheses parens, ExecutionContext ctx) { J p = super.visitParentheses(parens, ctx); @@ -67,10 +75,14 @@ public J visitParentheses(J.Parentheses parens, ExecutionContex @Override public J visitBinary(J.Binary binary, ExecutionContext ctx) { + JavaType.Array arrayType = TypeUtils.asArray(binary.getRight().getType()); if (J.Literal.isLiteralValue(binary.getLeft(), "") && binary.getOperator() == J.Binary.Type.Addition && !TypeUtils.isString(binary.getRight().getType()) && !J.Literal.isLiteralValue(binary.getRight(), null) && + // Concatenation renders a `char[]` like any other `Object`, while `String.valueOf(chars)` + // would select the `char[]` overload, rendering the contents or throwing for a null array + (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)) { @@ -80,6 +92,6 @@ public J visitBinary(J.Binary binary, ExecutionContext ctx) { } return super.visitBinary(binary, ctx); } - }; + }); } } diff --git a/src/main/resources/META-INF/rewrite/recipes.csv b/src/main/resources/META-INF/rewrite/recipes.csv index bdfbd9e95..1105bd8bd 100644 --- a/src/main/resources/META-INF/rewrite/recipes.csv +++ b/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.,, diff --git a/src/test/java/org/openrewrite/staticanalysis/ReplaceStringConcatenationWithStringValueOfTest.java b/src/test/java/org/openrewrite/staticanalysis/ReplaceStringConcatenationWithStringValueOfTest.java index 9a341a68e..ba40c5402 100644 --- a/src/test/java/org/openrewrite/staticanalysis/ReplaceStringConcatenationWithStringValueOfTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/ReplaceStringConcatenationWithStringValueOfTest.java @@ -21,6 +21,7 @@ import org.openrewrite.test.RecipeSpec; import org.openrewrite.test.RewriteTest; +import static org.openrewrite.groovy.Assertions.groovy; import static org.openrewrite.java.Assertions.java; @SuppressWarnings("StringConcatenationMissingWhitespace") @@ -218,6 +219,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( @@ -308,6 +338,64 @@ 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 doNotChangeGroovySources() { + rewriteRun( + groovy( + //language=groovy + """ + class Test { + String render(int[] ints) { + return "" + ints + } + } + """ + ) + ); + } + @Test void doNotChangeWhenEmptyStringIsOnRight() { rewriteRun( From ace3490fdec8ddc4a5f699282e4e8abe26075284 Mon Sep 17 00:00:00 2001 From: Tim te Beek Date: Tue, 11 Aug 2026 09:43:43 +0200 Subject: [PATCH 2/4] Review fixes: check the null literal through parentheses --- ...eStringConcatenationWithStringValueOf.java | 20 +++++++++++-------- ...ingConcatenationWithStringValueOfTest.java | 16 +++++++++++++++ 2 files changed, 28 insertions(+), 8 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/ReplaceStringConcatenationWithStringValueOf.java b/src/main/java/org/openrewrite/staticanalysis/ReplaceStringConcatenationWithStringValueOf.java index c129b1cc4..025b3a6d7 100644 --- a/src/main/java/org/openrewrite/staticanalysis/ReplaceStringConcatenationWithStringValueOf.java +++ b/src/main/java/org/openrewrite/staticanalysis/ReplaceStringConcatenationWithStringValueOf.java @@ -23,6 +23,7 @@ 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; @@ -57,9 +58,8 @@ public class ReplaceStringConcatenationWithStringValueOf extends Recipe { @Override public TreeVisitor getVisitor() { - // Only transform Java sources: the equivalence of `"" + x` and `String.valueOf(x)` relies on - // Java's string conversion. In Groovy, for example, `"" + x` renders a `Map` as `[a:1]` and - // an `int[]` as `[1, 2]`, while `String.valueOf(x)` produces `{a=1}` and a type-hash string. + // The equivalence of `"" + x` and `String.valueOf(x)` relies on Java's string conversion; Groovy, for + // example, renders an `int[]` as `[1, 2]` through `+` but as a type-hash string through `String.valueOf`. return Preconditions.check(new JavaFileChecker<>(), new JavaVisitor() { @Override public J visitParentheses(J.Parentheses parens, ExecutionContext ctx) { @@ -75,19 +75,23 @@ public J visitParentheses(J.Parentheses 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.Array arrayType = TypeUtils.asArray(binary.getRight().getType()); if (J.Literal.isLiteralValue(binary.getLeft(), "") && binary.getOperator() == J.Binary.Type.Addition && !TypeUtils.isString(binary.getRight().getType()) && - !J.Literal.isLiteralValue(binary.getRight(), null) && - // Concatenation renders a `char[]` like any other `Object`, while `String.valueOf(chars)` - // would select the `char[]` overload, rendering the contents or throwing for a null array + // `String.valueOf(null)` selects the `char[]` overload and throws, while `"" + null` yields "null" + !J.Literal.isLiteralValue(right, null) && + // Concatenation renders a `char[]` like any other `Object`, while `String.valueOf(char[])` + // renders its contents, or throws when the array is 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); diff --git a/src/test/java/org/openrewrite/staticanalysis/ReplaceStringConcatenationWithStringValueOfTest.java b/src/test/java/org/openrewrite/staticanalysis/ReplaceStringConcatenationWithStringValueOfTest.java index ba40c5402..426086b39 100644 --- a/src/test/java/org/openrewrite/staticanalysis/ReplaceStringConcatenationWithStringValueOfTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/ReplaceStringConcatenationWithStringValueOfTest.java @@ -322,6 +322,22 @@ void method() { ); } + @Test + void doNotChangeParenthesizedNullConcatenation() { + rewriteRun( + //language=java + java( + """ + class Test { + void method() { + String s = "" + (null); + } + } + """ + ) + ); + } + @Test void doNotChangeNonEmptyStringConcatenation() { rewriteRun( From 0286c26866a9182c6254a5b732c5087ff05e2dfc Mon Sep 17 00:00:00 2001 From: Tim te Beek Date: Wed, 12 Aug 2026 00:23:52 +0200 Subject: [PATCH 3/4] Trim commentary --- .../ReplaceStringConcatenationWithStringValueOf.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/ReplaceStringConcatenationWithStringValueOf.java b/src/main/java/org/openrewrite/staticanalysis/ReplaceStringConcatenationWithStringValueOf.java index 025b3a6d7..0f8467ce8 100644 --- a/src/main/java/org/openrewrite/staticanalysis/ReplaceStringConcatenationWithStringValueOf.java +++ b/src/main/java/org/openrewrite/staticanalysis/ReplaceStringConcatenationWithStringValueOf.java @@ -58,8 +58,8 @@ public class ReplaceStringConcatenationWithStringValueOf extends Recipe { @Override public TreeVisitor getVisitor() { - // The equivalence of `"" + x` and `String.valueOf(x)` relies on Java's string conversion; Groovy, for - // example, renders an `int[]` as `[1, 2]` through `+` but as a type-hash string through `String.valueOf`. + // 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() { @Override public J visitParentheses(J.Parentheses parens, ExecutionContext ctx) { @@ -85,8 +85,8 @@ public J visitBinary(J.Binary binary, ExecutionContext ctx) { !TypeUtils.isString(binary.getRight().getType()) && // `String.valueOf(null)` selects the `char[]` overload and throws, while `"" + null` yields "null" !J.Literal.isLiteralValue(right, null) && - // Concatenation renders a `char[]` like any other `Object`, while `String.valueOf(char[])` - // renders its contents, or throws when the array is 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) && From 92bf6c40982c07d2fe2076cf1e465557f82bfe9b Mon Sep 17 00:00:00 2001 From: martinfrancois Date: Sun, 16 Aug 2026 04:52:54 +0200 Subject: [PATCH 4/4] Skip untyped string concatenation operands --- ...ceStringConcatenationWithStringValueOf.java | 6 ++++-- ...ringConcatenationWithStringValueOfTest.java | 18 ++++++++++++++++++ 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/ReplaceStringConcatenationWithStringValueOf.java b/src/main/java/org/openrewrite/staticanalysis/ReplaceStringConcatenationWithStringValueOf.java index 0f8467ce8..69d4aa9af 100644 --- a/src/main/java/org/openrewrite/staticanalysis/ReplaceStringConcatenationWithStringValueOf.java +++ b/src/main/java/org/openrewrite/staticanalysis/ReplaceStringConcatenationWithStringValueOf.java @@ -79,10 +79,12 @@ public J visitBinary(J.Binary binary, ExecutionContext ctx) { while (right instanceof J.Parentheses && ((J.Parentheses) right).getTree() instanceof Expression) { right = (Expression) ((J.Parentheses) right).getTree(); } - JavaType.Array arrayType = TypeUtils.asArray(binary.getRight().getType()); + 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()) && + 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 diff --git a/src/test/java/org/openrewrite/staticanalysis/ReplaceStringConcatenationWithStringValueOfTest.java b/src/test/java/org/openrewrite/staticanalysis/ReplaceStringConcatenationWithStringValueOfTest.java index 426086b39..2b9025d19 100644 --- a/src/test/java/org/openrewrite/staticanalysis/ReplaceStringConcatenationWithStringValueOfTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/ReplaceStringConcatenationWithStringValueOfTest.java @@ -20,6 +20,7 @@ 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; @@ -396,6 +397,23 @@ void method(Object o, boolean b, char[] 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(