scala / bug

Scala 2 bug reports only. Please, no questions — proper bug reports only.
https://scala-lang.org
232 stars 21 forks source link

`immutable.HashMap.concat` concatenates twice for no apparent reason #13039

Closed noresttherein closed 1 month ago

noresttherein commented 2 months ago

Scala version: 2.13.14, prob'ly also in Scala 3 Not a bug in the sense that it doesn't produce wrong results, only a minor but serious ugliness

case hm: HashMap[K, V1] =>
      if (isEmpty) hm
      else {
        val newNode = rootNode.concat(hm.rootNode, 0)
        if (newNode eq hm.rootNode) hm
        else newHashMapOrThis(rootNode.concat(hm.rootNode, 0))
      }

For comparison, HashSet using the same implementation:

      case hs: HashSet[A] =>
        if (isEmpty) hs
        else {
          val newNode = rootNode.concat(hs.rootNode, 0)
          if (newNode eq hs.rootNode) hs
          else newHashSetOrThis(newNode)
        }

Problem

That operation ain't that cheap to repeat it, especially that it was amusingly done in order to gain a minor performance benefit. Yes, 'questions aren't bug reports', but if you know what's the complexity of concat I'd love to hear it, it's a bit hard to follow.

som-snytt commented 2 months ago

The ref is https://github.com/scala/scala/blob/v2.13.15/src/library/scala/collection/immutable/HashMap.scala#L168

Very hard to follow the archaeology through a forward merge commit. The ignored tests and TODO are still in HashMapTest.

But the OP looks like a typo or oversight.

SethTisue commented 1 month ago

@scala/collections

som-snytt commented 1 month ago

I wonder if it's an IDE bug in "extract expression", because https://github.com/scala/scala/commit/7c68757845d8adca4cee7fac9bd98de82c593ba8#diff-c5ce4b73b94143b5aa9a9ef45b66c5ca0a97c97862b3ae4ff7b01d72ee5b03b5R24

image

We should also figure out how to make the IDE add a space between a keyword and left paren.

SethTisue commented 1 month ago

very nice catch!