From 82b1142b615726cc36798b60b7f502b0e0cac0dc Mon Sep 17 00:00:00 2001 From: "P. Oscar Boykin" Date: Fri, 11 Aug 2017 11:20:15 -1000 Subject: [PATCH 1/3] Added more implementations of map2Eval I hit a case where I wanted laziness with map2Eval and realized in many cases where we could have it, we do not. There were all the cases I saw that we missed, which I hope our existing tests cover. --- core/src/main/scala/cats/data/EitherT.scala | 8 ++++++++ core/src/main/scala/cats/data/IdT.scala | 4 ++++ core/src/main/scala/cats/data/Ior.scala | 6 ++++++ core/src/main/scala/cats/data/OptionT.scala | 6 ++++++ core/src/main/scala/cats/data/WriterT.scala | 5 +++++ core/src/main/scala/cats/instances/list.scala | 7 ++++++- core/src/main/scala/cats/instances/map.scala | 7 ++++++- core/src/main/scala/cats/instances/stream.scala | 8 ++++++++ 8 files changed, 49 insertions(+), 2 deletions(-) diff --git a/core/src/main/scala/cats/data/EitherT.scala b/core/src/main/scala/cats/data/EitherT.scala index 12ccca61db..2124e34a57 100644 --- a/core/src/main/scala/cats/data/EitherT.scala +++ b/core/src/main/scala/cats/data/EitherT.scala @@ -535,6 +535,14 @@ private[data] trait EitherTFunctor[F[_], L] extends Functor[EitherT[F, L, ?]] { private[data] trait EitherTMonad[F[_], L] extends Monad[EitherT[F, L, ?]] with EitherTFunctor[F, L] { implicit val F: Monad[F] def pure[A](a: A): EitherT[F, L, A] = EitherT.pure(a) + + override def map2Eval[A, B, Z](fa: EitherT[F, L, A], fb: Eval[EitherT[F, L, B]])(f: (A, B) => Z): Eval[EitherT[F, L, Z]] = + F.map2Eval(fa.value, fb.map(_.value)) { // if F has a lazy map2Eval, leverage it + case (Right(a), Right(b)) => Right(f(a, b)) + case (l @ Left(_), _) => l.rightCast[Z] + case (_, l @ Left(_)) => l.rightCast[Z] + }.map(EitherT(_)) + def flatMap[A, B](fa: EitherT[F, L, A])(f: A => EitherT[F, L, B]): EitherT[F, L, B] = fa flatMap f def tailRecM[A, B](a: A)(f: A => EitherT[F, L, Either[A, B]]): EitherT[F, L, B] = EitherT(F.tailRecM(a)(a0 => F.map(f(a0).value) { diff --git a/core/src/main/scala/cats/data/IdT.scala b/core/src/main/scala/cats/data/IdT.scala index c1d013a6a4..93a7efbf3e 100644 --- a/core/src/main/scala/cats/data/IdT.scala +++ b/core/src/main/scala/cats/data/IdT.scala @@ -55,6 +55,10 @@ private[data] sealed trait IdTApply[F[_]] extends Apply[IdT[F, ?]] with IdTFunct implicit val F0: Apply[F] override def ap[A, B](ff: IdT[F, A => B])(fa: IdT[F, A]): IdT[F, B] = fa.ap(ff) + + override def map2Eval[A, B, Z](fa: IdT[F, A], fb: Eval[IdT[F, B]])(f: (A, B) => Z): Eval[IdT[F, Z]] = + F0.map2Eval(fa.value, fb.map(_.value))(f) // if F0 has a lazy map2Eval, leverage it + .map(IdT(_)) } private[data] sealed trait IdTApplicative[F[_]] extends Applicative[IdT[F, ?]] with IdTApply[F] { diff --git a/core/src/main/scala/cats/data/Ior.scala b/core/src/main/scala/cats/data/Ior.scala index a712276caf..565c9823c8 100644 --- a/core/src/main/scala/cats/data/Ior.scala +++ b/core/src/main/scala/cats/data/Ior.scala @@ -170,6 +170,12 @@ private[data] sealed abstract class IorInstances extends IorInstances0 { def flatMap[B, C](fa: Ior[A, B])(f: B => Ior[A, C]): Ior[A, C] = fa.flatMap(f) + override def map2Eval[B, C, Z](fa: Ior[A, B], fb: Eval[Ior[A, C]])(f: (B, C) => Z): Eval[Ior[A, Z]] = + fa match { + case l @ Ior.Left(_) => Eval.now(l) // no need to evaluate fb + case notLeft => fb.map(fb => map2(notLeft, fb)(f)) + } + def tailRecM[B, C](b: B)(fn: B => Ior[A, Either[B, C]]): A Ior C = { @tailrec def loop(v: Ior[A, Either[B, C]]): A Ior C = v match { diff --git a/core/src/main/scala/cats/data/OptionT.scala b/core/src/main/scala/cats/data/OptionT.scala index dfe8665675..0a390a2a8a 100644 --- a/core/src/main/scala/cats/data/OptionT.scala +++ b/core/src/main/scala/cats/data/OptionT.scala @@ -260,6 +260,12 @@ private[data] trait OptionTMonad[F[_]] extends Monad[OptionT[F, ?]] { override def map[A, B](fa: OptionT[F, A])(f: A => B): OptionT[F, B] = fa.map(f) + override def map2Eval[A, B, Z](fa: OptionT[F, A], fb: Eval[OptionT[F, B]])(f: (A, B) => Z): Eval[OptionT[F, Z]] = + F.map2Eval(fa.value, fb.map(_.value)) { // if F has a lazy map2Eval, leverage it + case (Some(a), Some(b)) => Some(f(a, b)) + case _ => None + }.map(OptionT(_)) + def tailRecM[A, B](a: A)(f: A => OptionT[F, Either[A, B]]): OptionT[F, B] = OptionT(F.tailRecM(a)(a0 => F.map(f(a0).value)( _.fold(Either.right[A, Option[B]](None))(_.map(b => Some(b): Option[B])) diff --git a/core/src/main/scala/cats/data/WriterT.scala b/core/src/main/scala/cats/data/WriterT.scala index 204d99e7b5..e6d48ff1a3 100644 --- a/core/src/main/scala/cats/data/WriterT.scala +++ b/core/src/main/scala/cats/data/WriterT.scala @@ -225,6 +225,11 @@ private[data] sealed trait WriterTApply[F[_], L] extends WriterTFunctor[F, L] wi def ap[A, B](f: WriterT[F, L, A => B])(fa: WriterT[F, L, A]): WriterT[F, L, B] = fa ap f + + override def map2Eval[A, B, Z](fa: WriterT[F, L, A], fb: Eval[WriterT[F, L, B]])(f: (A, B) => Z): Eval[WriterT[F, L, Z]] = + F0.map2Eval(fa.run, fb.map(_.run)) { case ((la, a), (lb, b)) => (L0.combine(la, lb), f(a, b)) } + .map(WriterT(_)) // F0 may have a lazy map2Eval + override def product[A, B](fa: WriterT[F, L, A], fb: WriterT[F, L, B]): WriterT[F, L, (A, B)] = WriterT(F0.map(F0.product(fa.run, fb.run)) { case ((l1, a), (l2, b)) => (L0.combine(l1, l2), (a, b)) }) } diff --git a/core/src/main/scala/cats/instances/list.scala b/core/src/main/scala/cats/instances/list.scala index df4bdec252..431e6f6d87 100644 --- a/core/src/main/scala/cats/instances/list.scala +++ b/core/src/main/scala/cats/instances/list.scala @@ -23,7 +23,12 @@ trait ListInstances extends cats.kernel.instances.ListInstances { fa.flatMap(f) override def map2[A, B, Z](fa: List[A], fb: List[B])(f: (A, B) => Z): List[Z] = - fa.flatMap(a => fb.map(b => f(a, b))) + if (fb.isEmpty) Nil // do O(1) work if fb is empty + else fa.flatMap(a => fb.map(b => f(a, b))) // already O(1) if fa is empty + + override def map2Eval[A, B, Z](fa: List[A], fb: Eval[List[B]])(f: (A, B) => Z): Eval[List[Z]] = + if (fa.isEmpty) Eval.now(Nil) // no need to evaluate fb + else fb.map(fb => map2(fa, fb)(f)) def tailRecM[A, B](a: A)(f: A => List[Either[A, B]]): List[B] = { val buf = List.newBuilder[B] diff --git a/core/src/main/scala/cats/instances/map.scala b/core/src/main/scala/cats/instances/map.scala index 4074f7419b..e9afc6fd84 100644 --- a/core/src/main/scala/cats/instances/map.scala +++ b/core/src/main/scala/cats/instances/map.scala @@ -29,7 +29,12 @@ trait MapInstances extends cats.kernel.instances.MapInstances { fa.map { case (k, a) => (k, f(a)) } override def map2[A, B, Z](fa: Map[K, A], fb: Map[K, B])(f: (A, B) => Z): Map[K, Z] = - fa.flatMap { case (k, a) => fb.get(k).map(b => (k, f(a, b))) } + if (fb.isEmpty) Map.empty // do O(1) work if fb is empty + else fa.flatMap { case (k, a) => fb.get(k).map(b => (k, f(a, b))) } + + override def map2Eval[A, B, Z](fa: Map[K, A], fb: Eval[Map[K, B]])(f: (A, B) => Z): Eval[Map[K, Z]] = + if (fa.isEmpty) Eval.now(Map.empty) // no need to evaluate fb + else fb.map(fb => map2(fa, fb)(f)) override def ap[A, B](ff: Map[K, A => B])(fa: Map[K, A]): Map[K, B] = fa.flatMap { case (k, a) => ff.get(k).map(f => (k, f(a))) } diff --git a/core/src/main/scala/cats/instances/stream.scala b/core/src/main/scala/cats/instances/stream.scala index c434363651..008a9c5c7c 100644 --- a/core/src/main/scala/cats/instances/stream.scala +++ b/core/src/main/scala/cats/instances/stream.scala @@ -21,6 +21,14 @@ trait StreamInstances extends cats.kernel.instances.StreamInstances { def flatMap[A, B](fa: Stream[A])(f: A => Stream[B]): Stream[B] = fa.flatMap(f) + override def map2[A, B, Z](fa: Stream[A], fb: Stream[B])(f: (A, B) => Z): Stream[Z] = + if (fb.isEmpty) Stream.empty // do O(1) work if fb is empty + else fa.flatMap(a => fb.map(b => f(a, b))) // already O(1) if fa is empty + + override def map2Eval[A, B, Z](fa: Stream[A], fb: Eval[Stream[B]])(f: (A, B) => Z): Eval[Stream[Z]] = + if (fa.isEmpty) Eval.now(Stream.empty) // no need to evaluate fb + else fb.map(fb => map2(fa, fb)(f)) + def coflatMap[A, B](fa: Stream[A])(f: Stream[A] => B): Stream[B] = fa.tails.toStream.init.map(f) From 53a4c0a4da0c37a37c9e8bacee9bd7dc40d70a09 Mon Sep 17 00:00:00 2001 From: "P. Oscar Boykin" Date: Sat, 12 Aug 2017 08:28:14 -1000 Subject: [PATCH 2/3] Add tests for map2 and map2Eval --- laws/src/main/scala/cats/laws/ApplyLaws.scala | 6 ++++++ laws/src/main/scala/cats/laws/discipline/ApplyTests.scala | 5 ++++- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/laws/src/main/scala/cats/laws/ApplyLaws.scala b/laws/src/main/scala/cats/laws/ApplyLaws.scala index 4d248cc647..760e7c3330 100644 --- a/laws/src/main/scala/cats/laws/ApplyLaws.scala +++ b/laws/src/main/scala/cats/laws/ApplyLaws.scala @@ -14,6 +14,12 @@ trait ApplyLaws[F[_]] extends FunctorLaws[F] with CartesianLaws[F] { val compose: (B => C) => (A => B) => (A => C) = _.compose fbc.ap(fab.ap(fa)) <-> fbc.map(compose).ap(fab).ap(fa) } + + def map2ProductConsistency[A, B, C](fa: F[A], fb: F[B], f: (A, B) => C): IsEq[F[C]] = + F.map(F.product(fa, fb)) { case (a, b) => f(a, b) } <-> F.map2(fa, fb)(f) + + def map2EvalConsistency[A, B, C](fa: F[A], fb: F[B], f: (A, B) => C): IsEq[F[C]] = + F.map2(fa, fb)(f) <-> (F.map2Eval(fa, Eval.now(fb))(f).value) } object ApplyLaws { diff --git a/laws/src/main/scala/cats/laws/discipline/ApplyTests.scala b/laws/src/main/scala/cats/laws/discipline/ApplyTests.scala index 715ee64fbe..88a1526489 100644 --- a/laws/src/main/scala/cats/laws/discipline/ApplyTests.scala +++ b/laws/src/main/scala/cats/laws/discipline/ApplyTests.scala @@ -26,7 +26,10 @@ trait ApplyTests[F[_]] extends FunctorTests[F] with CartesianTests[F] { val name = "apply" val parents = Seq(functor[A, B, C], cartesian[A, B, C]) val bases = Seq.empty - val props = Seq("apply composition" -> forAll(laws.applyComposition[A, B, C] _)) + val props = Seq( + "apply composition" -> forAll(laws.applyComposition[A, B, C] _), + "map2/product-map consistency" -> forAll(laws.map2ProductConsistency[A, B, C] _), + "map2/map2Eval consistency" -> forAll(laws.map2EvalConsistency[A, B, C] _)) } } From fee3c6437bd8efef41a382df0e31692017812d2e Mon Sep 17 00:00:00 2001 From: "P. Oscar Boykin" Date: Sat, 12 Aug 2017 09:29:17 -1000 Subject: [PATCH 3/3] remove incorrect instances --- core/src/main/scala/cats/data/EitherT.scala | 7 ------- core/src/main/scala/cats/data/OptionT.scala | 6 ------ core/src/main/scala/cats/data/Tuple2K.scala | 7 +++++++ 3 files changed, 7 insertions(+), 13 deletions(-) diff --git a/core/src/main/scala/cats/data/EitherT.scala b/core/src/main/scala/cats/data/EitherT.scala index 2124e34a57..879b7ffb52 100644 --- a/core/src/main/scala/cats/data/EitherT.scala +++ b/core/src/main/scala/cats/data/EitherT.scala @@ -536,13 +536,6 @@ private[data] trait EitherTMonad[F[_], L] extends Monad[EitherT[F, L, ?]] with E implicit val F: Monad[F] def pure[A](a: A): EitherT[F, L, A] = EitherT.pure(a) - override def map2Eval[A, B, Z](fa: EitherT[F, L, A], fb: Eval[EitherT[F, L, B]])(f: (A, B) => Z): Eval[EitherT[F, L, Z]] = - F.map2Eval(fa.value, fb.map(_.value)) { // if F has a lazy map2Eval, leverage it - case (Right(a), Right(b)) => Right(f(a, b)) - case (l @ Left(_), _) => l.rightCast[Z] - case (_, l @ Left(_)) => l.rightCast[Z] - }.map(EitherT(_)) - def flatMap[A, B](fa: EitherT[F, L, A])(f: A => EitherT[F, L, B]): EitherT[F, L, B] = fa flatMap f def tailRecM[A, B](a: A)(f: A => EitherT[F, L, Either[A, B]]): EitherT[F, L, B] = EitherT(F.tailRecM(a)(a0 => F.map(f(a0).value) { diff --git a/core/src/main/scala/cats/data/OptionT.scala b/core/src/main/scala/cats/data/OptionT.scala index 0a390a2a8a..dfe8665675 100644 --- a/core/src/main/scala/cats/data/OptionT.scala +++ b/core/src/main/scala/cats/data/OptionT.scala @@ -260,12 +260,6 @@ private[data] trait OptionTMonad[F[_]] extends Monad[OptionT[F, ?]] { override def map[A, B](fa: OptionT[F, A])(f: A => B): OptionT[F, B] = fa.map(f) - override def map2Eval[A, B, Z](fa: OptionT[F, A], fb: Eval[OptionT[F, B]])(f: (A, B) => Z): Eval[OptionT[F, Z]] = - F.map2Eval(fa.value, fb.map(_.value)) { // if F has a lazy map2Eval, leverage it - case (Some(a), Some(b)) => Some(f(a, b)) - case _ => None - }.map(OptionT(_)) - def tailRecM[A, B](a: A)(f: A => OptionT[F, Either[A, B]]): OptionT[F, B] = OptionT(F.tailRecM(a)(a0 => F.map(f(a0).value)( _.fold(Either.right[A, Option[B]](None))(_.map(b => Some(b): Option[B])) diff --git a/core/src/main/scala/cats/data/Tuple2K.scala b/core/src/main/scala/cats/data/Tuple2K.scala index dfa895a9f1..28a40bbc8e 100644 --- a/core/src/main/scala/cats/data/Tuple2K.scala +++ b/core/src/main/scala/cats/data/Tuple2K.scala @@ -104,6 +104,13 @@ private[data] sealed trait Tuple2KApply[F[_], G[_]] extends Apply[λ[α => Tuple Tuple2K(F.ap(f.first)(fa.first), G.ap(f.second)(fa.second)) override def product[A, B](fa: Tuple2K[F, G, A], fb: Tuple2K[F, G, B]): Tuple2K[F, G, (A, B)] = Tuple2K(F.product(fa.first, fb.first), G.product(fa.second, fb.second)) + override def map2Eval[A, B, Z](fa: Tuple2K[F, G, A], fb: Eval[Tuple2K[F, G, B]])(f: (A, B) => Z): Eval[Tuple2K[F, G, Z]] = { + val fbmemo = fb.memoize // don't recompute this twice internally + for { + fz <- F.map2Eval(fa.first, fbmemo.map(_.first))(f) + gz <- G.map2Eval(fa.second, fbmemo.map(_.second))(f) + } yield Tuple2K(fz, gz) + } } private[data] sealed trait Tuple2KApplicative[F[_], G[_]] extends Applicative[λ[α => Tuple2K[F, G, α]]] with Tuple2KApply[F, G] {