Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Reducible for Eval and Id #1475

Merged
merged 2 commits into from
Jan 3, 2017
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
24 changes: 24 additions & 0 deletions core/src/main/scala/cats/Eval.scala
Original file line number Diff line number Diff line change
Expand Up @@ -310,6 +310,30 @@ private[cats] trait EvalInstances extends EvalInstances0 {
}
}

implicit val catsReducibleForEval: Reducible[Eval] =
Copy link
Contributor Author

Choose a reason for hiding this comment

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

should this be combined with the monad/comonad above?

I don't think we can ever implement Traverse[Eval] (I'd love to be wrong about that), so it didn't have the same ambiguous implicit concern that Id has (since Traverse[Id] also extends Foldable[Id], we don't want Reducible[Id] we have to either use priority, or unify them in the same instance).

Copy link
Contributor

Choose a reason for hiding this comment

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

Couldn't a fairly trivial Traverse[Eval] be implemented where traverse looks something like this?

def traverse[G[_]: Applicative, A, B](fa: Eval[A])(f: A => G[B]): G[Eval[B]] =
  f(fa.value).map(Eval.now)

It's a bit wonky, but the Comonad, Reducible, etc instances are already based on eager calls to .value, so I don't know if it's any different.

Copy link
Contributor

Choose a reason for hiding this comment

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

It occurs to me that such a Traverse instance can be defined for any comonad, so it it's probably a known thing with some known properties to people who know more than I do :)

Copy link
Contributor

Choose a reason for hiding this comment

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

It also occurs to me that a big difference between this Traverse instance and the instances that you've added is that yours shouldn't ever lead to stack overflows while this one is probably pretty likely to bite people with them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

none of our instances call .value unless the return type requires it. It seems to me we should keep that since otherwise you lose the stack safety if Eval, so that is what I really meant. If you are willing to call .value, Eval is isomorphic to Id I think, but the best practice of Eval (though perhaps not required, but the real rule might somewhat complex and difficult to verify) is to only call .value once at the end.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that limiting ourselves to instances that (if they need to call .value at all) only call .value once at the end is probably a good general rule. 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm sorry for all of the thinking out loud here, but I just realized that the traverse implementation above only calls .value once, just as the Reducible instances do. Is there a reason that we would consider this to be different? Maybe because the Eval in the return type suggests that we aren't calling .value?

new Reducible[Eval] {
def foldLeft[A, B](fa: Eval[A], b: B)(f: (B, A) => B): B =
f(b, fa.value)
def foldRight[A, B](fa: Eval[A], lb: Eval[B])(f: (A, Eval[B]) => Eval[B]): Eval[B] =
fa.flatMap(f(_, lb))

override def reduce[A](fa: Eval[A])(implicit A: Semigroup[A]): A =
fa.value
override def reduceLeft[A](fa: Eval[A])(f: (A, A) => A): A =
fa.value
def reduceLeftTo[A, B](fa: Eval[A])(f: A => B)(g: (B, A) => B): B =
f(fa.value)
override def reduceRight[A](fa: Eval[A])(f: (A, Eval[A]) => Eval[A]): Eval[A] =
fa
def reduceRightTo[A, B](fa: Eval[A])(f: A => B)(g: (A, Eval[B]) => Eval[B]): Eval[B] =
fa.map(f)
override def reduceRightOption[A](fa: Eval[A])(f: (A, Eval[A]) => Eval[A]): Eval[Option[A]] =
fa.map(Some(_))
override def reduceRightToOption[A, B](fa: Eval[A])(f: A => B)(g: (A, Eval[B]) => Eval[B]): Eval[Option[B]] =
fa.map { a => Some(f(a)) }
override def size[A](f: Eval[A]): Long = 1L
}

