Fix ImmutableList.Builder to throw a useful exception when its size w…

…ould overflow. RELNOTES=n/a PiperOrigin-RevId: 662127972
google · Aug 12, 2024 · e232035 · e232035
1 parent 0cb9cc6
commit e232035
Show file tree

Hide file tree

Showing 5 changed files with 62 additions and 25 deletions.
diff --git a/android/guava-tests/test/com/google/common/collect/ImmutableListTest.java b/android/guava-tests/test/com/google/common/collect/ImmutableListTest.java
@@ -18,6 +18,7 @@
 
 import static com.google.common.collect.testing.features.CollectionFeature.ALLOWS_NULL_QUERIES;
 import static com.google.common.collect.testing.features.CollectionFeature.SERIALIZABLE;
+import static com.google.common.truth.Truth.assertThat;
 import static java.util.Arrays.asList;
 
 import com.google.common.annotations.GwtCompatible;
@@ -666,4 +667,20 @@ public void testReusedBuilder() {
     builder.add("baz");
     assertTrue(list.array != builder.contents);
   }
+
+  @SuppressWarnings("ModifiedButNotUsed")
+  @GwtIncompatible // actually allocates nCopies
+  @J2ktIncompatible // actually allocates nCopies
+  public void testAddOverflowCollection() {
+    ImmutableList.Builder<String> builder = ImmutableList.builder();
+    for (int i = 0; i < 100; i++) {
+      builder.add("a");
+    }
+    try {
+      builder.addAll(Collections.nCopies(Integer.MAX_VALUE - 50, "a"));
+      fail("Expected IllegalArgumentException");
+    } catch (IllegalArgumentException expected) {
+      assertThat(expected).hasMessageThat().contains("cannot store more than MAX_VALUE elements");
+    }
+  }
 }
