Throw an EvalException when run_shell is passed both a command string sequence and arguments.

Previously, this resulted in an uncaught UnsupportedOperationException.

Closes #5892.

RELNOTES: None.
PiperOrigin-RevId: 208865301

3 files changed, 19 insertions, 7 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 72e2a5f802..755a909214 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
@@ -290,7 +290,10 @@ public class SkylarkActionFactory implements SkylarkActionFactoryApi {
       }
     } else if (commandUnchecked instanceof SkylarkList) {
       SkylarkList commandList = (SkylarkList) commandUnchecked;
-
+      if (argumentList.size() > 0) {
+        throw new EvalException(location,
+            "'arguments' must be empty if 'command' is a sequence of strings");
+      }
       @SuppressWarnings("unchecked")
       List<String> command = commandList.getContents(String.class, "command");
       builder.setShellCommand(command);
diff --git a/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/SkylarkActionFactoryApi.java b/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/SkylarkActionFactoryApi.java
index 15d940ea74..282890de33 100644
--- a/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/SkylarkActionFactoryApi.java
+++ b/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/SkylarkActionFactoryApi.java
@@ -369,12 +369,11 @@ public interface SkylarkActionFactoryApi extends SkylarkValue {
             positional = false,
             doc =
                 "Shell command to execute.<br><br>"
-                    + "<b>Passing a sequence of strings to this attribute is deprecated and Blaze"
-                    + "may "
-                    + "stop accepting such values in the future.</b><br><br>"
-                    + "The command can access the elements of the <code>arguments</code> object "
-                    + "via "
-                    + "<code>$1</code>, <code>$2</code>, etc.<br>"
+                    + "<b>Passing a sequence of strings to this attribute is deprecated and Blaze "
+                    + "may stop accepting such values in the future.</b> If a sequence of strings "
+                    + "is passed, then <code>arguments</code> must not be used.<br><br> "
+                    + "The command string can access the elements of the <code>arguments</code> "
+                    + "object via <code>$1</code>, <code>$2</code>, etc.<br> "
                     + "When this argument is a string, it must be a valid shell command. For "
                     + "example: "
                     + "\"<code>echo foo > $1</code>\". Blaze uses the same shell to execute the "
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 92e384a4b7..6ab6fe42dd 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
@@ -400,6 +400,16 @@ public class SkylarkRuleImplementationFunctionsTest extends SkylarkTestCase {
         "  command = 'dummy_command')");
   }
 
+  @Test
+  public void testRunShellArgumentsWithCommandSequence() throws Exception {
+    checkErrorContains(
+        createRuleContext("//foo:foo"),
+        "'arguments' must be empty if 'command' is a sequence of strings",
+        "ruleContext.actions.run_shell(outputs = ruleContext.files.srcs,",
+        "  command = [\"echo\", \"'hello world'\", \"&&\", \"touch\"],",
+        "  arguments = [ruleContext.files.srcs[0].path])");
+  }
+
   private void setupToolInInputsTest(String... ruleImpl) throws Exception {
     ImmutableList.Builder<String> lines = ImmutableList.builder();
     lines.add("def _main_rule_impl(ctx):");