Make Java Map wrappers handle nulls according to put/remove contract

[som-snytt]

If map contains key, always return nonEmpty.
If binding to null, return Some(null) instead of None.

Fixes scala/bug#11894

[lrytz]

This looks consistent with the existing implementation of get above, so LGTM.

@ansvonwa could you also take a look?

[SethTisue]

any collections-crew opinions here? @Ichoran @NthPortal @joshlemer — we should make a team for y'all for easy @-mentioning

[Ichoran]

LGTM.

If we are content with not passing on every call to remove when there is no key, remove would be more efficiently and logically written (pseudocode)

if (containsKey(k)) Some(remove(k)) else None

But the change to the logic is good either way.

[som-snytt]

In my cut and pastey haste, I forgot my previous point that, if there's a race, checking contains can falsely return Some(null); it would be better to get the current value to return before applying the mutation.

[som-snytt]

The collections-crew could be called The Collective.

[ansvonwa]

And just for completeness, there is another place where this could be changed:
In JConcurrentMapWrapper the methods get, putIfAbsent and replace all treat the entry as absent if the key is present but the value is null.

This may in theory be the case, as the java concurrent maps explicitly (by a comment) allow maps handling null values. It is just the case that none of the maps in the java standard library does. And the wrapper around a scala concurrent map obtained by asJava is removed anyway by a call of asScala, so not even back-and-forth-conversions like asScala(asJava(new concurrent.TrieMap())) pose a problem.
So the only way to get this wrong result is to use a custom (java) map that can handle nulls and convert that to scala via asScala. I think this case is relatively rare, so I would be fine with not changing it for the benefit of keeping performance.

If we would change this, every call of get, putIfAbsent and replace in JConcurrentMapWrapper would require an additional call of containsKey, so take about twice the time.

What do you guys think?

[ansvonwa]

In my cut and pastey haste, I forgot my previous point that, if there's a race, checking contains can falsely return Some(null); it would be better to get the current value to return before applying the mutation.

Not sure if that is better. Maybe get itself already has this problem?

    def get(k: K) = {
// Assume k is not (yet) in the map
      val v = underlying get k
// What if, here, between these lines, another thread puts a value for key k in the map?
      if (v != null)
        Some(v)
      else if (underlying containsKey k)
        Some(null.asInstanceOf[V])
      else
        None
    }

If another thread puts a value between the calls of get and containsKey, we would get Some(null) instead of either None or Some(value). Right?

[som-snytt]

Right, something like compute which tells you what the current value is.

I'll spend a few minutes with the issue of other methods, thanks for your comment. I did spend a few minutes with Hashtable and Properties. In a way, it's nice to spend time with API I never use any more.

[Ichoran]

Maps cannot be safely used concurrently, so I wouldn't worry about that too much. The logic just needs to be right for non-concurrent access. (Unless it's a map that guarantees correct operation during concurrency.)

[som-snytt]

Collections.synchronizedMap(m).asScala seems easy, like an unlocked gun cabinet.

They could have a footgun app where instead of import language.selfHarm, you'd have to use 2FA to unlock the feature.

With a 48 hour cooling off period!

[joshlemer]

any collections-crew opinions here? @Ichoran @NthPortal @joshlemer — we should make a team for y'all for easy @-mentioning

import scala.jdk.CollectionConversers._

[SethTisue]

created https://github.com/orgs/scala/teams/collections/members

@-mention as @scala/collections

happy to add anyone else — I tend to assume that other folks like @som-snytt and the team here at Lightbend see everything anyway

[NthPortal]

I don't like it—in fact, I hate it—but I agree that it's probably the best solution

[som-snytt]

Pushed Ichoran's tweek. (Visual pun alert on geek.)

I reviewed ansvonwa's comment about whether more is desirable, especially in light of Ichoran's comment about concurrency.

My conclusion is that, as a convenience feature, it's enough to 1. ignore concurrency in the normal wrapper and 2. assume JDK implementations in the concurrent case, where null keys and values are disallowed.

I added a comment that consistency with get is the desideratum, because get is the basis for getOrElse, life, and unapply.

That means val m(x) = k throws on absent key but yields null on null value.

A map of boxed Double with a null value will unbox to 0.0.

[som-snytt]

Despite Because of a layer of Thanksgiving turkey brain fog overlaying the pandemic fog, I wondered if this is worth it:

    // support Some(null) if currently bound to null
    override def put(k: K, v: V): Option[V] =
      if (v == null) {
        val present = underlying.containsKey(k)
        val result  = underlying.put(k, v)
        if (present) Some(result) else None
      } else {
        var result: Option[V] = None
        def recompute(k0: K, v0: V): V = v.tap(_ =>
          if (v0 != null) result = Some(v0)
          else if (underlying.containsKey(k0)) result = Some(null.asInstanceOf[V])
        )
        underlying.compute(k, recompute)
        result
      }

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

    // support Some(null) if currently bound to null
    override def remove(k: K): Option[V] = {
      var result: Option[V] = None
      def recompute(k0: K, v0: V): V = {
        if (v0 != null) result = Some(v0)
        else if (underlying.containsKey(k0)) result = Some(null.asInstanceOf[V])
        null.asInstanceOf[V]
      }
      underlying.compute(k, recompute)
      result
    }

You can't compute a null value, but you can put it.

I probably won't push this change. Tired of waiting for the green check on the current PR.

[dwijnand]

This is still needs a review. @Ichoran or anyone in @scala/collections care to look at this, please?

[SethTisue]

okay... somebody else want to adopt this?

[Ichoran]

I'm not really around enough to reliably review things any more. I agree that this is the way to do it--it's a good implementation if we think that a double-lookup is worth it for correctness.

For what it's worth, Java itself decided it wasn't; all the methods added for JDK 8 treated mapping to null the same as no mapping. I don't use wrapped Java maps personally, though, so I don't have a strong opinion either way.

So, if someone decides we want this, then it can go in as is, no changes needed. If we decide we don't, then there's nothing to do.

[SethTisue]

thanks, Rex. we will still welcome your reviews if/when we can get them — but, noted that we shouldn't wait around too long if we haven't heard from you.

[lrytz]

I'm in favor of this change as it makes put/remove consistent with get.