From 43d652865c914bf25a1d4c5bc9e2ccb72121eb63 Mon Sep 17 00:00:00 2001 From: Arman Bilge Date: Mon, 7 Mar 2022 08:42:40 +0000 Subject: [PATCH 1/8] Try to reproduce https://github.com/typelevel/cats-effect/issues/2858 --- tests/shared/src/test/scala/cats/effect/IOSpec.scala | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/tests/shared/src/test/scala/cats/effect/IOSpec.scala b/tests/shared/src/test/scala/cats/effect/IOSpec.scala index 694518c56e..03d5a6fa11 100644 --- a/tests/shared/src/test/scala/cats/effect/IOSpec.scala +++ b/tests/shared/src/test/scala/cats/effect/IOSpec.scala @@ -1342,6 +1342,16 @@ class IOSpec extends BaseSpec with Discipline with IOPlatformSpecification { case Right(_) => SyncIO.raiseError[Unit](new RuntimeException("Boom!")) } must completeAsSync(()) } + + "should not execute side-effects twice (#2858)" in ticked { implicit ticker => + var i = 0 + val io = (IO(i += 1) *> IO.cede).syncStep.unsafeRunSync() match { + case Left(io) => io + case Right(_) => IO.unit + } + io must completeAs(()) + i must beEqualTo(1) + } } "fiber repeated yielding test" in real { From b6a06070719b4679d172b08e46690a55bb494bb6 Mon Sep 17 00:00:00 2001 From: Arman Bilge Date: Mon, 7 Mar 2022 08:56:18 +0000 Subject: [PATCH 2/8] Actually reproduce #2858 --- tests/shared/src/test/scala/cats/effect/IOSpec.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/shared/src/test/scala/cats/effect/IOSpec.scala b/tests/shared/src/test/scala/cats/effect/IOSpec.scala index 03d5a6fa11..51e6bd038e 100644 --- a/tests/shared/src/test/scala/cats/effect/IOSpec.scala +++ b/tests/shared/src/test/scala/cats/effect/IOSpec.scala @@ -1345,7 +1345,7 @@ class IOSpec extends BaseSpec with Discipline with IOPlatformSpecification { "should not execute side-effects twice (#2858)" in ticked { implicit ticker => var i = 0 - val io = (IO(i += 1) *> IO.cede).syncStep.unsafeRunSync() match { + val io = (IO(i += 1) *> IO.cede as ()).syncStep.unsafeRunSync() match { case Left(io) => io case Right(_) => IO.unit } From dbbbcb1aa6abf243d97779db7091c5e307fda75c Mon Sep 17 00:00:00 2001 From: Arman Bilge Date: Mon, 7 Mar 2022 09:02:34 +0000 Subject: [PATCH 3/8] Fix #2859, but I suspect more bugs --- core/shared/src/main/scala/cats/effect/IO.scala | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/core/shared/src/main/scala/cats/effect/IO.scala b/core/shared/src/main/scala/cats/effect/IO.scala index d752e383ed..1a9adda225 100644 --- a/core/shared/src/main/scala/cats/effect/IO.scala +++ b/core/shared/src/main/scala/cats/effect/IO.scala @@ -843,7 +843,7 @@ sealed abstract class IO[+A] private () extends IOPlatform[A] { case IO.Map(ioe, f, _) => interpret(ioe).map { - case Left(_) => Left(io) + case Left(io) => Left(io.map(f)) case Right(a) => Right(f(a)) } @@ -919,7 +919,7 @@ object IO extends IOCompanionPlatform with IOLowPriorityImplicits { * Newtype encoding for an `IO` datatype that has a `cats.Applicative` capable of doing * parallel processing in `ap` and `map2`, needed for implementing `cats.Parallel`. * - * For converting back and forth you can use either the `Parallel[IO]` instance or + * For converting back and forth you can use either the `Parallel[IO]` instance or * the methods `cats.effect.kernel.Par.ParallelF.apply` for wrapping any `IO` value and * `cats.effect.kernel.Par.ParallelF.value` for unwrapping it. * From 9be07c8224c7f3118a9b404b188bf00daca02ec2 Mon Sep 17 00:00:00 2001 From: Arman Bilge Date: Mon, 7 Mar 2022 09:06:16 +0000 Subject: [PATCH 4/8] Fix flatMap --- core/shared/src/main/scala/cats/effect/IO.scala | 2 +- tests/shared/src/test/scala/cats/effect/IOSpec.scala | 12 +++++++++++- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/core/shared/src/main/scala/cats/effect/IO.scala b/core/shared/src/main/scala/cats/effect/IO.scala index 1a9adda225..735e450a0f 100644 --- a/core/shared/src/main/scala/cats/effect/IO.scala +++ b/core/shared/src/main/scala/cats/effect/IO.scala @@ -849,7 +849,7 @@ sealed abstract class IO[+A] private () extends IOPlatform[A] { case IO.FlatMap(ioe, f, _) => interpret(ioe).flatMap { - case Left(_) => SyncIO.pure(Left(io)) + case Left(io) => SyncIO.pure(Left(io.flatMap(f))) case Right(a) => interpret(f(a)) } diff --git a/tests/shared/src/test/scala/cats/effect/IOSpec.scala b/tests/shared/src/test/scala/cats/effect/IOSpec.scala index 51e6bd038e..3f5a44bb01 100644 --- a/tests/shared/src/test/scala/cats/effect/IOSpec.scala +++ b/tests/shared/src/test/scala/cats/effect/IOSpec.scala @@ -1343,7 +1343,7 @@ class IOSpec extends BaseSpec with Discipline with IOPlatformSpecification { } must completeAsSync(()) } - "should not execute side-effects twice (#2858)" in ticked { implicit ticker => + "should not execute side-effects twice for map (#2858)" in ticked { implicit ticker => var i = 0 val io = (IO(i += 1) *> IO.cede as ()).syncStep.unsafeRunSync() match { case Left(io) => io @@ -1352,6 +1352,16 @@ class IOSpec extends BaseSpec with Discipline with IOPlatformSpecification { io must completeAs(()) i must beEqualTo(1) } + + "should not execute side-effects twice for flatMap (#2858)" in ticked { implicit ticker => + var i = 0 + val io = (IO(i += 1) *> IO.cede *> IO.unit).syncStep.unsafeRunSync() match { + case Left(io) => io + case Right(_) => IO.unit + } + io must completeAs(()) + i must beEqualTo(1) + } } "fiber repeated yielding test" in real { From 882ef916dc89bdf5afe5289ea853dfe4e431bd70 Mon Sep 17 00:00:00 2001 From: Arman Bilge Date: Mon, 7 Mar 2022 09:08:43 +0000 Subject: [PATCH 5/8] Fix attempt --- core/shared/src/main/scala/cats/effect/IO.scala | 2 +- tests/shared/src/test/scala/cats/effect/IOSpec.scala | 10 ++++++++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/core/shared/src/main/scala/cats/effect/IO.scala b/core/shared/src/main/scala/cats/effect/IO.scala index 735e450a0f..0464771438 100644 --- a/core/shared/src/main/scala/cats/effect/IO.scala +++ b/core/shared/src/main/scala/cats/effect/IO.scala @@ -856,7 +856,7 @@ sealed abstract class IO[+A] private () extends IOPlatform[A] { case IO.Attempt(ioe) => interpret(ioe) .map { - case Left(_) => Left(io) + case Left(io) => Left(io.attempt) case Right(a) => Right(a.asRight[Throwable]) } .handleError(t => Right(t.asLeft[IO[B]])) diff --git a/tests/shared/src/test/scala/cats/effect/IOSpec.scala b/tests/shared/src/test/scala/cats/effect/IOSpec.scala index 3f5a44bb01..5740e639ff 100644 --- a/tests/shared/src/test/scala/cats/effect/IOSpec.scala +++ b/tests/shared/src/test/scala/cats/effect/IOSpec.scala @@ -1362,6 +1362,16 @@ class IOSpec extends BaseSpec with Discipline with IOPlatformSpecification { io must completeAs(()) i must beEqualTo(1) } + + "should not execute side-effects twice for attempt (#2858)" in ticked { implicit ticker => + var i = 0 + val io = ((IO(i += 1) *> IO.cede).attempt.void).syncStep.unsafeRunSync() match { + case Left(io) => io + case Right(_) => IO.unit + } + io must completeAs(()) + i must beEqualTo(1) + } } "fiber repeated yielding test" in real { From 54fbfdd22af8ce81b43c01d445af55ae57a86105 Mon Sep 17 00:00:00 2001 From: Arman Bilge Date: Mon, 7 Mar 2022 09:11:59 +0000 Subject: [PATCH 6/8] Fix handleErrorWith --- core/shared/src/main/scala/cats/effect/IO.scala | 2 +- .../shared/src/test/scala/cats/effect/IOSpec.scala | 14 ++++++++++++++ 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/core/shared/src/main/scala/cats/effect/IO.scala b/core/shared/src/main/scala/cats/effect/IO.scala index 0464771438..d01f93a609 100644 --- a/core/shared/src/main/scala/cats/effect/IO.scala +++ b/core/shared/src/main/scala/cats/effect/IO.scala @@ -864,7 +864,7 @@ sealed abstract class IO[+A] private () extends IOPlatform[A] { case IO.HandleErrorWith(ioe, f, _) => interpret(ioe) .map { - case Left(_) => Left(io) + case Left(io) => Left(io.handleErrorWith(f)) case Right(a) => Right(a) } .handleErrorWith(t => interpret(f(t))) diff --git a/tests/shared/src/test/scala/cats/effect/IOSpec.scala b/tests/shared/src/test/scala/cats/effect/IOSpec.scala index 5740e639ff..8b6b645733 100644 --- a/tests/shared/src/test/scala/cats/effect/IOSpec.scala +++ b/tests/shared/src/test/scala/cats/effect/IOSpec.scala @@ -1372,6 +1372,20 @@ class IOSpec extends BaseSpec with Discipline with IOPlatformSpecification { io must completeAs(()) i must beEqualTo(1) } + + "should not execute side-effects twice for handleErrorWith (#2858)" in ticked { + implicit ticker => + var i = 0 + val io = ((IO(i += 1) *> IO.cede) + .handleErrorWith(_ => IO.unit)) + .syncStep + .unsafeRunSync() match { + case Left(io) => io + case Right(_) => IO.unit + } + io must completeAs(()) + i must beEqualTo(1) + } } "fiber repeated yielding test" in real { From 4b3e074754d295cc3b8df89cae99fc56a468f74d Mon Sep 17 00:00:00 2001 From: Arman Bilge Date: Mon, 7 Mar 2022 09:28:22 +0000 Subject: [PATCH 7/8] Fix 2.12 compile, linting --- tests/shared/src/test/scala/cats/effect/IOSpec.scala | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/shared/src/test/scala/cats/effect/IOSpec.scala b/tests/shared/src/test/scala/cats/effect/IOSpec.scala index 8b6b645733..b6dda8e3b1 100644 --- a/tests/shared/src/test/scala/cats/effect/IOSpec.scala +++ b/tests/shared/src/test/scala/cats/effect/IOSpec.scala @@ -1345,7 +1345,7 @@ class IOSpec extends BaseSpec with Discipline with IOPlatformSpecification { "should not execute side-effects twice for map (#2858)" in ticked { implicit ticker => var i = 0 - val io = (IO(i += 1) *> IO.cede as ()).syncStep.unsafeRunSync() match { + val io = (IO(i += 1) *> IO.cede).void.syncStep.unsafeRunSync() match { case Left(io) => io case Right(_) => IO.unit } @@ -1365,7 +1365,7 @@ class IOSpec extends BaseSpec with Discipline with IOPlatformSpecification { "should not execute side-effects twice for attempt (#2858)" in ticked { implicit ticker => var i = 0 - val io = ((IO(i += 1) *> IO.cede).attempt.void).syncStep.unsafeRunSync() match { + val io = (IO(i += 1) *> IO.cede).attempt.void.syncStep.unsafeRunSync() match { case Left(io) => io case Right(_) => IO.unit } @@ -1376,8 +1376,8 @@ class IOSpec extends BaseSpec with Discipline with IOPlatformSpecification { "should not execute side-effects twice for handleErrorWith (#2858)" in ticked { implicit ticker => var i = 0 - val io = ((IO(i += 1) *> IO.cede) - .handleErrorWith(_ => IO.unit)) + val io = (IO(i += 1) *> IO.cede) + .handleErrorWith(_ => IO.unit) .syncStep .unsafeRunSync() match { case Left(io) => io From af3c6021d956c3fbdca69bb6b67822f32b03e402 Mon Sep 17 00:00:00 2001 From: Arman Bilge Date: Mon, 7 Mar 2022 19:22:55 +0000 Subject: [PATCH 8/8] Better test names --- tests/shared/src/test/scala/cats/effect/IOSpec.scala | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/shared/src/test/scala/cats/effect/IOSpec.scala b/tests/shared/src/test/scala/cats/effect/IOSpec.scala index b6dda8e3b1..e800edeb28 100644 --- a/tests/shared/src/test/scala/cats/effect/IOSpec.scala +++ b/tests/shared/src/test/scala/cats/effect/IOSpec.scala @@ -1343,7 +1343,7 @@ class IOSpec extends BaseSpec with Discipline with IOPlatformSpecification { } must completeAsSync(()) } - "should not execute side-effects twice for map (#2858)" in ticked { implicit ticker => + "should not execute effects twice for map (#2858)" in ticked { implicit ticker => var i = 0 val io = (IO(i += 1) *> IO.cede).void.syncStep.unsafeRunSync() match { case Left(io) => io @@ -1353,7 +1353,7 @@ class IOSpec extends BaseSpec with Discipline with IOPlatformSpecification { i must beEqualTo(1) } - "should not execute side-effects twice for flatMap (#2858)" in ticked { implicit ticker => + "should not execute effects twice for flatMap (#2858)" in ticked { implicit ticker => var i = 0 val io = (IO(i += 1) *> IO.cede *> IO.unit).syncStep.unsafeRunSync() match { case Left(io) => io @@ -1363,7 +1363,7 @@ class IOSpec extends BaseSpec with Discipline with IOPlatformSpecification { i must beEqualTo(1) } - "should not execute side-effects twice for attempt (#2858)" in ticked { implicit ticker => + "should not execute effects twice for attempt (#2858)" in ticked { implicit ticker => var i = 0 val io = (IO(i += 1) *> IO.cede).attempt.void.syncStep.unsafeRunSync() match { case Left(io) => io @@ -1373,7 +1373,7 @@ class IOSpec extends BaseSpec with Discipline with IOPlatformSpecification { i must beEqualTo(1) } - "should not execute side-effects twice for handleErrorWith (#2858)" in ticked { + "should not execute effects twice for handleErrorWith (#2858)" in ticked { implicit ticker => var i = 0 val io = (IO(i += 1) *> IO.cede)