Refactor SkylarkNestedSet to not implement Iterable

This is not intended to be a user-visible semantic change, aside from error messages.

This is to help avoid unintentional flattening of depsets, and to narrow down the number of call sites where this can occur, to help us print warning/deprecation messages.

EvalUtils#toIterable will now return an ImmutableList in place of SkylarkNestedSet. This should be ok since the caller shouldn't be relying on the result being a Skylark-safe type. Code that takes Iterable because it accepts either a list or set, can instead be changed to take Object and use EvalUtils#toIterableStrict for validation.

Note that NestedSet still implements Iterable, so native code can still easily and accidentally flatten sets.

--
PiperOrigin-RevId: 145044023
MOS_MIGRATED_REVID=145044023

9 files changed, 130 insertions, 58 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/collect/nestedset/NestedSet.java b/src/main/java/com/google/devtools/build/lib/collect/nestedset/NestedSet.java
index 39265a6875..9f5b9d97c5 100644
--- a/src/main/java/com/google/devtools/build/lib/collect/nestedset/NestedSet.java
+++ b/src/main/java/com/google/devtools/build/lib/collect/nestedset/NestedSet.java
@@ -151,7 +151,7 @@ public final class NestedSet<E> implements Iterable<E> {
   }
 
   /**
-   * Returns true if the set is empty.
+   * Returns true if the set is empty. Runs in O(1) time (i.e. does not flatten the set).
    */
   public boolean isEmpty() {
     return children == EMPTY_CHILDREN;
diff --git a/src/main/java/com/google/devtools/build/lib/rules/SkylarkFileType.java b/src/main/java/com/google/devtools/build/lib/rules/SkylarkFileType.java
index 6b31fd8761..b077b013cf 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/SkylarkFileType.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/SkylarkFileType.java
@@ -19,6 +19,8 @@ import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.skylarkinterface.SkylarkCallable;
 import com.google.devtools.build.lib.skylarkinterface.SkylarkModule;
 import com.google.devtools.build.lib.skylarkinterface.SkylarkModuleCategory;
+import com.google.devtools.build.lib.syntax.EvalException;
+import com.google.devtools.build.lib.syntax.EvalUtils;
 import com.google.devtools.build.lib.util.FileType;
 import com.google.devtools.build.lib.util.FileTypeSet;
 
@@ -53,8 +55,16 @@ public class SkylarkFileType {
     + "<a href=\"File.html\"><code>File</code></a>s that match the FileType. The parameter "
     + "must be a <a href=\"set.html\"><code>set</code></a> or a "
     + "<a href=\"list.html\"><code>list</code></a>.")
-  public List<Artifact> filter(Iterable<Artifact> files) {
-    return ImmutableList.copyOf(FileType.filter(files, fileType));
+  // toIterablesStrict() will ensure the parameter is a SkylarkNestedSet or a java Iterable
+  // (including SkylarkList). If it fails, the error location information will be inserted by the
+  // Skylark interface framework. If there's a dynamic type error on a non-Artifact element, the
+  // error will also be handled by the Skylark interface framework.
+  @SuppressWarnings("unchecked")
+  public List<Artifact> filter(Object filesUnchecked) throws EvalException {
+    return ImmutableList.copyOf(
+        FileType.filter(
+            (Iterable<Artifact>) EvalUtils.toIterableStrict(filesUnchecked, null),
+            fileType));
   }
 
   @VisibleForTesting
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 f200cf5406..c58334d450 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
@@ -58,6 +58,7 @@ import com.google.devtools.build.lib.vfs.PathFragment;
 import java.nio.charset.StandardCharsets;
 import java.util.ArrayList;
 import java.util.Arrays;
+import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.UUID;
@@ -650,30 +651,43 @@ public class SkylarkRuleImplementationFunctions {
   };
 
 
+  /**
+   * Ensures the given {@link Map} has keys that have {@link Label} type and values that have either
+   * {@link Iterable} or {@link SkylarkNestedSet} type, and raises {@link EvalException} otherwise.
+   * Returns a corresponding map where any sets are replaced by iterables.
+   */
   // TODO(bazel-team): find a better way to typecheck this argument.
   @SuppressWarnings("unchecked")
   private static Map<Label, Iterable<Artifact>> checkLabelDict(
       Map<?, ?> labelDict, Location loc)
       throws EvalException {
+    Map<Label, Iterable<Artifact>> convertedMap = new HashMap<Label, Iterable<Artifact>>();
     for (Map.Entry<?, ?> entry : labelDict.entrySet()) {
       Object key = entry.getKey();
       if (!(key instanceof Label)) {
         throw new EvalException(
             loc, Printer.format("invalid key %r in 'label_dict'", key));
       }
+      ImmutableList.Builder<Artifact> files = ImmutableList.builder();
       Object val = entry.getValue();
-      if (!(val instanceof Iterable)) {
+      Iterable<?> valIter;
+      try {
+        valIter = EvalUtils.toIterableStrict(val, loc);
+      } catch (EvalException ex) {
+        // EvalException is thrown only if the type is wrong.
         throw new EvalException(
-            loc, Printer.format("invalid value %r in 'label_dict'", val));
+            loc, Printer.format("invalid value %r in 'label_dict': " + ex, val));
       }
-      for (Object file : (Iterable) val) {
+      for (Object file : valIter) {
         if (!(file instanceof Artifact)) {
           throw new EvalException(
               loc, Printer.format("invalid value %r in 'label_dict'", val));
         }
+        files.add((Artifact) file);
       }
+      convertedMap.put((Label) key, files.build());
     }
-    return (Map<Label, Iterable<Artifact>>) labelDict;
+    return convertedMap;
   }
 
   /** suffix of script to be used in case the command is too long to fit on a single line */
@@ -682,12 +696,11 @@ public class SkylarkRuleImplementationFunctions {
   @SkylarkSignature(
     name = "resolve_command",
     doc =
-        "Experimental."
-            + "Returns a tuple (inputs, command, input_manifests) of the list of resolved inputs, "
-            + "the argv list for the resolved command, and "
-            + "the dict mapping locations to runfiles required to run the command, "
-            + "all of them suitable for passing as the same-named arguments of the ctx.action "
-            + "method.",
+        "<i>(Experimental)</i> "
+            + "Returns a tuple <code>(inputs, command, input_manifests)</code> of the list of "
+            + "resolved inputs, the argv list for the resolved command, and the dict mapping "
+            + "locations to runfiles required to run the command, all of them suitable for passing "
+            + "as the same-named arguments of the <code>ctx.action</code> method.",
     objectType = SkylarkRuleContext.class,
     returnType = Tuple.class,
     parameters = {
diff --git a/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcProviderSkylarkConverters.java b/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcProviderSkylarkConverters.java
index 7e3b443119..ee8a37c35f 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcProviderSkylarkConverters.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcProviderSkylarkConverters.java
@@ -94,7 +94,7 @@ public class ObjcProviderSkylarkConverters {
     @Override
     public Iterable<?> valueForJava(Key<?> javaKey, Object skylarkValue) {
       validateTypes(skylarkValue, javaKey.getType(), javaKey.getSkylarkKeyName());
-      return (SkylarkNestedSet) skylarkValue;
+      return ((SkylarkNestedSet) skylarkValue).toCollection();
     }
   }
 
@@ -118,7 +118,7 @@ public class ObjcProviderSkylarkConverters {
     public Iterable<?> valueForJava(Key<?> javaKey, Object skylarkValue) {
       validateTypes(skylarkValue, String.class, javaKey.getSkylarkKeyName());
       NestedSetBuilder<PathFragment> result = NestedSetBuilder.stableOrder();
-      for (String path : (Iterable<String>) skylarkValue) {
+      for (String path : ((SkylarkNestedSet) skylarkValue).toCollection(String.class)) {
         result.add(new PathFragment(path));
       }
       return result.build();
@@ -145,7 +145,7 @@ public class ObjcProviderSkylarkConverters {
     public Iterable<?> valueForJava(Key<?> javaKey, Object skylarkValue) {
       validateTypes(skylarkValue, String.class, javaKey.getSkylarkKeyName());
       NestedSetBuilder<SdkFramework> result = NestedSetBuilder.stableOrder();
-      for (String path : (Iterable<String>) skylarkValue) {
+      for (String path : ((SkylarkNestedSet) skylarkValue).toCollection(String.class)) {
         result.add(new SdkFramework(path));
       }
       return result.build();
@@ -178,7 +178,8 @@ public class ObjcProviderSkylarkConverters {
     public Iterable<?> valueForJava(Key<?> javaKey, Object skylarkValue) {
       validateTypes(skylarkValue, SkylarkClassObject.class, javaKey.getSkylarkKeyName());
       NestedSetBuilder<BundleableFile> result = NestedSetBuilder.stableOrder();
-      for (SkylarkClassObject struct : (Iterable<SkylarkClassObject>) skylarkValue) {
+      for (SkylarkClassObject struct :
+          ((SkylarkNestedSet) skylarkValue).toCollection(SkylarkClassObject.class)) {
         Artifact artifact;
         String path;
         try {
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/BazelLibrary.java b/src/main/java/com/google/devtools/build/lib/syntax/BazelLibrary.java
index 799f850c8a..284483616f 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/BazelLibrary.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/BazelLibrary.java
@@ -145,7 +145,7 @@ public class BazelLibrary {
             + "the input depset as well as all additional elements.",
     parameters = {
       @Param(name = "input", type = SkylarkNestedSet.class, doc = "The input depset."),
-      @Param(name = "new_elements", type = Iterable.class, doc = "The elements to be added.")
+      @Param(name = "new_elements", type = Object.class, doc = "The elements to be added.")
     },
     useLocation = true
   )
@@ -153,8 +153,11 @@ public class BazelLibrary {
       new BuiltinFunction("union") {
         @SuppressWarnings("unused")
         public SkylarkNestedSet invoke(
-            SkylarkNestedSet input, Iterable<Object> newElements, Location loc)
+            SkylarkNestedSet input, Object newElements, Location loc)
             throws EvalException {
+          // newElements' type is Object because of the polymorphism on unioning two
+          // SkylarkNestedSets versus a set and another kind of iterable.
+          // Can't use EvalUtils#toIterable since that would discard this information.
           return new SkylarkNestedSet(input, newElements, loc);
         }
       };
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/EvalUtils.java b/src/main/java/com/google/devtools/build/lib/syntax/EvalUtils.java
index 83d2b9f3a4..b3d7bc0ad6 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/EvalUtils.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/EvalUtils.java
@@ -353,6 +353,9 @@ public final class EvalUtils {
       // This is not as efficient as special casing String in for and dict and list comprehension
       // statements. However this is a more unified way.
       return split((String) o);
+    } else if (o instanceof SkylarkNestedSet) {
+      // TODO(bazel-team): Add a deprecation warning: don't implicitly flatten depsets.
+      return ((SkylarkNestedSet) o).toCollection();
     } else if (o instanceof Iterable) {
       return (Iterable<?>) o;
     } else if (o instanceof Map) {
@@ -363,6 +366,35 @@ public final class EvalUtils {
     }
   }
 
+  /**
+   * Given an {@link Iterable}, returns it as-is. Given a {@link SkylarkNestedSet}, returns its
+   * contents as an iterable. Throws {@link EvalException} for any other value.
+   *
+   * <p>This is a kludge for the change that made {@code SkylarkNestedSet} not implement {@code
+   * Iterable}. It is different from {@link #toIterable} in its behavior for strings and other types
+   * that are not strictly Java-iterable.
+   *
+   * @throws EvalException if {@code o} is not an iterable or set
+   * @deprecated avoid writing APIs that implicitly treat depsets as iterables. It encourages
+   * unnecessary flattening of depsets.
+   *
+   * <p>TODO(bazel-team): Remove this if/when implicit iteration over {@code SkylarkNestedSet} is no
+   * longer supported.
+   */
+  @Deprecated
+  public static Iterable<?> toIterableStrict(Object o, Location loc) throws EvalException {
+    if (o instanceof Iterable) {
+      return (Iterable<?>) o;
+    } else if (o instanceof SkylarkNestedSet) {
+      // TODO(bazel-team): Add a deprecation warning: don't implicitly flatten depsets.
+      return ((SkylarkNestedSet) o).toCollection();
+    } else {
+      throw new EvalException(loc,
+          "expected Iterable or depset, but got '" + getDataTypeName(o) + "' (strings and maps "
+          + "are not allowed here)");
+    }
+  }
+
   public static void lock(Object object, Location loc) {
     if (object instanceof SkylarkMutable) {
       ((SkylarkMutable) object).lock(loc);
@@ -392,7 +424,10 @@ public final class EvalUtils {
     } else if (arg instanceof Map) {
       return ((Map<?, ?>) arg).size();
     } else if (arg instanceof SkylarkList) {
-      return ((SkylarkList) arg).size();
+      return ((SkylarkList<?>) arg).size();
+    } else if (arg instanceof SkylarkNestedSet) {
+      // TODO(bazel-team): Add a deprecation warning: don't implicitly flatten depsets.
+      return ((SkylarkNestedSet) arg).toCollection().size();
     } else if (arg instanceof Iterable) {
       // Iterables.size() checks if arg is a Collection so it's efficient in that sense.
       return Iterables.size((Iterable<?>) arg);
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/MethodLibrary.java b/src/main/java/com/google/devtools/build/lib/syntax/MethodLibrary.java
index 47ca735a2a..3a0a176d28 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/MethodLibrary.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/MethodLibrary.java
@@ -1024,23 +1024,16 @@ public class MethodLibrary {
    */
   private static Object findExtreme(SkylarkList<?> args, Ordering<Object> maxOrdering, Location loc)
       throws EvalException {
-    // Args can either be a list of elements or a list whose first element is a non-empty iterable
-    // of elements.
+    // Args can either be a list of items to compare, or a singleton list whose element is an
+    // iterable of items to compare. In either case, there must be at least one item to compare.
     try {
-      return maxOrdering.max(getIterable(args, loc));
+      Iterable<?> items = (args.size() == 1) ? EvalUtils.toIterable(args.get(0), loc) : args;
+      return maxOrdering.max(items);
     } catch (NoSuchElementException ex) {
-      throw new EvalException(loc, "expected at least one argument");
+      throw new EvalException(loc, "expected at least one item");
     }
   }
 
-  /**
-   * This method returns the first element of the list, if that particular element is an
-   * Iterable<?>. Otherwise, it will return the entire list.
-   */
-  private static Iterable<?> getIterable(SkylarkList<?> list, Location loc) throws EvalException {
-    return (list.size() == 1) ? EvalUtils.toIterable(list.get(0), loc) : list;
-  }
-
   @SkylarkSignature(
     name = "all",
     returnType = Boolean.class,
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 530efc0994..3edfb99b31 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
@@ -27,7 +27,6 @@ import com.google.devtools.build.lib.syntax.SkylarkList.MutableList;
 import com.google.devtools.build.lib.util.Preconditions;
 import java.util.ArrayList;
 import java.util.Collection;
-import java.util.Iterator;
 import java.util.List;
 import javax.annotation.Nullable;
 
@@ -101,7 +100,7 @@ import javax.annotation.Nullable;
         + "nested depsets."
 )
 @Immutable
-public final class SkylarkNestedSet implements Iterable<Object>, SkylarkValue, SkylarkQueryable {
+public final class SkylarkNestedSet implements SkylarkValue, SkylarkQueryable {
 
   private final SkylarkType contentType;
   private final NestedSet<?> set;
@@ -151,7 +150,7 @@ public final class SkylarkNestedSet implements Iterable<Object>, SkylarkValue, S
       throw new EvalException(
           loc,
           String.format(
-              "cannot add value of type '%s' to a depset", EvalUtils.getDataTypeName(item)));
+              "cannot union value of type '%s' to a depset", EvalUtils.getDataTypeName(item)));
     }
     this.contentType = Preconditions.checkNotNull(contentType, "type cannot be null");
 
@@ -252,10 +251,23 @@ public final class SkylarkNestedSet implements Iterable<Object>, SkylarkValue, S
     }
   }
 
+  private void checkHasContentType(Class<?> type) {
+    // Empty sets should be SkylarkType.TOP anyway.
+    if (!set.isEmpty()) {
+      Preconditions.checkArgument(
+          contentType.canBeCastTo(type),
+          "Expected a depset of '%s' but got a depset of '%s'",
+          EvalUtils.getDataTypeNameFromClass(type), contentType);
+    }
+  }
+
   /**
    * Returns the embedded {@link NestedSet}, while asserting that its elements all have the given
    * type.
    *
+   * <p>If you do not specifically need the {@code NestedSet} and you are going to flatten it
+   * anyway, prefer {@link #toCollection} to make your intent clear.
+   *
    * @param type a {@link Class} representing the expected type of the contents
    * @return the {@code NestedSet}, with the appropriate generic type
    * @throws IllegalArgumentException if the type does not accurately describe all elements
@@ -263,17 +275,32 @@ public final class SkylarkNestedSet implements Iterable<Object>, SkylarkValue, S
   // The precondition ensures generic type safety.
   @SuppressWarnings("unchecked")
   public <T> NestedSet<T> getSet(Class<T> type) {
-    // Empty sets don't need have to have a type since they don't have items
-    if (set.isEmpty()) {
-      return (NestedSet<T>) set;
-    }
-    Preconditions.checkArgument(
-        contentType.canBeCastTo(type),
-        "Expected a depset of '%s' but got a depset of '%s'",
-        EvalUtils.getDataTypeNameFromClass(type), contentType);
+    checkHasContentType(type);
     return (NestedSet<T>) set;
   }
 
+  /**
+   * Returns the contents of the set as a {@link Collection}.
+   */
+  public Collection<Object> toCollection() {
+    // Do not remove <Object>: workaround for Java 7 type inference.
+    return ImmutableList.<Object>copyOf(set.toCollection());
+  }
+
+  /**
+   * Returns the contents of the set as a {@link Collection}, asserting that the set type is
+   * compatible with {@code T}.
+   *
+   * @param type a {@link Class} representing the expected type of the contents
+   * @throws IllegalArgumentException if the type does not accurately describe all elements
+   */
+  // The precondition ensures generic type safety.
+  @SuppressWarnings("unchecked")
+  public <T> Collection<T> toCollection(Class<T> type) {
+    checkHasContentType(type);
+    return (Collection<T>) toCollection();
+  }
+
   @SkylarkCallable(
       name = "to_list",
       doc = "Returns a frozen list of the elements, without duplicates, in the depset's traversal "
@@ -282,18 +309,6 @@ public final class SkylarkNestedSet implements Iterable<Object>, SkylarkValue, S
     return new MutableList<Object>(set, null);
   }
 
-  // For some reason this cast is unsafe in Java
-  @SuppressWarnings("unchecked")
-  @Override
-  public Iterator<Object> iterator() {
-    return (Iterator<Object>) set.iterator();
-  }
-
-  public Collection<Object> toCollection() {
-    // Do not remove <Object>: workaround for Java 7 type inference.
-    return ImmutableList.<Object>copyOf(set.toCollection());
-  }
-
   public boolean isEmpty() {
     return set.isEmpty();
   }
@@ -319,7 +334,7 @@ public final class SkylarkNestedSet implements Iterable<Object>, SkylarkValue, S
   @Override
   public void write(Appendable buffer, char quotationMark) {
     Printer.append(buffer, "set(");
-    Printer.printList(buffer, this, "[", ", ", "]", null, quotationMark);
+    Printer.printList(buffer, set, "[", ", ", "]", null, quotationMark);
     Order order = getOrder();
     if (order != Order.STABLE_ORDER) {
       Printer.append(buffer, ", order = \"" + order.getSkylarkName() + "\"");
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Type.java b/src/main/java/com/google/devtools/build/lib/syntax/Type.java
index e2cc62fcbb..dbbfc79c42 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/Type.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/Type.java
@@ -530,11 +530,13 @@ public abstract class Type<T> {
     @Override
     public List<ElemT> convert(Object x, Object what, Object context)
         throws ConversionException {
-      if (!(x instanceof Iterable<?>)) {
+      Iterable<?> iterable;
+      try {
+        iterable = EvalUtils.toIterableStrict(x, null);
+      } catch (EvalException ex) {
         throw new ConversionException(this, x, what);
       }
       int index = 0;
-      Iterable<?> iterable = (Iterable<?>) x;
       List<ElemT> result = new ArrayList<>(Iterables.size(iterable));
       ListConversionContext conversionContext = new ListConversionContext(what);
       for (Object elem : iterable) {