NestedSet no longer implements Iterable

This makes the Java version match the Starlark depset to make it more
explicit when / where nested sets are flattened. In many, but not all,
of these cases, the flattening implies quadratic cpu/memory consumption.

PiperOrigin-RevId: 289836851

src/main/java/com/google/devtools/build/lib/analysis/actions/CustomCommandLine.java

@@ -21,7 +21,6 @@
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.Interner;
-import com.google.common.collect.Iterables;
 import com.google.devtools.build.lib.actions.ActionKeyContext;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.Artifact.ArtifactExpander;
@@ -172,7 +171,7 @@ public static class VectorArg<T> {
      */
     @AutoCodec
     public static class SimpleVectorArg<T> extends VectorArg<T> {
-      private final Iterable<T> values;
+      private final Object values;
 
       private SimpleVectorArg(Builder builder, @Nullable Collection<T> values) {
         this(
@@ -205,7 +204,7 @@ private SimpleVectorArg(Builder builder, @Nullable NestedSet<T> values) {
           String formatEach,
           String beforeEach,
           String joinWith,
-          @Nullable Iterable<T> values) {
+          @Nullable Object values) {
         super(isNestedSet, isEmpty, count, formatEach, beforeEach, joinWith);
         this.values = values;
       }
@@ -218,7 +217,7 @@ public MappedVectorArg<T> mapped(CommandLineItem.MapFn<? super T> mapFn) {
 
     /** A vector arg that maps some type T to strings. */
     static class MappedVectorArg<T> extends VectorArg<String> {
-      private final Iterable<T> values;
+      private final Object values;
       private final CommandLineItem.MapFn<? super T> mapFn;
 
       private MappedVectorArg(SimpleVectorArg<T> other, CommandLineItem.MapFn<? super T> mapFn) {
@@ -292,9 +291,9 @@ public <T> SimpleVectorArg<T> each(NestedSet<T> values) {
       }
     }
 
-    @SuppressWarnings("unchecked")
     private static void push(List<Object> arguments, VectorArg<?> vectorArg) {
-      final Iterable<?> values;
+      // This is either a Collection or a NestedSet.
+      final Object values;
       final CommandLineItem.MapFn<?> mapFn;
       if (vectorArg instanceof SimpleVectorArg) {
         values = ((SimpleVectorArg) vectorArg).values;
@@ -323,7 +322,7 @@ private static void push(List<Object> arguments, VectorArg<?> vectorArg) {
       } else {
         // Simply expand any ordinary collection into the argv
         arguments.add(vectorArg.count);
-        Iterables.addAll(arguments, values);
+        arguments.addAll((Collection<?>) values);
       }
       if (vectorArgFragment.hasFormatEach) {
         arguments.add(vectorArg.formatEach);
@@ -1245,7 +1244,9 @@ private Iterable<String> argumentsInternal(@Nullable ArtifactExpander artifactEx
     for (int i = 0; i < count; ) {
       Object arg = arguments.get(i++);
       Object substitutedArg = substituteTreeFileArtifactArgvFragment(arg);
-      if (substitutedArg instanceof Iterable) {
+      if (substitutedArg instanceof NestedSet) {
+        evalSimpleVectorArg(((NestedSet<?>) substitutedArg).toList(), builder);
+      } else if (substitutedArg instanceof Iterable) {
         evalSimpleVectorArg((Iterable<?>) substitutedArg, builder);
       } else if (substitutedArg instanceof ArgvFragment) {
         if (artifactExpander != null
@@ -1286,14 +1287,18 @@ private Object substituteTreeFileArtifactArgvFragment(Object arg) {
   }
 
   @Override
+  @SuppressWarnings("unchecked")
   public void addToFingerprint(ActionKeyContext actionKeyContext, Fingerprint fingerprint) {
     int count = arguments.size();
     for (int i = 0; i < count; ) {
       Object arg = arguments.get(i++);
       Object substitutedArg = substituteTreeFileArtifactArgvFragment(arg);
-      if (substitutedArg instanceof Iterable) {
-        addSimpleVectorArgToFingerprint(
-            (Iterable<?>) substitutedArg, actionKeyContext, fingerprint);
+      if (substitutedArg instanceof NestedSet) {
+        actionKeyContext.addNestedSetToFingerprint(fingerprint, (NestedSet<Object>) substitutedArg);
+      } else if (substitutedArg instanceof Iterable) {
+        for (Object value : (Iterable<Object>) substitutedArg) {
+          fingerprint.addString(CommandLineItem.expandToCommandLine(value));
+        }
       } else if (substitutedArg instanceof ArgvFragment) {
         i =
             ((ArgvFragment) substitutedArg)
@@ -1303,16 +1308,4 @@ public void addToFingerprint(ActionKeyContext actionKeyContext, Fingerprint fing
       }
     }
   }
-
-  @SuppressWarnings("unchecked")
-  private void addSimpleVectorArgToFingerprint(
-      Iterable<?> arg, ActionKeyContext actionKeyContext, Fingerprint fingerprint) {
-    if (arg instanceof NestedSet) {
-      actionKeyContext.addNestedSetToFingerprint(fingerprint, (NestedSet<Object>) arg);
-    } else {
-      for (Object value : arg) {
-        fingerprint.addString(CommandLineItem.expandToCommandLine(value));
-      }
-    }
-  }
 }

src/main/java/com/google/devtools/build/lib/collect/CollectionUtils.java

@@ -23,7 +23,6 @@
 import com.google.common.collect.Iterables;
 import com.google.common.collect.Maps;
 import com.google.devtools.build.lib.collect.compacthashset.CompactHashSet;
-import com.google.devtools.build.lib.collect.nestedset.NestedSet;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collection;
@@ -116,7 +115,6 @@ public static <T> Set<T> duplicatedElementsOf(Iterable<T> input) {
   /**
    * Returns an immutable set of all non-null parameters in the order in which they are specified.
    */
-  @SuppressWarnings("unchecked")
   public static <T> ImmutableSet<T> asSetWithoutNulls(T... elements) {
     return Arrays.stream(elements).filter(Objects::nonNull).collect(toImmutableSet());
   }
@@ -130,7 +128,6 @@ public static <T> boolean isImmutable(Iterable<T> iterable) {
     return iterable instanceof ImmutableList<?>
         || iterable instanceof ImmutableSet<?>
         || iterable instanceof IterablesChain<?>
-        || iterable instanceof NestedSet<?>
         || iterable instanceof ImmutableIterable<?>;
   }
 
@@ -168,7 +165,6 @@ public static <T extends Enum<T>> int toBits(Set<T> values) {
    * Converts a set of enum values to a bit field. Requires that the enum contains at most 32
    * elements.
    */
-  @SuppressWarnings("unchecked")
   public static <T extends Enum<T>> int toBits(T... values) {
     return toBits(ImmutableSet.copyOf(values));
   }
@@ -213,8 +209,7 @@ public static <KEY_1, KEY_2, VALUE> Map<KEY_1, Map<KEY_2, VALUE>> copyOf(
    * sets.
    */
   public static <T> boolean isEmpty(Iterable<T> iterable) {
-    return (iterable instanceof NestedSet)
-        ? ((NestedSet) iterable).isEmpty()
-        : Iterables.isEmpty(iterable);
+    // Only caller is IterablesChain.
+    return Iterables.isEmpty(iterable);
   }
 }

src/main/java/com/google/devtools/build/lib/collect/nestedset/NestedSet.java

@@ -46,7 +46,7 @@
  */
 @SuppressWarnings("unchecked")
 @AutoCodec
-public final class NestedSet<E> implements Iterable<E> {
+public final class NestedSet<E> {
   private static final Logger logger = Logger.getLogger(NestedSet.class.getName());
 
   /**
@@ -410,12 +410,6 @@ public static String childrenToString(Object children) {
     }
   }
 
-  @Override
-  public Iterator<E> iterator() {
-    // TODO: would it help to have a proper lazy iterator?  seems like it might reduce garbage.
-    return toList().iterator();
-  }
-
   /**
    * Implementation of {@link #toList}. Uses one of three strategies based on the value of {@code
    * this.memo}: wrap our direct items in a list, call {@link #lockedExpand} to perform the initial

src/main/java/com/google/devtools/build/lib/collect/nestedset/NestedSetBuilder.java

@@ -93,14 +93,6 @@ public NestedSetBuilder<E> add(E element) {
    */
   public NestedSetBuilder<E> addAll(Iterable<? extends E> elements) {
     Preconditions.checkNotNull(elements);
-    if (elements instanceof NestedSet) {
-      if (order.equals(Order.STABLE_ORDER)) {
-        // If direct/transitive order doesn't matter, add the nested set as a transitive member to
-        // avoid copying its elements.
-        return addTransitive((NestedSet<? extends E>) elements);
-      }
-      throw new IllegalArgumentException("NestedSet should be added as a transitive member");
-    }
     if (items == null) {
       int n = Iterables.size(elements);
       if (n == 0) {
@@ -234,7 +226,6 @@ public static <E> NestedSet<E> wrap(Order order, Iterable<E> wrappedItems) {
   /**
    * Creates a nested set with the given list of items as its elements.
    */
-  @SuppressWarnings("unchecked")
   public static <E> NestedSet<E> create(Order order, E... elems) {
     return wrap(order, ImmutableList.copyOf(elems));
   }

src/main/java/com/google/devtools/build/lib/rules/android/BusyBoxActionBuilder.java

@@ -96,10 +96,6 @@ public BusyBoxActionBuilder addInput(@CompileTimeConstant String arg, Artifact v
    */
   public BusyBoxActionBuilder addInput(
       @CompileTimeConstant String arg, String value, Iterable<Artifact> valueArtifacts) {
-    Preconditions.checkState(
-        !(valueArtifacts instanceof NestedSet),
-        "NestedSet values should not be added here, since they will be inefficiently collapsed in"
-            + " analysis time. Use one of the transitive input methods instead.");
     commandLine.add(arg, value);
     inputs.addAll(valueArtifacts);
     return this;

src/test/java/com/google/devtools/build/lib/bazel/rules/python/BazelPyBinaryConfiguredTargetTest.java

@@ -387,7 +387,8 @@ public void explicitInitPy_CanBeGloballyEnabled() throws Exception {
             "    srcs = ['foo.py'],",
             ")"));
     useConfiguration("--incompatible_default_to_explicit_init_py=true");
-    assertThat(getDefaultRunfiles(getConfiguredTarget("//pkg:foo")).getEmptyFilenames()).isEmpty();
+    assertThat(getDefaultRunfiles(getConfiguredTarget("//pkg:foo")).getEmptyFilenames().toList())
+        .isEmpty();
   }
 
   @Test
@@ -401,7 +402,7 @@ public void explicitInitPy_CanBeSelectivelyDisabled() throws Exception {
             "    legacy_create_init = True,",
             ")"));
     useConfiguration("--incompatible_default_to_explicit_init_py=true");
-    assertThat(getDefaultRunfiles(getConfiguredTarget("//pkg:foo")).getEmptyFilenames())
+    assertThat(getDefaultRunfiles(getConfiguredTarget("//pkg:foo")).getEmptyFilenames().toList())
         .containsExactly("pkg/__init__.py");
   }
 
@@ -415,7 +416,7 @@ public void explicitInitPy_CanBeGloballyDisabled() throws Exception {
             "    srcs = ['foo.py'],",
             ")"));
     useConfiguration("--incompatible_default_to_explicit_init_py=false");
-    assertThat(getDefaultRunfiles(getConfiguredTarget("//pkg:foo")).getEmptyFilenames())
+    assertThat(getDefaultRunfiles(getConfiguredTarget("//pkg:foo")).getEmptyFilenames().toList())
         .containsExactly("pkg/__init__.py");
   }
 
@@ -430,6 +431,7 @@ public void explicitInitPy_CanBeSelectivelyEnabled() throws Exception {
             "    legacy_create_init = False,",
             ")"));
     useConfiguration("--incompatible_default_to_explicit_init_py=false");
-    assertThat(getDefaultRunfiles(getConfiguredTarget("//pkg:foo")).getEmptyFilenames()).isEmpty();
+    assertThat(getDefaultRunfiles(getConfiguredTarget("//pkg:foo")).getEmptyFilenames().toList())
+        .isEmpty();
   }
 }

src/test/java/com/google/devtools/build/lib/collect/CollectionUtilsTest.java

@@ -19,11 +19,8 @@
 
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
-import com.google.common.collect.Iterables;
 import com.google.common.collect.Lists;
 import com.google.common.collect.Sets;
-import com.google.devtools.build.lib.collect.nestedset.NestedSet;
-import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
 import java.util.EnumSet;
 import java.util.HashSet;
 import java.util.List;
@@ -58,13 +55,6 @@ public void testIsImmutable() throws Exception {
     assertThat(CollectionUtils.isImmutable(ImmutableList.of(1, 2, 3))).isTrue();
     assertThat(CollectionUtils.isImmutable(ImmutableSet.of(1, 2, 3))).isTrue();
 
-    NestedSet<Integer> ns = NestedSetBuilder.<Integer>compileOrder()
-        .add(1).add(2).add(3).build();
-    assertThat(CollectionUtils.isImmutable(ns)).isTrue();
-
-    NestedSet<Integer> ns2 = NestedSetBuilder.<Integer>linkOrder().add(1).add(2).add(3).build();
-    assertThat(CollectionUtils.isImmutable(ns2)).isTrue();
-
     Iterable<Integer> chain = IterablesChain.<Integer>builder().addElement(1).build();
 
     assertThat(CollectionUtils.isImmutable(chain)).isTrue();
@@ -73,14 +63,6 @@ public void testIsImmutable() throws Exception {
     assertThat(CollectionUtils.isImmutable(Lists.newLinkedList())).isFalse();
     assertThat(CollectionUtils.isImmutable(Sets.newHashSet())).isFalse();
     assertThat(CollectionUtils.isImmutable(Sets.newLinkedHashSet())).isFalse();
-
-    // The result of Iterables.concat() actually is immutable, but we have no way of checking if
-    // a given Iterable comes from concat().
-    assertThat(CollectionUtils.isImmutable(Iterables.concat(ns, ns2))).isFalse();
-
-    // We can override the check by using the ImmutableIterable wrapper.
-    assertThat(CollectionUtils.isImmutable(ImmutableIterable.from(Iterables.concat(ns, ns2))))
-        .isTrue();
   }
 
   @Test
@@ -101,7 +83,7 @@ public void testMakeImmutable() throws Exception {
     Iterable<Integer> immutableList = ImmutableList.of(1, 2, 3);
     assertThat(CollectionUtils.makeImmutable(immutableList)).isSameInstanceAs(immutableList);
 
-    Iterable<Integer> mutableList = Lists.newArrayList(1, 2, 3);
+    List<Integer> mutableList = Lists.newArrayList(1, 2, 3);
     Iterable<Integer> converted = CollectionUtils.makeImmutable(mutableList);
     assertThat(converted).isNotSameInstanceAs(mutableList);
     assertThat(ImmutableList.copyOf(converted)).isEqualTo(mutableList);