Update format implementation in ctx.actions.args.

Since we're only supporting a single %s anyway, make this explicit in the docs, fail in the analysis phase, and use a more efficient format method in the implementation.

RELNOTES: None
PiperOrigin-RevId: 193099585

5 files changed, 163 insertions, 32 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/actions/CommandLineItemSimpleFormatter.java b/src/main/java/com/google/devtools/build/lib/actions/CommandLineItemSimpleFormatter.java
new file mode 100644
index 0000000000..462fe8278c
--- /dev/null
+++ b/src/main/java/com/google/devtools/build/lib/actions/CommandLineItemSimpleFormatter.java
@@ -0,0 +1,102 @@
+// Copyright 2018 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+package com.google.devtools.build.lib.actions;
+
+/**
+ * Implementation of a formatter that supports only a single '%s'
+ *
+ * <p>This implementation is used in command line item expansions that use formatting. We use a
+ * custom implementation to improve performance and avoid GC.
+ */
+public class CommandLineItemSimpleFormatter {
+
+  /**
+   * Returns true if the format string contains a single '%s'.
+   *
+   * <p>%% escape sequences are supported.
+   */
+  public static boolean isValid(String formatStr) {
+    return onlyOccurrence(formatStr, formatStr.length()) != -1;
+  }
+
+  /**
+   * Returns the equivalent result of <code>String.format(formatStr, subject)</code>, under the
+   * assumption that the format string contains a single %s.
+   *
+   * <p>Use {@link #isValid} to validate the format string.
+   *
+   * @throws IllegalArgumentException if the format string is invalid.
+   */
+  public static String format(String formatStr, String subject) {
+    int n = formatStr.length();
+    int idx = onlyOccurrence(formatStr, n);
+    if (idx < 0) {
+      throw new IllegalArgumentException(
+          "Expected format string with single '%s', found: " + formatStr);
+    }
+    return new StringBuilder(n + subject.length() - 2)
+        .append(formatStr, 0, idx)
+        .append(subject)
+        .append(formatStr, idx + 2, n)
+        .toString();
+  }
+
+  /*
+   * Returns the index of the only occurrence of %s. Skips any %%. Returns -1 if {@code formatStr}
+   * is malformed.
+   */
+  private static int onlyOccurrence(String formatStr, int n) {
+    int idx = nextOccurrence(formatStr, n, 0);
+    if (idx < 0) {
+      return -1;
+    }
+    // Only one occurrence please
+    if (nextOccurrence(formatStr, n, idx + 2) != -1) {
+      return -1;
+    }
+    return idx;
+  }
+
+  /**
+   * Returns next occurrence of %s. Skips any %%.
+   *
+   * @return
+   *     <li>[0-n]: Index of next %s
+   *     <li>-1: No %s found until end of string
+   *     <li>-2: Illegal sequence found, eg. %f
+   */
+  private static int nextOccurrence(String formatStr, int n, int idx) {
+    while (idx < n) {
+      idx = formatStr.indexOf('%', idx);
+      if (idx == -1) {
+        break;
+      }
+      if ((idx + 1) < n) {
+        char c = formatStr.charAt(idx + 1);
+        if (c == 's') {
+          return idx;
+        }
+        if (c != '%') {
+          // Illegal sequence found
+          return -2;
+        }
+        idx += 2;
+      } else {
+        // Terminating '%' found, illegal
+        return -2;
+      }
+    }
+    return -1;
+  }
+}
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/actions/CustomCommandLine.java b/src/main/java/com/google/devtools/build/lib/analysis/actions/CustomCommandLine.java
index 1fa50a6628..6f04a0eb6f 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/actions/CustomCommandLine.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/actions/CustomCommandLine.java
@@ -28,6 +28,7 @@ import com.google.devtools.build.lib.actions.Artifact.ArtifactExpander;
 import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact;
 import com.google.devtools.build.lib.actions.CommandLine;
 import com.google.devtools.build.lib.actions.CommandLineItem;
+import com.google.devtools.build.lib.actions.CommandLineItemSimpleFormatter;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.collect.nestedset.NestedSet;
 import com.google.devtools.build.lib.concurrent.BlazeInterners;
@@ -239,7 +240,7 @@ public final class CustomCommandLine extends CommandLine {
       return new Builder().each(values);
     }
 
-    /** Each argument is formatted via {@link String#format}. */
+    /** Each argument is formatted via {@link CommandLineItemSimpleFormatter#format}. */
     public static Builder format(@CompileTimeConstant String formatEach) {
       return new Builder().format(formatEach);
     }
