Fix crash when unioning depsets with different orders

Also refactor FAIL_FAST_HANDLER to throw something more specific than IllegalArgumentException. This bug was masked because the test assertion that would've caught it considered IllegalArgumentException to be an expected error, the same as EvalException.

RELNOTES: None
PiperOrigin-RevId: 161809957

6 files changed, 30 insertions, 16 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleImplementationFunctions.java b/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleImplementationFunctions.java
index 3eb1c062cf..f565442c22 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleImplementationFunctions.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleImplementationFunctions.java
@@ -496,7 +496,8 @@ public class SkylarkRuleImplementationFunctions {
         // Also, allow empty set for init
         @Param(name = "transitive_files", type = SkylarkNestedSet.class, generic1 = Artifact.class,
             noneable = true, defaultValue = "None",
-            doc = "The (transitive) set of files to be added to the runfiles."),
+            doc = "The (transitive) set of files to be added to the runfiles. The depset should "
+            + "use the `default` order (which, as the name implies, is the default)."),
         @Param(name = "collect_data", type = Boolean.class, defaultValue = "False",
             doc = "Whether to collect the data "
             + "runfiles from the dependencies in srcs, data and deps attributes."),
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/BinaryOperatorExpression.java b/src/main/java/com/google/devtools/build/lib/syntax/BinaryOperatorExpression.java
index e4e4b01970..459858aad4 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/BinaryOperatorExpression.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/BinaryOperatorExpression.java
@@ -273,7 +273,7 @@ public final class BinaryOperatorExpression extends Expression {
       }
     }
 
-    // TODO(bazel-team): Remove this case. Union of sets should use '|' instead of '+'.
+    // TODO(bazel-team): Deprecate + and | on depsets. Needs new API design.
     if (lval instanceof SkylarkNestedSet) {
       return new SkylarkNestedSet((SkylarkNestedSet) lval, rval, location);
     }
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Environment.java b/src/main/java/com/google/devtools/build/lib/syntax/Environment.java
index 9b17cead89..38cbb7c35f 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/Environment.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/Environment.java
@@ -940,16 +940,28 @@ public final class Environment implements Freezable {
     }
   }
 
+  /** An exception thrown by {@link #FAIL_FAST_HANDLER}. */
+  // TODO(bazel-team): Possibly extend RuntimeException instead of IllegalArgumentException.
+  public static class FailFastException extends IllegalArgumentException {
+    public FailFastException(String s) {
+      super(s);
+    }
+  }
 
   /**
-   * The fail fast handler, which throws an {@link IllegalArgumentException} whenever an error or
-   * warning occurs.
+   * A handler that immediately throws {@link FailFastException} whenever an error or warning
+   * occurs.
+   *
+   * We do not reuse an existing unchecked exception type, because callers (e.g., test assertions)
+   * need to be able to distinguish between organically occurring exceptions and exceptions thrown
+   * by this handler.
    */
   public static final EventHandler FAIL_FAST_HANDLER = new EventHandler() {
-      @Override
-      public void handle(Event event) {
-        Preconditions.checkArgument(
-            !EventKind.ERRORS_AND_WARNINGS.contains(event.getKind()), event);
+    @Override
+    public void handle(Event event) {
+      if (EventKind.ERRORS_AND_WARNINGS.contains(event.getKind())) {
+        throw new FailFastException(event.toString());
       }
-    };
+    }
+  };
 }
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkNestedSet.java b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkNestedSet.java
index 12a01326da..c38fe89b6a 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkNestedSet.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkNestedSet.java
@@ -159,7 +159,8 @@ public final class SkylarkNestedSet implements SkylarkValue, SkylarkQueryable {
       for (NestedSet<?> nestedSet : transitiveItems) {
         builder.addTransitive(nestedSet);
       }
-    } catch (IllegalStateException e) {
+    } catch (IllegalArgumentException e) {
+      // Order mismatch between item and builder.
       throw new EvalException(loc, e.getMessage());
     }
     this.set = builder.build();
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/SkylarkEvaluationTest.java b/src/test/java/com/google/devtools/build/lib/syntax/SkylarkEvaluationTest.java
index 1cd8a98c62..c9b8d1c4e0 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/SkylarkEvaluationTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/SkylarkEvaluationTest.java
@@ -635,7 +635,7 @@ public class SkylarkEvaluationTest extends EvaluationTest {
   private void flowStatementInsideFunction(String statement) throws Exception {
     checkEvalErrorContains(statement + " statement must be inside a for loop",
         "def foo():",
-        "  " + statement + "",
+        "  " + statement,
         "x = foo()");
   }
 
@@ -644,7 +644,7 @@ public class SkylarkEvaluationTest extends EvaluationTest {
         "def foo2():",
         "   for i in range(0, 3):",
         "      pass",
-        "   " + statement + "",
+        "   " + statement,
         "y = foo2()");
   }
 
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/util/EvaluationTestCase.java b/src/test/java/com/google/devtools/build/lib/syntax/util/EvaluationTestCase.java
index 70604c6349..0fa14bcec0 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/util/EvaluationTestCase.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/util/EvaluationTestCase.java
@@ -26,6 +26,7 @@ import com.google.devtools.build.lib.events.util.EventCollectionApparatus;
 import com.google.devtools.build.lib.syntax.BazelLibrary;
 import com.google.devtools.build.lib.syntax.BuildFileAST;
 import com.google.devtools.build.lib.syntax.Environment;
+import com.google.devtools.build.lib.syntax.Environment.FailFastException;
 import com.google.devtools.build.lib.syntax.Environment.Phase;
 import com.google.devtools.build.lib.syntax.EvalException;
 import com.google.devtools.build.lib.syntax.Expression;
@@ -201,12 +202,11 @@ public class EvaluationTestCase {
   }
 
   public void checkEvalError(String msg, String... input) throws Exception {
-    setFailFast(true);
     try {
       eval(input);
       fail("Expected error '" + msg + "' but got no error");
-    } catch (IllegalArgumentException | EvalException e) {
-      assertThat(e).hasMessage(msg);
+    } catch (EvalException | FailFastException e) {
+      assertThat(e).hasMessageThat().isEqualTo(msg);
     }
   }
 
@@ -214,7 +214,7 @@ public class EvaluationTestCase {
     try {
       eval(input);
       fail("Expected error containing '" + msg + "' but got no error");
-    } catch (IllegalArgumentException | EvalException e) {
+    } catch (EvalException | FailFastException e) {
       assertThat(e).hasMessageThat().contains(msg);
     }
   }