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, 73 insertions, 1 deletions

diff --git a/src/test/java/com/google/devtools/build/lib/actions/CommandLineItemSimpleFormatterTest.java b/src/test/java/com/google/devtools/build/lib/actions/CommandLineItemSimpleFormatterTest.java
new file mode 100644
index 0000000000..d666c7a629
--- /dev/null
+++ b/src/test/java/com/google/devtools/build/lib/actions/CommandLineItemSimpleFormatterTest.java
@@ -0,0 +1,54 @@
+// 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;
+
+import static com.google.common.truth.Truth.assertThat;
+import static com.google.devtools.build.lib.actions.CommandLineItemSimpleFormatter.format;
+import static com.google.devtools.build.lib.actions.CommandLineItemSimpleFormatter.isValid;
+
+import com.google.devtools.build.lib.testutil.MoreAsserts;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.JUnit4;
+
+/** Tests for {@link CommandLineItemSimpleFormatter} */
+@RunWith(JUnit4.class)
+public class CommandLineItemSimpleFormatterTest {
+
+  @Test
+  public void testValidate() {
+    assertThat(isValid("hello %s")).isTrue();
+    assertThat(isValid("%s hello")).isTrue();
+    assertThat(isValid("hello %%s")).isFalse();
+    assertThat(isValid("hello %%s %s")).isTrue();
+    assertThat(isValid("hello %s %%s")).isTrue();
+    assertThat(isValid("hello %s %s")).isFalse();
+    assertThat(isValid("%s hello %s")).isFalse();
+    assertThat(isValid("hello %")).isFalse();
+    assertThat(isValid("hello %f")).isFalse();
+    assertThat(isValid("hello %s %f")).isFalse();
+    assertThat(isValid("hello %s %")).isFalse();
+  }
+
+  @Test
+  public void testFormat() {
+    assertThat(format("hello %s", "world")).isEqualTo("hello world");
+    assertThat(format("%s hello", "world")).isEqualTo("world hello");
+    assertThat(format("hello %s, hello", "world")).isEqualTo("hello world, hello");
+    assertThat(format("hello %s, hello", "world")).isEqualTo("hello world, hello");
+    assertThat(format("hello %%s %s", "world")).isEqualTo("hello %%s world");
+    MoreAsserts.assertThrows(IllegalArgumentException.class, () -> format("hello", "hello"));
+    MoreAsserts.assertThrows(IllegalArgumentException.class, () -> format("hello %s %s", "hello"));
+  }
+}
diff --git a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleImplementationFunctionsTest.java b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleImplementationFunctionsTest.java
index 918dc6d6c7..36affa50cf 100644
--- a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleImplementationFunctionsTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleImplementationFunctionsTest.java
@@ -2284,7 +2284,8 @@ public class SkylarkRuleImplementationFunctionsTest extends SkylarkTestCase {
   }
 
   @Test
-  public void testLazyArgIllegalFormatString() throws Exception {
+  public void testLazyArgIllegalLegacyFormatString() throws Exception {
+    setSkylarkSemanticsOptions("--incompatible_disallow_old_style_args_add=false");
     SkylarkRuleContext ruleContext = createRuleContext("//foo:foo");
     evalRuleContextCode(
         ruleContext,
@@ -2309,6 +2310,23 @@ public class SkylarkRuleImplementationFunctionsTest extends SkylarkTestCase {
   }
 
   @Test
+  public void testLazyArgIllegalFormatString() throws Exception {
+    setSkylarkSemanticsOptions("--incompatible_disallow_old_style_args_add=true");
+    SkylarkRuleContext ruleContext = createRuleContext("//foo:foo");
+    checkError(
+        ruleContext,
+        "Invalid value for parameter \"format\": Expected string with a single \"%s\"",
+        "args = ruleContext.actions.args()",
+        "args.add('foo', format='illegal_format')", // Expects two args, will only be given one
+        "ruleContext.actions.run(",
+        "  inputs = depset(ruleContext.files.srcs),",
+        "  outputs = ruleContext.files.srcs,",
+        "  arguments = [args],",
+        "  executable = ruleContext.files.tools[0],",
+        ")");
+  }
+
+  @Test
   public void testLazyArgMapEachWrongArgCount() throws Exception {
     SkylarkRuleContext ruleContext = createRuleContext("//foo:foo");
     checkErrorContains(