@@ -259,7 +260,7 @@ public final class CustomCommandLine extends CommandLine {
       private String beforeEach;
       private String joinWith;
 
-      /** Each argument is formatted via {@link String#format}. */
+      /** Each argument is formatted via {@link CommandLineItemSimpleFormatter#format}. */
       public Builder format(@CompileTimeConstant String formatEach) {
         Preconditions.checkNotNull(formatEach);
         this.formatEach = formatEach;
@@ -402,7 +403,8 @@ public final class CustomCommandLine extends CommandLine {
         if (hasFormatEach) {
           String formatStr = (String) arguments.get(argi++);
           for (int i = 0; i < count; ++i) {
-            mutatedValues.set(i, String.format(formatStr, mutatedValues.get(i)));
+            mutatedValues.set(
+                i, CommandLineItemSimpleFormatter.format(formatStr, mutatedValues.get(i)));
           }
         }
         if (hasBeforeEach) {
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java b/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java
index 9dbe7be02a..8f9e18aa97 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java
@@ -39,6 +39,7 @@ import com.google.devtools.build.lib.actions.BaseSpawn;
 import com.google.devtools.build.lib.actions.CommandAction;
 import com.google.devtools.build.lib.actions.CommandLine;
 import com.google.devtools.build.lib.actions.CommandLineExpansionException;
+import com.google.devtools.build.lib.actions.CommandLineItemSimpleFormatter;
 import com.google.devtools.build.lib.actions.CompositeRunfilesSupplier;
 import com.google.devtools.build.lib.actions.EmptyRunfilesSupplier;
 import com.google.devtools.build.lib.actions.ExecException;
@@ -1384,9 +1385,9 @@ public class SpawnAction extends AbstractAction implements ExecutionInfoSpecifie
         } else if (value instanceof Artifact) {
           Artifact paramFile = (Artifact) value;
           String flagFormatString = (String) values[++i];
-          // TODO(bazel-team): Should probably use something that recognizes %% escapes. Possibly
-          // SkylarkPrinter#formatWithList, though that's kinda heavyweight for this usage.
-          result.add(flagFormatString.replaceFirst("%s", paramFile.getExecPathString()));
+          result.add(
+              CommandLineItemSimpleFormatter.format(
+                  flagFormatString, paramFile.getExecPathString()));
         } else if (value instanceof CommandLine) {
           CommandLine commandLine = (CommandLine) value;
           if (artifactExpander != null) {
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkActionFactory.java b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkActionFactory.java
index 691a1e23a9..a3878a6300 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkActionFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkActionFactory.java
@@ -21,6 +21,7 @@ import com.google.devtools.build.lib.actions.ActionAnalysisMetadata;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.ArtifactRoot;
 import com.google.devtools.build.lib.actions.CommandLine;
+import com.google.devtools.build.lib.actions.CommandLineItemSimpleFormatter;
 import com.google.devtools.build.lib.actions.ParamFileInfo;
 import com.google.devtools.build.lib.actions.ParameterFile.ParameterFileType;
 import com.google.devtools.build.lib.actions.RunfilesSupplier;
@@ -1144,6 +1145,7 @@ public class SkylarkActionFactory implements SkylarkValue {
             false /* uniquify */,
             null /* terminateWith */,
             loc);
+
       } else {
         if (mapFn != Runtime.NONE && skylarkSemantics.incompatibleDisallowOldStyleArgsAdd()) {
           throw new EvalException(
@@ -1254,7 +1256,8 @@ public class SkylarkActionFactory implements SkylarkValue {
           noneable = true,
           doc =
               "An optional format string pattern, applied to each string returned by the "
-                  + "<code>map_each</code> function."
+                  + "<code>map_each</code> function. "
+                  + "The format string must have exactly one '%s' placeholder."
         ),
         @Param(
           name = "before_each",
@@ -1423,7 +1426,9 @@ public class SkylarkActionFactory implements SkylarkValue {
           positional = false,
           defaultValue = "None",
           noneable = true,
-          doc = "An optional format string pattern applied to the joined string."
+          doc =
+              "An optional format string pattern applied to the joined string. "
+                  + "The format string must have exactly one '%s' placeholder."
         ),
         @Param(
           name = "omit_if_empty",
@@ -1511,9 +1516,9 @@ public class SkylarkActionFactory implements SkylarkValue {
         SkylarkList skylarkList = (SkylarkList) value;
         vectorArg = new SkylarkCustomCommandLine.VectorArg.Builder(skylarkList);
       }
-      if (mapEach != null) {
-        validateMapEach(mapEach, loc);
-      }
+      validateMapEach(mapEach, loc);
+      validateFormatString("format_each", formatEach);
+      validateFormatString("format_joined", formatJoined);
       vectorArg
           .setLocation(loc)
           .setArgName(argName)
@@ -1549,7 +1554,11 @@ public class SkylarkActionFactory implements SkylarkValue {
       }
     }
 
-    private void validateMapEach(BaseFunction mapEach, Location loc) throws EvalException {
+    private void validateMapEach(@Nullable BaseFunction mapEach, Location loc)
+        throws EvalException {
+      if (mapEach == null) {
+        return;
+      }
       Shape shape = mapEach.getSignature().getSignature().getShape();
       boolean valid =
           shape.getMandatoryPositionals() == 1
@@ -1562,11 +1571,22 @@ public class SkylarkActionFactory implements SkylarkValue {
       }
     }
 
-    private void addScalarArg(Object value, String format, BaseFunction mapFn, Location loc)
+    private void validateFormatString(String argumentName, @Nullable String formatStr)
         throws EvalException {
-      if (!EvalUtils.isImmutable(value)) {
-        throw new EvalException(null, "arg must be an immutable type");
+      if (formatStr != null
+          && skylarkSemantics.incompatibleDisallowOldStyleArgsAdd()
+          && !CommandLineItemSimpleFormatter.isValid(formatStr)) {
+        throw new EvalException(
+            null,
+            String.format(
+                "Invalid value for parameter \"%s\": Expected string with a single \"%%s\"",
+                argumentName));
       }
+    }
+
+    private void addScalarArg(Object value, String format, BaseFunction mapFn, Location loc)
+        throws EvalException {
+      validateFormatString("format", format);
       if (format == null && mapFn == null) {
         commandLine.add(value);
       } else {
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkCustomCommandLine.java b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkCustomCommandLine.java
index 2512907e18..a67223a98e 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkCustomCommandLine.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkCustomCommandLine.java
@@ -23,6 +23,7 @@ import com.google.devtools.build.lib.actions.ActionKeyContext;
 import com.google.devtools.build.lib.actions.CommandLine;
 import com.google.devtools.build.lib.actions.CommandLineExpansionException;
 import com.google.devtools.build.lib.actions.CommandLineItem;
+import com.google.devtools.build.lib.actions.CommandLineItemSimpleFormatter;
 import com.google.devtools.build.lib.collect.nestedset.NestedSet;
 import com.google.devtools.build.lib.concurrent.BlazeInterners;
 import com.google.devtools.build.lib.events.Location;
@@ -245,11 +246,11 @@ public class SkylarkCustomCommandLine extends CommandLine {
       }
       if ((features & HAS_FORMAT_EACH) != 0) {
         String formatStr = (String) arguments.get(argi++);
-        Formatter formatter = new Formatter(formatStr, skylarkSemantics, location);
+        Formatter formatter = Formatter.get(location, skylarkSemantics);
         try {
           int count = stringValues.size();
           for (int i = 0; i < count; ++i) {
-            stringValues.set(i, formatter.format(stringValues.get(i)));
+            stringValues.set(i, formatter.format(formatStr, stringValues.get(i)));
           }
         } catch (IllegalFormatException e) {
           throw new CommandLineExpansionException(errorMessage(e.getMessage(), location, null));
@@ -269,9 +270,9 @@ public class SkylarkCustomCommandLine extends CommandLine {
         if (!isEmptyAndShouldOmit) {
           String result = Joiner.on(joinWith).join(stringValues);
           if (formatJoined != null) {
-            Formatter formatter = new Formatter(formatJoined, skylarkSemantics, location);
+            Formatter formatter = Formatter.get(location, skylarkSemantics);
             try {
-              result = formatter.format(result);
+              result = formatter.format(formatJoined, result);
             } catch (IllegalFormatException e) {
               throw new CommandLineExpansionException(errorMessage(e.getMessage(), location, null));
             }
@@ -538,8 +539,8 @@ public class SkylarkCustomCommandLine extends CommandLine {
       String stringValue = CommandLineItem.expandToCommandLine(object);
       if (hasFormat) {
         String formatStr = (String) arguments.get(argi++);
-        Formatter formatter = new Formatter(formatStr, skylarkSemantics, location);
-        stringValue = formatter.format(stringValue);
+        Formatter formatter = Formatter.get(location, skylarkSemantics);
+        stringValue = formatter.format(formatStr, stringValue);
       }
       builder.add(stringValue);
       return argi;
@@ -696,26 +697,31 @@ public class SkylarkCustomCommandLine extends CommandLine {
     }
   }
 
-  private static class Formatter {
-    private final String formatStr;
-    private final SkylarkSemantics skylarkSemantics;
+  private interface Formatter {
+    String format(String formatStr, String subject) throws CommandLineExpansionException;
+
+    static Formatter get(Location location, SkylarkSemantics skylarkSemantics) {
+      return skylarkSemantics.incompatibleDisallowOldStyleArgsAdd()
+          ? CommandLineItemSimpleFormatter::format
+          : new LegacyFormatter(location);
+    }
+  }
+
+  private static class LegacyFormatter implements Formatter {
     @Nullable private final Location location;
     private final ArrayList<Object> args;
 
-    public Formatter(String formatStr, SkylarkSemantics skylarkSemantics, Location location) {
-      this.formatStr = formatStr;
-      this.skylarkSemantics = skylarkSemantics;
+    public LegacyFormatter(Location location) {
       this.location = location;
       this.args = new ArrayList<>(1); // Reused arg list to reduce GC
       this.args.add(null);
     }
 
-    String format(Object object) throws CommandLineExpansionException {
+    @Override
+    public String format(String formatStr, String subject) throws CommandLineExpansionException {
       try {
-        args.set(0, object);
-        SkylarkPrinter printer =
-            skylarkSemantics.incompatibleDisallowOldStyleArgsAdd()
-                ? Printer.getSimplifiedPrinter() : Printer.getPrinter();
+        args.set(0, subject);
+        SkylarkPrinter printer = Printer.getPrinter();
         return printer.formatWithList(formatStr, args).toString();
       } catch (IllegalFormatException e) {
         throw new CommandLineExpansionException(errorMessage(e.getMessage(), location, null));