ReplaceDeprecatedRuntimeExecMethods: preserve the argument vector

src/main/java/org/openrewrite/staticanalysis/ReplaceDeprecatedRuntimeExecMethods.java

@@ -30,6 +30,7 @@
 import java.time.Duration;
 import java.util.ArrayList;
 import java.util.List;
+import java.util.StringTokenizer;
 
 public class ReplaceDeprecatedRuntimeExecMethods extends Recipe {
     private static final MethodMatcher RUNTIME_EXEC_CMD = new MethodMatcher("java.lang.Runtime exec(String)");
@@ -41,7 +42,10 @@ public class ReplaceDeprecatedRuntimeExecMethods extends Recipe {
 
     @Getter
     final String description = "Replace `Runtime#exec(String)` methods to use `exec(String[])` instead because the former is deprecated " +
-            "after Java 18 and is no longer recommended for use by the Java documentation.";
+            "after Java 18 and is no longer recommended for use by the Java documentation. Only commands made up entirely of " +
+            "string literals are replaced, because only then is it known at compile time which arguments " +
+            "`Runtime#exec(String)` would build; any other command is left unchanged rather than launched with " +
+            "different arguments.";
 
     @Getter
     final Duration estimatedEffortPerOccurrence = Duration.ofMinutes(3);
@@ -71,7 +75,8 @@ public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, Execu
                     StringBuilder sb = new StringBuilder();
                     if (flattenAble) {
                         for (Expression e : commands) {
-                            if (e instanceof J.Literal && ((J.Literal) e).getType() == JavaType.Primitive.String) {
+                            if (e instanceof J.Literal && ((J.Literal) e).getType() == JavaType.Primitive.String &&
+                                    isDecoded((J.Literal) e)) {
                                 sb.append(((J.Literal) e).getValue());
                             } else {
                                 flattenAble = false;
@@ -80,66 +85,79 @@ public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, Execu
                         }
                     }
 
-                    updateCursor(m);
-                    if (flattenAble) {
-                        String[] cmds = sb.toString().split(" ");
-                        String templateCode = String.format("new String[] {%s}", toStringArguments(cmds));
-                        JavaTemplate template = JavaTemplate.builder(templateCode).build();
-
-                        List<Expression> args = m.getArguments();
-                        Cursor cursor = new Cursor(getCursor(), args.get(0));
-                        args.set(0, template.apply(cursor, args.get(0).getCoordinates().replace()));
-
-                        if (m.getMethodType() != null) {
-                            List<JavaType> parameterTypes = m.getMethodType().getParameterTypes();
-                            parameterTypes.set(0, JavaType.ShallowClass.build("java.lang.String[]"));
-
-                            return m.withArguments(args)
-                                    .withMethodType(m.getMethodType().withParameterTypes(parameterTypes));
-                        }
-                    } else {
-                        // replace argument to 'command.split(" ")'
-                        List<Expression> args = m.getArguments();
-                        boolean needWrap = false;
-                        Expression arg0 = args.get(0);
-                        if (!(arg0 instanceof J.Identifier) &&
-                            !(arg0 instanceof J.Literal) &&
-                            !(arg0 instanceof J.MethodInvocation)) {
-                            needWrap = true;
-                        }
-
-                        String code = needWrap ? "(#{any()}).split(\" \")" : "#{any()}.split(\" \")";
-                        JavaTemplate template = JavaTemplate.builder(code).contextSensitive().build();
-                        Cursor cursor = new Cursor(getCursor(), args.get(0));
-                        arg0 = template.apply(cursor, args.get(0).getCoordinates().replace(), args.get(0));
-                        args.set(0, arg0);
-
-                        if (m.getMethodType() != null) {
-                            List<JavaType> parameterTypes = m.getMethodType().getParameterTypes();
-                            parameterTypes.set(0, JavaType.ShallowClass.build("java.lang.String[]"));
+                    // Only an all-literal command can be tokenized the way `Runtime#exec(String)` does, on any of
+                    // ' ', '\t', '\n', '\r' and '\f', collapsing runs; anything else would launch a different process
+                    if (!flattenAble) {
+                        return m;
+                    }
 
-                            return m.withArguments(args).withMethodType(m.getMethodType().withParameterTypes(parameterTypes));
-                        }
+                    List<String> cmds = new ArrayList<>();
+                    for (StringTokenizer tokenizer = new StringTokenizer(sb.toString()); tokenizer.hasMoreTokens(); ) {
+                        cmds.add(tokenizer.nextToken());
+                    }
+                    // `exec("")` throws `IllegalArgumentException` where `exec(new String[]{})` throws
+                    // `IndexOutOfBoundsException`, so a command tokenizing to nothing is left alone
+                    JavaType.Method methodType = m.getMethodType();
+                    if (cmds.isEmpty() || methodType == null) {
                         return m;
                     }
+
+                    updateCursor(m);
+                    JavaTemplate template = JavaTemplate.builder(String.format("new String[] {%s}", toStringArguments(cmds))).build();
+
+                    List<Expression> args = m.getArguments();
+                    Cursor cursor = new Cursor(getCursor(), args.get(0));
+                    args.set(0, template.apply(cursor, args.get(0).getCoordinates().replace()));
+
+                    // `getParameterTypes()` writes through to the interned `JavaType.Method` shared by every call
+                    // of this overload, so copy before replacing
+                    List<JavaType> parameterTypes = new ArrayList<>(methodType.getParameterTypes());
+                    parameterTypes.set(0, new JavaType.Array(null, JavaType.ShallowClass.build("java.lang.String"), null));
+                    return m.withArguments(args)
+                            .withMethodType(methodType.withParameterTypes(parameterTypes));
                 }
 
                 return m;
             }
         });
     }
 