implicit def catsOrderForEval[A: Order]: Order[Eval[A]] =
new Order[Eval[A]] {
def compare(lx: Eval[A], ly: Eval[A]): Int =
Expand Down
20 changes: 18 additions & 2 deletions core/src/main/scala/cats/package.scala
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,8 @@ package object cats {
* encodes pure unary function application.
*/
type Id[A] = A
implicit val catsInstancesForId: Bimonad[Id] with Monad[Id] with Traverse[Id] =
new Bimonad[Id] with Monad[Id] with Traverse[Id] {
implicit val catsInstancesForId: Bimonad[Id] with Monad[Id] with Traverse[Id] with Reducible[Id] =
new Bimonad[Id] with Monad[Id] with Traverse[Id] with Reducible[Id] {
def pure[A](a: A): A = a
def extract[A](a: A): A = a
def flatMap[A, B](a: A)(f: A => B): B = f(a)
Expand All @@ -48,6 +48,22 @@ package object cats {
f(a, lb)
def traverse[G[_], A, B](a: A)(f: A => G[B])(implicit G: Applicative[G]): G[B] =
f(a)
override def reduce[A](fa: Id[A])(implicit A: Semigroup[A]): A =
fa
def reduceLeftTo[A, B](fa: Id[A])(f: A => B)(g: (B, A) => B): B =
f(fa)
override def reduceLeft[A](fa: Id[A])(f: (A, A) => A): A =
fa
override def reduceLeftToOption[A, B](fa: Id[A])(f: A => B)(g: (B, A) => B): Option[B] =
Some(f(fa))
override def reduceRight[A](fa: Id[A])(f: (A, Eval[A]) => Eval[A]): Eval[A] =
Now(fa)
def reduceRightTo[A, B](fa: Id[A])(f: A => B)(g: (A, Eval[B]) => Eval[B]): Eval[B] =
Now(f(fa))
override def reduceRightToOption[A, B](fa: Id[A])(f: A => B)(g: (A, Eval[B]) => Eval[B]): Eval[Option[B]] =
Now(Some(f(fa)))
override def reduceMap[A, B](fa: Id[A])(f: A => B)(implicit B: Semigroup[B]): B = f(fa)
override def size[A](fa: Id[A]): Long = 1L
}

type Eq[A] = cats.kernel.Eq[A]
Expand Down
5 changes: 4 additions & 1 deletion tests/src/test/scala/cats/tests/EvalTests.scala
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ package tests

import scala.math.min
import cats.laws.ComonadLaws
import cats.laws.discipline.{BimonadTests, CartesianTests, MonadTests, SerializableTests}
import cats.laws.discipline.{BimonadTests, CartesianTests, MonadTests, ReducibleTests, SerializableTests}
import cats.laws.discipline.arbitrary._
import cats.kernel.laws.{GroupLaws, OrderLaws}

Expand Down Expand Up @@ -99,6 +99,9 @@ class EvalTests extends CatsSuite {
checkAll("Bimonad[Eval]", SerializableTests.serializable(Bimonad[Eval]))
checkAll("Monad[Eval]", SerializableTests.serializable(Monad[Eval]))

checkAll("Eval[Int]", ReducibleTests[Eval].reducible[Option, Int, Int])
checkAll("Reducible[Eval]", SerializableTests.serializable(Reducible[Eval]))

checkAll("Eval[Int]", GroupLaws[Eval[Int]].group)

{
Expand Down
3 changes: 3 additions & 0 deletions tests/src/test/scala/cats/tests/IdTests.scala
Original file line number Diff line number Diff line change
Expand Up @@ -14,4 +14,7 @@ class IdTests extends CatsSuite {

checkAll("Id[Int]", TraverseTests[Id].traverse[Int, Int, Int, Int, Option, Option])
checkAll("Traverse[Id]", SerializableTests.serializable(Traverse[Id]))

checkAll("Id[Int]", ReducibleTests[Id].reducible[Option, Int, Int])
checkAll("Reducible[Id]", SerializableTests.serializable(Reducible[Id]))
}