Merge pull request scala/scala#9174 from NthPortal/topic/mutation-tracking-iterators/PR

[bug#12009] Make ListBuffer's iterator fail-fast

Author: NthPortal

library/src/scala/collection/mutable/Growable.scala

@@ -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
@@ -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
  }

library/src/scala/collection/mutable/ListBuffer.scala

@@ -35,13 +35,15 @@ import scala.runtime.Statics.releaseFence
  * @define mayNotTerminateInf
  * @define willNotTerminateInf
  */
+@SerialVersionUID(-8428291952499836345L)
 class ListBuffer[A]
  extends AbstractBuffer[A]
  with SeqOps[A, ListBuffer, ListBuffer[A]]
  with StrictOptimizedSeqOps[A, ListBuffer, ListBuffer[A]]
  with ReusableBuilder[A, immutable.List[A]]
  with IterableFactoryDefaults[A, ListBuffer]
  with DefaultSerializable {
+ @transient private[this] var mutationCount: Int = 0
 
  private var first: List[A] = Nil
  private var last0: ::[A] = null
@@ -50,7 +52,7 @@ class ListBuffer[A]
 
  private type Predecessor[A0] = ::[A0] /*| Null*/
 
- def iterator = first.iterator
+ def iterator: Iterator[A] = new MutationTracker.CheckedIterator(first.iterator, mutationCount)
 
  override def iterableFactory: SeqFactory[ListBuffer] = ListBuffer
 
@@ -69,7 +71,12 @@ class ListBuffer[A]
  aliased = false
  }
 
- private def ensureUnaliased() = if (aliased) copyElems()
+ // we only call this before mutating things, so it's
+ // a good place to track mutations for the iterator
+ private def ensureUnaliased(): Unit = {
+ mutationCount += 1
+ if (aliased) copyElems()
+ }
 
  // Avoids copying where possible.
  override def toList: List[A] = {
@@ -97,6 +104,7 @@ class ListBuffer[A]
  }
 
  def clear(): Unit = {
+ mutationCount += 1
  first = Nil
  len = 0
  last0 = null
@@ -114,18 +122,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
@@ -230,13 +248,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)
+ }
  }
  }
 
@@ -275,15 +309,17 @@ class ListBuffer[A]
  }
 
  def mapInPlace(f: A => A): this.type = {
- ensureUnaliased()
+ mutationCount += 1
  val buf = new ListBuffer[A]
  for (elem <- this) buf += f(elem)
  first = buf.first
  last0 = buf.last0
+ aliased = false // we just assigned from a new instance
  this
  }
 
  def flatMapInPlace(f: A => IterableOnce[A]): this.type = {
+ mutationCount += 1
  var src = first
  var dst: List[A] = null
  last0 = null
@@ -299,6 +335,7 @@ class ListBuffer[A]
  src = src.tail
  }
  first = if(dst eq null) Nil else dst
+ aliased = false // we just rebuilt a fresh, unaliased instance
  this
  }
 
@@ -322,12 +359,24 @@ class ListBuffer[A]
  }
 
  def patchInPlace(from: Int, patch: collection.IterableOnce[A], replaced: Int): this.type = {
- val i = math.min(math.max(from, 0), length)
- val n = math.min(math.max(replaced, 0), length)
- ensureUnaliased()
- val p = locate(i)
- removeAfter(p, math.min(n, len - i))
- insertAfter(p, patch.iterator)
+ val _len = len
+ val _from = math.max(from, 0) // normalized
+ val _replaced = math.max(replaced, 0) // normalized
+ val it = patch.iterator
+
+ val nonEmptyPatch = it.hasNext
+ val nonEmptyReplace = (_from < _len) && (_replaced > 0)
+
+ // don't want to add a mutation or check aliasing (potentially expensive)
+ // if there's no patching to do
+ if (nonEmptyPatch || nonEmptyReplace) {
+ ensureUnaliased()
+ val i = math.min(_from, _len)
+ val n = math.min(_replaced, _len)
+ val p = locate(i)
+ removeAfter(p, math.min(n, _len - i))
+ insertAfter(p, it)
+ }
  this
  }
 

library/src/scala/collection/mutable/MutationTracker.scala

@@ -0,0 +1,78 @@
+/*
+ * Scala (https://www.scala-lang.org)
+ *
+ * Copyright EPFL and Lightbend, Inc.
+ *
+ * Licensed under Apache License 2.0
+ * (http://www.apache.org/licenses/LICENSE-2.0).
+ *
+ * See the NOTICE file distributed with this work for
+ * additional information regarding copyright ownership.
+ */
+
+package scala
+package collection
+package mutable
+
+import java.util.ConcurrentModificationException
+
+/**
+ * Utilities to check that mutations to a client that tracks
+ * its mutations have not occurred since a given point.
+ * [[Iterator `Iterator`]]s that perform this check automatically
+ * during iteration can be created by wrapping an `Iterator`
+ * in a [[MutationTracker.CheckedIterator `CheckedIterator`]],
+ * or by manually using the [[MutationTracker.checkMutations() `checkMutations`]]
+ * and [[MutationTracker.checkMutationsForIteration() `checkMutationsForIteration`]]
+ * methods.
+ */
+private object MutationTracker {
+
+ /**
+ * Checks whether or not the actual mutation count differs from
+ * the expected one, throwing an exception, if it does.
+ *
+ * @param expectedCount the expected mutation count
+ * @param actualCount the actual mutation count
+ * @param message the exception message in case of mutations
+ * @throws ConcurrentModificationException if the expected and actual
+ * mutation counts differ
+ */
+ @throws[ConcurrentModificationException]
+ def checkMutations(expectedCount: Int, actualCount: Int, message: String): Unit = {
+ if (actualCount != expectedCount) throw new ConcurrentModificationException(message)
+ }
+
+ /**
+ * Checks whether or not the actual mutation count differs from
+ * the expected one, throwing an exception, if it does. This method
+ * produces an exception message saying that it was called because a
+ * backing collection was mutated during iteration.
+ *
+ * @param expectedCount the expected mutation count
+ * @param actualCount the actual mutation count
+ * @throws ConcurrentModificationException if the expected and actual
+ * mutation counts differ
+ */
+ @throws[ConcurrentModificationException]
+ @inline def checkMutationsForIteration(expectedCount: Int, actualCount: Int): Unit =
+ checkMutations(expectedCount, actualCount, "mutation occurred during iteration")
+
+ /**
+ * An iterator wrapper that checks if the underlying collection has
+ * been mutated.
+ *
+ * @param underlying the underlying iterator
+ * @param mutationCount a by-name provider of the current mutation count
+ * @tparam A the type of the iterator's elements
+ */
+ final class CheckedIterator[A](underlying: Iterator[A], mutationCount: => Int) extends AbstractIterator[A] {
+ private[this] val expectedCount = mutationCount
+
+ def hasNext: Boolean = {
+ checkMutationsForIteration(expectedCount, mutationCount)
+ underlying.hasNext
+ }
+ def next(): A = underlying.next()
+ }
+}

library/src/scala/collection/mutable/Shrinkable.scala

@@ -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.
@@ -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
  }