-    private static String toStringArguments(String[] cmds) {
+    // getValue() is not fully decoded when the literal contains Unicode escapes.
+    private static boolean isDecoded(J.Literal literal) {
+        List<J.Literal.UnicodeEscape> unicodeEscapes = literal.getUnicodeEscapes();
+        return literal.getValue() != null && (unicodeEscapes == null || unicodeEscapes.isEmpty());
+    }
+
+    private static final String TEMPLATE_INTERPOLATION_START = "#{";
+
+    private static String toStringArguments(List<String> cmds) {
         StringBuilder sb = new StringBuilder();
-        for (int i = 0; i < cmds.length; i++) {
-            String token = cmds[i];
-            if (i != 0) {
+        for (String token : cmds) {
+            if (sb.length() != 0) {
                 sb.append(", ");
             }
-            sb.append("\"")
-                    .append(token)
-                    .append("\"");
+            sb.append('"');
+            for (int i = 0; i < token.length(); i++) {
+                char character = token.charAt(i);
+                if (character == '"' || character == '\\') {
+                    // Prefix a backslash so the character survives inside the generated string literal
+                    sb.append('\\').append(character);
+                } else if (Character.isISOControl(character) ||
+                        token.startsWith(TEMPLATE_INTERPOLATION_START, i)) {
+                    // Control characters stay legible rather than becoming invisible bytes, and `#{` needs escaping
+                    // because this text is also `JavaTemplate` source
+                    sb.append(unicodeEscape(character));
+                } else {
+                    sb.append(character);
+                }
+            }
+            sb.append('"');
         }
         return sb.toString();
     }
+
+    // Formats the character as a Java Unicode escape, `\u0007` for the bell character
+    private static String unicodeEscape(char character) {
+        return String.format("\\u%04x", (int) character);
+    }
 }

src/main/resources/META-INF/rewrite/recipes.csv

@@ -143,7 +143,7 @@ According to the `Collection#toArray(T[])` documentation:
 However, although it's not intuitive, allocating a right-sized array ahead of time to pass to the API appears to be [generally worse for performance](https://shipilev.net/blog/2016/arrays-wisdom-ancients/#_conclusion) according to benchmarking and JVM developers due to a number of implementation details in both Java and the virtual machine.
 
 H2 achieved significant performance gains by [switching to empty arrays instead pre-sized ones](https://github.com/h2database/h2database/issues/311).",1,,Static analysis and remediation,,Remediations for issues identified by SAST tools.,,
-maven,org.openrewrite.recipe:rewrite-static-analysis,org.openrewrite.staticanalysis.ReplaceDeprecatedRuntimeExecMethods,Replace deprecated `Runtime#exec()` methods,Replace `Runtime#exec(String)` methods to use `exec(String[])` instead because the former is deprecated after Java 18 and is no longer recommended for use by the Java documentation.,1,,Static analysis and remediation,,Remediations for issues identified by SAST tools.,,
+maven,org.openrewrite.recipe:rewrite-static-analysis,org.openrewrite.staticanalysis.ReplaceDeprecatedRuntimeExecMethods,Replace deprecated `Runtime#exec()` methods,"Replace `Runtime#exec(String)` methods to use `exec(String[])` instead because the former is deprecated after Java 18 and is no longer recommended for use by the Java documentation. Only commands made up entirely of string literals are replaced, because only then is it known at compile time which arguments `Runtime#exec(String)` would build; any other command is left unchanged rather than launched with different arguments.",1,,Static analysis and remediation,,Remediations for issues identified by SAST tools.,,
 maven,org.openrewrite.recipe:rewrite-static-analysis,org.openrewrite.staticanalysis.ReplaceDuplicateStringLiterals,Replace duplicate `String` literals,"Replaces `String` literals with a length of 5 or greater repeated a minimum of 3 times. Qualified `String` literals include final Strings, method invocations, and new class invocations. Adds a new `private static final String` or uses an existing equivalent class field. A new variable name will be generated based on the literal value if an existing field does not exist. The generated name will append a numeric value to the variable name if a name already exists in the compilation unit. Centralizing repeated string values into constants makes refactoring safer and reduces the risk of inconsistent updates.",1,,Static analysis and remediation,,Remediations for issues identified by SAST tools.,"[{""name"":""includeTestSources"",""type"":""Boolean"",""displayName"":""Apply recipe to test source set"",""description"":""Changes only apply to main by default. `includeTestSources` will apply the recipe to `test` source files.""}]",
 maven,org.openrewrite.recipe:rewrite-static-analysis,org.openrewrite.staticanalysis.ReplaceHashtableWithHashMap,Replace `java.util.Hashtable` with `java.util.HashMap`,"`Hashtable` synchronizes every operation, which adds overhead in the common single-threaded case. This recipe replaces a local `Hashtable` with a `HashMap` when data flow analysis can prove the `Hashtable` 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, escaping variables, and `Hashtable`-specific method usages (`contains`, `elements`, `keys`) are left untouched. `HashMap` permits `null` keys and values, so it accepts every input `Hashtable` did.",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.ReplaceLambdaWithMethodReference,Use method references in lambda,"Replaces the single statement lambdas `o -> o instanceOf X`, `o -> (A) o`, `o -> System.out.println(o)`, `o -> o != null`, `o -> o == null` with the equivalent method reference. Method references are often more concise and readable than their lambda equivalents, making the code's intent clearer at a glance.",1,,Static analysis and remediation,,Remediations for issues identified by SAST tools.,,