diff --git a/src/main/java/org/openrewrite/staticanalysis/NoValueOfOnStringType.java b/src/main/java/org/openrewrite/staticanalysis/NoValueOfOnStringType.java index 5a79fc14d..3ec682119 100644 --- a/src/main/java/org/openrewrite/staticanalysis/NoValueOfOnStringType.java +++ b/src/main/java/org/openrewrite/staticanalysis/NoValueOfOnStringType.java @@ -64,7 +64,7 @@ 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)); } } @@ -72,14 +72,43 @@ public J visitMethodInvocation(J.MethodInvocation method, ExecutionContext ctx) } /** - * 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. + * {@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 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) { - if (TypeUtils.asPrimitive(argument.getType()) != null) { + 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(