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

2 files changed, 52 insertions, 26 deletions

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));