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

Polymorphic fromFuture and ContextShift #563

Open
durban opened this issue Jun 13, 2019 · 7 comments

Comments

6 participants
@durban
Copy link
Contributor

commented Jun 13, 2019

As noted in #453, there is a simple solution to have fromFuture for a type parameter F[_]:

IO.fromFuture(IO(someFutureThing)).to[F]

Until recently, a LiftIO[F] was enough for this. But now, due to #546, a ContextShift[IO] is also necessary. Note that that is ContextShift[IO] and not ContextShift[F]. Which makes it significantly more inconvenient. Also note, that while #546 made fromFuture safer, an also safe and polymorphic version was already simple before (if ContextShift[F] is available):

IO.fromFuture(IO(someFutureThing)).to[F].guarantee(ContextShift[F].shift)

Now this is not possible without a ContextShift[IO].

I see two possible solutions:

  • have a polymorphic fromFuture (e.g., like in #453), which takes a ContextShift[F]; or
  • have a function to create a ContextShift[IO] from a ContextShift[F] (but Effect[F] seems to be necessary for this).
@alexandru

This comment has been minimized.

Copy link
Member

commented Jun 13, 2019

Until recently, a LiftIO[F] was enough for this

Just a note, working with LiftIO[F] is effectively working with IO and you're going to have a ContextShift[IO] anyway if you want to convert IO, polymorphically, to any F[_] that's cancellable, because you need ContextShift[IO] for the Concurrent[IO] instance.

In other words your concern is valid, however in actual use wherever there's a LiftIO[F] there's a high probability that the Concurrent[IO] instance is needed and thus ContextShift[IO] too.

That said I'm beginning to be in favor of a polymorphic fromFuture. Too many users are asking for it. I have a type class for it in Monix, but I don't mind having a polymorphic function in Cats-Effect, in the Async companion object.

@djspiewak

This comment has been minimized.

Copy link
Member

commented Jun 13, 2019

Yeah I'm thinking about this as well. I would actually prefer to put it on the typeclass, rather than the companion object, since implementors may have nicer implementations than the default.

@alexandru

This comment has been minimized.

Copy link
Member

commented Jun 13, 2019

TBH I don't think there's much variation on it.

My problem is that, while Scala's Future is a nice and popular implementation, it's not the only Future implementation that people have to deal with and the only thing that makes it special is that it's from the standard library. Otherwise people also have to deal with Java's old Future or with the newer CompletableFuture or with Twitter's Future, these being the most popular.

Adding a function isn't risky, but adding it to the type class is, in the sense that we'll be stuck with it.

But yeah, I guess the standard library isn't going away and I hear Future got some speeds improvements in 2.13 that got people excited, so it will be around for some time, therefore I personally wouldn't mind a function on the type class that much.

@rossabaker

This comment has been minimized.

Copy link
Member

commented Jun 13, 2019

👍. I have reimplemented it repeatedly, or pulled in monix-catnap (which is great) for just the one function. I also repeatedly have needed CompletableFuture, and support a function for that. java.util.concurrent.Future requires enough thoughtful tradeoffs to be out of scope in a conservative library. And Twitter future is a dependency liability that certainly belongs elsewhere.

Adding to the typeclass will further break compatibility on 2.11, though the ship already sailed on all versions with the change to IO.fromFuture. I suppose there's not an additional disadvantage?

@djspiewak

This comment has been minimized.

Copy link
Member

commented Jun 14, 2019

I suppose there's not an additional disadvantage?

Just typeclass bloat. It's also worth noting that we probably can't add it to Async, with the current organization of the hierarchy; it would need to be on Concurrent, unless we want to conflate the notion of shifting and pools with the current unshifted Async.

@djspiewak djspiewak added this to the 2.0 milestone Jun 25, 2019

@lorandszakacs

This comment has been minimized.

Copy link

commented Jun 26, 2019

How about a special "typeclass" for this operation?

I rolled my own thing in my projects (non-altered copy-paste):

@implicitAmbiguous(
  "FutureLift needs a bunch of machinery. Make sure you don't have two implicit scala.concurrent.ExecutionContext, or two cats.effect.ContextShift[F] in scope, or two LiftIO[F]",
)
@implicitNotFound(
  "FutureLift can only be instantiated for ContextShift[IO] for F[_]: LiftIO. You should instantiate one of these in your main, and propagate it further",
)
trait FutureLift[F[_]] {
  def fromFuture[A](fut: => Future[A]): F[A]
}

object FutureLift {

  def apply[F[_]](implicit fl: FutureLift[F]): FutureLift[F] = fl

  import cats.effect._

  implicit def instance[F[_]: LiftIO](implicit cs: ContextShift[IO]): FutureLift[F] = new FutureLiftIO[F]

  final private[FutureLift] class FutureLiftIO[F[_]](
    implicit
    private val cs:     ContextShift[IO],
    private val liftIO: LiftIO[F],
  ) extends FutureLift[F] {
    override def fromFuture[A](fut: => Future[A]): F[A] = IO.fromFuture(IO(fut)).to[F]
  }
}

Works like a charm. Instantiate once in main where I have ContextShift[IO], propagate where needed in polymorphic code.

Later edit:
Of course, for cats-effect it makes more sense to take a IO[Future[_]] instead of => Future[_].

@kubukoz

This comment has been minimized.

Copy link
Contributor

commented Jun 26, 2019

First thing is that this typeclass would be pretty much lawless. It's probably more feasible to implement it for F[_]: Async: ContextShift, inside Async's companion object...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.