8308167: SequencedMap::firstEntry throws NPE when first entry has null key or value

Reviewed-by: bchristi

src/java.base/share/classes/java/util/SequencedMap.java

@@ -25,6 +25,8 @@
 
 package java.util;
 
+import jdk.internal.util.NullableKeyValueHolder;
+
 /**
  * A Map that has a well-defined encounter order, that supports operations at both ends, and
  * that is reversible. The <a href="SequencedCollection.html#encounter">encounter order</a>
@@ -148,7 +150,7 @@ public interface SequencedMap<K, V> extends Map<K, V> {
      */
     default Map.Entry<K,V> firstEntry() {
         var it = entrySet().iterator();
-        return it.hasNext() ? Map.Entry.copyOf(it.next()) : null;
+        return it.hasNext() ? new NullableKeyValueHolder<>(it.next()) : null;
     }
 
     /**
@@ -165,7 +167,7 @@ public interface SequencedMap<K, V> extends Map<K, V> {
      */
     default Map.Entry<K,V> lastEntry() {
         var it = reversed().entrySet().iterator();
-        return it.hasNext() ? Map.Entry.copyOf(it.next()) : null;
+        return it.hasNext() ? new NullableKeyValueHolder<>(it.next()) : null;
     }
 
     /**
@@ -185,7 +187,7 @@ public interface SequencedMap<K, V> extends Map<K, V> {
     default Map.Entry<K,V> pollFirstEntry() {
         var it = entrySet().iterator();
         if (it.hasNext()) {
-            var entry = Map.Entry.copyOf(it.next());
+            var entry = new NullableKeyValueHolder<>(it.next());
             it.remove();
             return entry;
         } else {
@@ -210,7 +212,7 @@ public interface SequencedMap<K, V> extends Map<K, V> {
     default Map.Entry<K,V> pollLastEntry() {
         var it = reversed().entrySet().iterator();
         if (it.hasNext()) {
-            var entry = Map.Entry.copyOf(it.next());
+            var entry = new NullableKeyValueHolder<>(it.next());
             it.remove();
             return entry;
         } else {

src/java.base/share/classes/jdk/internal/util/NullableKeyValueHolder.java

@@ -0,0 +1,166 @@
+/*
+ * Copyright (c) 2023, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.  Oracle designates this
+ * particular file as subject to the "Classpath" exception as provided
+ * by Oracle in the LICENSE file that accompanied this code.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+package jdk.internal.util;
+
+import java.util.Map;
+import java.util.Objects;
+
+/**
+ * An immutable container for a key and a value, both of which are nullable.
+ *
+ * <p>This is a <a href="{@docRoot}/java.base/java/lang/doc-files/ValueBased.html">value-based</a>
+ * class; programmers should treat instances that are
+ * {@linkplain #equals(Object) equal} as interchangeable and should not
+ * use instances for synchronization, or unpredictable behavior may
+ * occur. For example, in a future release, synchronization may fail.
+ *
+ * @apiNote
+ * This class is not exported. Instances are created by various Map implementations
+ * when they need a Map.Entry that isn't connected to the Map.
+ *
+ * <p>This class differs from AbstractMap.SimpleImmutableEntry in that it is not
+ * serializable and that it is final. This class differs from java.util.KeyValueHolder
+ * in that the key and value are nullable.
+ *
+ * <p>In principle this class could be a variation on KeyValueHolder. However,
+ * making that class selectively support nullable keys and values is quite intricate.
+ * Various specifications (such as Map.ofEntries and Map.entry) specify non-nullability
+ * of the key and the value. Map.Entry.copyOf also requires non-null keys and values;
+ * but it simply passes through KeyValueHolder instances, assuming their keys and values
+ * are non-nullable. If a KVH with nullable keys and values were introduced, some way
+ * to distinguish it would be necessary. This could be done by introducing a subclass
+ * (requiring KVH to be made non-final) or by introducing some kind of "mode" field
+ * (potentially increasing the size of every KVH instance, though another field could
+ * probably fit into the object's padding in most JVMs.) More critically, a mode field
+ * would have to be checked in all the right places to get the right behavior.
+ *
+ * <p>A longer range possibility is to selectively relax the restrictions against nulls in
+ * Map.entry and Map.Entry.copyOf. This would also require some intricate specification
+ * changes and corresponding implementation changes (e.g., the implementations backing
+ * Map.of might still need to reject nulls, and so would Map.ofEntries) but allowing
+ * a Map.Entry itself to contain nulls seems beneficial in general. If this is done,
+ * merging KeyValueHolder and NullableKeyValueHolder should be reconsidered.
+ *
+ * @param <K> the key type
+ * @param <V> the value type
+ */
+@jdk.internal.ValueBased
+public final class NullableKeyValueHolder<K,V> implements Map.Entry<K,V> {
+    final K key;
+    final V value;
+
+    /**
+     * Constructs a NullableKeyValueHolder.
+     *
+     * @param k the key, may be null
+     * @param v the value, may be null
+     */
+    public NullableKeyValueHolder(K k, V v) {
+        key = k;
+        value = v;
+    }
+
+    /**
+     * Constructs a NullableKeyValueHolder from a Map.Entry. No need for an
+     * idempotent copy at this time.
+     *
+     * @param entry the entry, must not be null
+     */
+    public NullableKeyValueHolder(Map.Entry<K,V> entry) {
+        Objects.requireNonNull(entry);
+        key = entry.getKey();
+        value = entry.getValue();
+    }
+
+    /**
+     * Gets the key from this holder.
+     *
+     * @return the key, may be null
+     */
+    @Override
+    public K getKey() {
+        return key;
+    }
+
+    /**
+     * Gets the value from this holder.
+     *
+     * @return the value, may be null
+     */
+    @Override
+    public V getValue() {
+        return value;
+    }
+
+    /**
+     * Throws {@link UnsupportedOperationException}.
+     *
+     * @param value ignored
+     * @return never returns normally
+     */
+    @Override
+    public V setValue(V value) {
+        throw new UnsupportedOperationException("not supported");
+    }
+
+    /**
+     * Compares the specified object with this entry for equality.
+     * Returns {@code true} if the given object is also a map entry and
+     * the two entries' keys and values are equal.
+     */
+    @Override
+    public boolean equals(Object o) {
+        return o instanceof Map.Entry<?, ?> e
+                && Objects.equals(key, e.getKey())
+                && Objects.equals(value, e.getValue());
+    }
+
+    private int hash(Object obj) {
+        return (obj == null) ? 0 : obj.hashCode();
+    }
+
+    /**
+     * Returns the hash code value for this map entry. The hash code
+     * is {@code key.hashCode() ^ value.hashCode()}.
+     */
+    @Override
+    public int hashCode() {
+        return hash(key) ^ hash(value);
+    }
+
+    /**
+     * Returns a String representation of this map entry.  This
+     * implementation returns the string representation of this
+     * entry's key followed by the equals character ("{@code =}")
+     * followed by the string representation of this entry's value.
+     *
+     * @return a String representation of this map entry
+     */
+    @Override
+    public String toString() {
+        return key + "=" + value;
+    }
+}

