Skip to content

Commit

Permalink
[bug#3088][bug#12121] Fix adding/subtracting ListBuffer to/from itself
Browse files Browse the repository at this point in the history
Fix appending a `ListBuffer` to itself and subtracting
a `ListBuffer` from itself.

Fix appending a `Growable` that uses the default
`addAll` implementation to itself.
  • Loading branch information
NthPortal committed Oct 21, 2020
1 parent 9faf48b commit 1390315
Show file tree
Hide file tree
Showing 5 changed files with 99 additions and 26 deletions.
12 changes: 8 additions & 4 deletions src/library/scala/collection/mutable/Growable.scala
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ package scala
package collection
package mutable

import scala.collection.IterableOnce
import scala.annotation.nowarn

/** This trait forms part of collections that can be augmented
* using a `+=` operator and that can be cleared of all elements using
Expand Down Expand Up @@ -56,10 +56,14 @@ trait Growable[-A] extends Clearable {
* @param xs the IterableOnce producing the elements to $add.
* @return the $coll itself.
*/
@nowarn("msg=will most likely never compare equal")
def addAll(xs: IterableOnce[A]): this.type = {
val it = xs.iterator
while (it.hasNext) {
addOne(it.next())
if (xs.asInstanceOf[AnyRef] eq this) addAll(Buffer.from(xs)) // avoid mutating under our own iterator
else {
val it = xs.iterator
while (it.hasNext) {
addOne(it.next())
}
}
this
}
Expand Down
58 changes: 42 additions & 16 deletions src/library/scala/collection/mutable/ListBuffer.scala
Original file line number Diff line number Diff line change
Expand Up @@ -114,18 +114,28 @@ class ListBuffer[A]

// Overridden for performance
override final def addAll(xs: IterableOnce[A]): this.type = {
val it = xs.iterator
if (it.hasNext) {
ensureUnaliased()
val last1 = new ::[A](it.next(), Nil)
if (len == 0) first = last1 else last0.next = last1
last0 = last1
len += 1
while (it.hasNext) {
if (xs.asInstanceOf[AnyRef] eq this) { // avoid mutating under our own iterator
if (len > 0) {
ensureUnaliased()
val copy = ListBuffer.from(this)
last0.next = copy.first
last0 = copy.last0
len *= 2
}
} else {
val it = xs.iterator
if (it.hasNext) {
ensureUnaliased()
val last1 = new ::[A](it.next(), Nil)
last0.next = last1
if (len == 0) first = last1 else last0.next = last1
last0 = last1
len += 1
while (it.hasNext) {
val last1 = new ::[A](it.next(), Nil)
last0.next = last1
last0 = last1
len += 1
}
}
}
this
Expand Down Expand Up @@ -230,13 +240,29 @@ class ListBuffer[A]
}

def insertAll(idx: Int, elems: IterableOnce[A]): Unit = {
ensureUnaliased()
val it = elems.iterator
if (it.hasNext) {
ensureUnaliased()
if (idx < 0 || idx > len) throw new IndexOutOfBoundsException(s"$idx is out of bounds (min 0, max ${len-1})")
if (idx == len) ++=(elems)
else insertAfter(locate(idx), it)
if (idx < 0 || idx > len) throw new IndexOutOfBoundsException(s"$idx is out of bounds (min 0, max ${len-1})")
elems match {
case elems: AnyRef if elems eq this => // avoid mutating under our own iterator
if (len > 0) {
val copy = ListBuffer.from(this)
if (idx == 0 || idx == len) { // prepend/append
last0.next = copy.first
last0 = copy.last0
} else {
val prev = locate(idx) // cannot be `null` because other condition catches that
val follow = prev.next
prev.next = copy.first
copy.last0.next = follow
}
len *= 2
}
case elems =>
val it = elems.iterator
if (it.hasNext) {
ensureUnaliased()
if (idx == len) ++=(elems)
else insertAfter(locate(idx), it)
}
}
}

Expand Down
16 changes: 12 additions & 4 deletions src/library/scala/collection/mutable/Shrinkable.scala
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
package scala
package collection.mutable

import scala.annotation.tailrec
import scala.annotation.{nowarn, tailrec}

/** This trait forms part of collections that can be reduced
* using a `-=` operator.
Expand Down Expand Up @@ -52,16 +52,24 @@ trait Shrinkable[-A] {
* @param xs the iterator producing the elements to remove.
* @return the $coll itself
*/
@nowarn("msg=will most likely never compare equal")
def subtractAll(xs: collection.IterableOnce[A]): this.type = {
@tailrec def loop(xs: collection.LinearSeq[A]): Unit = {
if (xs.nonEmpty) {
subtractOne(xs.head)
loop(xs.tail)
}
}
xs match {
case xs: collection.LinearSeq[A] => loop(xs)
case xs => xs.iterator.foreach(subtractOne)
if (xs.asInstanceOf[AnyRef] eq this) { // avoid mutating under our own iterator
xs match {
case xs: Clearable => xs.clear()
case xs => subtractAll(Buffer.from(xs))
}
} else {
xs match {
case xs: collection.LinearSeq[A] => loop(xs)
case xs => xs.iterator.foreach(subtractOne)
}
}
this
}
Expand Down
3 changes: 1 addition & 2 deletions test/files/run/t3088.scala
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,7 @@ import collection.mutable._

object Test {
def main(args: Array[String]): Unit = {
val b = new ListBuffer[Int]
b += 1
val b = ListBuffer(1, 2, 3)
b ++= b
}
}
36 changes: 36 additions & 0 deletions test/junit/scala/collection/mutable/ListBufferTest.scala
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ package scala.collection.mutable
import org.junit.Assert.{assertEquals, assertTrue}
import org.junit.Test

import scala.tools.testkit.AssertUtil.assertSameElements

import scala.annotation.nowarn

class ListBufferTest {
Expand Down Expand Up @@ -228,4 +230,38 @@ class ListBufferTest {
b += "b"
assertEquals(Seq("a1", "b"), b)
}

/* tests for scala/bug#12121 */

@Test
def self_addAll(): Unit = {
val b = ListBuffer(1, 2, 3)
b ++= b
assertSameElements(List(1, 2, 3, 1, 2, 3), b)
}

@Test
def self_prependAll(): Unit = {
val b = ListBuffer(1, 2, 3)
b prependAll b
assertSameElements(List(1, 2, 3, 1, 2, 3), b)
}

@Test
def self_insertAll(): Unit = {
val b1 = ListBuffer(1, 2, 3)
b1.insertAll(1, b1)
assertSameElements(List(1, 1, 2, 3, 2, 3), b1)

val b2 = ListBuffer(1, 2, 3)
b2.insertAll(3, b2)
assertSameElements(List(1, 2, 3, 1, 2, 3), b2)
}

@Test
def self_subtractAll(): Unit = {
val b = ListBuffer(1, 2, 3)
b --= b
assertSameElements(Nil, b)
}
}

0 comments on commit 1390315

Please sign in to comment.