MonadError.map should not directly translate to zio.map

[domdorn]

I'm just trying to integrate a lib (https://github.com/bot4s/telegram/) that uses cats effect and I'm using the interop-cats module to get hold off a CatsMonadError.

The library uses map(f)
(
https://github.com/bot4s/telegram/blob/c40d679be5865d1a5caa897e0c0f2307922a0cca/core/src/com/bot4s/telegram/clients/SttpClient.scala#L79-L81
) where f can throw an exception ( https://github.com/bot4s/telegram/blob/c40d679be5865d1a5caa897e0c0f2307922a0cca/core/src/com/bot4s/telegram/api/RequestHandler.scala#L113-L120) .

In my local app, I've now overridden the map in

interop-cats/interop-cats/shared/src/main/scala/zio/interop/cats.scala

Lines 275 to 276 in 06a80e2

	override final def pure[A](a: A): ZIO[R, E, A] = ZIO.succeedNow(a)
	override final def map[A, B](fa: ZIO[R, E, A])(f: A => B): ZIO[R, E, B] = fa.map(f)

from

  override final def map[A, B](fa: ZIO[R, E, A])(f: A => B): ZIO[R, E, B]                = fa.map(f)

to

  override final def map[A, B](fa: ZIO[R, E, A])(f: A => B): ZIO[R, E, B]                = fa.flatMap(v => ZIO.effect(f(v)))

I'm also wondering if

  override final def pure[A](a: A): ZIO[R, E, A]                                         = ZIO.succeedNow(a)

should be really implemented this way, as I've noticed that ,e.g. the library is using stuff like
monadError.pure(logger.debug("blabla")) so I think it should be

  override final def pure[A](a: A): ZIO[R, E, A]                                         = ZIO.efffect(a)

( https://github.com/bot4s/telegram/blob/c40d679be5865d1a5caa897e0c0f2307922a0cca/core/src/com/bot4s/telegram/api/RequestHandler.scala#L34-L37 )

As I'm in no way an expert in this area, any advice would be greatly appreciated!

To integrate with the library, I've created the following class in my project:

import zio.{ZEnv, ZIO}
import cats.{MonadError => CMonadError}
import com.softwaremill.sttp.{
  Request,
  Response,
  SttpBackend,
  MonadAsyncError => SttpMonadAsyncError,
  MonadError => SttpMonadError
}
import zio.{Task, ZIO}

import scala.concurrent.Future


object ZIOSttp {
  type MyEffect[T] = ZIO[ZEnv, Throwable, T]

  case class ZIOSttpBackendWrapper[S](underlying: SttpBackend[Future, S]) extends SttpBackend[MyEffect, S] {
    override def send[T](request: Request[T, S]): MyEffect[Response[T]] = ZIO.fromFuture(ec => underlying.send(request))
    override def close(): Unit = underlying.close()
    override def responseMonad: SttpMonadError[MyEffect] = sttpMyEffectMonadError
  }


  implicit val sttpMyEffectMonadError: SttpMonadError[MyEffect] = new SttpMonadAsyncError[MyEffect] {
    override def async[T](register: (Either[Throwable, T] => Unit) => Unit): Task[T] = for {
        p <- zio.Promise.make[Throwable, T]
        _ = register {
          case Left(t) => p.fail(t)
          case Right(v) => p.succeed(v)
        }
        v <- p.await
      } yield v

    override def unit[T](t: T): MyEffect[T] = Task.effect(t)
    override def map[T, T2](fa: MyEffect[T])(f: T => T2): MyEffect[T2] = fa.flatMap(e => ZIO.effect(f(e)))
    override def flatMap[T, T2](fa: MyEffect[T])(f: T => MyEffect[T2]): MyEffect[T2] = fa.flatMap(f)
    override def error[T](t: Throwable): MyEffect[T] = ZIO.fail(t)
    override protected def handleWrappedError[T](rt: MyEffect[T])(h: PartialFunction[Throwable, MyEffect[T]]): MyEffect[T] = rt.catchAll(h)
  }


  def wrappedMonadError(): CMonadError[MyEffect, Throwable] = zio.interop.catz.monadErrorInstance.asInstanceOf[CMonadError[MyEffect, Throwable]]

  implicit def myEffectMonadError(): CMonadError[MyEffect, Throwable] = new CMonadError[MyEffect, Throwable] {

    override def map[A, B](fa: MyEffect[A])(f: A => B): MyEffect[B] = fa.flatMap(v => ZIO.effect(f(v)))

    override def raiseError[A](e: Throwable): MyEffect[A] = wrappedMonadError().raiseError(e)

    override def handleErrorWith[A](fa: MyEffect[A])(f: Throwable => MyEffect[A]): MyEffect[A] = fa.catchAll(e => f(e))

    override def pure[A](x: A): MyEffect[A] = ZIO.effect(x)

    override def flatMap[A, B](fa: MyEffect[A])(f: A => MyEffect[B]): MyEffect[B] = fa.flatMap(f)

    override def tailRecM[A, B](a: A)(f: A => MyEffect[Either[A, B]]): MyEffect[B] = wrappedMonadError().tailRecM(a)(f)
  }

}

[domdorn]

To check the behavior, I've created this test which passes:

import cats.MonadError
import org.scalatest.concurrent.ScalaFutures
import org.scalatest.matchers.should.Matchers
import org.scalatest.wordspec.AnyWordSpec

import scala.concurrent.Future
import cats.implicits._

import scala.concurrent.ExecutionContext.Implicits.global

class MonadErrorTest extends AnyWordSpec with Matchers with ScalaFutures {

  "my test" should {
    "demonstrate the behavior" in {


      val monadE = MonadError[Future,Throwable]

      def f(a: String): String = {
        throw new IllegalStateException("b")
      }

      val value = monadE.pure("a")

      try {
        val result = value.map(f)
        whenReady(result.failed) {
          case x: IllegalStateException =>
            println("received what was expected")
          case e => fail("received unexpected exception ", e)
        }
      } catch {
        case e => fail("an unhandled exception was thrown", e)
      }

    }
  }

}

[jdegoes]

@domdorn IMO the best fix would be in the library, which is relying on behavior that is undefined in Cats Effect. Other libraries like http4s and Doobie successfully cleaned up their undefined use of partial functions and no longer have this issue.

[domdorn]

@jdegoes so would you say, that the change I proposed here bot4s/telegram#106
for translating

response
      .map(_.unsafeBody)
      .map(processApiResponse[R])

to

     monadError.flatMap(monadError
     .map(response)(_.unsafeBody))(
     t => monadError.fromTry(Try(processApiResponse[R](t))) ) 

would be the right approach? I was actually surprised that the MonadError from cats for Future transformed the thrown exception, but then again, I'm no expert in cats and have only read the cats book once.

[domdorn]

Ok, I will try to make this library move to proper effects.
However, I'm wondering, if for the overall user experience it still wouldn't be better if we changed the methods in this compatibility-library to act in the "undefined but defacto way of cats effect".
That way, people would include this library, use the right MonadError and stuff would just work instead that they would need to chase bugs at runtime, spend hours on debugging and we tell them "the library you're using is doing it wrong" ..

We could even make this explicit by providing two MonadErrors.. a pure one (the current implementation) and one that just safeguards wherever possible (with obviously reduced performance).

[joroKr21]

This is only possible if the error type is fixed to Throwable and that at the time when we instantiate MonadError.
What if I use a generic MonadError and only later fix the error type to Throwable?
Then I would have different semantics depending at which point I fixed the error type. 😕

[joroKr21]

Related to #277

[adamgfraser]

Can we close this? I don't think it is actionable.

[domdorn]

hmm.. in the lib I'm integrating with, I created my own monad error which gracefully handles errors.. I'm quite sure others will also stumble upon this. However, I don't have any hard feelings about closing it.

[adamgfraser]

I agree it can be a problem if you have libraries that incorrectly throw exceptions within map but I'm just not sure what we can do about it in this library. If we want to be polymorphic in the error type, which I think we have to because we can't have different behavior when one type takes on one particular value, then we have no channel for this exception other than the Die channel.