Skip to content

Commit 986dcc1

Browse files
lrytzlrytz
authored
Merge pull request #10129 from som-snytt/followup/12586-preserve-NPE
Preserve null policy in wrapped Java Map
2 parents d578a02 + b824b84 commit 986dcc1

3 files changed

Lines changed: 106 additions & 8 deletions

File tree

‎src/library/scala/collection/concurrent/Map.scala‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -97,7 +97,7 @@ trait Map[K, V] extends scala.collection.mutable.Map[K, V] {
9797
case None =>
9898
val v = op
9999
putIfAbsent(key, v) match {
100-
case Some(nv) => nv
100+
case Some(ov) => ov
101101
case None => v
102102
}
103103
}

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

Lines changed: 37 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ import java.{lang => jl, util => ju}
2121
import scala.jdk.CollectionConverters._
2222
import scala.util.Try
2323
import scala.util.chaining._
24+
import scala.util.control.ControlThrowable
2425

2526
/** Wrappers for exposing Scala collections as Java collections and vice-versa */
2627
@SerialVersionUID(3L)
@@ -332,7 +333,12 @@ private[collection] object JavaCollectionWrappers extends Serializable {
332333
else
333334
None
334335
}
335-
override def getOrElseUpdate(key: K, op: => V): V = underlying.computeIfAbsent(key, _ => op)
336+
337+
override def getOrElseUpdate(key: K, op: => V): V =
338+
underlying.computeIfAbsent(key, _ => op) match {
339+
case null => update(key, null.asInstanceOf[V]); null.asInstanceOf[V]
340+
case v => v
341+
}
336342

