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

1 files changed, 5 insertions, 12 deletions

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,