Merge pull request scala#3848 from Ichoran/issue/8680

SI-8680  Stream.addString is too eager

Author: lrytz

bincompat-backward.whitelist.conf

@@ -194,6 +194,19 @@ filter {
     {
         matchName="scala.collection.immutable.Stream.filteredTail"
         problemName=MissingMethodProblem
+    },
+    // https://github.com/scala/scala/pull/3848 -- SI-8680
+    {
+        matchName="scala.collection.immutable.Stream.scala$collection$immutable$Stream$$loop$6"
+        problemName=MissingMethodProblem
+    },
+    {
+        matchName="scala.collection.immutable.Stream.scala$collection$immutable$Stream$$loop$5"
+        problemName=MissingMethodProblem
+    },
+    {
+        matchName="scala.collection.immutable.Stream.scala$collection$immutable$Stream$$loop$4"
+        problemName=MissingMethodProblem
     }
   ]
 }

bincompat-forward.whitelist.conf

@@ -368,6 +368,27 @@ filter {
     {
         matchName="scala.reflect.io.AbstractFile.filterImpl"
         problemName=MissingMethodProblem
+    },
+    // https://github.com/scala/scala/pull/3848 -- SI-8680
+    {
+        matchName="scala.collection.immutable.Stream.scala$collection$immutable$Stream$$loop$6"
+        problemName=MissingMethodProblem
+    },
+    {
+        matchName="scala.collection.immutable.Stream.scala$collection$immutable$Stream$$loop$5"
+        problemName=MissingMethodProblem
+    },
+    {
+        matchName="scala.collection.immutable.Stream.scala$collection$immutable$Stream$$loop$4"
+        problemName=MissingMethodProblem
+    },
+    {
+        matchName="scala.collection.immutable.Stream.scala$collection$immutable$Stream$$loop$3"
+        problemName=MissingMethodProblem
+    },
+    {
+        matchName="scala.collection.immutable.Stream.scala$collection$immutable$Stream$$loop$2"
+        problemName=MissingMethodProblem
     }
   ]
 }

src/library/scala/collection/immutable/Stream.scala

@@ -176,6 +176,12 @@ import scala.language.implicitConversions
  *    loop(1, 1)
  *  }
  *  }}}
+ * 
+ *  Note that `mkString` forces evaluation of a `Stream`, but `addString` does
+ *  not.  In both cases, a `Stream` that is or ends in a cycle 
+ *  (e.g. `lazy val s: Stream[Int] = 0 #:: s`) will convert additional trips
+ *  through the cycle to `...`.  Additionally, `addString` will display an
+ *  un-memoized tail as `?`.
  *
  *  @tparam A    the type of the elements contained in this stream.
  *
@@ -253,12 +259,22 @@ self =>
    * @note Often we use `Stream`s to represent an infinite set or series.  If
    * that's the case for your particular `Stream` then this function will never
    * return and will probably crash the VM with an `OutOfMemory` exception.
+   * This function will not hang on a finite cycle, however.
    *
    *  @return The fully realized `Stream`.
    */
   def force: Stream[A] = {
-    var these = this
-    while (!these.isEmpty) these = these.tail
+    // Use standard 2x 1x iterator trick for cycle detection ("those" is slow one)
+    var these, those = this
+    if (!these.isEmpty) these = these.tail
+    while (those ne these) {
+      if (these.isEmpty) return this
+      these = these.tail
+      if (these.isEmpty) return this
+      these = these.tail
+      if (these eq those) return this
+      those = those.tail
+    }
     this
   }
 
@@ -309,9 +325,24 @@ self =>
 
   override def toStream: Stream[A] = this
 
