8159404: throw UnsupportedOperationException unconditionally for mutator methods

Reviewed-by: martin, psandoz

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<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 @@ class ImmutableCollections {
         }
     }
 
-    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 @@ class ImmutableCollections {
         }
     }
 
-    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 @@ class ImmutableCollections {
         }
     }
 
-    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 @@ class ImmutableCollections {
 
     // ---------- 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 @@ class ImmutableCollections {
         }
     }
 
-    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 @@ class ImmutableCollections {
         }
     }
 
-    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 @@ class ImmutableCollections {
      * 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 @@ class ImmutableCollections {
 
     // ---------- 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 @@ class ImmutableCollections {
         }
     }
 
-    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 @@ class ImmutableCollections {
      * @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
 

jdk/src/java.base/share/classes/java/util/List.java

@@ -94,7 +94,8 @@ import java.util.function.UnaryOperator;
  *
  * <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

jdk/src/java.base/share/classes/java/util/Map.java

@@ -119,7 +119,8 @@ import java.io.Serializable;
  *
  * <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

jdk/src/java.base/share/classes/java/util/Set.java

@@ -70,7 +70,8 @@ package java.util;
  *
  * <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

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<Integer,Integer>());
         testMap(new LinkedHashMap<Integer,Integer>());
@@ -134,6 +133,20 @@ public class MOAT {
         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 @@ public class MOAT {
 
         // 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 @@ 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<Integer> set : Arrays.asList(
                 Set.<Integer>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<Integer,Integer> map : Arrays.asList(
                 Map.<Integer,Integer>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<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); }