8159404: throw UnsupportedOperationException unconditionally for mutator methods
Reviewed-by: martin, psandoz
--- a/jdk/src/java.base/share/classes/java/util/ImmutableCollections.java Tue Sep 06 17:07:06 2016 -0400
+++ b/jdk/src/java.base/share/classes/java/util/ImmutableCollections.java Tue Sep 06 16:08:54 2016 -0700
@@ -31,6 +31,10 @@
import java.io.ObjectOutputStream;
import java.io.ObjectStreamException;
import java.io.Serializable;
+import java.util.function.BiFunction;
+import java.util.function.Function;
+import java.util.function.Predicate;
+import java.util.function.UnaryOperator;
/**
* Container class for immutable collections. Not part of the public API.
@@ -61,9 +65,25 @@
*/
static final double EXPAND_FACTOR = 2.0;
+ static UnsupportedOperationException uoe() { return new UnsupportedOperationException(); }
+
// ---------- List Implementations ----------
- static final class List0<E> extends AbstractList<E> implements RandomAccess, Serializable {
+ abstract static class AbstractImmutableList<E> extends AbstractList<E>
+ implements RandomAccess, Serializable {
+ @Override public boolean add(E e) { throw uoe(); }
+ @Override public boolean addAll(Collection<? extends E> c) { throw uoe(); }
+ @Override public boolean addAll(int index, Collection<? extends E> c) { throw uoe(); }
+ @Override public void clear() { throw uoe(); }
+ @Override public boolean remove(Object o) { throw uoe(); }
+ @Override public boolean removeAll(Collection<?> c) { throw uoe(); }
+ @Override public boolean removeIf(Predicate<? super E> filter) { throw uoe(); }
+ @Override public void replaceAll(UnaryOperator<E> operator) { throw uoe(); }
+ @Override public boolean retainAll(Collection<?> c) { throw uoe(); }
+ @Override public void sort(Comparator<? super E> c) { throw uoe(); }
+ }
+
+ static final class List0<E> extends AbstractImmutableList<E> {
List0() { }
@Override
@@ -86,7 +106,7 @@
}
}
- static final class List1<E> extends AbstractList<E> implements RandomAccess, Serializable {
+ static final class List1<E> extends AbstractImmutableList<E> {
private final E e0;
List1(E e0) {
@@ -114,7 +134,7 @@
}
}
- static final class List2<E> extends AbstractList<E> implements RandomAccess, Serializable {
+ static final class List2<E> extends AbstractImmutableList<E> {
private final E e0;
private final E e1;
@@ -147,7 +167,7 @@
}
}
- static final class ListN<E> extends AbstractList<E> implements RandomAccess, Serializable {
+ static final class ListN<E> extends AbstractImmutableList<E> {
private final E[] elements;
@SafeVarargs
@@ -183,7 +203,17 @@
// ---------- Set Implementations ----------
- static final class Set0<E> extends AbstractSet<E> implements Serializable {
+ abstract static class AbstractImmutableSet<E> extends AbstractSet<E> implements Serializable {
+ @Override public boolean add(E e) { throw uoe(); }
+ @Override public boolean addAll(Collection<? extends E> c) { throw uoe(); }
+ @Override public void clear() { throw uoe(); }
+ @Override public boolean remove(Object o) { throw uoe(); }
+ @Override public boolean removeAll(Collection<?> c) { throw uoe(); }
+ @Override public boolean removeIf(Predicate<? super E> filter) { throw uoe(); }
+ @Override public boolean retainAll(Collection<?> c) { throw uoe(); }
+ }
+
+ static final class Set0<E> extends AbstractImmutableSet<E> {
Set0() { }
@Override
@@ -210,7 +240,7 @@
}
}
- static final class Set1<E> extends AbstractSet<E> implements Serializable {
+ static final class Set1<E> extends AbstractImmutableSet<E> {
private final E e0;
Set1(E e0) {
@@ -241,7 +271,7 @@
}
}
- static final class Set2<E> extends AbstractSet<E> implements Serializable {
+ static final class Set2<E> extends AbstractImmutableSet<E> {
private final E e0;
private final E e1;
@@ -312,7 +342,7 @@
* least one null is always present.
* @param <E> the element type
*/
- static final class SetN<E> extends AbstractSet<E> implements Serializable {
+ static final class SetN<E> extends AbstractImmutableSet<E> {
private final E[] elements;
private final int size;
@@ -403,7 +433,23 @@
// ---------- Map Implementations ----------
- static final class Map0<K,V> extends AbstractMap<K,V> implements Serializable {
+ abstract static class AbstractImmutableMap<K,V> extends AbstractMap<K,V> implements Serializable {
+ @Override public void clear() { throw uoe(); }
+ @Override public V compute(K key, BiFunction<? super K,? super V,? extends V> rf) { throw uoe(); }
+ @Override public V computeIfAbsent(K key, Function<? super K,? extends V> mf) { throw uoe(); }
+ @Override public V computeIfPresent(K key, BiFunction<? super K,? super V,? extends V> rf) { throw uoe(); }
+ @Override public V merge(K key, V value, BiFunction<? super V,? super V,? extends V> rf) { throw uoe(); }
+ @Override public V put(K key, V value) { throw uoe(); }
+ @Override public void putAll(Map<? extends K,? extends V> m) { throw uoe(); }
+ @Override public V putIfAbsent(K key, V value) { throw uoe(); }
+ @Override public V remove(Object key) { throw uoe(); }
+ @Override public boolean remove(Object key, Object value) { throw uoe(); }
+ @Override public V replace(K key, V value) { throw uoe(); }
+ @Override public boolean replace(K key, V oldValue, V newValue) { throw uoe(); }
+ @Override public void replaceAll(BiFunction<? super K,? super V,? extends V> f) { throw uoe(); }
+ }
+
+ static final class Map0<K,V> extends AbstractImmutableMap<K,V> {
Map0() { }
@Override
@@ -430,7 +476,7 @@
}
}
- static final class Map1<K,V> extends AbstractMap<K,V> implements Serializable {
+ static final class Map1<K,V> extends AbstractImmutableMap<K,V> {
private final K k0;
private final V v0;
@@ -472,7 +518,7 @@
* @param <K> the key type
* @param <V> the value type
*/
- static final class MapN<K,V> extends AbstractMap<K,V> implements Serializable {
+ static final class MapN<K,V> extends AbstractImmutableMap<K,V> {
private final Object[] table; // pairs of key, value
private final int size; // number of pairs
--- a/jdk/src/java.base/share/classes/java/util/List.java Tue Sep 06 17:07:06 2016 -0400
+++ b/jdk/src/java.base/share/classes/java/util/List.java Tue Sep 06 16:08:54 2016 -0700
@@ -94,7 +94,8 @@
*
* <ul>
* <li>They are <em>structurally immutable</em>. Elements cannot be added, removed,
- * or replaced. Attempts to do so result in {@code UnsupportedOperationException}.
+ * or replaced. Calling any mutator method will always cause
+ * {@code UnsupportedOperationException} to be thrown.
* However, if the contained elements are themselves mutable,
* this may cause the List's contents to appear to change.
* <li>They disallow {@code null} elements. Attempts to create them with
--- a/jdk/src/java.base/share/classes/java/util/Map.java Tue Sep 06 17:07:06 2016 -0400
+++ b/jdk/src/java.base/share/classes/java/util/Map.java Tue Sep 06 16:08:54 2016 -0700
@@ -119,7 +119,8 @@
*
* <ul>
* <li>They are <em>structurally immutable</em>. Keys and values cannot be added,
- * removed, or updated. Attempts to do so result in {@code UnsupportedOperationException}.
+ * removed, or updated. Calling any mutator method will always cause
+ * {@code UnsupportedOperationException} to be thrown.
* However, if the contained keys or values are themselves mutable, this may cause the
* Map to behave inconsistently or its contents to appear to change.
* <li>They disallow {@code null} keys and values. Attempts to create them with
--- a/jdk/src/java.base/share/classes/java/util/Set.java Tue Sep 06 17:07:06 2016 -0400
+++ b/jdk/src/java.base/share/classes/java/util/Set.java Tue Sep 06 16:08:54 2016 -0700
@@ -70,7 +70,8 @@
*
* <ul>
* <li>They are <em>structurally immutable</em>. Elements cannot be added or
- * removed. Attempts to do so result in {@code UnsupportedOperationException}.
+ * removed. Calling any mutator method will always cause
+ * {@code UnsupportedOperationException} to be thrown.
* However, if the contained elements are themselves mutable, this may cause the
* Set to behave inconsistently or its contents to appear to change.
* <li>They disallow {@code null} elements. Attempts to create them with
--- a/jdk/test/java/util/Collection/MOAT.java Tue Sep 06 17:07:06 2016 -0400
+++ b/jdk/test/java/util/Collection/MOAT.java Tue Sep 06 16:08:54 2016 -0700
@@ -112,7 +112,6 @@
testCollection(Arrays.asList(1,2,3));
testCollection(nCopies(25,1));
testImmutableList(nCopies(25,1));
- testImmutableList(unmodifiableList(Arrays.asList(1,2,3)));
testMap(new HashMap<Integer,Integer>());
testMap(new LinkedHashMap<Integer,Integer>());
@@ -134,6 +133,20 @@
testMap(Collections.synchronizedSortedMap(new TreeMap<Integer,Integer>()));
testMap(Collections.synchronizedNavigableMap(new TreeMap<Integer,Integer>()));
+ // Unmodifiable wrappers
+ testImmutableSet(unmodifiableSet(new HashSet<>(Arrays.asList(1,2,3))));
+ testImmutableList(unmodifiableList(Arrays.asList(1,2,3)));
+ testImmutableMap(unmodifiableMap(Collections.singletonMap(1,2)));
+ testCollMutatorsAlwaysThrow(unmodifiableSet(new HashSet<>(Arrays.asList(1,2,3))));
+ testCollMutatorsAlwaysThrow(unmodifiableSet(Collections.emptySet()));
+ testEmptyCollMutatorsAlwaysThrow(unmodifiableSet(Collections.emptySet()));
+ testListMutatorsAlwaysThrow(unmodifiableList(Arrays.asList(1,2,3)));
+ testListMutatorsAlwaysThrow(unmodifiableList(Collections.emptyList()));
+ testEmptyListMutatorsAlwaysThrow(unmodifiableList(Collections.emptyList()));
+ testMapMutatorsAlwaysThrow(unmodifiableMap(Collections.singletonMap(1,2)));
+ testMapMutatorsAlwaysThrow(unmodifiableMap(Collections.emptyMap()));
+ testEmptyMapMutatorsAlwaysThrow(unmodifiableMap(Collections.emptyMap()));
+
// Empty collections
final List<Integer> emptyArray = Arrays.asList(new Integer[]{});
testCollection(emptyArray);
@@ -196,6 +209,8 @@
// Immutable List
testEmptyList(List.of());
+ testListMutatorsAlwaysThrow(List.of());
+ testEmptyListMutatorsAlwaysThrow(List.of());
for (List<Integer> list : Arrays.asList(
List.<Integer>of(),
List.of(1),
@@ -211,10 +226,13 @@
List.of(integerArray))) {
testCollection(list);
testImmutableList(list);
+ testListMutatorsAlwaysThrow(list);
}
// Immutable Set
testEmptySet(Set.of());
+ testCollMutatorsAlwaysThrow(Set.of());
+ testEmptyCollMutatorsAlwaysThrow(Set.of());
for (Set<Integer> set : Arrays.asList(
Set.<Integer>of(),
Set.of(1),
@@ -230,6 +248,7 @@
Set.of(integerArray))) {
testCollection(set);
testImmutableSet(set);
+ testCollMutatorsAlwaysThrow(set);
}
// Immutable Map
@@ -241,6 +260,8 @@
}
testEmptyMap(Map.of());
+ testMapMutatorsAlwaysThrow(Map.of());
+ testEmptyMapMutatorsAlwaysThrow(Map.of());
for (Map<Integer,Integer> map : Arrays.asList(
Map.<Integer,Integer>of(),
Map.of(1, 101),
@@ -256,6 +277,7 @@
Map.ofEntries(ea))) {
testMap(map);
testImmutableMap(map);
+ testMapMutatorsAlwaysThrow(map);
}
}
@@ -358,6 +380,93 @@
it.remove(); });
}
+ /**
+ * Test that calling a mutator always throws UOE, even if the mutator
+ * wouldn't actually do anything, given its arguments.
+ *
+ * @param c the collection instance to test
+ */
+ private static void testCollMutatorsAlwaysThrow(Collection<Integer> c) {
+ THROWS(UnsupportedOperationException.class,
+ () -> c.addAll(Collections.emptyList()),
+ () -> c.remove(ABSENT_VALUE),
+ () -> c.removeAll(Collections.emptyList()),
+ () -> c.removeIf(x -> false),
+ () -> c.retainAll(c));
+ }
+
+ /**
+ * Test that calling a mutator always throws UOE, even if the mutator
+ * wouldn't actually do anything on an empty collection.
+ *
+ * @param c the collection instance to test, must be empty
+ */
+ private static void testEmptyCollMutatorsAlwaysThrow(Collection<Integer> c) {
+ if (! c.isEmpty()) {
+ fail("collection is not empty");
+ }
+ THROWS(UnsupportedOperationException.class,
+ () -> c.clear());
+ }
+
+ /**
+ * As above, for a list.
+ *
+ * @param c the list instance to test
+ */
+ private static void testListMutatorsAlwaysThrow(List<Integer> c) {
+ testCollMutatorsAlwaysThrow(c);
+ THROWS(UnsupportedOperationException.class,
+ () -> c.addAll(0, Collections.emptyList()));
+ }
+
+ /**
+ * As above, for an empty list.
+ *
+ * @param c the list instance to test, must be empty
+ */
+ private static void testEmptyListMutatorsAlwaysThrow(List<Integer> c) {
+ if (! c.isEmpty()) {
+ fail("list is not empty");
+ }
+ testEmptyCollMutatorsAlwaysThrow(c);
+ THROWS(UnsupportedOperationException.class,
+ () -> c.replaceAll(x -> x),
+ () -> c.sort(null));
+ }
+
+ /**
+ * As above, for a map.
+ *
+ * @param m the map instance to test
+ */
+ private static void testMapMutatorsAlwaysThrow(Map<Integer,Integer> m) {
+ THROWS(UnsupportedOperationException.class,
+ () -> m.compute(ABSENT_VALUE, (k, v) -> null),
+ () -> m.computeIfAbsent(ABSENT_VALUE, k -> null),
+ () -> m.computeIfPresent(ABSENT_VALUE, (k, v) -> null),
+ () -> m.merge(ABSENT_VALUE, 0, (k, v) -> null),
+ () -> m.putAll(Collections.emptyMap()),
+ () -> m.remove(ABSENT_VALUE),
+ () -> m.remove(ABSENT_VALUE, 0),
+ () -> m.replace(ABSENT_VALUE, 0),
+ () -> m.replace(ABSENT_VALUE, 0, 1));
+ }
+
+ /**
+ * As above, for an empty map.
+ *
+ * @param map the map instance to test, must be empty
+ */
+ private static void testEmptyMapMutatorsAlwaysThrow(Map<Integer,Integer> m) {
+ if (! m.isEmpty()) {
+ fail("map is not empty");
+ }
+ THROWS(UnsupportedOperationException.class,
+ () -> m.clear(),
+ () -> m.replaceAll((k, v) -> v));
+ }
+
private static void clear(Collection<Integer> c) {
try { c.clear(); }
catch (Throwable t) { unexpected(t); }