Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 13 additions & 2 deletions core/src/main/scala/cats/data/Kleisli.scala
Original file line number Diff line number Diff line change
Expand Up @@ -656,8 +656,19 @@ private[data] trait KleisliApply[F[_], A] extends Apply[Kleisli[F, A, *]] with K

override def map2Eval[B, C, Z](fa: Kleisli[F, A, B], fb: Eval[Kleisli[F, A, C]])(
f: (B, C) => Z
): Eval[Kleisli[F, A, Z]] =
Eval.now(Kleisli(a => F.map2Eval(fa.run(a), fb.map(_.run(a)))(f).value))
): Eval[Kleisli[F, A, Z]] = {
// We should only evaluate fb once
val memoFb = fb.memoize

Eval.now(Kleisli { a =>
val fb = fa.run(a)
val efc = memoFb.map(_.run(a))
val efz: Eval[F[Z]] = F.map2Eval(fb, efc)(f)
// This is not safe and results in stack overflows:
// see: https://github.com/typelevel/cats/issues/3947
efz.value
})
}

override def product[B, C](fb: Kleisli[F, A, B], fc: Kleisli[F, A, C]): Kleisli[F, A, (B, C)] =
Kleisli(a => F.product(fb.run(a), fc.run(a)))
Expand Down
40 changes: 40 additions & 0 deletions core/src/main/scala/cats/instances/list.scala
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,46 @@ trait ListInstances extends cats.kernel.instances.ListInstances {
wrapMutableIndexedSeq(as)
}(f))(_.toList)

/**
* This avoids making a very deep stack by building a tree instead
*/
override def traverse_[G[_], A, B](fa: List[A])(f: A => G[B])(implicit G: Applicative[G]): G[Unit] = {
// the cost of this is O(size log size)
// c(n) = n + 2 * c(n/2) = n + 2(n/2 log (n/2)) = n + n (logn - 1) = n log n
// invariant: size >= 1
def runHalf(size: Int, fa: List[A]): Eval[G[Unit]] =
if (size > 1) {
val leftSize = size / 2
val rightSize = size - leftSize
val (leftL, rightL) = fa.splitAt(leftSize)
runHalf(leftSize, leftL)
.flatMap { left =>
val right = runHalf(rightSize, rightL)
G.map2Eval(left, right) { (_, _) => () }
}
} else {
// avoid pattern matching when we know that there is only one element
val a = fa.head
// we evaluate this at most one time,
// always is a bit cheaper in such cases
//
// Here is the point of the laziness using Eval:
// we avoid calling f(a) or G.void in the
// event that the computation has already
// failed. We do not use laziness to avoid
// traversing fa, which we will do fully
// in all cases.
Eval.always {
val gb = f(a)
G.void(gb)
}
}

val len = fa.length
if (len == 0) G.unit
else runHalf(len, fa).value
}

def functor: Functor[List] = this

