Refactor SkylarkNestedSet type checks and tests

Moved some tests, fixed formatting, changed to use assertThat().

--
PiperOrigin-RevId: 144356402
MOS_MIGRATED_REVID=144356402

4 files changed, 141 insertions, 82 deletions

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 fc78da5385..b4cab445b5 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
@@ -31,7 +31,17 @@ import java.util.List;
 import java.util.Set;
 import javax.annotation.Nullable;
 
-/** A generic type safe NestedSet wrapper for Skylark. */
+/**
+ * A generic, type-safe {@link NestedSet} wrapper for Skylark.
+ *
+ * <p>The content type of a {@code SkylarkNestedSet} is the intersection of the {@link SkylarkType}
+ * of each of its elements. It is an error if this intersection is {@link SkylarkType#BOTTOM}. An
+ * empty set has a content type of {@link SkylarkType#TOP}.
+ *
+ * <p>It is also an error if this type has a non-bottom intersection with {@link SkylarkType#DICT}
+ * or {@link SkylarkType#LIST}, unless the set is empty.
+ * TODO(bazel-team): Decide whether this restriction is still useful.
+ */
 @SkylarkModule(
     name = "depset",
     category = SkylarkModuleCategory.BUILTIN,
@@ -117,13 +127,13 @@ public final class SkylarkNestedSet implements Iterable<Object>, SkylarkValue, S
     if (item instanceof SkylarkNestedSet) {
       SkylarkNestedSet nestedSet = (SkylarkNestedSet) item;
       if (!nestedSet.isEmpty()) {
-        contentType = checkType(contentType, nestedSet.contentType, loc);
+        contentType = getTypeAfterInsert(contentType, nestedSet.contentType, loc);
         transitiveItems.add(nestedSet.set);
       }
     } else if (item instanceof SkylarkList) {
       // TODO(bazel-team): we should check ImmutableList here but it screws up genrule at line 43
       for (Object object : (SkylarkList) item) {
-        contentType = checkType(contentType, SkylarkType.of(object.getClass()), loc);
+        contentType = getTypeAfterInsert(contentType, SkylarkType.of(object.getClass()), loc);
         checkImmutable(object, loc);
         items.add(object);
       }
@@ -184,24 +194,48 @@ public final class SkylarkNestedSet implements Iterable<Object>, SkylarkValue, S
     this(SkylarkType.of(contentType), set);
   }
 
-  private static SkylarkType checkType(SkylarkType builderType, SkylarkType itemType, Location loc)
+  private static final SkylarkType DICT_LIST_UNION =
+      SkylarkType.Union.of(SkylarkType.DICT, SkylarkType.LIST);
+
+  /**
+   * Throws EvalException if a type overlaps with DICT or LIST.
+   */
+  private static void checkTypeNotDictOrList(SkylarkType type, Location loc)
       throws EvalException {
-    if (SkylarkType.intersection(
-        SkylarkType.Union.of(SkylarkType.DICT, SkylarkType.LIST),
-        itemType) != SkylarkType.BOTTOM) {
+    if (SkylarkType.intersection(DICT_LIST_UNION, type) != SkylarkType.BOTTOM) {
       throw new EvalException(
-          loc, String.format("depsets cannot contain items of type '%s'", itemType));
+          loc, String.format("depsets cannot contain items of type '%s'", type));
     }
-    SkylarkType newType = SkylarkType.intersection(builderType, itemType);
-    if (newType == SkylarkType.BOTTOM) {
+  }
+
+  /**
+   * Returns the intersection of two types, and throws EvalException if the intersection is bottom.
+   */
+  private static SkylarkType commonNonemptyType(
+      SkylarkType depsetType, SkylarkType itemType, Location loc) throws EvalException {
+    SkylarkType resultType = SkylarkType.intersection(depsetType, itemType);
+    if (resultType == SkylarkType.BOTTOM) {
       throw new EvalException(
           loc,
           String.format(
-              "cannot add an item of type '%s' to a depset of '%s'", itemType, builderType));
+              "cannot add an item of type '%s' to a depset of '%s'", itemType, depsetType));
     }
-    return newType;
+    return resultType;
   }
 