diff --git a/android/guava/src/com/google/common/collect/ImmutableCollection.java b/android/guava/src/com/google/common/collect/ImmutableCollection.java
@@ -404,7 +404,9 @@ public abstract static class Builder<E> {
 
     static int expandedCapacity(int oldCapacity, int minCapacity) {
       if (minCapacity < 0) {
-        throw new AssertionError("cannot store more than MAX_VALUE elements");
+        throw new IllegalArgumentException("cannot store more than MAX_VALUE elements");
+      } else if (minCapacity <= oldCapacity) {
+        return oldCapacity;
       }
       // careful of overflow!
       int newCapacity = oldCapacity + (oldCapacity >> 1) + 1;
@@ -513,13 +515,12 @@ abstract static class ArrayBasedBuilder<E> extends ImmutableCollection.Builder<E
      * elements without being resized. Also, if we've already built a collection backed by the
      * current array, create a new array.
      */
-    private void getReadyToExpandTo(int minCapacity) {
-      if (contents.length < minCapacity) {
-        this.contents =
-            Arrays.copyOf(this.contents, expandedCapacity(contents.length, minCapacity));
-        forceCopy = false;
-      } else if (forceCopy) {
-        this.contents = contents.clone();
+    private void ensureRoomFor(int newElements) {
+      Object[] contents = this.contents;
+      int newCapacity = expandedCapacity(contents.length, size + newElements);
+      // expandedCapacity handles the overflow case
+      if (newCapacity > contents.length || forceCopy) {
+        this.contents = Arrays.copyOf(this.contents, newCapacity);
         forceCopy = false;
       }
     }
@@ -528,7 +529,7 @@ private void getReadyToExpandTo(int minCapacity) {
     @Override
     public ArrayBasedBuilder<E> add(E element) {
       checkNotNull(element);
-      getReadyToExpandTo(size + 1);
+      ensureRoomFor(1);
       contents[size++] = element;
       return this;
     }
@@ -542,7 +543,7 @@ public Builder<E> add(E... elements) {
 
     final void addAll(@Nullable Object[] elements, int n) {
       checkElementsNotNull(elements, n);
-      getReadyToExpandTo(size + n);
+      ensureRoomFor(n);
       /*
        * The following call is not statically checked, since arraycopy accepts plain Object for its
        * parameters. If it were statically checked, the checker would still be OK with it, since
@@ -560,7 +561,7 @@ final void addAll(@Nullable Object[] elements, int n) {
     public Builder<E> addAll(Iterable<? extends E> elements) {
       if (elements instanceof Collection) {
         Collection<?> collection = (Collection<?>) elements;
-        getReadyToExpandTo(size + collection.size());
+        ensureRoomFor(collection.size());
         if (collection instanceof ImmutableCollection) {
           ImmutableCollection<?> immutableCollection = (ImmutableCollection<?>) collection;
           size = immutableCollection.copyIntoArray(contents, size);

diff --git a/guava-tests/test/com/google/common/collect/ImmutableListTest.java b/guava-tests/test/com/google/common/collect/ImmutableListTest.java
@@ -18,6 +18,7 @@
 
 import static com.google.common.collect.testing.features.CollectionFeature.ALLOWS_NULL_QUERIES;
 import static com.google.common.collect.testing.features.CollectionFeature.SERIALIZABLE;
+import static com.google.common.truth.Truth.assertThat;
 import static java.util.Arrays.asList;
 
 import com.google.common.annotations.GwtCompatible;
@@ -652,4 +653,20 @@ public void testAsList() {
     ImmutableList<String> list = ImmutableList.of("a", "b");
     assertSame(list, list.asList());
   }
+
+  @SuppressWarnings("ModifiedButNotUsed")
+  @GwtIncompatible // actually allocates nCopies
+  @J2ktIncompatible // actually allocates nCopies
+  public void testAddOverflowCollection() {
+    ImmutableList.Builder<String> builder = ImmutableList.builder();
+    for (int i = 0; i < 100; i++) {
+      builder.add("a");
+    }
+    try {
+      builder.addAll(Collections.nCopies(Integer.MAX_VALUE - 50, "a"));
+      fail("Expected IllegalArgumentException");
+    } catch (IllegalArgumentException expected) {
+      assertThat(expected).hasMessageThat().contains("cannot store more than MAX_VALUE elements");
+    }
+  }
 }
diff --git a/guava/src/com/google/common/collect/ImmutableCollection.java b/guava/src/com/google/common/collect/ImmutableCollection.java
@@ -415,7 +415,9 @@ public abstract static class Builder<E> {
 
     static int expandedCapacity(int oldCapacity, int minCapacity) {
       if (minCapacity < 0) {
-        throw new AssertionError("cannot store more than MAX_VALUE elements");
+        throw new IllegalArgumentException("cannot store more than MAX_VALUE elements");
+      } else if (minCapacity <= oldCapacity) {
+        return oldCapacity;
       }
       // careful of overflow!
       int newCapacity = oldCapacity + (oldCapacity >> 1) + 1;

diff --git a/guava/src/com/google/common/collect/ImmutableList.java b/guava/src/com/google/common/collect/ImmutableList.java
@@ -803,7 +803,7 @@ public static final class Builder<E> extends ImmutableCollection.Builder<E> {
     // The first `size` elements are non-null.
     @VisibleForTesting @Nullable Object[] contents;
     private int size;
-    private boolean forceCopy;
+    private boolean copyOnWrite;
 
     /**
      * Creates a new builder. The returned builder is equivalent to the builder generated by {@link
@@ -818,13 +818,13 @@ public Builder() {
       this.size = 0;
     }
 
-    private void getReadyToExpandTo(int minCapacity) {
-      if (contents.length < minCapacity) {
-        this.contents = Arrays.copyOf(contents, expandedCapacity(contents.length, minCapacity));
-        forceCopy = false;
-      } else if (forceCopy) {
-        contents = Arrays.copyOf(contents, contents.length);
-        forceCopy = false;
+    private void ensureRoomFor(int newElements) {
+      Object[] contents = this.contents;
+      int newCapacity = expandedCapacity(contents.length, size + newElements);
+      // expandedCapacity handles the overflow case
+      if (contents.length < newCapacity || copyOnWrite) {
+        this.contents = Arrays.copyOf(contents, newCapacity);
+        copyOnWrite = false;
       }
     }
 
@@ -839,7 +839,7 @@ private void getReadyToExpandTo(int minCapacity) {
     @Override
     public Builder<E> add(E element) {
       checkNotNull(element);
-      getReadyToExpandTo(size + 1);
+      ensureRoomFor(1);
       contents[size++] = element;
       return this;
     }
@@ -860,7 +860,7 @@ public Builder<E> add(E... elements) {
     }
 
     private void add(@Nullable Object[] elements, int n) {
-      getReadyToExpandTo(size + n);
+      ensureRoomFor(n);
       /*
        * The following call is not statically checked, since arraycopy accepts plain Object for its
        * parameters. If it were statically checked, the checker would still be OK with it, since
@@ -886,7 +886,7 @@ public Builder<E> addAll(Iterable<? extends E> elements) {
       checkNotNull(elements);
       if (elements instanceof Collection) {
         Collection<?> collection = (Collection<?>) elements;
-        getReadyToExpandTo(size + collection.size());
+        ensureRoomFor(collection.size());
         if (collection instanceof ImmutableCollection) {
           ImmutableCollection<?> immutableCollection = (ImmutableCollection<?>) collection;
           size = immutableCollection.copyIntoArray(contents, size);
@@ -923,7 +923,7 @@ Builder<E> combine(Builder<E> builder) {
      */
     @Override
     public ImmutableList<E> build() {
-      forceCopy = true;
+      copyOnWrite = true;
       return asImmutableList(contents, size);
     }
 
@@ -937,7 +937,7 @@ ImmutableList<E> buildSorted(Comparator<? super E> comparator) {
       // In particular, this implies that the comparator can never get "removed," so this can't
       // invalidate future builds.
 
-      forceCopy = true;
+      copyOnWrite = true;
       Arrays.sort((E[]) contents, 0, size, comparator);
       return asImmutableList(contents, size);
     }