-  override def hasDefiniteSize = {
-    def loop(s: Stream[A]): Boolean = s.isEmpty || s.tailDefined && loop(s.tail)
-    loop(this)
+  override def hasDefiniteSize: Boolean = isEmpty || {
+    if (!tailDefined) false
+    else {
+      // Two-iterator trick (2x & 1x speed) for cycle detection.
+      var those = this
+      var these = tail
+      while (those ne these) {
+        if (these.isEmpty) return true
+        if (!these.tailDefined) return false
+        these = these.tail
+        if (these.isEmpty) return true
+        if (!these.tailDefined) return false
+        these = these.tail
+        if (those eq these) return false
+        those = those.tail
+      }
+      false  // Cycle detected
+    }
   }
 
   /** Create a new stream which contains all elements of this stream followed by
@@ -690,7 +721,8 @@ self =>
    *  `end`. Inside, the string representations of defined elements (w.r.t.
    *  the method `toString()`) are separated by the string `sep`. The method will
    *  not force evaluation of undefined elements. A tail of such elements will be
-   * represented by a `"?"` instead.
+   * represented by a `"?"` instead.  A cyclic stream is represented by a `"..."`
+   * at the point where the cycle repeats.
    *
    * @param b The [[collection.mutable.StringBuilder]] factory to which we need
    * to add the string elements.
@@ -701,16 +733,81 @@ self =>
    * resulting string.
    */
   override def addString(b: StringBuilder, start: String, sep: String, end: String): StringBuilder = {
-    def loop(pre: String, these: Stream[A]) {
-      if (these.isEmpty) b append end
-      else {
-        b append pre append these.head
-        if (these.tailDefined) loop(sep, these.tail)
-        else b append sep append "?" append end
+    b append start
+    if (!isEmpty) {
+      b append head
+      var cursor = this
+      var n = 1
+      if (cursor.tailDefined) {  // If tailDefined, also !isEmpty
+        var scout = tail
+        if (scout.isEmpty) {
+          // Single element.  Bail out early.
+          b append end
+          return b
+        }
+        if ((cursor ne scout) && scout.tailDefined) {
+          cursor = scout
+          scout = scout.tail        
+          // Use 2x 1x iterator trick for cycle detection; slow iterator can add strings
+          while ((cursor ne scout) && scout.tailDefined) {
+            b append sep append cursor.head
+            n += 1
+            cursor = cursor.tail
+            scout = scout.tail
+            if (scout.tailDefined) scout = scout.tail
+          }
+        }
+        if (!scout.tailDefined) {  // Not a cycle, scout hit an end
+          while (cursor ne scout) {
+            b append sep append cursor.head
+            n += 1
+            cursor = cursor.tail
+          }
+        }
+        else {
+          // Cycle.
+          // If we have a prefix of length P followed by a cycle of length C,
+          // the scout will be at position (P%C) in the cycle when the cursor
+          // enters it at P.  They'll then collide when the scout advances another
+          // C - (P%C) ahead of the cursor.
+          // If we run the scout P farther, then it will be at the start of
+          // the cycle: (C - (P%C) + (P%C)) == C == 0.  So if another runner
+          // starts at the beginning of the prefix, they'll collide exactly at
+          // the start of the loop.
+          var runner = this
+          var k = 0
+          while (runner ne scout) {
+            runner = runner.tail
+            scout = scout.tail
+            k += 1
+          }
+          // Now runner and scout are at the beginning of the cycle.  Advance
+          // cursor, adding to string, until it hits; then we'll have covered
+          // everything once.  If cursor is already at beginning, we'd better
+          // advance one first unless runner didn't go anywhere (in which case
+          // we've already looped once).
+          if ((cursor eq scout) && (k > 0)) {
+            b append sep append cursor.head
+            n += 1
+            cursor = cursor.tail
+          }
+          while (cursor ne scout) {
+            b append sep append cursor.head
+            n += 1
+            cursor = cursor.tail
+          }
+          // Subtract prefix length from total length for cycle reporting.
+          // (Not currently used, but probably a good idea for the future.)
+          n -= k
+        }
+      }
+      if (!cursor.isEmpty) {
+        // Either undefined or cyclic; we can check with tailDefined
+        if (!cursor.tailDefined) b append sep append "?"
+        else b append sep append "..."
       }
     }
-    b append start
-    loop("", this)
+    b append end
     b
   }
 

test/files/run/t8680.scala

@@ -0,0 +1,53 @@
+object Test extends App {
+  def pre(n: Int) = (-n to -1).toStream
+
+  def cyc(m: Int) = {
+    lazy val s: Stream[Int] = (0 until m).toStream #::: s
+    s
+  }
+
+  def precyc(n: Int, m: Int) = pre(n) #::: cyc(m)
+
+  def str(s: Stream[Int]) = {
+    val b = new StringBuilder
+    s.addString(b, "", "", "")
+    b.toString
+  }
+
+  def goal(n: Int, m: Int) = (-n until m).mkString + "..."
+
+  // Check un-forced cyclic and non-cyclic streams
+  assert(str(pre(2)) == pre(2).take(1).toList.mkString + "?")
+  assert(str(cyc(2)) == cyc(2).take(1).toList.mkString + "?")
+  assert(str(precyc(2,2)) == precyc(2,2).take(1).toList.mkString + "?")
+  assert(!pre(2).hasDefiniteSize)
+  assert(!cyc(2).hasDefiniteSize)
+  assert(!precyc(2,2).hasDefiniteSize)
+
+  // Check forced cyclic and non-cyclic streams
+  assert(str(pre(2).force) == (-2 to -1).mkString)
+  assert(str(cyc(2).force) == (0 until 2).mkString + "...")
+  assert(str(precyc(2,2).force) == (-2 until 2).mkString + "...")
+  assert(pre(2).force.hasDefiniteSize)
+  assert(!cyc(2).force.hasDefiniteSize)
+  assert(!precyc(2,2).force.hasDefiniteSize)
+
+  // Special cases
+  assert(str(cyc(1).force) == goal(0,1))
+  assert(str(precyc(1,6).force) == goal(1,6))
+  assert(str(precyc(6,1).force) == goal(6,1))
+
+  // Make sure there are no odd/even problems
+  for (n <- 3 to 4; m <- 3 to 4) {
+    assert(precyc(n,m).mkString == goal(n,m), s"mkString $n $m")
+    assert(!precyc(n,m).force.hasDefiniteSize, s"hasDef $n$m")
+  }
+
+  // Make sure there are no cycle/prefix modulus problems
+  for (i <- 6 to 8) {
+    assert(precyc(i,3).mkString == goal(i,3), s"mkString $i 3")
+    assert(precyc(3,i).mkString == goal(3,i), s"mkString 3 $i")
+    assert(!precyc(i,3).force.hasDefiniteSize, s"hasDef $i 3")
+    assert(!precyc(3,i).force.hasDefiniteSize, s"hasDef 3 $i")
+  }
+}