From 8a5c8676c8fd458c2b6bdc9592c6562274147578 Mon Sep 17 00:00:00 2001 From: Niloyyy Date: Sat, 22 Aug 2026 03:46:17 +0600 Subject: [PATCH 1/2] fix: keep String.valueOf when the argument can be null in NoValueOfOnStringType --- .../staticanalysis/NoValueOfOnStringType.java | 33 ++++++++- .../NoValueOfOnStringTypeTest.java | 73 +++++++++++++++++++ 2 files changed, 104 insertions(+), 2 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/NoValueOfOnStringType.java b/src/main/java/org/openrewrite/staticanalysis/NoValueOfOnStringType.java index 5a79fc14d..3bd90e52e 100644 --- a/src/main/java/org/openrewrite/staticanalysis/NoValueOfOnStringType.java +++ b/src/main/java/org/openrewrite/staticanalysis/NoValueOfOnStringType.java @@ -64,13 +64,40 @@ public J visitMethodInvocation(J.MethodInvocation method, ExecutionContext ctx) J.MethodInvocation mi = (J.MethodInvocation) super.visitMethodInvocation(method, ctx); if (VALUE_OF.matches(mi) && mi.getArguments().size() == 1) { Expression argument = mi.getArguments().get(0); - if ((TypeUtils.isString(argument.getType()) && !(argument instanceof J.MethodInvocation)) || removeValueOfForStringConcatenation(argument)) { + if ((TypeUtils.isString(argument.getType()) && isNeverNull(argument)) || removeValueOfForStringConcatenation(argument)) { return maybeParenthesize(argument.withPrefix(mi.getPrefix()), updateCursor(mi)); } } return mi; } + /** + * {@code String.valueOf(s)} only equals {@code s} when {@code s} is not null; for a null {@code String} + * it yields {@code "null"} instead. Removing the call is therefore only safe for arguments that cannot + * be null. Method invocations were already excluded for this reason; identifiers, field accesses and + * casts are no safer, so require the argument to be demonstrably non-null instead. + * + * @param argument The argument of the valueOf method. + * @return True if the argument can never be null. + */ + private boolean isNeverNull(Expression argument) { + Expression e = argument; + while (e instanceof J.Parentheses) { + J tree = ((J.Parentheses) e).getTree(); + if (!(tree instanceof Expression)) { + return false; + } + e = (Expression) tree; + } + if (e instanceof J.Literal) { + return ((J.Literal) e).getValue() != null; + } + // String concatenation always produces a non-null String. + return e instanceof J.Binary && + ((J.Binary) e).getOperator() == J.Binary.Type.Addition && + TypeUtils.isString(e.getType()); + } + /** * If the String#valueOf method is within a binary expression and the argument is a primitive, the valueOf * can be removed if the binary expression's type is a String. @@ -79,7 +106,9 @@ public J visitMethodInvocation(J.MethodInvocation method, ExecutionContext ctx) * @return True if the method can be removed. */ private boolean removeValueOfForStringConcatenation(Expression argument) { - if (TypeUtils.asPrimitive(argument.getType()) != null) { + // A String argument is safe here too: concatenation renders a null operand as "null", + // exactly as String#valueOf would. + if (TypeUtils.asPrimitive(argument.getType()) != null || TypeUtils.isString(argument.getType())) { J parent = getCursor().getParent() != null ? getCursor().getParent().firstEnclosing(J.class) : null; if (parent instanceof J.Binary) { J.Binary b = (J.Binary) parent; diff --git a/src/test/java/org/openrewrite/staticanalysis/NoValueOfOnStringTypeTest.java b/src/test/java/org/openrewrite/staticanalysis/NoValueOfOnStringTypeTest.java index ec8a75812..e2b943cc4 100644 --- a/src/test/java/org/openrewrite/staticanalysis/NoValueOfOnStringTypeTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/NoValueOfOnStringTypeTest.java @@ -295,6 +295,79 @@ static void method(int i) { ); } + @Issue("https://github.com/openrewrite/rewrite-static-analysis/issues/1009") + @Test + void doNotRemoveValueOfOnNullInitializedConstant() { + // String.valueOf(null String) is "null"; passing the constant directly throws in replace(..). + rewriteRun( + //language=java + java( + """ + class Test { + private static final String INCIDENT_OPEN_TASK = null; + + String replaceDummyValues(String templateBody) { + return templateBody.replace("$INCIDENT_OPEN_TASK", String.valueOf(INCIDENT_OPEN_TASK)); + } + } + """ + ) + ); + } + + @Issue("https://github.com/openrewrite/rewrite-static-analysis/issues/1009") + @Test + void doNotRemoveValueOfOnNullableStringVariables() { + rewriteRun( + //language=java + java( + """ + class Test { + String field; + + String returned(String parameter) { + return String.valueOf(parameter); + } + + String assigned() { + String local = String.valueOf(field); + return local; + } + + int argument(String parameter) { + return "text".indexOf(String.valueOf(parameter)); + } + } + """ + ) + ); + } + + @Issue("https://github.com/openrewrite/rewrite-static-analysis/issues/1009") + @Test + void stillRemovesValueOfOnStringWithinConcatenation() { + // Concatenation renders a null operand as "null" already, so removal is safe here. + rewriteRun( + //language=java + java( + """ + class Test { + String method(String parameter) { + return "prefix" + String.valueOf(parameter); + } + } + """, + """ + class Test { + String method(String parameter) { + return "prefix" + parameter; + } + } + """ + ) + ); + } + @Test void doNotRemoveValueOfForNullableStrings() { rewriteRun( From 3f0e66821b6039d85274bb20faa768cecd6bdcfa Mon Sep 17 00:00:00 2001 From: Niloyyy Date: Wed, 26 Aug 2026 12:07:59 +0600 Subject: [PATCH 2/2] docs: correct removeValueOfForStringConcatenation javadoc for String arguments --- .../openrewrite/staticanalysis/NoValueOfOnStringType.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/NoValueOfOnStringType.java b/src/main/java/org/openrewrite/staticanalysis/NoValueOfOnStringType.java index 3bd90e52e..3ec682119 100644 --- a/src/main/java/org/openrewrite/staticanalysis/NoValueOfOnStringType.java +++ b/src/main/java/org/openrewrite/staticanalysis/NoValueOfOnStringType.java @@ -99,15 +99,15 @@ private boolean isNeverNull(Expression argument) { } /** - * If the String#valueOf method is within a binary expression and the argument is a primitive, the valueOf - * can be removed if the binary expression's type is a String. + * If the String#valueOf method is within a binary expression and the argument is a primitive or a String, + * the valueOf can be removed if the binary expression's type is a String. A String argument is safe here + * even when it is null, because concatenation renders a null operand as {@code "null"} exactly as + * String#valueOf would. * * @param argument The argument of the valueOf method. * @return True if the method can be removed. */ private boolean removeValueOfForStringConcatenation(Expression argument) { - // A String argument is safe here too: concatenation renders a null operand as "null", - // exactly as String#valueOf would. if (TypeUtils.asPrimitive(argument.getType()) != null || TypeUtils.isString(argument.getType())) { J parent = getCursor().getParent() != null ? getCursor().getParent().firstEnclosing(J.class) : null; if (parent instanceof J.Binary) {