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(