From 97fa8cd04e629e51aaf66ff26465ad1543d30724 Mon Sep 17 00:00:00 2001 From: Stuart Marks Date: Tue, 6 Sep 2016 16:08:54 -0700 Subject: [PATCH] 8159404: throw UnsupportedOperationException unconditionally for mutator methods Reviewed-by: martin, psandoz --- .../java/util/ImmutableCollections.java | 68 +++++++++-- .../share/classes/java/util/List.java | 3 +- .../share/classes/java/util/Map.java | 3 +- .../share/classes/java/util/Set.java | 3 +- jdk/test/java/util/Collection/MOAT.java | 111 +++++++++++++++++- 5 files changed, 173 insertions(+), 15 deletions(-) diff --git a/jdk/src/java.base/share/classes/java/util/ImmutableCollections.java b/jdk/src/java.base/share/classes/java/util/ImmutableCollections.java index 8414311e425..494dce83a89 100644 --- a/jdk/src/java.base/share/classes/java/util/ImmutableCollections.java +++ b/jdk/src/java.base/share/classes/java/util/ImmutableCollections.java @@ -31,6 +31,10 @@ import java.io.ObjectInputStream; 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 @@ class ImmutableCollections { */ static final double EXPAND_FACTOR = 2.0; + static UnsupportedOperationException uoe() { return new UnsupportedOperationException(); } + // ---------- List Implementations ---------- - static final class List0 extends AbstractList implements RandomAccess, Serializable { + abstract static class AbstractImmutableList extends AbstractList + implements RandomAccess, Serializable { + @Override public boolean add(E e) { throw uoe(); } + @Override public boolean addAll(Collection c) { throw uoe(); } + @Override public boolean addAll(int index, Collection 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 filter) { throw uoe(); } + @Override public void replaceAll(UnaryOperator operator) { throw uoe(); } + @Override public boolean retainAll(Collection c) { throw uoe(); } + @Override public void sort(Comparator c) { throw uoe(); } + } + + static final class List0 extends AbstractImmutableList { List0() { } @Override @@ -86,7 +106,7 @@ class ImmutableCollections { } } - static final class List1 extends AbstractList implements RandomAccess, Serializable { + static final class List1 extends AbstractImmutableList { private final E e0; List1(E e0) { @@ -114,7 +134,7 @@ class ImmutableCollections { } } - static final class List2 extends AbstractList implements RandomAccess, Serializable { + static final class List2 extends AbstractImmutableList { private final E e0; private final E e1; @@ -147,7 +167,7 @@ class ImmutableCollections { } } - static final class ListN extends AbstractList implements RandomAccess, Serializable { + static final class ListN extends AbstractImmutableList { private final E[] elements; @SafeVarargs @@ -183,7 +203,17 @@ class ImmutableCollections { // ---------- Set Implementations ---------- - static final class Set0 extends AbstractSet implements Serializable { + abstract static class AbstractImmutableSet extends AbstractSet implements Serializable { + @Override public boolean add(E e) { throw uoe(); } + @Override public boolean addAll(Collection 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 filter) { throw uoe(); } + @Override public boolean retainAll(Collection c) { throw uoe(); } + } + + static final class Set0 extends AbstractImmutableSet { Set0() { } @Override @@ -210,7 +240,7 @@ class ImmutableCollections { } } - static final class Set1 extends AbstractSet implements Serializable { + static final class Set1 extends AbstractImmutableSet { private final E e0; Set1(E e0) { @@ -241,7 +271,7 @@ class ImmutableCollections { } } - static final class Set2 extends AbstractSet implements Serializable { + static final class Set2 extends AbstractImmutableSet { private final E e0; private final E e1; @@ -312,7 +342,7 @@ class ImmutableCollections { * least one null is always present. * @param the element type */ - static final class SetN extends AbstractSet implements Serializable { + static final class SetN extends AbstractImmutableSet { private final E[] elements; private final int size; @@ -403,7 +433,23 @@ class ImmutableCollections { // ---------- Map Implementations ---------- - static final class Map0 extends AbstractMap implements Serializable { + abstract static class AbstractImmutableMap extends AbstractMap implements Serializable { + @Override public void clear() { throw uoe(); } + @Override public V compute(K key, BiFunction rf) { throw uoe(); } + @Override public V computeIfAbsent(K key, Function mf) { throw uoe(); } + @Override public V computeIfPresent(K key, BiFunction rf) { throw uoe(); } + @Override public V merge(K key, V value, BiFunction rf) { throw uoe(); } + @Override public V put(K key, V value) { throw uoe(); } + @Override public void putAll(Map 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 f) { throw uoe(); } + } + + static final class Map0 extends AbstractImmutableMap { Map0() { } @Override @@ -430,7 +476,7 @@ class ImmutableCollections { } } - static final class Map1 extends AbstractMap implements Serializable { + static final class Map1 extends AbstractImmutableMap { private final K k0; private final V v0; @@ -472,7 +518,7 @@ class ImmutableCollections { * @param the key type * @param the value type */ - static final class MapN extends AbstractMap implements Serializable { + static final class MapN extends AbstractImmutableMap { private final Object[] table; // pairs of key, value private final int size; // number of pairs diff --git a/jdk/src/java.base/share/classes/java/util/List.java b/jdk/src/java.base/share/classes/java/util/List.java index 3819d94e831..8e97c2ea259 100644 --- a/jdk/src/java.base/share/classes/java/util/List.java +++ b/jdk/src/java.base/share/classes/java/util/List.java @@ -94,7 +94,8 @@ import java.util.function.UnaryOperator; * *
    *
  • They are structurally immutable. 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. *
  • They disallow {@code null} elements. Attempts to create them with diff --git a/jdk/src/java.base/share/classes/java/util/Map.java b/jdk/src/java.base/share/classes/java/util/Map.java index 9b71b0552b6..d4ea203edee 100644 --- a/jdk/src/java.base/share/classes/java/util/Map.java +++ b/jdk/src/java.base/share/classes/java/util/Map.java @@ -119,7 +119,8 @@ import java.io.Serializable; * *
      *
    • They are structurally immutable. 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. *
    • They disallow {@code null} keys and values. Attempts to create them with diff --git a/jdk/src/java.base/share/classes/java/util/Set.java b/jdk/src/java.base/share/classes/java/util/Set.java index 15a594a5e28..a1911c13e1b 100644 --- a/jdk/src/java.base/share/classes/java/util/Set.java +++ b/jdk/src/java.base/share/classes/java/util/Set.java @@ -70,7 +70,8 @@ package java.util; * *
        *
      • They are structurally immutable. 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. *
      • They disallow {@code null} elements. Attempts to create them with diff --git a/jdk/test/java/util/Collection/MOAT.java b/jdk/test/java/util/Collection/MOAT.java index fe17c5034a3..f74a1785e26 100644 --- a/jdk/test/java/util/Collection/MOAT.java +++ b/jdk/test/java/util/Collection/MOAT.java @@ -112,7 +112,6 @@ public class MOAT { testCollection(Arrays.asList(1,2,3)); testCollection(nCopies(25,1)); testImmutableList(nCopies(25,1)); - testImmutableList(unmodifiableList(Arrays.asList(1,2,3))); testMap(new HashMap()); testMap(new LinkedHashMap()); @@ -134,6 +133,20 @@ public class MOAT { testMap(Collections.synchronizedSortedMap(new TreeMap())); testMap(Collections.synchronizedNavigableMap(new TreeMap())); + // 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 emptyArray = Arrays.asList(new Integer[]{}); testCollection(emptyArray); @@ -196,6 +209,8 @@ public class MOAT { // Immutable List testEmptyList(List.of()); + testListMutatorsAlwaysThrow(List.of()); + testEmptyListMutatorsAlwaysThrow(List.of()); for (List list : Arrays.asList( List.of(), List.of(1), @@ -211,10 +226,13 @@ public class MOAT { List.of(integerArray))) { testCollection(list); testImmutableList(list); + testListMutatorsAlwaysThrow(list); } // Immutable Set testEmptySet(Set.of()); + testCollMutatorsAlwaysThrow(Set.of()); + testEmptyCollMutatorsAlwaysThrow(Set.of()); for (Set set : Arrays.asList( Set.of(), Set.of(1), @@ -230,6 +248,7 @@ public class MOAT { Set.of(integerArray))) { testCollection(set); testImmutableSet(set); + testCollMutatorsAlwaysThrow(set); } // Immutable Map @@ -241,6 +260,8 @@ public class MOAT { } testEmptyMap(Map.of()); + testMapMutatorsAlwaysThrow(Map.of()); + testEmptyMapMutatorsAlwaysThrow(Map.of()); for (Map map : Arrays.asList( Map.of(), Map.of(1, 101), @@ -256,6 +277,7 @@ public class MOAT { Map.ofEntries(ea))) { testMap(map); testImmutableMap(map); + testMapMutatorsAlwaysThrow(map); } } @@ -358,6 +380,93 @@ public class MOAT { 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 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 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 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 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 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 m) { + if (! m.isEmpty()) { + fail("map is not empty"); + } + THROWS(UnsupportedOperationException.class, + () -> m.clear(), + () -> m.replaceAll((k, v) -> v)); + } + private static void clear(Collection c) { try { c.clear(); } catch (Throwable t) { unexpected(t); }