+  /**
+   * Checks that an item type is allowed in a given set type, and returns the type of a new depset
+   * with that item inserted.
+   */
+  private static SkylarkType getTypeAfterInsert(
+      SkylarkType depsetType, SkylarkType itemType, Location loc) throws EvalException {
+    checkTypeNotDictOrList(itemType, loc);
+    return commonNonemptyType(depsetType, itemType, loc);
+  }
+
+  /**
+   * Throws EvalException if a given value is mutable.
+   */
   private static void checkImmutable(Object o, Location loc) throws EvalException {
     if (!EvalUtils.isImmutable(o)) {
       throw new EvalException(loc, "depsets cannot contain mutable items");
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkType.java b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkType.java
index 09f2467e70..ffef285f26 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkType.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkType.java
@@ -156,7 +156,6 @@ public abstract class SkylarkType implements Serializable {
   // TODO(bazel-team): resolve this inconsistency, one way or the other.
   public static final Simple NONE = Simple.of(Runtime.NoneType.class);
 
-  private static final class Global {}
   /** The STRING type, for strings */
   public static final Simple STRING = Simple.of(String.class);
 
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/MethodLibraryTest.java b/src/test/java/com/google/devtools/build/lib/syntax/MethodLibraryTest.java
index b2910a25c0..e0c6c99c24 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/MethodLibraryTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/MethodLibraryTest.java
@@ -15,9 +15,7 @@
 package com.google.devtools.build.lib.syntax;
 
 import static com.google.common.truth.Truth.assertThat;
-import static org.junit.Assert.assertEquals;
 
-import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.Iterables;
 import com.google.devtools.build.lib.syntax.SkylarkList.MutableList;
@@ -1410,56 +1408,6 @@ public class MethodLibraryTest extends EvaluationTestCase {
   }
 
   @Test
-  public void testSetUnionWithList() throws Exception {
-    evaluateSet("depset([]).union(['a', 'b', 'c'])", "a", "b", "c");
-    evaluateSet("depset(['a']).union(['b', 'c'])", "a", "b", "c");
-    evaluateSet("depset(['a', 'b']).union(['c'])", "a", "b", "c");
-    evaluateSet("depset(['a', 'b', 'c']).union([])", "a", "b", "c");
-  }
-
-  @Test
-  public void testSetUnionWithSet() throws Exception {
-    evaluateSet("depset([]).union(depset(['a', 'b', 'c']))", "a", "b", "c");
-    evaluateSet("depset(['a']).union(depset(['b', 'c']))", "a", "b", "c");
-    evaluateSet("depset(['a', 'b']).union(depset(['c']))", "a", "b", "c");
-    evaluateSet("depset(['a', 'b', 'c']).union(depset([]))", "a", "b", "c");
-  }
-
-  @Test
-  public void testSetUnionDuplicates() throws Exception {
-    evaluateSet("depset(['a', 'b', 'c']).union(['a', 'b', 'c'])", "a", "b", "c");
-    evaluateSet("depset(['a', 'a', 'a']).union(['a', 'a'])", "a");
-
-    evaluateSet("depset(['a', 'b', 'c']).union(depset(['a', 'b', 'c']))", "a", "b", "c");
-    evaluateSet("depset(['a', 'a', 'a']).union(depset(['a', 'a']))", "a");
-  }
-
-  @Test
-  public void testSetUnionError() throws Exception {
-    new BothModesTest()
-        .testIfErrorContains("insufficient arguments received by union", "depset(['a']).union()")
-        .testIfErrorContains(
-            "method depset.union(new_elements: Iterable) is not applicable for arguments (string): "
-                + "'new_elements' is 'string', but should be 'Iterable'",
-            "depset(['a']).union('b')");
-  }
-
-  @Test
-  public void testSetUnionSideEffects() throws Exception {
-    eval(
-        "def func():",
-        "  n1 = depset(['a'])",
-        "  n2 = n1.union(['b'])",
-        "  return n1",
-        "n = func()");
-    assertEquals(ImmutableList.of("a"), ((SkylarkNestedSet) lookup("n")).toCollection());
-  }
-
-  private void evaluateSet(String statement, Object... expectedElements) throws Exception {
-    new BothModesTest().testCollection(statement, expectedElements);
-  }
-
-  @Test
   public void testListIndexMethod() throws Exception {
     new BothModesTest()
         .testStatement("['a', 'b', 'c'].index('a')", 0)
@@ -1753,6 +1701,8 @@ public class MethodLibraryTest extends EvaluationTestCase {
         .testStatement("type(str)", "function");
   }
 
+  // TODO(bazel-team): Move this into a new BazelLibraryTest.java file, or at least out of
+  // MethodLibraryTest.java.
   @Test
   public void testSelectFunction() throws Exception {
     enableSkylarkMode();
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/SkylarkNestedSetTest.java b/src/test/java/com/google/devtools/build/lib/syntax/SkylarkNestedSetTest.java
index b9fd9ac951..3f06c5495a 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/SkylarkNestedSetTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/SkylarkNestedSetTest.java
@@ -14,7 +14,6 @@
 package com.google.devtools.build.lib.syntax;
 
 import static com.google.common.truth.Truth.assertThat;
-import static org.junit.Assert.assertEquals;
 
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
@@ -52,19 +51,87 @@ public class SkylarkNestedSetTest extends EvaluationTestCase {
   @Test
   public void testNsetOrder() throws Exception {
     eval("n = depset(['a', 'b'], order='compile')");
-    assertEquals(Order.COMPILE_ORDER, get("n").getSet(String.class).getOrder());
+    assertThat(get("n").getSet(String.class).getOrder()).isEqualTo(Order.COMPILE_ORDER);
   }
 
   @Test
   public void testEmptyNsetGenericType() throws Exception {
     eval("n = depset()");
-    assertEquals(SkylarkType.TOP, get("n").getContentType());
+    assertThat(get("n").getContentType()).isEqualTo(SkylarkType.TOP);
+  }
+
+  @Test
+  public void testDepsetBadOrder() throws Exception {
+    new BothModesTest().testIfExactError("Invalid order: bad", "depset(['a'], order='bad')");
+  }
+
+  @Test
+  public void testSetUnionWithList() throws Exception {
+    assertContainsInOrder("depset([]).union(['a', 'b', 'c'])", "a", "b", "c");
+    assertContainsInOrder("depset(['a']).union(['b', 'c'])", "a", "b", "c");
+    assertContainsInOrder("depset(['a', 'b']).union(['c'])", "a", "b", "c");
+    assertContainsInOrder("depset(['a', 'b', 'c']).union([])", "a", "b", "c");
+  }
+
+  @Test
+  public void testSetUnionWithSet() throws Exception {
+    assertContainsInOrder("depset([]).union(depset(['a', 'b', 'c']))", "a", "b", "c");
+    assertContainsInOrder("depset(['a']).union(depset(['b', 'c']))", "a", "b", "c");
+    assertContainsInOrder("depset(['a', 'b']).union(depset(['c']))", "a", "b", "c");
+    assertContainsInOrder("depset(['a', 'b', 'c']).union(depset([]))", "a", "b", "c");
+  }
+
+  @Test
+  public void testDepsetUnionWithBadType() throws Exception {
+    new BothModesTest()
+        .testIfErrorContains("is not applicable for arguments (int)", "depset([]).union(5)");
+  }
+
+  @Test
+  public void testSetUnionDuplicates() throws Exception {
+    assertContainsInOrder("depset(['a', 'b', 'c']).union(['a', 'b', 'c'])", "a", "b", "c");
+    assertContainsInOrder("depset(['a', 'a', 'a']).union(['a', 'a'])", "a");
+
+    assertContainsInOrder("depset(['a', 'b', 'c']).union(depset(['a', 'b', 'c']))", "a", "b", "c");
+    assertContainsInOrder("depset(['a', 'a', 'a']).union(depset(['a', 'a']))", "a");
+  }
+
+  @Test
+  public void testSetUnionError() throws Exception {
+    new BothModesTest()
+        .testIfErrorContains("insufficient arguments received by union", "depset(['a']).union()")
+        .testIfErrorContains(
+            "method depset.union(new_elements: Iterable) is not applicable for arguments (string): "
+                + "'new_elements' is 'string', but should be 'Iterable'",
+            "depset(['a']).union('b')");
+  }
+
+  @Test
+  public void testSetUnionSideEffects() throws Exception {
+    eval(
+        "def func():",
+        "  n1 = depset(['a'])",
+        "  n2 = n1.union(['b'])",
+        "  return n1",
+        "n = func()");
+    assertThat(((SkylarkNestedSet) lookup("n")).toCollection()).isEqualTo(ImmutableList.of("a"));
+  }
+
+  private void assertContainsInOrder(String statement, Object... expectedElements)
+      throws Exception {
+    new BothModesTest().testCollection(statement, expectedElements);
   }
 
   @Test
   public void testFunctionReturnsNset() throws Exception {
-    eval("def func():", "  n = depset()", "  n += ['a']", "  return n", "s = func()");
-    assertEquals(ImmutableList.of("a"), get("s").toCollection());
+    eval(
+        "def func():",
+        "  n = depset()",
+        "  n += ['a']",
+        "  return n",
+        "s = func()");
+    assertThat(get("s")).isInstanceOf(SkylarkNestedSet.class);
+    assertThat(get("s").toCollection()).containsExactly("a");
   }
 
   @Test
@@ -77,11 +144,11 @@ public class SkylarkNestedSetTest extends EvaluationTestCase {
         "  n2 += ['b']",
         "  return n1",
         "n = func()");
-    assertEquals(ImmutableList.of("a"), get("n").toCollection());
+    assertThat(get("n").toCollection()).containsExactly("a");
   }
 
   @Test
-  public void testNsetNestedItem() throws Exception {
+  public void testNsetUnionOrder() throws Exception {
     eval(
         "def func():",
         "  n1 = depset()",
@@ -91,7 +158,7 @@ public class SkylarkNestedSetTest extends EvaluationTestCase {
         "  n1 += n2",
         "  return n1",
         "n = func()");
-    assertEquals(ImmutableList.of("b", "a"), get("n").toCollection());
+    assertThat(get("n").toCollection()).containsExactly("b", "a").inOrder();
   }
 
   @Test
@@ -103,8 +170,13 @@ public class SkylarkNestedSetTest extends EvaluationTestCase {
 
   @Test
   public void testNsetItemList() throws Exception {
-    eval("def func():", "  n = depset()", "  n += ['a', 'b']", "  return n", "n = func()");
-    assertEquals(ImmutableList.of("a", "b"), get("n").toCollection());
+    eval(
+        "def func():",
+        "  n = depset()",
+        "  n += ['a', 'b']",
+        "  return n",
+        "n = func()");
+    assertThat(get("n").toCollection()).containsExactly("a", "b").inOrder();
   }
 
   @Test
@@ -118,7 +190,7 @@ public class SkylarkNestedSetTest extends EvaluationTestCase {
         "  func1(n)",
         "  return n",
         "n = func2()");
-    assertEquals(ImmutableList.of("a"), get("n").toCollection());
+    assertThat(get("n").toCollection()).containsExactly("a");
   }
 
   @Test
@@ -131,7 +203,7 @@ public class SkylarkNestedSetTest extends EvaluationTestCase {
         "  return depset() + nb + nc",
         "n = func()");
     // The iterator lists the Transitive sets first
-    assertEquals(ImmutableList.of("b", "a", "c"), get("n").toCollection());
+    assertThat(get("n").toCollection()).containsExactly("b", "a", "c").inOrder();
   }
 
   @Test
@@ -145,7 +217,7 @@ public class SkylarkNestedSetTest extends EvaluationTestCase {
         "  return na",
         "n = func()");
     // The iterator lists the Transitive sets first
-    assertEquals(ImmutableList.of(4, 2, 3, 5), get("n").toCollection());
+    assertThat(get("n").toCollection()).containsExactly(4, 2, 3, 5).inOrder();
   }
 
   @Test
@@ -160,14 +232,18 @@ public class SkylarkNestedSetTest extends EvaluationTestCase {
 
   @Test
   public void testNsetToString() throws Exception {
-    eval("s = depset() + [2, 4, 6] + [3, 4, 5]", "x = str(s)");
-    assertEquals("set([2, 4, 6, 3, 5])", lookup("x"));
+    eval(
+        "s = depset() + [2, 4, 6] + [3, 4, 5]",
+        "x = str(s)");
+    assertThat(lookup("x")).isEqualTo("set([2, 4, 6, 3, 5])");
   }
 
   @Test
   public void testNsetToStringWithOrder() throws Exception {
-    eval("s = depset(order = 'link') + [2, 4, 6] + [3, 4, 5]", "x = str(s)");
-    assertEquals("set([2, 4, 6, 3, 5], order = \"link\")", lookup("x"));
+    eval(
+        "s = depset(order = 'link') + [2, 4, 6] + [3, 4, 5]",
+        "x = str(s)");
+    assertThat(lookup("x")).isEqualTo("set([2, 4, 6, 3, 5], order = \"link\")");
   }
 
   @SuppressWarnings("unchecked")