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.

Sign up for GitHub

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

Jump to bottom

Pull up laws #69

Merged
merged 4 commits into from
Jul 7, 2017
Merged

Pull up laws #69

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ded7e29
Moved stack safety laws into SyncLaws
djspiewak Jun 18, 2017
6cffbeb
Removed Kleisli instances (because of https://github.com/typelevel/ca…
djspiewak Jun 18, 2017
5490053
Pulled up error propagation laws
djspiewak Jun 18, 2017
09e64a7
Added runAsync handler law
djspiewak Jun 18, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
16 changes: 1 addition & 15 deletions core/shared/src/main/scala/cats/effect/Async.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ package effect

import simulacrum._

import cats.data.{EitherT, Kleisli, OptionT, StateT, WriterT}
import cats.data.{EitherT, OptionT, StateT, WriterT}

import scala.annotation.implicitNotFound
import scala.concurrent.ExecutionContext
Expand Down Expand Up @@ -67,9 +67,6 @@ private[effect] trait AsyncInstances {
implicit def catsEitherTAsync[F[_]: Async, L]: Async[EitherT[F, L, ?]] =
new EitherTAsync[F, L] { def F = Async[F] }

implicit def catsKleisliAsync[F[_]: Async, R]: Async[Kleisli[F, R, ?]] =
new KleisliAsync[F, R] { def F = Async[F] }

implicit def catsOptionTAsync[F[_]: Async]: Async[OptionT[F, ?]] =
new OptionTAsync[F] { def F = Async[F] }

Expand All @@ -92,17 +89,6 @@ private[effect] trait AsyncInstances {
EitherT.liftT(F.async(k))
}

private[effect] trait KleisliAsync[F[_], R]
extends Async[Kleisli[F, R, ?]]
with Sync.KleisliSync[F, R]
with LiftIO.KleisliLiftIO[F, R] {

override protected def F: Async[F]

def async[A](k: (Either[Throwable, A] => Unit) => Unit): Kleisli[F, R, A] =
Kleisli.lift(F.async(k))
}

private[effect] trait OptionTAsync[F[_]]
extends Async[OptionT[F, ?]]
with Sync.OptionTSync[F]
Expand Down
32 changes: 1 addition & 31 deletions core/shared/src/main/scala/cats/effect/Sync.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ package effect

import simulacrum._

import cats.data.{EitherT, Kleisli, OptionT, StateT, WriterT}
import cats.data.{EitherT, OptionT, StateT, WriterT}
import cats.effect.internals.NonFatal

/**
Expand Down Expand Up @@ -80,9 +80,6 @@ private[effect] trait SyncInstances {
}
}

implicit def catsKleisliSync[F[_]: Sync, R]: Sync[Kleisli[F, R, ?]] =
new KleisliSync[F, R] { def F = Sync[F] }

implicit def catsOptionTSync[F[_]: Sync]: Sync[OptionT[F, ?]] =
new OptionTSync[F] { def F = Sync[F] }

Expand Down Expand Up @@ -116,33 +113,6 @@ private[effect] trait SyncInstances {
}


private[effect] trait KleisliSync[F[_], R] extends Sync[Kleisli[F, R, ?]] {
protected def F: Sync[F]
private implicit def _F = F

def pure[A](x: A): Kleisli[F, R, A] = Kleisli.pure(x)

// remove duplication when we upgrade to cats 1.0
def handleErrorWith[A](fa: Kleisli[F, R, A])(f: Throwable => Kleisli[F, R, A]): Kleisli[F, R, A] = {
Kleisli { r: R =>
F.handleErrorWith(fa.run(r))(e => f(e).run(r))
}
}

// remove duplication when we upgrade to cats 1.0
def raiseError[A](e: Throwable): Kleisli[F, R, A] =
Kleisli.lift(F.raiseError(e))

def flatMap[A, B](fa: Kleisli[F, R, A])(f: A => Kleisli[F, R, B]): Kleisli[F, R, B] =
fa.flatMap(f)

def tailRecM[A, B](a: A)(f: A => Kleisli[F, R, Either[A, B]]): Kleisli[F, R, B] =
Kleisli.catsDataMonadReaderForKleisli[F, R].tailRecM(a)(f)

def suspend[A](thunk: => Kleisli[F, R, A]): Kleisli[F, R, A] =
Kleisli(r => F.suspend(thunk.run(r)))
}

private[effect] trait OptionTSync[F[_]] extends Sync[OptionT[F, ?]] {
protected def F: Sync[F]
private implicit def _F = F
Expand Down
2 changes: 1 addition & 1 deletion ...laws/discipline/EffectTestsPlatform.scala → ...ffect/laws/discipline/TestsPlatform.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,6 @@ package effect
package laws
package discipline

private[discipline] trait EffectTestsPlatform {
private[discipline] trait TestsPlatform {
final def isJVM = false
}
2 changes: 1 addition & 1 deletion ...laws/discipline/EffectTestsPlatform.scala → ...ffect/laws/discipline/TestsPlatform.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,6 @@ package effect
package laws
package discipline

private[discipline] trait EffectTestsPlatform {
private[discipline] trait TestsPlatform {
final def isJVM = true
}
6 changes: 6 additions & 0 deletions laws/shared/src/main/scala/cats/effect/laws/AsyncLaws.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,12 @@ trait AsyncLaws[F[_]] extends SyncLaws[F] {

change >> change >> read <-> F.pure(f(f(a)))
}

def propagateErrorsThroughBindAsync[A](t: Throwable) = {
val fa = F.attempt(F.async[A](_(Left(t))).flatMap(x => F.pure(x)))

fa <-> F.pure(Left(t))
}
}

object AsyncLaws {
Expand Down
48 changes: 5 additions & 43 deletions laws/shared/src/main/scala/cats/effect/laws/EffectLaws.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,11 @@ trait EffectLaws[F[_]] extends AsyncLaws[F] {
F.runAsync(fa)(e => IO { result = Some(e) }) >> read <-> IO.pure(Left(e))
}

def runAsyncIgnoresErrorInHandler[A](e: Throwable) = {
val fa = F.pure(())
F.runAsync(fa)(_ => IO.raiseError(e)) <-> IO.pure(())
}

def repeatedCallbackIgnored[A](a: A, f: A => A) = {
var cur = a
val change = F.delay(cur = f(cur))
Expand All @@ -54,49 +59,6 @@ trait EffectLaws[F[_]] extends AsyncLaws[F] {

test >> readResult <-> IO.pure(f(a))
}

lazy val stackSafetyOnRepeatedLeftBinds = {
val result = (0 until 10000).foldLeft(F.delay(())) { (acc, _) =>
acc.flatMap(_ => F.delay(()))
}

F.runAsync(result)(_ => IO.unit).unsafeRunSync() <-> (())
}

lazy val stackSafetyOnRepeatedRightBinds = {
val result = (0 until 10000).foldRight(F.delay(())) { (_, acc) =>
F.delay(()).flatMap(_ => acc)
}

F.runAsync(result)(_ => IO.unit).unsafeRunSync() <-> (())
}

lazy val stackSafetyOnRepeatedAttempts = {
val result = (0 until 10000).foldLeft(F.delay(())) { (acc, _) =>
F.attempt(acc).map(_ => ())
}

F.runAsync(result)(_ => IO.unit).unsafeRunSync() <-> (())
}

// the following law(s) should really be on MonadError
def propagateErrorsThroughBindSuspend[A](t: Throwable) = {
val fa = F.attempt(F.delay[A](throw t).flatMap(x => F.pure(x)))

var result: Either[Throwable, Either[Throwable, A]] = Left(new AssertionError)
val read = IO { result }

F.runAsync(fa)(e => IO { result = e }) >> read <-> IO.pure(Right(Left(t)))
}

def propagateErrorsThroughBindAsync[A](t: Throwable) = {
val fa = F.attempt(F.async[A](_(Left(t))).flatMap(x => F.pure(x)))

var result: Either[Throwable, Either[Throwable, A]] = Left(new AssertionError)
val read = IO { result }

F.runAsync(fa)(e => IO { result = e }) >> read <-> IO.pure(Right(Left(t)))
}
}

object EffectLaws {
Expand Down
30 changes: 30 additions & 0 deletions laws/shared/src/main/scala/cats/effect/laws/SyncLaws.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,36 @@ trait SyncLaws[F[_]] extends MonadErrorLaws[F, Throwable] {

change >> change >> read <-> F.pure(f(f(a)))
}

def propagateErrorsThroughBindSuspend[A](t: Throwable) = {
val fa = F.attempt(F.delay[A](throw t).flatMap(x => F.pure(x)))

fa <-> F.pure(Left(t))
}

lazy val stackSafetyOnRepeatedLeftBinds = {
val result = (0 until 10000).foldLeft(F.delay(())) { (acc, _) =>
acc.flatMap(_ => F.delay(()))
}

result <-> F.pure(())
}

lazy val stackSafetyOnRepeatedRightBinds = {
val result = (0 until 10000).foldRight(F.delay(())) { (_, acc) =>
F.delay(()).flatMap(_ => acc)
}

result <-> F.pure(())
}

lazy val stackSafetyOnRepeatedAttempts = {
val result = (0 until 10000).foldLeft(F.delay(())) { (acc, _) =>
F.attempt(acc).map(_ => ())
}

result <-> F.pure(())
}
}

object SyncLaws {
Expand Down
4 changes: 3 additions & 1 deletion laws/shared/src/main/scala/cats/effect/laws/discipline/AsyncTests.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ trait AsyncTests[F[_]] extends SyncTests[F] {
EqFA: Eq[F[A]],
EqFB: Eq[F[B]],
EqFC: Eq[F[C]],
EqFU: Eq[F[Unit]],
EqT: Eq[Throwable],
EqFEitherTU: Eq[F[Either[Throwable, Unit]]],
EqFEitherTA: Eq[F[Either[Throwable, A]]],
Expand All @@ -57,7 +58,8 @@ trait AsyncTests[F[_]] extends SyncTests[F] {
val props = Seq(
"async right is pure" -> forAll(laws.asyncRightIsPure[A] _),
"async left is raiseError" -> forAll(laws.asyncLeftIsRaiseError[A] _),
"repeated async evaluation not memoized" -> forAll(laws.repeatedAsyncEvaluationNotMemoized[A] _))
"repeated async evaluation not memoized" -> forAll(laws.repeatedAsyncEvaluationNotMemoized[A] _),
"propagate errors through bind (async)" -> forAll(laws.propagateErrorsThroughBindSuspend[A] _))
}
}
}
Expand Down
22 changes: 6 additions & 16 deletions laws/shared/src/main/scala/cats/effect/laws/discipline/EffectTests.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,12 @@ package effect
package laws
package discipline

import cats.instances.all._
import cats.laws.discipline._
import cats.laws.discipline.CartesianTests.Isomorphisms

import org.scalacheck._, Prop.forAll

trait EffectTests[F[_]] extends AsyncTests[F] with EffectTestsPlatform {
trait EffectTests[F[_]] extends AsyncTests[F] {
def laws: EffectLaws[F]

def effect[A: Arbitrary: Eq, B: Arbitrary: Eq, C: Arbitrary: Eq](
Expand All @@ -43,35 +42,26 @@ trait EffectTests[F[_]] extends AsyncTests[F] with EffectTestsPlatform {
EqFA: Eq[F[A]],
EqFB: Eq[F[B]],
EqFC: Eq[F[C]],
EqFU: Eq[F[Unit]],
EqT: Eq[Throwable],
EqFEitherTU: Eq[F[Either[Throwable, Unit]]],
EqFEitherTA: Eq[F[Either[Throwable, A]]],
EqFABC: Eq[F[(A, B, C)]],
EqFInt: Eq[F[Int]],
EqIOA: Eq[IO[A]],
EqIOU: Eq[IO[Unit]],
EqIOEitherTA: Eq[IO[Either[Throwable, A]]],
EqIOEitherEitherTA: Eq[IO[Either[Throwable, Either[Throwable, A]]]],
iso: Isomorphisms[F]): RuleSet = {
new RuleSet {
val name = "effect"
val bases = Nil
val parents = Seq(async[A, B, C])

val baseProps = Seq(
val props = Seq(
"runAsync pure produces right IO" -> forAll(laws.runAsyncPureProducesRightIO[A] _),
"runAsync raiseError produces left IO" -> forAll(laws.runAsyncRaiseErrorProducesLeftIO[A] _),
"repeated callback ignored" -> forAll(laws.repeatedCallbackIgnored[A] _),
"propagate errors through bind (suspend)" -> forAll(laws.propagateErrorsThroughBindSuspend[A] _),
"propagate errors through bind (async)" -> forAll(laws.propagateErrorsThroughBindSuspend[A] _))

val jvmProps = Seq(
"stack-safe on left-associated binds" -> Prop.lzy(laws.stackSafetyOnRepeatedLeftBinds),
"stack-safe on right-associated binds" -> Prop.lzy(laws.stackSafetyOnRepeatedRightBinds),
"stack-safe on repeated attempts" -> Prop.lzy(laws.stackSafetyOnRepeatedAttempts))

val jsProps = Seq.empty

val props = baseProps ++ (if (isJVM) jvmProps else jsProps)
"runAsync ignores error in handler" -> forAll(laws.runAsyncIgnoresErrorInHandler[A] _),
"repeated callback ignored" -> forAll(laws.repeatedCallbackIgnored[A] _))
}
}
}
Expand Down
18 changes: 15 additions & 3 deletions laws/shared/src/main/scala/cats/effect/laws/discipline/SyncTests.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ import cats.laws.discipline.CartesianTests.Isomorphisms

import org.scalacheck._, Prop.forAll

trait SyncTests[F[_]] extends MonadErrorTests[F, Throwable] {
trait SyncTests[F[_]] extends MonadErrorTests[F, Throwable] with TestsPlatform {
def laws: SyncLaws[F]

def sync[A: Arbitrary: Eq, B: Arbitrary: Eq, C: Arbitrary: Eq](
Expand All @@ -43,6 +43,7 @@ trait SyncTests[F[_]] extends MonadErrorTests[F, Throwable] {
EqFA: Eq[F[A]],
EqFB: Eq[F[B]],
EqFC: Eq[F[C]],
EqFU: Eq[F[Unit]],
EqT: Eq[Throwable],
EqFEitherTU: Eq[F[Either[Throwable, Unit]]],
EqFEitherTA: Eq[F[Either[Throwable, A]]],
Expand All @@ -54,13 +55,24 @@ trait SyncTests[F[_]] extends MonadErrorTests[F, Throwable] {
val name = "sync"
val bases = Nil
val parents = Seq(monadError[A, B, C])
val props = Seq(

val baseProps = Seq(
"delay constant is pure" -> forAll(laws.delayConstantIsPure[A] _),
"suspend constant is pure join" -> forAll(laws.suspendConstantIsPureJoin[A] _),
"throw in delay is raiseError" -> forAll(laws.delayThrowIsRaiseError[A] _),
"throw in suspend is raiseError" -> forAll(laws.suspendThrowIsRaiseError[A] _),
"unsequenced delay is no-op" -> forAll(laws.unsequencedDelayIsNoop[A] _),
"repeated sync evaluation not memoized" -> forAll(laws.repeatedSyncEvaluationNotMemoized[A] _))
"repeated sync evaluation not memoized" -> forAll(laws.repeatedSyncEvaluationNotMemoized[A] _),
"propagate errors through bind (suspend)" -> forAll(laws.propagateErrorsThroughBindSuspend[A] _))

val jvmProps = Seq(
Copy link
Member

Choose a reason for hiding this comment

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

Running stack safety tests on top of Javascript is still valuable and what I'd do here is to use a different count, e.g. 1000 instead of 10000 binds.

This is because we might end up with projects implementing Sync that don't care about the JVM at all.

Copy link
Member Author

Choose a reason for hiding this comment

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

What would be the best way to empirically determine the appropriate count?

Copy link
Member

Choose a reason for hiding this comment

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

I have no idea; basically you pick a value, then see if the tests run in a reasonable amount of time for cats.effect.IO.

Copy link
Member

Choose a reason for hiding this comment

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

I would set it to 1000. It's better than nothing and should be manageable on top of Node.js

Copy link
Member Author

Choose a reason for hiding this comment

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

In the interest of not stalling this on the petard of my lack of time, I'm going to merge this PR and open a new issue for reenabling the stack safety tests on JS with lower limits.

Copy link

Choose a reason for hiding this comment

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

The threshold for stack-safety is in my experience closer to 70000 function compositions than 10000, let alone 1000. That's what every cats stack-safety test uses. Does this test fail as is with a stack-unsafe monad?

Copy link
Member

Choose a reason for hiding this comment

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

I've seen tests fail for a stack depth of 1000, because it's usually multiplied by whatever else you're doing for each of those frames.

But indeed the number is chosen arbitrarily.

We should do a quick / simple Thunk-based implementation and measure with that.

Copy link
Member Author

Choose a reason for hiding this comment

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

@edmundnoble It does fail as it is with a stack-unsafe monad. I think it's just because there are more stack frames floating around. Or maybe it's just because my machine is allocating less stack space. Either way, it did seem to be pretty consistent when I was playing with it.

Copy link

Choose a reason for hiding this comment

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

Alright nice, thanks :) I bet we could cut it down for cats too.

"stack-safe on left-associated binds" -> Prop.lzy(laws.stackSafetyOnRepeatedLeftBinds),
"stack-safe on right-associated binds" -> Prop.lzy(laws.stackSafetyOnRepeatedRightBinds),
"stack-safe on repeated attempts" -> Prop.lzy(laws.stackSafetyOnRepeatedAttempts))

val jsProps = Seq()

val props = baseProps ++ (if (isJVM) jvmProps else jsProps)
}
}
}
Expand Down
4 changes: 0 additions & 4 deletions laws/shared/src/test/scala/cats/effect/InstancesTests.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ import cats.implicits._
import cats.laws.discipline.arbitrary.{
catsLawsArbitraryForEitherT,
catsLawsArbitraryForEval,
catsLawsArbitraryForKleisli,
catsLawsArbitraryForOptionT,
catsLawsArbitraryForWriterT
}
Expand All @@ -50,9 +49,6 @@ class InstancesTests extends BaseTestsSuite {
checkAllAsync("OptionT[IO, ?]",
implicit ec => AsyncTests[OptionT[IO, ?]].async[Int, Int, Int])

checkAllAsync("Kleisli[IO, Int, ?]",
implicit ec => AsyncTests[Kleisli[IO, Int, ?]].async[Int, Int, Int])

checkAllAsync("EitherT[IO, Throwable, ?]",
implicit ec => EffectTests[EitherT[IO, Throwable, ?]].effect[Int, Int, Int])

Expand Down