diff --git a/src/java.base/share/classes/java/util/SequencedMap.java b/src/java.base/share/classes/java/util/SequencedMap.java index c8f88d004bc..aeab9501736 100644 --- a/src/java.base/share/classes/java/util/SequencedMap.java +++ b/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 encounter order @@ -148,7 +150,7 @@ public interface SequencedMap extends Map { */ default Map.Entry 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 extends Map { */ default Map.Entry 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 extends Map { default Map.Entry 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 extends Map { default Map.Entry 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 { diff --git a/src/java.base/share/classes/jdk/internal/util/NullableKeyValueHolder.java b/src/java.base/share/classes/jdk/internal/util/NullableKeyValueHolder.java new file mode 100644 index 00000000000..b81606b6e4c --- /dev/null +++ b/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. + * + *

This is a value-based + * 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. + * + *

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. + * + *

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. + * + *

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 the key type + * @param the value type + */ +@jdk.internal.ValueBased +public final class NullableKeyValueHolder implements Map.Entry { + 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 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; + } +} diff --git a/test/jdk/java/util/AbstractMap/SimpleEntries.java b/test/jdk/java/util/AbstractMap/SimpleEntries.java index 1b66062ea92..bb565d20e0c 100644 --- a/test/jdk/java/util/AbstractMap/SimpleEntries.java +++ b/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(k,v)); testEntry(new SimpleImmutableEntry(k,v)); + testEntry(Map.entry(k,v)); + testEntry(Map.Entry.copyOf(Map.entry(k,v))); + testEntry(new NullableKeyValueHolder(k,v)); testNullEntry(new SimpleEntry(null,null)); testNullEntry(new SimpleImmutableEntry(null,null)); + testNullEntry(new NullableKeyValueHolder(null,null)); } private static void testEntry(Map.Entry e) { @@ -52,6 +58,7 @@ public class SimpleEntries { check(! e.equals(null)); equal(e, new SimpleImmutableEntry(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(null, null)); equal(e, new SimpleImmutableEntry(null, null)); equal(e.toString(), "null=null"); + check(e.hashCode() == 0); if (e instanceof SimpleEntry) { equal(e.setValue(v), null); equal(e.getValue(), v); diff --git a/test/jdk/java/util/SequencedCollection/BasicMap.java b/test/jdk/java/util/SequencedCollection/BasicMap.java index 96a18cc88b7..6bcbba74d09 100644 --- a/test/jdk/java/util/SequencedCollection/BasicMap.java +++ b/test/jdk/java/util/SequencedCollection/BasicMap.java @@ -258,6 +258,16 @@ public class BasicMap { return cases.iterator(); } + @DataProvider(name="nullableEntries") + public Iterator 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 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 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); + } + } }