From 342b7af1da0b215f2ff4236dabcd278a930cdbf8 Mon Sep 17 00:00:00 2001 From: Som Snytt Date: Tue, 24 Nov 2020 21:06:15 -0800 Subject: [PATCH] Jave Map wrapper respects put/remove contract If map contains key, always return nonEmpty. If binding to null, return Some(null) instead of None. This behavior is consistent with get, which is the basis for getOrElse, lift, and unapply. --- .../convert/JavaCollectionWrappers.scala | 18 +++-- .../convert/CollectionConvertersTest.scala | 80 +++++++++++++++++++ .../convert/NullSafetyToJavaTest.scala | 23 +++--- .../convert/NullSafetyToScalaTest.scala | 23 +++--- 4 files changed, 117 insertions(+), 27 deletions(-) create mode 100644 test/junit/scala/collection/convert/CollectionConvertersTest.scala diff --git a/src/library/scala/collection/convert/JavaCollectionWrappers.scala b/src/library/scala/collection/convert/JavaCollectionWrappers.scala index 011980b70aca..15f546d1d402 100644 --- a/src/library/scala/collection/convert/JavaCollectionWrappers.scala +++ b/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 diff --git a/test/junit/scala/collection/convert/CollectionConvertersTest.scala b/test/junit/scala/collection/convert/CollectionConvertersTest.scala new file mode 100644 index 000000000000..37d3b1579332 --- /dev/null +++ b/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)) + } +} diff --git a/test/junit/scala/collection/convert/NullSafetyToJavaTest.scala b/test/junit/scala/collection/convert/NullSafetyToJavaTest.scala index ef94216e0bb7..073674169f39 100644 --- a/test/junit/scala/collection/convert/NullSafetyToJavaTest.scala +++ b/test/junit/scala/collection/convert/NullSafetyToJavaTest.scala @@ -3,17 +3,18 @@ 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 @@ -21,55 +22,55 @@ class NullSafetyToJavaTest { @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 diff --git a/test/junit/scala/collection/convert/NullSafetyToScalaTest.scala b/test/junit/scala/collection/convert/NullSafetyToScalaTest.scala index fd078110e982..ba68faa57d76 100644 --- a/test/junit/scala/collection/convert/NullSafetyToScalaTest.scala +++ b/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) } }