diff --git a/android/guava-tests/test/com/google/common/collect/ImmutableListTest.java b/android/guava-tests/test/com/google/common/collect/ImmutableListTest.java index 82d789d64482..0ad68bcaf458 100644 --- 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 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 index 25d3336eadae..8d3bcbe3cfca 100644 --- 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 { 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 extends ImmutableCollection.Builder contents.length || forceCopy) { + this.contents = Arrays.copyOf(this.contents, newCapacity); forceCopy = false; } } @@ -528,7 +529,7 @@ private void getReadyToExpandTo(int minCapacity) { @Override public ArrayBasedBuilder add(E element) { checkNotNull(element); - getReadyToExpandTo(size + 1); + ensureRoomFor(1); contents[size++] = element; return this; } @@ -542,7 +543,7 @@ public Builder 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 addAll(Iterable 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 index 760efe1e5fb0..f3936a34fe0a 100644 --- 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 list = ImmutableList.of("a", "b"); assertSame(list, list.asList()); } + + @SuppressWarnings("ModifiedButNotUsed") + @GwtIncompatible // actually allocates nCopies + @J2ktIncompatible // actually allocates nCopies + public void testAddOverflowCollection() { + ImmutableList.Builder 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 index 80987788cda7..dc0c46a9a560 100644 --- 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 { 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 index e2604c7ce5a1..399ca995906c 100644 --- 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 extends ImmutableCollection.Builder { // 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 add(E element) { checkNotNull(element); - getReadyToExpandTo(size + 1); + ensureRoomFor(1); contents[size++] = element; return this; } @@ -860,7 +860,7 @@ public Builder 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 addAll(Iterable 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 combine(Builder builder) { */ @Override public ImmutableList build() { - forceCopy = true; + copyOnWrite = true; return asImmutableList(contents, size); } @@ -937,7 +937,7 @@ ImmutableList buildSorted(Comparator 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); }