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

Should we define instances for () => Future[A] ? #169

Closed
alexandru opened this issue Mar 22, 2018 · 23 comments
Closed

Should we define instances for () => Future[A] ? #169

alexandru opened this issue Mar 22, 2018 · 23 comments

Comments

@alexandru
Copy link
Member

alexandru commented Mar 22, 2018

Sync, Async and Effect instances are possible for () => Future[A].
Should we provide them?

In fact we have such instances used in testing, see:
https://github.com/typelevel/cats-effect/blob/v0.10/laws/shared/src/test/scala/cats/effect/LTask.scala

I believe we should.

@johnynek
Copy link

I agree. I don't like it when we shame certain types we don't like even though they may be commonly used. As long as the intended use is lawful, let's do it.

@rossabaker
Copy link
Member

I agree. I think it carries its weight educationally, if not practically.

@johnynek
Copy link

I don't know if the left flatMap law will pass. tailRecM works here, but requiring the left flatmap to pass may require returning something like:

case class FlatMap[A, B](fn0: () => Future[A], fmfn: A => (() => Future[B])) extends Function0[Future[B]] {
  // trampoline inside apply

@SystemFw
Copy link
Contributor

I'm 👎 (mild)

why not provide instances for () => Either[Throwable, A] as well then?

@LukaJCB
Copy link
Member

LukaJCB commented Mar 22, 2018

We do have those instances for EitherT over Eval, which is basically isomorphic, though. We could probably avoid the stack safety issues @johnynek mentioned if we used Eval[Future [A]]. Overall I'm very neutral on this, but I do like the educational aspect.

@SystemFw
Copy link
Contributor

We do have those instances for EitherT over Eval,

That sounds dodgier actually. Anyway don't take my opinion as a blocker, I don't really care that much as long as the laws pass.

@johnynek
Copy link

providing a Sync instance for () => Try[A] and the Either variant is a good idea!

@djspiewak
Copy link
Member

djspiewak commented Mar 22, 2018

I know I've been a little silent recently, but I think it's worth weighing in on this one…

If the SyncLaws pass for Sync[Future], then something is fundamentally broken. The explicit intention of the repeated evaluation law was to rule out instances such as Sync[Future]. The reason for this has nothing to do with a subjective dislike for Future, but simply the fact that Future cannot be meaningfully used to manage effects due to the (default) eager evaluation and (unavoidable) memoization! In fact, there is actually a very strong argument to be made that Future does not even form a lawful Monad, though side-effects are required to observe the inequalities.

I'm very 👎 on this. If I write a function that expects an F[_]: Sync, I have certain reasonable expectations about that F[_]. Referential transparency is 100% part of that expectation set, and Future violates this.

It is possible to write a lawful instance for Eval[Future[A]], and I certainly wouldn't be opposed to it. Though I suspect there will still be issues with error handling due to the way that Future boxes exceptions. (at least, this was an issue the last time I experimented with defining such an instance)

@SystemFw
Copy link
Contributor

SystemFw commented Mar 22, 2018

Thankfully an instance for Sync[Future] isn't being proposed (or I would strongly oppose it).
What's being proposed in an instance for Function0[Future[A]], and I still don't like it for the same reason I don't like Sync[Eval[A]], they both have Comonad instances whose extract method would then become unsafeRun. In fact, after finding out that Function0 has a Comonad instance, my 👎 is now stronger.

@djspiewak
Copy link
Member

Ah, I misread! My bad! :-) Sorry about that.

@SystemFw
Copy link
Contributor

object Problem {
  import cats._, implicits._
  import scala.concurrent._
  import cats.effect._

  type LTask[A] = () => Future[A]

  implicit def effInsLT(implicit ec: ExecutionContext): Sync[LTask] =
    new Sync[LTask] {
      def pure[A](x: A): LTask[A] =
        () => Future.successful(x)
      def raiseError[A](e: Throwable): LTask[A] =
        () => Future.failed(e)
      def suspend[A](thunk: => LTask[A]): LTask[A] =
        () => Future.successful(()).flatMap(_ => thunk())

      def flatMap[A, B](fa: LTask[A])(f: A => LTask[B]): LTask[B] =
        () =>
          Future.successful(()).flatMap { _ =>
            fa().flatMap { a => f(a)() }
          }


      def tailRecM[A, B](a: A)(f: A => LTask[Either[A, B]]): LTask[B] =
        flatMap(f(a)) {
          case Left(a) => tailRecM(a)(f)
          case Right(b) => pure(b)
        }

      def handleErrorWith[A](fa: LTask[A])(f: Throwable => LTask[A]): LTask[A] =
        () => {
          Future.successful(()).flatMap { _ =>
            fa().recoverWith {
              case err: ExecutionException if err.getCause ne null =>
                f(err.getCause)()
              case err =>
                f(err)()
            }
          }
        }
    }