test/jdk/java/util/AbstractMap/SimpleEntries.java

@@ -23,12 +23,14 @@
 
 /*
  * @test
- * @bug 4904074 6328220 6330389
- * @summary Basic tests for SimpleEntry, SimpleImmutableEntry
+ * @bug 4904074 6328220 6330389 8308167
+ * @modules java.base/jdk.internal.util
+ * @summary Basic tests for several Map.Entry implementations
  * @author Martin Buchholz
  */
 
 import java.util.Map;
+import jdk.internal.util.NullableKeyValueHolder;
 
 import static java.util.AbstractMap.SimpleEntry;
 import static java.util.AbstractMap.SimpleImmutableEntry;
@@ -40,8 +42,12 @@ public class SimpleEntries {
     private static void realMain(String[] args) throws Throwable {
         testEntry(new SimpleEntry<String,Long>(k,v));
         testEntry(new SimpleImmutableEntry<String,Long>(k,v));
+        testEntry(Map.entry(k,v));
+        testEntry(Map.Entry.copyOf(Map.entry(k,v)));
+        testEntry(new NullableKeyValueHolder(k,v));
         testNullEntry(new SimpleEntry<String,Long>(null,null));
         testNullEntry(new SimpleImmutableEntry<String,Long>(null,null));
+        testNullEntry(new NullableKeyValueHolder(null,null));
     }
 
     private static void testEntry(Map.Entry<String,Long> e) {
@@ -52,6 +58,7 @@ public class SimpleEntries {
         check(! e.equals(null));
         equal(e, new SimpleImmutableEntry<String,Long>(k,v));
         equal(e.toString(), k+"="+v);
+        check(e.hashCode() == 101575); // hash("foo") ^ hash(1L)
         if (e instanceof SimpleEntry) {
             equal(e.setValue(v2), v);
             equal(e.getValue(), v2);
@@ -70,6 +77,7 @@ public class SimpleEntries {
         equal(e, new SimpleEntry<String,Long>(null, null));
         equal(e, new SimpleImmutableEntry<String,Long>(null, null));
         equal(e.toString(), "null=null");
+        check(e.hashCode() == 0);
         if (e instanceof SimpleEntry) {
             equal(e.setValue(v), null);
             equal(e.getValue(), v);

test/jdk/java/util/SequencedCollection/BasicMap.java

@@ -258,6 +258,16 @@ public class BasicMap {
         return cases.iterator();
     }
 
+    @DataProvider(name="nullableEntries")
+    public Iterator<Object[]> nullableEntries() {
+        return Arrays.asList(
+            new Object[] { "firstEntry" },
+            new Object[] { "lastEntry" },
+            new Object[] { "pollFirstEntry" },
+            new Object[] { "pollLastEntry" }
+        ).iterator();
+    }
+
     // ========== Assertions ==========
 
     /**
@@ -519,6 +529,11 @@ public class BasicMap {
         assertThrows(CCE, () -> { objMap.reversed().sequencedEntrySet().reversed().getFirst().setValue(new Object()); });
     }
 
+    public void checkEntry(Map.Entry<String, Integer> entry, String key, Integer value) {
+        assertEquals(entry.getKey(), key);
+        assertEquals(entry.getValue(), value);
+    }
+
     // ========== Tests ==========
 
     @Test(dataProvider="all")
@@ -889,4 +904,21 @@ public class BasicMap {
             assertThrows(UOE, () -> entrySet.addFirst(Map.entry("x", 99)));
         checkContents(map, baseref);
     }
+
+    @Test(dataProvider="nullableEntries")
+    public void testNullableKeyValue(String mode) {
+        // TODO this relies on LHM to inherit SequencedMap default
+        // methods which are actually being tested here.
+        SequencedMap<String, Integer> map = new LinkedHashMap<>();
+        map.put(null, 1);
+        map.put("two", null);
+
+        switch (mode) {
+            case "firstEntry"     -> checkEntry(map.firstEntry(), null, 1);
+            case "lastEntry"      -> checkEntry(map.lastEntry(), "two", null);
+            case "pollFirstEntry" -> checkEntry(map.pollFirstEntry(), null, 1);
+            case "pollLastEntry"  -> checkEntry(map.pollLastEntry(), "two", null);
+            default               -> throw new AssertionError("illegal mode " + mode);
+        }
+    }
 }