def align[A, B](fa: List[A], fb: List[B]): List[A Ior B] =
Expand Down
37 changes: 37 additions & 0 deletions core/src/main/scala/cats/instances/vector.scala
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,43 @@ trait VectorInstances extends cats.kernel.instances.VectorInstances {
final override def traverse[G[_], A, B](fa: Vector[A])(f: A => G[B])(implicit G: Applicative[G]): G[Vector[B]] =
G.map(Chain.traverseViaChain(fa)(f))(_.toVector)

/**
* This avoids making a very deep stack by building a tree instead
*/
override def traverse_[G[_], A, B](fa: Vector[A])(f: A => G[B])(implicit G: Applicative[G]): G[Unit] = {
// the cost of this is O(size)
// c(n) = 1 + 2 * c(n/2)
// invariant: size >= 1
def runHalf(size: Int, idx: Int): Eval[G[Unit]] =
if (size > 1) {
val leftSize = size / 2
val rightSize = size - leftSize
runHalf(leftSize, idx)
.flatMap { left =>
val right = runHalf(rightSize, idx + leftSize)
G.map2Eval(left, right) { (_, _) => () }
}
} else {
val a = fa(idx)
// we evaluate this at most one time,
// always is a bit cheaper in such cases
//
// Here is the point of the laziness using Eval:
// we avoid calling f(a) or G.void in the
// event that the computation has already
// failed. We do not use laziness to avoid
// traversing fa, which we will do fully
// in all cases.
Eval.always {
val gb = f(a)
G.void(gb)
}
}

val len = fa.length
if (len == 0) G.unit
else runHalf(len, 0).value
}
override def mapWithIndex[A, B](fa: Vector[A])(f: (A, Int) => B): Vector[B] =
fa.iterator.zipWithIndex.map(ai => f(ai._1, ai._2)).toVector

Expand Down
1 change: 1 addition & 0 deletions core/src/main/scala/cats/package.scala
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ package object cats {
override def ap[A, B](ff: A => B)(fa: A): B = ff(fa)
override def flatten[A](ffa: A): A = ffa
override def map2[A, B, Z](fa: A, fb: B)(f: (A, B) => Z): Z = f(fa, fb)
override def map2Eval[A, B, Z](fa: A, fb: Eval[B])(f: (A, B) => Z): Eval[Z] = fb.map(f(fa, _))
override def lift[A, B](f: A => B): A => B = f
override def imap[A, B](fa: A)(f: A => B)(fi: B => A): B = f(fa)
def foldLeft[A, B](a: A, b: B)(f: (B, A) => B) = f(b, a)
Expand Down
1 change: 1 addition & 0 deletions laws/src/main/scala/cats/laws/TraverseLaws.scala
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@ trait TraverseLaws[F[_]] extends FunctorLaws[F] with FoldableLaws[F] with Unorde

first <-> traverseFirst
}

def mapWithIndexRef[A, B](fa: F[A], f: (A, Int) => B): IsEq[F[B]] = {
val lhs = F.mapWithIndex(fa)(f)
val rhs = F.traverse(fa)(a => State((s: Int) => (s + 1, f(a, s)))).runA(0).value
Expand Down
12 changes: 12 additions & 0 deletions tests/src/test/scala/cats/tests/KleisliSuite.scala
Original file line number Diff line number Diff line change
Expand Up @@ -354,6 +354,18 @@ class KleisliSuite extends CatsSuite {
assertEquals(program.run(A123), List((1, "2", true)))
}

test("traverse_ doesn't stack overflow") {
// see: https://github.com/typelevel/cats/issues/3947
val resL = (1 to 10000).toList.traverse_(_ => Kleisli.liftF[Id, String, Unit](())).run("")
val resV = (1 to 10000).toVector.traverse_(_ => Kleisli.liftF[Id, String, Unit](())).run("")
assert(resL == resV)
}

test("traverse_ doesn't stack overflow with List + Eval") {
// see: https://github.com/typelevel/cats/issues/3947
(1 to 10000).toList.traverse_(_ => Kleisli.liftF[Eval, String, Unit](Eval.Unit)).run("").value
}

/**
* Testing that implicit resolution works. If it compiles, the "test" passes.
*/
Expand Down
6 changes: 6 additions & 0 deletions tests/src/test/scala/cats/tests/TraverseSuite.scala
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,12 @@ abstract class TraverseSuite[F[_]: Traverse](name: String)(implicit ArbFInt: Arb
}
}

test(s"Traverse[$name].traverse matches traverse_ with Option") {
forAll { (fa: F[Int], fn: Int => Option[Int]) =>
assert(Applicative[Option].void(fa.traverse(fn)) == fa.traverse_(fn))

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is no law on this, and probably should be, but requires source and binary changes to laws.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI laws can be introduced without breaking either source or binary compatibility

}
}

}

object TraverseSuite {
Expand Down