337343
def addOne(kv: (K, V)): this.type = { underlying.put(kv._1, kv._2); this }
338344
def subtractOne(key: K): this.type = { underlying remove key; this }
@@ -355,8 +361,17 @@ private[collection] object JavaCollectionWrappers extends Serializable {
355361

356362
override def update(k: K, v: V): Unit = underlying.put(k, v)
357363

358-
override def updateWith(key: K)(remappingFunction: Option[V] => Option[V]): Option[V] = Option {
359-
underlying.compute(key, (_, v) => remappingFunction(Option(v)).getOrElse(null.asInstanceOf[V]))
364+
override def updateWith(key: K)(remappingFunction: Option[V] => Option[V]): Option[V] = {
365+
def remap(k: K, v: V): V =
366+
remappingFunction(Option(v)) match {
367+
case Some(null) => throw PutNull
368+
case Some(x) => x
369+
case None => null.asInstanceOf[V]
370+
}
371+
try Option(underlying.compute(key, remap))
372+
catch {
373+
case PutNull => update(key, null.asInstanceOf[V]); Some(null.asInstanceOf[V])
374+
}
360375
}
361376

362377
// support Some(null) if currently bound to null
@@ -441,7 +456,11 @@ private[collection] object JavaCollectionWrappers extends Serializable {
441456

442457
override def get(k: K) = Option(underlying get k)
443458

444-
override def getOrElseUpdate(key: K, op: => V): V = underlying.computeIfAbsent(key, _ => op)
459+
override def getOrElseUpdate(key: K, op: => V): V =
460+
underlying.computeIfAbsent(key, _ => op) match {
461+
case null => super/*[concurrent.Map]*/.getOrElseUpdate(key, op)
462+
case v => v
463+
}
445464

446465
override def isEmpty: Boolean = underlying.isEmpty
447466
override def knownSize: Int = if (underlying.isEmpty) 0 else super.knownSize
@@ -462,8 +481,17 @@ private[collection] object JavaCollectionWrappers extends Serializable {
462481
case _ => Try(last).toOption
463482
}
464483

465-
override def updateWith(key: K)(remappingFunction: Option[V] => Option[V]): Option[V] = Option {
466-
underlying.compute(key, (_, v) => remappingFunction(Option(v)).getOrElse(null.asInstanceOf[V]))
484+
override def updateWith(key: K)(remappingFunction: Option[V] => Option[V]): Option[V] = {
485+
def remap(k: K, v: V): V =
486+
remappingFunction(Option(v)) match {
487+
case Some(null) => throw PutNull // see scala/scala#10129
488+
case Some(x) => x
489+
case None => null.asInstanceOf[V]
490+
}
491+
try Option(underlying.compute(key, remap))
492+
catch {
493+
case PutNull => super/*[concurrent.Map]*/.updateWith(key)(remappingFunction)
494+
}
467495
}
468496
}
469497

@@ -572,4 +600,7 @@ private[collection] object JavaCollectionWrappers extends Serializable {
572600

573601
override def mapFactory = mutable.HashMap
574602
}
603+
604+
/** Thrown when certain Map operations attempt to put a null value. */
605+
private val PutNull = new ControlThrowable {}
575606
}

‎test/junit/scala/collection/convert/MapWrapperTest.scala‎

Lines changed: 68 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,12 +2,13 @@ package scala.collection.convert
22

33
import java.{util => jutil}
44

5-
import org.junit.Assert._
5+
import org.junit.Assert.{assertEquals, assertFalse, assertTrue}
66
import org.junit.Test
77
import org.junit.runner.RunWith
88
import org.junit.runners.JUnit4
99

1010
import scala.jdk.CollectionConverters._
11+
import scala.tools.testkit.AssertUtil.assertThrows
1112
import scala.util.chaining._
1213

1314
@RunWith(classOf[JUnit4])
@@ -107,4 +108,70 @@ class MapWrapperTest {
107108
loki.done = true
108109
runner.join()
109110
}
111+
@Test def `updateWith and getOrElseUpdate should reflect null policy of update`: Unit = {
112+
val jmap = new jutil.concurrent.ConcurrentHashMap[String, String]()
113+
val wrapped = jmap.asScala
114+
assertThrows[NullPointerException](jmap.put("K", null))
115+
assertThrows[NullPointerException](jmap.putIfAbsent("K", null))
116+
assertThrows[NullPointerException](wrapped.put("K", null))
117+
assertThrows[NullPointerException](wrapped.update("K", null))
118+
assertThrows[NullPointerException](wrapped.updateWith("K")(_ => Some(null)))
119+
assertThrows[NullPointerException](wrapped.getOrElseUpdate("K", null))
120+
121+
var count = 0
122+
def v = {
123+
count += 1
124+
null
125+
}
126+
assertThrows[NullPointerException](wrapped.update("K", v))
127+
assertEquals(1, count)
128+
assertThrows[NullPointerException](wrapped.updateWith("K")(_ => Some(v)))
129+
assertEquals(3, count) // extra count in retry
130+
}
131+
@Test def `more updateWith and getOrElseUpdate should reflect null policy of update`: Unit = {
132+
val jmap = new jutil.HashMap[String, String]()
133+
val wrapped = jmap.asScala
134+
wrapped.put("K", null)
135+
assertEquals(1, wrapped.size)
136+
wrapped.remove("K")
137+
assertEquals(0, wrapped.size)
138+
wrapped.update("K", null)
139+
assertEquals(1, wrapped.size)
140+
wrapped.remove("K")
141+
wrapped.updateWith("K")(_ => Some(null))
142+
assertEquals(1, wrapped.size)
143+
wrapped.remove("K")
144+
wrapped.getOrElseUpdate("K", null)
145+
assertEquals(1, wrapped.size)
146+
147+
var count = 0
148+
def v = {
149+
count += 1
150+
null
151+
}
152+
wrapped.update("K", v)
153+
assertEquals(1, count)
154+
wrapped.remove("K")
155+
wrapped.updateWith("K")(_ => Some(v))
156+
assertEquals(2, count)
157+
}
158+
159+
@Test def `getOrElseUpdate / updateWith support should insert null`: Unit = {
160+
val jmap = new jutil.HashMap[String, String]()
161+
val wrapped = jmap.asScala
162+
163+
wrapped.getOrElseUpdate("a", null)
164+
assertTrue(jmap.containsKey("a"))
165+
166+
wrapped.getOrElseUpdate(null, "x")
167+
assertTrue(jmap.containsKey(null))
168+
169+
jmap.clear()
170+
171+
wrapped.updateWith("b")(_ => Some(null))
172+
assertTrue(jmap.containsKey("b"))
173+
174+
wrapped.updateWith(null)(_ => Some("x"))
175+
assertTrue(jmap.containsKey(null))
176+
}
110177
}

0 commit comments

Comments
 (0)