Cats-effect: Nifty implicit conversions to Scala's Future to IO

Created on 27 Mar 2018  Â·  5Comments  Â·  Source: typelevel/cats-effect

I've been lifting the in Scala's Future to Async for a while now and came up with a simple implicit conversion for this:

final implicit class FutureOps[+T](f: Future[T]) {
  final def toIO: IO[T] = IO.async { callback ⇒
    f.onComplete {
      case Success(v) ⇒ callback(Right(v))
      case Failure(e) ⇒ callback(Left(e))
    }
  }
}

I was wondering if this is probably such common pattern in our daily work – that requires us to interact with libraries that expose Futures – so would make sense to add to the cats.effect.implicits package.

Do you guys have anything against adding this conversion?

If not so, let me know what the guidelines I should follow to submit a PR and contribute to this awesome project.

Thanks in advance!

Most helpful comment

I don't think implicit conversions are a good feature especially in a library as general as cats-effect.

The problem with this conversion is basically that it looks like you're delaying all effects untill you actually run an IO, but with your method once you have a Future it's already running that effect giving you a false sense of security, so I'm 👎 on this, if you need to convert your futures use the one provided that's guaranteed to be safe :)

All 5 comments

we already have that conversion IO.fromFuture(IO(yourFuture)) :)

I am aware of that but looked a little bit of cumbersome IMO and also remain the proposal to add a implicit conversion.

final implicit class FutureOps[+T](f: Future[T]) {
  final def toIO: IO[T] = final def toIO: IO[T] = IO.fromFuture(IO(f))
}

I don't think implicit conversions are a good feature especially in a library as general as cats-effect.

The problem with this conversion is basically that it looks like you're delaying all effects untill you actually run an IO, but with your method once you have a Future it's already running that effect giving you a false sense of security, so I'm 👎 on this, if you need to convert your futures use the one provided that's guaranteed to be safe :)

@hilios note that your implementation does not reflect the reality, which should be:

IO.fromFuture(IO.pure(f))

IO.pure because f is an already known Future value and so there's no point to delay anything via IO.apply. Which can catch users by surprise. Note that I'm not opposed to a function signature like this, which is what I actually proposed back in the early days of the project:

def fromFuture[A](fa: Future[A]): IO[A]

This is because the signature makes the evaluation model pretty clear imo and the IDE shows you the type of that parameter as soon as you have to supply it. And also in terms of naming, this is pretty clear:

IO.fromFuture(launchMissiles())

But then such a function is much more explicit than the extension method that you're proposing here.
Consider how it looks in code:

launchMissiles().toIO

Usability is not good, because the signature of toIO gets noticed after the fact if at all. Readability isn't good either.

If this will ever happen, it will happen part of a type-class like the one proposed at https://github.com/typelevel/cats-effect/issues/73; when I wrote that issue, I was thinking of Future, but the same argument can be used against that one as well. The @typeclass mechanism that we have (via Simulacrum) would provide a .toIO extension method for things implementing this ToIO type class. But that's a bit of a turn-off.

Also in the case of cats-effect we are wary of adding stuff, because it's a foundational library that other libraries depend on (aka middleware). So more useful utilities like converting from Java's Future type might get rejected as well: https://github.com/typelevel/cats-effect/issues/160

Guys, thanks so much for so elucidative and prompt feedback.

I agree that the implicit might cause the wrong impression because of the Future’s default behavior and a type class would be the preferred way to go in this matter.

Cheers.

Was this page helpful?
0 / 5 - 0 ratings

Related issues

Odomontois picture Odomontois  Â·  8Comments

kailuowang picture kailuowang  Â·  5Comments

vasilmkd picture vasilmkd  Â·  9Comments

kubukoz picture kubukoz  Â·  7Comments

RaasAhsan picture RaasAhsan  Â·  4Comments