Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Poor behaviour in s.c.i.HashMap#merged (2.13.0) #11559

Open
NthPortal opened this issue Jun 6, 2019 · 2 comments

Comments

Projects
None yet
3 participants
@NthPortal
Copy link

commented Jun 6, 2019

If one of the maps in s.c.i.HashMap#merged (either this or that) has a single element, and the other has the key of that element, it will put the merged element into the other map without checking that the result of the merge function returns the same key; thus, if the merge function returns a different key, the resulting map will contain both its original mapping for the key in both maps and the mapping produced by the merge function.

For reference, I have copied the snippet covering the case where this.size == 1. You can see that that.rootNode.updated(mergedK, ...) is called, but kis not removed ifk != mergedK`.

if (size == 1) {
  val payload@(k, v) = rootNode.getPayload(0)
  val originalHash = rootNode.getHash(0)
  val improved = improve(originalHash)

  if (that.rootNode.containsKey(k, originalHash, improved, 0)) {
    val thatPayload = that.rootNode.getTuple(k, originalHash, improved, 0)
    val (mergedK, mergedV) = mergef(payload, thatPayload)
    val mergedOriginalHash = mergedK.##
    val mergedImprovedHash = improve(mergedOriginalHash)
    new HashMap(that.rootNode.updated(mergedK, mergedV, mergedOriginalHash, mergedImprovedHash, 0, replaceValue = true))
  } else {
    new HashMap(that.rootNode.updated(k, v, originalHash, improved, 0, replaceValue = true))
  }
}

The result of the above snippet is the following behaviour:

scala> import scala.collection.immutable.HashMap
import scala.collection.immutable.HashMap

scala> HashMap("uh" -> "oh").merged(HashMap("uh" -> "oh"))((_, _) => "the other entry" -> "shouldn't be there")
res0: scala.collection.immutable.HashMap[String,String] = HashMap(the other entry -> shouldn't be there, uh -> oh)

It's possible that this behaviour is not a bug, given that the method is not spec-ed in any way and is free to do pretty much whatever it wants, but this behaviour seems at least undesirable and surprising.

@NthPortal

This comment has been minimized.

Copy link
Author

commented Jun 6, 2019

The behaviour is also inconsistent with the case where both maps have more than one entry:

scala> import scala.collection.immutable.HashMap
import scala.collection.immutable.HashMap

scala> HashMap(1 -> 1, 2 -> 2).merged(HashMap(1 -> 1, 3 -> 3))((_, _) => 4 -> 4)
res1: scala.collection.immutable.HashMap[Int,Int] = HashMap(2 -> 2, 3 -> 3, 4 -> 4)

scala> HashMap(1 -> 1).merged(HashMap(1 -> 1))((_, _) => 4 -> 4)
res2: scala.collection.immutable.HashMap[Int,Int] = HashMap(1 -> 1, 4 -> 4)
@joshlemer

This comment has been minimized.

Copy link
Member

commented Jun 8, 2019

This looks like a bug to me, I will try and fix this (maybe not until Scala days is over with)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.