Merge pull request #9344 from som-snytt/issue/11894-wrap

Java Map wrapper respects put/remove contract

src/library/scala/collection/convert/JavaCollectionWrappers.scala

@@ -319,11 +319,12 @@ private[collection] object JavaCollectionWrappers extends Serializable {
 
     override def size = underlying.size
 
+    // support Some(null) if currently bound to null
     def get(k: K) = {
-      val v = underlying get k
+      val v = underlying.get(k)
       if (v != null)
         Some(v)
-      else if (underlying containsKey k)
+      else if (underlying.containsKey(k))
         Some(null.asInstanceOf[V])
       else
         None
@@ -332,11 +333,18 @@ private[collection] object JavaCollectionWrappers extends Serializable {
     def addOne(kv: (K, V)): this.type = { underlying.put(kv._1, kv._2); this }
     def subtractOne(key: K): this.type = { underlying remove key; this }
 
-    override def put(k: K, v: V): Option[V] = Option(underlying.put(k, v))
+    // support Some(null) if currently bound to null
+    override def put(k: K, v: V): Option[V] = {
+      val present = underlying.containsKey(k)
+      val result  = underlying.put(k, v)
+      if (present) Some(result) else None
+    }
 
-    override def update(k: K, v: V): Unit = { underlying.put(k, v) }
+    override def update(k: K, v: V): Unit = underlying.put(k, v)
 
-    override def remove(k: K): Option[V] = Option(underlying remove k)
+    // support Some(null) if currently bound to null
+    override def remove(k: K): Option[V] =
+      if (underlying.containsKey(k)) Some(underlying.remove(k)) else None
 
     def iterator: Iterator[(K, V)] = new AbstractIterator[(K, V)] {
       val ui = underlying.entrySet.iterator

test/junit/scala/collection/convert/CollectionConvertersTest.scala

@@ -0,0 +1,80 @@
+/*
+ * Scala (https://www.scala-lang.org)
+ *
+ * Copyright EPFL and Lightbend, Inc.
+ *
+ * Licensed under Apache License 2.0
+ * (http://www.apache.org/licenses/LICENSE-2.0).
+ *
+ * See the NOTICE file distributed with this work for
+ * additional information regarding copyright ownership.
+ */
+
+package scala.collection.convert
+
+import java.util.{
+  Dictionary,
+  HashMap    => JMap,
+  Hashtable  => JTable,
+  Properties => JProperties,
+}
+import java.util.concurrent.{
+  ConcurrentHashMap => JCMap,
+}
+
+import org.junit.Assert.{assertEquals, assertNull, assertTrue}
+import org.junit.Test
+
+import scala.jdk.CollectionConverters._
+import scala.tools.testkit.AssertUtil.assertThrows
+
+class CollectionConvertersTest {
+  // Binding to null is represented as Some(null).
+  // This is consistent with get, which is the basis for getOrElse, lift, unapply
+  //
+  @Test def `t11894 Map wrapper respects put contract`(): Unit = {
+    val sut = new JMap[String, String].asScala
+    assertTrue(sut.isInstanceOf[JavaCollectionWrappers.JMapWrapper[_, _]])
+    assertThrows[NoSuchElementException](sut("one"))
+    assertEquals(None, sut.unapply("one"))
+    assertEquals(None, sut.put("one", null))
+    assertNull(sut("one"))
+    assertEquals(Some(null), sut.unapply("one"))
+    assertEquals(Some(null), sut.lift("one"))
+    assertEquals(Some(null), sut.put("one", "eins"))
+    assertEquals(Some("eins"), sut.put("one", "uno"))
+    assertEquals(Some("uno"), sut.remove("one"))
+    assertEquals(None, sut.remove("one"))
+    assertEquals(None, sut.put("one", null))
+    assertEquals(Some(null), sut.remove("one"))
+    assertEquals(None, sut.remove("one"))
+  }
+  @Test def `t11894 Map wrapper iterator witnesses null values`(): Unit = {
+    val sut = new JMap[String, String].asScala
+    assertTrue(sut.isInstanceOf[JavaCollectionWrappers.JMapWrapper[_, _]])
+    assertEquals(None, sut.put("one", null))
+    assertEquals(None, sut.put("deuce", "zwey"))
+    assertTrue(sut.iterator.find { case (_, v) => v == null }.nonEmpty)
+  }
+  @Test def `t11894 Dictionary wrapper disallows nulls`(): Unit = {
+    val tbl = new JTable[String, String]
+    val sut = tbl.asInstanceOf[Dictionary[String, String]].asScala
+    assertTrue(sut.isInstanceOf[JavaCollectionWrappers.JDictionaryWrapper[_, _]])
+    assertThrows[NullPointerException](sut.put("any", null))
+  }
+  @Test def `t11894 Properties wrapper enforces Strings`(): Unit = {
+    val ps  = new JProperties()
+    val sut = ps.asScala
+    assertTrue(sut.isInstanceOf[JavaCollectionWrappers.JPropertiesWrapper])
+    assertThrows[NullPointerException](sut.put("any", null))
+    assertEquals(None, sut.put("one", "eins"))
+    assertEquals("eins", sut("one"))
+    ps.put("one", new Object())
+    assertThrows[ClassCastException](sut.put("one", "uno"))
+  }
+  @Test def `t11894 JDK concurrent Map impl disallows nulls`(): Unit = {
+    val sut = new JCMap[String, String].asScala
+    assertTrue(sut.isInstanceOf[JavaCollectionWrappers.JConcurrentMapWrapper[_, _]])
+    assertThrows[NullPointerException](sut.put("any", null))
+  }
+}

test/junit/scala/collection/convert/NullSafetyToJavaTest.scala

@@ -3,73 +3,74 @@ package scala.collection.convert
 import java.{lang => jl}
 
 import org.junit.Test
+import org.junit.Assert.assertNull
 
 import scala.collection.{concurrent, mutable}
 import scala.jdk.CollectionConverters._
 
-// scala/bug#9113: tests to insure that wrappers return null instead of wrapping it as a collection
+// scala/bug#9113: tests to ensure that wrappers return null instead of wrapping it as a collection
 class NullSafetyToJavaTest {
   @Test def testIterableWrapping(): Unit = {
     val nullIterable: Iterable[AnyRef] = null
     val iterable: jl.Iterable[AnyRef] = nullIterable.asJava
 
-    assert(iterable == null)
+    assertNull(iterable)
   }
 
   // Implicit conversion to ju.Properties is not available
 
   @Test def testIteratorDecoration(): Unit = {
     val nullIterator: Iterator[AnyRef] = null
 
-    assert(nullIterator.asJava == null)
+    assertNull(nullIterator.asJava)
   }
 
   @Test def testEnumerationDecoration(): Unit = {
     val nullEnumeration: Iterator[AnyRef] = null
 
-    assert(nullEnumeration.asJavaEnumeration == null)
+    assertNull(nullEnumeration.asJavaEnumeration)
   }
 
   @Test def testIterableDecoration(): Unit = {
     val nullIterable: Iterable[AnyRef] = null
 
-    assert(nullIterable.asJava == null)
+    assertNull(nullIterable.asJava)
   }
 
   @Test def testCollectionDecoration(): Unit = {
     val nullCollection: Iterable[AnyRef] = null
 
-    assert(nullCollection.asJavaCollection == null)
+    assertNull(nullCollection.asJavaCollection)
   }
 
   @Test def testBufferDecoration(): Unit = {
     val nullBuffer: mutable.Buffer[AnyRef] = null
 
-    assert(nullBuffer.asJava == null)
+    assertNull(nullBuffer.asJava)
   }
 
   @Test def testSetDecoration(): Unit = {
     val nullSet: Set[AnyRef] = null
 
-    assert(nullSet.asJava == null)
+    assertNull(nullSet.asJava)
   }
 
   @Test def testMapDecoration(): Unit = {
     val nullMap: mutable.Map[AnyRef, AnyRef] = null
 
-    assert(nullMap.asJava == null)
+    assertNull(nullMap.asJava)
   }
 
   @Test def testConcurrentMapDecoration(): Unit = {
     val nullConMap: concurrent.Map[AnyRef, AnyRef] = null
 
-    assert(nullConMap.asJava == null)
+    assertNull(nullConMap.asJava)
   }
 
   @Test def testDictionaryDecoration(): Unit = {
     val nullDict: mutable.Map[AnyRef, AnyRef] = null
 
-    assert(nullDict.asJavaDictionary == null)
+    assertNull(nullDict.asJavaDictionary)
   }
 
   // Decorator conversion to ju.Properties is not available

test/junit/scala/collection/convert/NullSafetyToScalaTest.scala

@@ -4,69 +4,70 @@ import java.util.{concurrent => juc}
 import java.{lang => jl, util => ju}
 
 import org.junit.Test
+import org.junit.Assert.assertNull
 
 import scala.jdk.CollectionConverters._
 
-// scala/bug#9113: tests to insure that wrappers return null instead of wrapping it as a collection
+// scala/bug#9113: tests to ensure that wrappers return null instead of wrapping it as a collection
 
 class NullSafetyToScalaTest {
   @Test def testIteratorDecoration(): Unit = {
     val nullJIterator: ju.Iterator[AnyRef] = null
 
-    assert(nullJIterator.asScala == null)
+    assertNull(nullJIterator.asScala)
   }
 
   @Test def testEnumerationDecoration(): Unit = {
     val nullJEnumeration: ju.Enumeration[AnyRef] = null
 
-    assert(nullJEnumeration.asScala == null)
+    assertNull(nullJEnumeration.asScala)
   }
 
   @Test def testIterableDecoration(): Unit = {
     val nullJIterable: jl.Iterable[AnyRef] = null
 
-    assert(nullJIterable.asScala == null)
+    assertNull(nullJIterable.asScala)
   }
 
   @Test def testCollectionDecoration(): Unit = {
     val nullJCollection: ju.Collection[AnyRef] = null
 
-    assert(nullJCollection.asScala == null)
+    assertNull(nullJCollection.asScala)
   }
 
   @Test def testBufferDecoration(): Unit = {
     val nullJBuffer: ju.List[AnyRef] = null
 
-    assert(nullJBuffer.asScala == null)
+    assertNull(nullJBuffer.asScala)
   }
 
   @Test def testSetDecoration(): Unit = {
     val nullJSet: ju.Set[AnyRef] = null
 
-    assert(nullJSet.asScala == null)
+    assertNull(nullJSet.asScala)
   }
 
   @Test def testMapDecoration(): Unit = {
     val nullJMap: ju.Map[AnyRef, AnyRef] = null
 
-    assert(nullJMap.asScala == null)
+    assertNull(nullJMap.asScala)
   }
 
   @Test def testConcurrentMapDecoration(): Unit = {
     val nullJConMap: juc.ConcurrentMap[AnyRef, AnyRef] = null
 
-    assert(nullJConMap.asScala == null)
+    assertNull(nullJConMap.asScala)
   }
 
   @Test def testDictionaryDecoration(): Unit = {
     val nullJDict: ju.Dictionary[AnyRef, AnyRef] = null
 
-    assert(nullJDict.asScala == null)
+    assertNull(nullJDict.asScala)
   }
 
   @Test def testPropertiesDecoration(): Unit = {
     val nullJProperties: ju.Properties = null
 
-    assert(nullJProperties.asScala == null)
+    assertNull(nullJProperties.asScala)
   }
 }