From 50ba29c9914d4eb345893e3bfb57e7ae04d7da06 Mon Sep 17 00:00:00 2001 From: Andy Scott Date: Thu, 6 Jul 2017 23:58:41 -0700 Subject: [PATCH 1/7] Add indexed to Traverse --- core/src/main/scala/cats/Traverse.scala | 12 ++++++ .../test/scala/cats/tests/TraverseTests.scala | 41 +++++++++++++++++++ 2 files changed, 53 insertions(+) create mode 100644 tests/src/test/scala/cats/tests/TraverseTests.scala diff --git a/core/src/main/scala/cats/Traverse.scala b/core/src/main/scala/cats/Traverse.scala index 24015db6b9..f7fb664f2e 100644 --- a/core/src/main/scala/cats/Traverse.scala +++ b/core/src/main/scala/cats/Traverse.scala @@ -1,5 +1,7 @@ package cats +import cats.data.State + import simulacrum.typeclass /** @@ -97,4 +99,14 @@ import simulacrum.typeclass override def map[A, B](fa: F[A])(f: A => B): F[B] = traverse[Id, A, B](fa)(f) + + /** + * Traverses through the structure F, pairing the values with + * assigned indices. + * + * The behavior is consistent with the Scala collection library's + * `zipWithIndex` for collections such as `List`. + */ + def indexed[A](fa: F[A]): F[(A, Int)] = + traverse(fa)(a => State((s: Int) => (s + 1, (a, s)))).runA(0).value } diff --git a/tests/src/test/scala/cats/tests/TraverseTests.scala b/tests/src/test/scala/cats/tests/TraverseTests.scala new file mode 100644 index 0000000000..5e5f86d8c1 --- /dev/null +++ b/tests/src/test/scala/cats/tests/TraverseTests.scala @@ -0,0 +1,41 @@ +package cats +package tests + +import org.scalatest.prop.PropertyChecks +import org.scalacheck.Arbitrary + +import cats.instances.all._ + +abstract class TraverseCheck[F[_]: Traverse](name: String)(implicit ArbFInt: Arbitrary[F[Int]]) extends CatsSuite with PropertyChecks { + + test(s"Traverse[$name].indexed") { + forAll { (fa: F[Int]) => + fa.indexed.toList should === (fa.toList.zipWithIndex) + } + } + +} + +class TraverseListCheck extends TraverseCheck[List]("list") +class TraverseStreamCheck extends TraverseCheck[Stream]("stream") +class TraverseVectorCheck extends TraverseCheck[Vector]("vector") + +class TraverseTestsAdditional extends CatsSuite { + + def checkIndexedStackSafety[F[_]](fromRange: Range => F[Int])(implicit F: Traverse[F]): Unit = { + F.indexed(fromRange(1 to 500000)) + () + } + + test("Traverse[List].indexed stack safety") { + checkIndexedStackSafety[List](_.toList) + } + + test("Traverse[Stream].indexed stack safety") { + checkIndexedStackSafety[Stream](_.toStream) + } + + test("Traverse[Vector].indexed stack safety") { + checkIndexedStackSafety[Vector](_.toVector) + } +} From 44130583da2cb070c3d4ef131b99ba5a73367000 Mon Sep 17 00:00:00 2001 From: Andy Scott Date: Fri, 7 Jul 2017 13:49:04 -0700 Subject: [PATCH 2/7] Test .indexed with a smaller collection so builds aren't terribly slow ase enter the commit message for your changes. Lines starting --- tests/src/test/scala/cats/tests/TraverseTests.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/src/test/scala/cats/tests/TraverseTests.scala b/tests/src/test/scala/cats/tests/TraverseTests.scala index 5e5f86d8c1..bdfd3e3c4d 100644 --- a/tests/src/test/scala/cats/tests/TraverseTests.scala +++ b/tests/src/test/scala/cats/tests/TraverseTests.scala @@ -23,7 +23,7 @@ class TraverseVectorCheck extends TraverseCheck[Vector]("vector") class TraverseTestsAdditional extends CatsSuite { def checkIndexedStackSafety[F[_]](fromRange: Range => F[Int])(implicit F: Traverse[F]): Unit = { - F.indexed(fromRange(1 to 500000)) + F.indexed(fromRange(1 to 70000)) () } From 83a0883f24fd6a221bce9d8ca0c6e17a6c7dac9f Mon Sep 17 00:00:00 2001 From: Andy Scott Date: Fri, 7 Jul 2017 16:49:40 -0700 Subject: [PATCH 3/7] Add mapWithIndex to Traverse; implement indexed in terms of mapWithIndex --- core/src/main/scala/cats/Traverse.scala | 8 +++++++- tests/src/test/scala/cats/tests/TraverseTests.scala | 6 ++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/core/src/main/scala/cats/Traverse.scala b/core/src/main/scala/cats/Traverse.scala index f7fb664f2e..31f3521997 100644 --- a/core/src/main/scala/cats/Traverse.scala +++ b/core/src/main/scala/cats/Traverse.scala @@ -108,5 +108,11 @@ import simulacrum.typeclass * `zipWithIndex` for collections such as `List`. */ def indexed[A](fa: F[A]): F[(A, Int)] = - traverse(fa)(a => State((s: Int) => (s + 1, (a, s)))).runA(0).value + mapWithIndex(fa)((a, i) => (a, i)) + + /** + * Akin to [[map]], but also provides the value's index in structure `F`. + */ + def mapWithIndex[A, B](fa: F[A])(f: (A, Int) => B): F[B] = + traverse(fa)(a => State((s: Int) => (s + 1, f(a, s)))).runA(0).value } diff --git a/tests/src/test/scala/cats/tests/TraverseTests.scala b/tests/src/test/scala/cats/tests/TraverseTests.scala index bdfd3e3c4d..e6ccab21fe 100644 --- a/tests/src/test/scala/cats/tests/TraverseTests.scala +++ b/tests/src/test/scala/cats/tests/TraverseTests.scala @@ -14,6 +14,12 @@ abstract class TraverseCheck[F[_]: Traverse](name: String)(implicit ArbFInt: Arb } } + test(s"Traverse[$name].mapWithIndex") { + forAll { (fa: F[Int]) => + fa.mapWithIndex((a, i) => (a, i)).toList should === (fa.toList.zipWithIndex) + } + } + } class TraverseListCheck extends TraverseCheck[List]("list") From df74985b3b1786ee13d7b24bd20c5d8bf1646ee7 Mon Sep 17 00:00:00 2001 From: Andy Scott Date: Sun, 9 Jul 2017 19:46:28 -0700 Subject: [PATCH 4/7] Add .traverseWithIndex to Traverse --- core/src/main/scala/cats/Traverse.scala | 15 +++++++++++++-- .../src/test/scala/cats/tests/TraverseTests.scala | 6 ++++++ 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/core/src/main/scala/cats/Traverse.scala b/core/src/main/scala/cats/Traverse.scala index 31f3521997..e0a7e3efd8 100644 --- a/core/src/main/scala/cats/Traverse.scala +++ b/core/src/main/scala/cats/Traverse.scala @@ -1,6 +1,7 @@ package cats import cats.data.State +import cats.data.StateT import simulacrum.typeclass @@ -111,8 +112,18 @@ import simulacrum.typeclass mapWithIndex(fa)((a, i) => (a, i)) /** - * Akin to [[map]], but also provides the value's index in structure `F`. + * Akin to [[map]], but also provides the value's index in structure + * F when calling the function. */ def mapWithIndex[A, B](fa: F[A])(f: (A, Int) => B): F[B] = - traverse(fa)(a => State((s: Int) => (s + 1, f(a, s)))).runA(0).value + traverse(fa)(a => + State((s: Int) => (s + 1, f(a, s)))).runA(0).value + + /** + * Akin to [[traverse]], but also provides the value's index in + * structure F when calling the function. + */ + def traverseWithIndex[G[_], A, B](fa: F[A])(f: (A, Int) => G[B])(implicit G: Monad[G]): G[F[B]] = + traverse(fa)(a => + StateT((s: Int) => G.map(f(a, s))(b => (s + 1, b)))).runA(0) } diff --git a/tests/src/test/scala/cats/tests/TraverseTests.scala b/tests/src/test/scala/cats/tests/TraverseTests.scala index e6ccab21fe..61592ee905 100644 --- a/tests/src/test/scala/cats/tests/TraverseTests.scala +++ b/tests/src/test/scala/cats/tests/TraverseTests.scala @@ -20,6 +20,12 @@ abstract class TraverseCheck[F[_]: Traverse](name: String)(implicit ArbFInt: Arb } } + test(s"Traverse[$name].traverseWithIndex") { + forAll { (fa: F[Int]) => + fa.traverseWithIndex((a, i) => Option((a, i))).map(_.toList) should === (Option(fa.toList.zipWithIndex)) + } + } + } class TraverseListCheck extends TraverseCheck[List]("list") From f6d9ade1c27d5a5aa7b10215b76ff8c2bdd5198b Mon Sep 17 00:00:00 2001 From: Andy Scott Date: Mon, 10 Jul 2017 21:27:40 -0700 Subject: [PATCH 5/7] Rename indexed to zipWithIndex; Improve tests; add faster overrides --- core/src/main/scala/cats/Traverse.scala | 20 ++++++------- core/src/main/scala/cats/instances/list.scala | 6 ++++ .../main/scala/cats/instances/stream.scala | 6 ++++ .../main/scala/cats/instances/vector.scala | 6 ++++ .../test/scala/cats/tests/TraverseTests.scala | 30 ++++++++++--------- 5 files changed, 44 insertions(+), 24 deletions(-) diff --git a/core/src/main/scala/cats/Traverse.scala b/core/src/main/scala/cats/Traverse.scala index e0a7e3efd8..2a8bd178d7 100644 --- a/core/src/main/scala/cats/Traverse.scala +++ b/core/src/main/scala/cats/Traverse.scala @@ -101,16 +101,6 @@ import simulacrum.typeclass override def map[A, B](fa: F[A])(f: A => B): F[B] = traverse[Id, A, B](fa)(f) - /** - * Traverses through the structure F, pairing the values with - * assigned indices. - * - * The behavior is consistent with the Scala collection library's - * `zipWithIndex` for collections such as `List`. - */ - def indexed[A](fa: F[A]): F[(A, Int)] = - mapWithIndex(fa)((a, i) => (a, i)) - /** * Akin to [[map]], but also provides the value's index in structure * F when calling the function. @@ -126,4 +116,14 @@ import simulacrum.typeclass def traverseWithIndex[G[_], A, B](fa: F[A])(f: (A, Int) => G[B])(implicit G: Monad[G]): G[F[B]] = traverse(fa)(a => StateT((s: Int) => G.map(f(a, s))(b => (s + 1, b)))).runA(0) + + /** + * Traverses through the structure F, pairing the values with + * assigned indices. + * + * The behavior is consistent with the Scala collection library's + * `zipWithIndex` for collections such as `List`. + */ + def zipWithIndex[A](fa: F[A]): F[(A, Int)] = + mapWithIndex(fa)((a, i) => (a, i)) } diff --git a/core/src/main/scala/cats/instances/list.scala b/core/src/main/scala/cats/instances/list.scala index 9cb87d619c..8d8f614e26 100644 --- a/core/src/main/scala/cats/instances/list.scala +++ b/core/src/main/scala/cats/instances/list.scala @@ -70,6 +70,12 @@ trait ListInstances extends cats.kernel.instances.ListInstances { G.map2Eval(f(a), lglb)(_ :: _) }.value + override def mapWithIndex[A, B](fa: List[A])(f: (A, Int) => B): List[B] = + fa.view.zipWithIndex.map(ai => f(ai._1, ai._2)).toList + + override def zipWithIndex[A](fa: List[A]): List[(A, Int)] = + fa.zipWithIndex + @tailrec override def get[A](fa: List[A])(idx: Long): Option[A] = fa match { diff --git a/core/src/main/scala/cats/instances/stream.scala b/core/src/main/scala/cats/instances/stream.scala index 4af61f8144..8ade719625 100644 --- a/core/src/main/scala/cats/instances/stream.scala +++ b/core/src/main/scala/cats/instances/stream.scala @@ -55,6 +55,12 @@ trait StreamInstances extends cats.kernel.instances.StreamInstances { }.value } + override def mapWithIndex[A, B](fa: Stream[A])(f: (A, Int) => B): Stream[B] = + fa.zipWithIndex.map(ai => f(ai._1, ai._2)) + + override def zipWithIndex[A](fa: Stream[A]): Stream[(A, Int)] = + fa.zipWithIndex + def tailRecM[A, B](a: A)(fn: A => Stream[Either[A, B]]): Stream[B] = { val it: Iterator[B] = new Iterator[B] { var stack: Stream[Either[A, B]] = fn(a) diff --git a/core/src/main/scala/cats/instances/vector.scala b/core/src/main/scala/cats/instances/vector.scala index caeed44fb8..66a21d44fd 100644 --- a/core/src/main/scala/cats/instances/vector.scala +++ b/core/src/main/scala/cats/instances/vector.scala @@ -82,6 +82,12 @@ trait VectorInstances extends cats.kernel.instances.VectorInstances { G.map2Eval(f(a), lgvb)(_ +: _) }.value + override def mapWithIndex[A, B](fa: Vector[A])(f: (A, Int) => B): Vector[B] = + fa.view.zipWithIndex.map(ai => f(ai._1, ai._2)).toVector + + override def zipWithIndex[A](fa: Vector[A]): Vector[(A, Int)] = + fa.zipWithIndex + override def exists[A](fa: Vector[A])(p: A => Boolean): Boolean = fa.exists(p) diff --git a/tests/src/test/scala/cats/tests/TraverseTests.scala b/tests/src/test/scala/cats/tests/TraverseTests.scala index 61592ee905..8beb3f5842 100644 --- a/tests/src/test/scala/cats/tests/TraverseTests.scala +++ b/tests/src/test/scala/cats/tests/TraverseTests.scala @@ -8,21 +8,23 @@ import cats.instances.all._ abstract class TraverseCheck[F[_]: Traverse](name: String)(implicit ArbFInt: Arbitrary[F[Int]]) extends CatsSuite with PropertyChecks { - test(s"Traverse[$name].indexed") { + test(s"Traverse[$name].zipWithIndex") { forAll { (fa: F[Int]) => - fa.indexed.toList should === (fa.toList.zipWithIndex) + fa.zipWithIndex.toList should === (fa.toList.zipWithIndex) } } test(s"Traverse[$name].mapWithIndex") { - forAll { (fa: F[Int]) => - fa.mapWithIndex((a, i) => (a, i)).toList should === (fa.toList.zipWithIndex) + forAll { (fa: F[Int], fn: ((Int, Int)) => Int) => + fa.mapWithIndex((a, i) => fn((a, i))).toList should === (fa.toList.zipWithIndex.map(fn)) } } test(s"Traverse[$name].traverseWithIndex") { - forAll { (fa: F[Int]) => - fa.traverseWithIndex((a, i) => Option((a, i))).map(_.toList) should === (Option(fa.toList.zipWithIndex)) + forAll { (fa: F[Int], fn: ((Int, Int)) => (Int, Int)) => + val left = fa.traverseWithIndex((a, i) => fn((a, i))).map(_.toList) + val (xs, values) = fa.toList.zipWithIndex.map(fn).unzip + left should === ((xs.combineAll, values)) } } @@ -34,20 +36,20 @@ class TraverseVectorCheck extends TraverseCheck[Vector]("vector") class TraverseTestsAdditional extends CatsSuite { - def checkIndexedStackSafety[F[_]](fromRange: Range => F[Int])(implicit F: Traverse[F]): Unit = { - F.indexed(fromRange(1 to 70000)) + def checkZipWithIndexedStackSafety[F[_]](fromRange: Range => F[Int])(implicit F: Traverse[F]): Unit = { + F.zipWithIndex(fromRange(1 to 70000)) () } - test("Traverse[List].indexed stack safety") { - checkIndexedStackSafety[List](_.toList) + test("Traverse[List].zipWithIndex stack safety") { + checkZipWithIndexedStackSafety[List](_.toList) } - test("Traverse[Stream].indexed stack safety") { - checkIndexedStackSafety[Stream](_.toStream) + test("Traverse[Stream].zipWithIndex stack safety") { + checkZipWithIndexedStackSafety[Stream](_.toStream) } - test("Traverse[Vector].indexed stack safety") { - checkIndexedStackSafety[Vector](_.toVector) + test("Traverse[Vector].zipWithIndex stack safety") { + checkZipWithIndexedStackSafety[Vector](_.toVector) } } From e0109a7c56056009ddf86e83c5a9a2f9e8424cca Mon Sep 17 00:00:00 2001 From: Andy Scott Date: Mon, 10 Jul 2017 23:11:24 -0700 Subject: [PATCH 6/7] Force testing of overridden traverse methods for list/vector/stream instances --- .../test/scala/cats/tests/TraverseTests.scala | 27 ++++++++++++++++--- 1 file changed, 24 insertions(+), 3 deletions(-) diff --git a/tests/src/test/scala/cats/tests/TraverseTests.scala b/tests/src/test/scala/cats/tests/TraverseTests.scala index 8beb3f5842..7a68ae6b98 100644 --- a/tests/src/test/scala/cats/tests/TraverseTests.scala +++ b/tests/src/test/scala/cats/tests/TraverseTests.scala @@ -30,9 +30,30 @@ abstract class TraverseCheck[F[_]: Traverse](name: String)(implicit ArbFInt: Arb } -class TraverseListCheck extends TraverseCheck[List]("list") -class TraverseStreamCheck extends TraverseCheck[Stream]("stream") -class TraverseVectorCheck extends TraverseCheck[Vector]("vector") +object TraverseCheck { + // forces testing of the underlying implementation (avoids overridden methods) + abstract class Underlying[F[_]: Traverse](name: String)(implicit ArbFInt: Arbitrary[F[Int]]) + extends TraverseCheck(s"$name (underlying)")(proxyTraverse[F], ArbFInt) + + // proxies a traverse instance so we can test default implementations + // to achieve coverage using default datatype instances + private def proxyTraverse[F[_]: Traverse]: Traverse[F] = new Traverse[F] { + def foldLeft[A, B](fa: F[A], b: B)(f: (B, A) => B): B = + Traverse[F].foldLeft(fa, b)(f) + def foldRight[A, B](fa: F[A], lb: cats.Eval[B])(f: (A, Eval[B]) => Eval[B]): Eval[B] = + Traverse[F].foldRight(fa, lb)(f) + def traverse[G[_]: Applicative, A, B](fa: F[A])(f: A => G[B]): G[F[B]] = + Traverse[F].traverse(fa)(f) + } +} + +class TraverseListCheck extends TraverseCheck[List]("List") +class TraverseStreamCheck extends TraverseCheck[Stream]("Stream") +class TraverseVectorCheck extends TraverseCheck[Vector]("Vector") + +class TraverseListCheckUnderlying extends TraverseCheck.Underlying[List]("List") +class TraverseStreamCheckUnderlying extends TraverseCheck.Underlying[Stream]("Stream") +class TraverseVectorCheckUnderlying extends TraverseCheck.Underlying[Vector]("Vector") class TraverseTestsAdditional extends CatsSuite { From 9789c2d874f4e9ce3ea8325bea8d009364dcc298 Mon Sep 17 00:00:00 2001 From: Andy Scott Date: Fri, 14 Jul 2017 08:46:42 -0700 Subject: [PATCH 7/7] Rename traverseWithIndex to traverseWithIndexM; Change .view to .iterator for performance --- core/src/main/scala/cats/Traverse.scala | 6 +++++- core/src/main/scala/cats/instances/list.scala | 2 +- core/src/main/scala/cats/instances/vector.scala | 2 +- tests/src/test/scala/cats/tests/TraverseTests.scala | 4 ++-- 4 files changed, 9 insertions(+), 5 deletions(-) diff --git a/core/src/main/scala/cats/Traverse.scala b/core/src/main/scala/cats/Traverse.scala index 2a8bd178d7..4b6f86f534 100644 --- a/core/src/main/scala/cats/Traverse.scala +++ b/core/src/main/scala/cats/Traverse.scala @@ -112,8 +112,12 @@ import simulacrum.typeclass /** * Akin to [[traverse]], but also provides the value's index in * structure F when calling the function. + * + * This performs the traversal in a single pass but requires that + * effect G is monadic. An applicative traveral can be performed in + * two passes using [[zipWithIndex]] followed by [[traverse]]. */ - def traverseWithIndex[G[_], A, B](fa: F[A])(f: (A, Int) => G[B])(implicit G: Monad[G]): G[F[B]] = + def traverseWithIndexM[G[_], A, B](fa: F[A])(f: (A, Int) => G[B])(implicit G: Monad[G]): G[F[B]] = traverse(fa)(a => StateT((s: Int) => G.map(f(a, s))(b => (s + 1, b)))).runA(0) diff --git a/core/src/main/scala/cats/instances/list.scala b/core/src/main/scala/cats/instances/list.scala index 8d8f614e26..eb203aed25 100644 --- a/core/src/main/scala/cats/instances/list.scala +++ b/core/src/main/scala/cats/instances/list.scala @@ -71,7 +71,7 @@ trait ListInstances extends cats.kernel.instances.ListInstances { }.value override def mapWithIndex[A, B](fa: List[A])(f: (A, Int) => B): List[B] = - fa.view.zipWithIndex.map(ai => f(ai._1, ai._2)).toList + fa.iterator.zipWithIndex.map(ai => f(ai._1, ai._2)).toList override def zipWithIndex[A](fa: List[A]): List[(A, Int)] = fa.zipWithIndex diff --git a/core/src/main/scala/cats/instances/vector.scala b/core/src/main/scala/cats/instances/vector.scala index 66a21d44fd..5c466aa76c 100644 --- a/core/src/main/scala/cats/instances/vector.scala +++ b/core/src/main/scala/cats/instances/vector.scala @@ -83,7 +83,7 @@ trait VectorInstances extends cats.kernel.instances.VectorInstances { }.value override def mapWithIndex[A, B](fa: Vector[A])(f: (A, Int) => B): Vector[B] = - fa.view.zipWithIndex.map(ai => f(ai._1, ai._2)).toVector + fa.iterator.zipWithIndex.map(ai => f(ai._1, ai._2)).toVector override def zipWithIndex[A](fa: Vector[A]): Vector[(A, Int)] = fa.zipWithIndex diff --git a/tests/src/test/scala/cats/tests/TraverseTests.scala b/tests/src/test/scala/cats/tests/TraverseTests.scala index 7a68ae6b98..de48c339a1 100644 --- a/tests/src/test/scala/cats/tests/TraverseTests.scala +++ b/tests/src/test/scala/cats/tests/TraverseTests.scala @@ -20,9 +20,9 @@ abstract class TraverseCheck[F[_]: Traverse](name: String)(implicit ArbFInt: Arb } } - test(s"Traverse[$name].traverseWithIndex") { + test(s"Traverse[$name].traverseWithIndexM") { forAll { (fa: F[Int], fn: ((Int, Int)) => (Int, Int)) => - val left = fa.traverseWithIndex((a, i) => fn((a, i))).map(_.toList) + val left = fa.traverseWithIndexM((a, i) => fn((a, i))).map(_.toList) val (xs, values) = fa.toList.zipWithIndex.map(fn).unzip left should === ((xs.combineAll, values)) }