  import ExecutionContext.Implicits.global
  def foo[F[_]: Sync] = Sync[F].delay { println("hello") } // lawful code
  def bar[F[_]: Comonad]: F[_] => Int = fa => {
    fa.extract 
    1
  }  //lawful code
  def baz = bar[Function0] //lawful code
  def ooops = baz(foo[LTask]) //lawful code...with side effects!

}
scala> Problem.ooops
hello
res1: Int = 1

@alexandru
Copy link
Member Author

Yeah, Function0 having a Comonad instance is kind of a blocker.

@alexandru
Copy link
Member Author

So OK, the use-case is for people that aren't into FP, abusing Future in code-bases but that would still like to use streaming abstractions without having to adopt IO / Task. This should be low on the list of priorities, I'm not even sure if this would help and I wasn't thinking of Function0 having a Comonad either.

Due to being harder to remove things, than it is to add, we'll not do this for now, deferring this to the moment users will begin screaming for a Sync[Function0[Future[?]]] and not even then.

That said, whenever @djspiewak goes silent, I'll just invoke Future 😜

@johnynek
Copy link

Yeah, ironically the Function0 was the problem here. We have elsewhere assumes evaluation of he Function is pure. We shouldn’t change that here.

Very arguably Eval has the same issue. I’m on my phone but I think it defines a Comonad as well.

@nrktkt
Copy link

nrktkt commented Aug 1, 2020

I've actually implemented this so that I can use tagless-final libraries in Futurefull code.
Is there a alleycats-effect where some Effect[() => Future] could live to be semi-official? I think the need is real and alleycats is a good way for those with dirty codebases to get the instances they need.

@joroKr21
Copy link
Member

joroKr21 commented Aug 2, 2020

I think an opaque newtype around () => Future[A] or Eval[Future[A]] would resolve the Comonad problem by hiding information. However how come you can refactor your codebase to use such a weird type but not IO? You can easily convert back and forth to Future.

@djspiewak
Copy link
Member

However how come you can refactor your codebase to use such a weird type but not IO? You can easily convert back and forth to Future.

This is kind of my feeling on this. It's very easy to convert a () => Future to an IO (and vice versa), so I'm not sure under what circumstances you would want such a thin wrapper when you can have the "real thing". I get that some codebases are stuck partially in Future-land, but is it actually easier to wrap in this kind of thing than it is to convert to IO?

@nrktkt
Copy link

nrktkt commented Aug 3, 2020

is it actually easier to wrap in this kind of thing than it is to convert to IO?

Yes and no.

TaglessFinalLibrary.someMethod(
  IO.fromFuture(IO(
    thisMethodReturnsAFuture()
  ))
).unsafeToFuture()

isn't rocket surgery, but it does bear some explanation compared to

TaglessFinalLibrary.someMethod(
  () => thisMethodReturnsAFuture()
)()

if nothing else, it breaks the flow of a developer who isn't familiar with IO and now needs to pull up the documentation when they could have otherwise inferred what was happening from the type signatures.

@payurgin
Copy link

@SystemFw Maybe a new type will solve problem with Comonad?

final class LazyFuture[T](unsafeRun: () => Future[T]) extends AnyVal

@payurgin
Copy link

This is kind of my feeling on this. It's very easy to convert a () => Future to an IO (and vice versa), so I'm not sure under what circumstances you would want such a thin wrapper when you can have the "real thing". I get that some codebases are stuck partially in Future-land, but is it actually easier to wrap in this kind of thing than it is to convert to IO?

I'm not sure it would be compatible with context propagation based on scala.concurrent.ExecutionContext#prepare.
http://yanns.github.io/blog/2014/05/04/slf4j-mapped-diagnostic-context-mdc-with-play-framework/

@djspiewak
Copy link
Member

Thread local shenanigans are almost always incompatible with any asynchronous code, regardless of the framework. Under certain circumstances, some tricks can be used to get around it (I did this for Hadoop and Scalaz Task a number of years ago), but it's flaky and bespoke.

Honestly, frameworks need to simply stop doing this kind of thing. There's no defense for it (there are better alternatives), and the only thing that makes this sort of thing work with async is Loom, and even then only if used in a certain way.

@nrktkt
Copy link

nrktkt commented Sep 28, 2020

@djspiewak a little off topic, but

there are better alternatives

any suggestions? I've been using monix local which works... sometimes.

@djspiewak
Copy link
Member

any suggestions?

Depends on what you're doing. Threading fiber state is literally just StateT (or Stateful from Cats MTL), or Kleisli of Ref if you want to have a bit more control. Of course, that has some (usually imperceptible) performance implications, and it isn't widely understood how to use MTL without losing your mind (lifting is really hard and worth avoiding).

The broader observation is that we have an effect context, and we can use that to do fancy things; we don't have to rely on automagical things like locals.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants