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

[martinfrancois]

Suggested review order: 27 of 52 (Score: 4)
Review first: openrewrite/rewrite-testing-frameworks#1087

What's changed?

ReplaceStringConcatenationWithStringValueOf no longer rewrites "" + x when x is a char[] or when x has no type attribution, and the recipe no longer runs on source files that are not Java.

Before

String render(char[] chars) {
    return "" + chars;
}

Actual after the recipe

String render(char[] chars) {
    return String.valueOf(chars);
}

Expected after the recipe

(unchanged)

There are three independent guards:

• visitBinary skips the rewrite when the right operand has no type attribution. Without a known type, the recipe cannot prove that the generated String.valueOf(...) selects a behavior-preserving overload.
• visitBinary skips the rewrite when the right operand is an array whose element type is char. Every other array is still rewritten: for char[][], Character[], int[], String[] and Object[] the compiler picks the String.valueOf(Object) overload, which in a Java source file produces exactly the text that "" + x produces.
• The visitor is wrapped in Preconditions.check(new JavaFileChecker<>(), ...). JavaFileChecker passes only a J.CompilationUnit, so the recipe stops running on Groovy and on every other language whose parser produces its own compilation unit type. Fifteen other recipe classes here, among them UseTryWithResources and StringLiteralEquality, already declare themselves Java-only this way. That is a minority of the roughly 170 recipe classes in the repository, so it is an established pattern rather than a universal one.

The recipe description and the matching row in src/main/resources/META-INF/rewrite/recipes.csv, the third file in the diff, now both mention the char[] case.

What's your motivation?

Recipe: org.openrewrite.staticanalysis.ReplaceStringConcatenationWithStringValueOf.

The problem is in the recipe as it stands on main today, and this branch does not introduce it. I reproduced it on v2.39.0, v2.40.0 and current main. The recipe is listed in common-static-analysis.yml, so it reaches everyone who runs CommonStaticAnalysis.

Concatenation applies string conversion, and JLS 5.1.11 says a reference value is converted by invoking its toString method, so a char[] renders through Object.toString(). String.valueOf has a dedicated String.valueOf(char[]) overload that copies the array contents instead. For a char[] holding {'o', 'k'}, "" + chars produces [C@<identity hash>, where [C is the JVM type descriptor and the identity hash code in hexadecimal differs from run to run, while String.valueOf(chars) produces ok. For a null char[], "" + chars produces the four character text null while String.valueOf(chars) throws NullPointerException.

On main the recipe also runs on sources that are not Java, because the visitor is a plain JavaVisitor with no language check, and Groovy gives + different semantics: for an int[] holding {1, 2}, "" + intArray produces [1, 2] there while String.valueOf(intArray) produces [I@<identity hash>. That does not contradict int[] still being rewritten, which is a statement about Java sources, where both forms produce [I@<identity hash>.

Missing operand type

The missing-type case was first isolated while preparing this pull request.

Before

class Test {
    String method(Unresolved holder) {
        return "" + holder.chars();
    }
}

Actual after the recipe

class Test {
    String method(Unresolved holder) {
        return String.valueOf(holder.chars());
    }
}

Expected after the recipe

(unchanged)

When type attribution is missing, holder.chars() can represent a char[]. The two expressions then produce different values. The recipe MUST leave the expression unchanged unless the operand type proves that the selected String.valueOf(...) overload preserves behavior.

Affected code in real projects

• JnRouvignac/AutoRefactor StringValueOfRatherThanConcatSample.java: this sample pins down that AutoRefactor's own cleanup must leave "" + chars unchanged for a char[] parameter. The recipe from main rewrites that line to String.valueOf(chars), which selects the valueOf(char[]) overload, so the produced text changes from the array's identity string to its contents and the sample the tool's tests compare against is broken.

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

This change adds 5 tests to ReplaceStringConcatenationWithStringValueOfTest. Without the code change in this pull request, these 4 tests fail:

• doNotChangeCharArrayConcatenation
• doNotChangeCharArrayConcatenationForAnyOperandShape, covering a field, method call, cast, ternary, and parentheses
• doNotChangeGroovySources
• doNotChangeWhenOperandTypeIsMissing

The missing-type test disables type validation and verifies that an untyped operand remains unchanged.

The fifth test, replaceOtherArrayConcatenations, passes either way: it pins the other side of the boundary by asserting that char[][], int[], String[] and Object[] concatenations are all still rewritten, so the new guard cannot quietly grow and start skipping arrays it is not meant to skip.

No existing test expectation changed. The test file has added lines only.

Two limitations remain. Both are present on main today and I left them out of scope:

• The recipe still rewrites "" + o for a reference type o that is not an array of char, and that rewrite does not keep the value when o.toString() returns null: "" + o then produces the four character text null, while String.valueOf(o) returns a null String reference.
• visitParentheses removes the parentheses around any parenthesized String.valueOf(...) call, even in a file where the recipe changed nothing else.

Have you considered any alternatives or workarounds?

Rewriting to String.valueOf((Object) chars) would also keep the runtime value, and would keep the char[] case rewritten. I left the concatenation alone instead, because that cast exists only to pick a different overload and reads as noise. If you prefer the cast, it is a small change to visitBinary and to the test expectations.

I can also move the Java-only guard into its own pull request if you would rather keep this one about char[] only.

Any additional context

ReplaceStringBuilderWithString has a char[] problem that is the mirror image of this one, and the fix there goes in the opposite direction. There the source calls StringBuilder.append(char[]), which appends the characters, and the recipe turns that into concatenation, which does not, so the fix there is to introduce String.valueOf(...). Here the source already concatenates and the recipe replaces that with String.valueOf(chars), so the fix is to stop introducing it. I am sending that change separately, and neither depends on the other.

This change was prepared with AI assistance (Claude Code). I reviewed the code, the tests and this description.

I ran the formatter with the repository's .editorconfig. It also wanted to re-indent lines that this change does not touch, so I left those alone and kept the diff limited to this change.

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 run ./gradlew build locally, and committed any resulting changes to recipes.csv
• I've formatted the lines I changed, without reformatting code I didn't touch