SI-8153 Mutation is hard, let's go shopping with an empty list

Changed the implementation of iterator to be more robust to mutation of the underlying ListBuffer.

Added a test to make sure bug is gone.

Fixed an "unsafe" usage of ListBuffer.iterator in the compiler, and added a comment explaining the (new) policy for iterating over a changing ListBuffer.

The compiler now uses a synchronized-wrapped ArrayBuffer instead of ListBuffer, as that makes the (custom) units Iterator more natural to write (and avoids O(n) lookups).

1 files changed, 23 insertions, 7 deletions

diff --git a/src/compiler/scala/tools/nsc/Global.scala b/src/compiler/scala/tools/nsc/Global.scala
index 1617db7517..81e96b76ac 100644
--- a/src/compiler/scala/tools/nsc/Global.scala
+++ b/src/compiler/scala/tools/nsc/Global.scala
@@ -1214,7 +1214,26 @@ class Global(var currentSettings: Settings, var reporter: Reporter)
     /** Have we already supplemented the error message of a compiler crash? */
     private[nsc] final var supplementedError = false
 
-    private val unitbuf = new mutable.ListBuffer[CompilationUnit]
+    private class SyncedCompilationBuffer { self =>
+      private val underlying = new mutable.ArrayBuffer[CompilationUnit]
+      def size = synchronized { underlying.size }
+      def +=(cu: CompilationUnit): this.type = { synchronized { underlying += cu }; this }
+      def head: CompilationUnit = synchronized{ underlying.head }
+      def apply(i: Int): CompilationUnit = synchronized { underlying(i) }
+      def iterator: Iterator[CompilationUnit] = new collection.AbstractIterator[CompilationUnit] {
+        private var used = 0
+        def hasNext = self.synchronized{ used < underlying.size }
+        def next = self.synchronized {
+          if (!hasNext) throw new NoSuchElementException("next on empty Iterator")
+          used += 1
+          underlying(used-1)
+        }
+      }
+      def toList: List[CompilationUnit] = synchronized{ underlying.toList }
+    }
+
+    private val unitbuf = new SyncedCompilationBuffer
+      
     val compiledFiles   = new mutable.HashSet[String]
 
     /** A map from compiled top-level symbols to their source files */
@@ -1225,9 +1244,8 @@ class Global(var currentSettings: Settings, var reporter: Reporter)
 
     private var phasec: Int  = 0   // phases completed
     private var unitc: Int   = 0   // units completed this phase
-    private var _unitbufSize = 0
 
-    def size = _unitbufSize
+    def size = unitbuf.size
     override def toString = "scalac Run for:\n  " + compiledFiles.toList.sorted.mkString("\n  ")
 
     // Calculate where to stop based on settings -Ystop-before or -Ystop-after.
@@ -1452,7 +1470,6 @@ class Global(var currentSettings: Settings, var reporter: Reporter)
     /** add unit to be compiled in this run */
     private def addUnit(unit: CompilationUnit) {
       unitbuf += unit
-      _unitbufSize += 1 // counting as they're added so size is cheap
       compiledFiles += unit.source.file.path
     }
     private def checkDeprecatedSettings(unit: CompilationUnit) {
@@ -1468,11 +1485,10 @@ class Global(var currentSettings: Settings, var reporter: Reporter)
     /* !!! Note: changing this to unitbuf.toList.iterator breaks a bunch
        of tests in tests/res.  This is bad, it means the resident compiler
        relies on an iterator of a mutable data structure reflecting changes
-       made to the underlying structure (in whatever accidental way it is
-       currently depending upon.)
+       made to the underlying structure.
      */
     def units: Iterator[CompilationUnit] = unitbuf.iterator
-
+    
     def registerPickle(sym: Symbol): Unit = ()
 
     /** does this run compile given class, module, or case factory? */