From d55f5597bd11bf1a6efdfe20b96d57fbb21e87a9 Mon Sep 17 00:00:00 2001 From: Erik Osheim Date: Mon, 16 Nov 2015 10:00:51 -0500 Subject: [PATCH 1/3] Fix Streaming#memoization. This commit fixes a bug noticed by @ceedubs. The basic issue is that we have to be sure to memoize the Eval instance holding the tail while *also* memoizing the tail itself when it is calculated. Previously we were only memoizing the head node. --- core/src/main/scala/cats/data/Streaming.scala | 15 +++++++++++---- .../test/scala/cats/tests/StreamingTests.scala | 18 ++++++++++++++++++ 2 files changed, 29 insertions(+), 4 deletions(-) diff --git a/core/src/main/scala/cats/data/Streaming.scala b/core/src/main/scala/cats/data/Streaming.scala index 63177de81a..1ec4c8a770 100644 --- a/core/src/main/scala/cats/data/Streaming.scala +++ b/core/src/main/scala/cats/data/Streaming.scala @@ -642,14 +642,21 @@ sealed abstract class Streaming[A] extends Product with Serializable { lhs => * Ensure that repeated traversals of the stream will not cause * repeated tail computations. * - * By default stream does not memoize to avoid memory leaks when the - * head of the stream is retained. + * By default this structure does not memoize to avoid memory leaks + * when the head of the stream is retained. However, the user + * ultimately has control of the memoization approach based on what + * kinds of Eval instances they use. + * + * There are two calls to memoized here -- one is a recursive call + * to this method (on the tail) and the other is a call to memoize + * the Eval instance holding the tail. For more information on how + * .memoize works see Eval#memoize. */ def memoize: Streaming[A] = this match { case Empty() => Empty() - case Wait(lt) => Wait(lt.memoize) - case Cons(a, lt) => Cons(a, lt.memoize) + case Wait(lt) => Wait(lt.map(_.memoize).memoize) + case Cons(a, lt) => Cons(a, lt.map(_.memoize).memoize) } /** diff --git a/tests/src/test/scala/cats/tests/StreamingTests.scala b/tests/src/test/scala/cats/tests/StreamingTests.scala index e4eff26282..3cc9840c9c 100644 --- a/tests/src/test/scala/cats/tests/StreamingTests.scala +++ b/tests/src/test/scala/cats/tests/StreamingTests.scala @@ -23,6 +23,24 @@ class StreamingTests extends CatsSuite { class AdHocStreamingTests extends CatsSuite { + test("results aren't reevaluated after memoize") { + forAll { (orig: Streaming[Int]) => + val ns = orig.toList + val size = ns.size + + var i = 0 + val memoized = orig.map { n => i += 1; n }.memoize + + val xs = memoized.toList + i should === (size) + xs should === (ns) + + val ys = memoized.toList + i should === (size) + ys should === (ns) + } + } + // convert List[A] to Streaming[A] def convert[A](as: List[A]): Streaming[A] = Streaming.fromList(as) From 34edb5d2cb2e4185e0624f3062f86c8577c21a91 Mon Sep 17 00:00:00 2001 From: Erik Osheim Date: Mon, 16 Nov 2015 10:08:24 -0500 Subject: [PATCH 2/3] Improve comment. --- core/src/main/scala/cats/data/Streaming.scala | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/core/src/main/scala/cats/data/Streaming.scala b/core/src/main/scala/cats/data/Streaming.scala index 1ec4c8a770..6d020fdc39 100644 --- a/core/src/main/scala/cats/data/Streaming.scala +++ b/core/src/main/scala/cats/data/Streaming.scala @@ -647,10 +647,10 @@ sealed abstract class Streaming[A] extends Product with Serializable { lhs => * ultimately has control of the memoization approach based on what * kinds of Eval instances they use. * - * There are two calls to memoized here -- one is a recursive call + * There are two calls to .memoize here -- one is a recursive call * to this method (on the tail) and the other is a call to memoize * the Eval instance holding the tail. For more information on how - * .memoize works see Eval#memoize. + * this works see [[cats.Eval.memoize]]. */ def memoize: Streaming[A] = this match { From 8ed18f5501111752abb4fae2bee60918110b9d1d Mon Sep 17 00:00:00 2001 From: Erik Osheim Date: Thu, 19 Nov 2015 09:23:59 -0500 Subject: [PATCH 3/3] Removed unused code from streaming test. --- tests/src/test/scala/cats/tests/StreamingTests.scala | 4 ---- 1 file changed, 4 deletions(-) diff --git a/tests/src/test/scala/cats/tests/StreamingTests.scala b/tests/src/test/scala/cats/tests/StreamingTests.scala index 3c4b1601b1..c850729d59 100644 --- a/tests/src/test/scala/cats/tests/StreamingTests.scala +++ b/tests/src/test/scala/cats/tests/StreamingTests.scala @@ -55,10 +55,6 @@ class AdHocStreamingTests extends CatsSuite { } } - // convert List[A] to Streaming[A] - def convert[A](as: List[A]): Streaming[A] = - Streaming.fromList(as) - test("fromList/toList") { forAll { (xs: List[Int]) => Streaming.fromList(xs).toList should === (xs)