Bike shedding

[mpilquist]

• Remove cats.effect.Attempt alias. It's currently private[effect] which is confusing for folks reading the code. We could make it public but I don't think an effect library should provide a minor syntax convenience like this.
• Replacing Catchable with MonadError means we lose this method:

  def attempt[A](fa: F[A]): F[Attempt[A]]

This is commonly used in FS2. I suppose the right thing to do here is PRing this method to MonadError in cats.

• We need unsafeRunAsync somewhere -- either on Effect or perhaps on a subtype of Effect if we want to limit where it might be invoked via parametric polymorphism.
• Not sure how I feel about StdIO -- seems like a bucket of random things for which there's no guiding principle on deciding what should be added. E.g., how about randomUUID: IO[UUID]?
• Change IO from a sealed trait to a sealed abstract class to help with binary compat issues in the future.
• Note that we are switching to scala.util.control.NonFatal instead of fs2.util.NonFatal. I'm fine with this but both Paul and Daniel supported using a custom notion of NonFatal that didn't catch ControlException and some others.
• Probably should think of a new name for IO#ensuring given the ensuring from Predef
• To make sure I've got it right, Task.delay is now IO.apply and Task.apply doesn't exist, right?

[mpilquist]

IO is missing the equivalent of Task#async, which moves a computation to an execution context. I'd also like to get start somehow (without pulling in Ref).

If we have IO#async, the equivalent of Task.apply would become IO { ... }.async which seems really nice and clear.

[djspiewak]

• Remove cats.effect.Attempt alias. It's currently private[effect] which is confusing for folks reading the code. We could make it public but I don't think an effect library should provide a minor syntax convenience like this.

Agreed.

• Replacing Catchable with MonadError means we lose this method:

https://github.com/typelevel/cats/blob/master/core/src/main/scala/cats/ApplicativeError.scala#L46

• We need unsafeRunAsync somewhere

Why? fa.unsafeRunAsync(cb) is roughly equivalent to fa.runAsync(cb).unsafeRunSync(), modulo the fact that cb returns Unit in the former and IO[Unit] in the latter. In fact, it's relatively trivial to implement the former in terms of the latter.

• Not sure how I feel about StdIO -- seems like a bucket of random things for which there's no guiding principle on deciding what should be added. E.g., how about randomUUID: IO[UUID]?

I too am unsure of how I feel about it. Happy to remove. The guiding principle was basically "things from System and Runtime that I personally deem useful". Not a great principle.

• Change IO from a sealed trait to a sealed abstract class to help with binary compat issues in the future.

👍

• Note that we are switching to scala.util.control.NonFatal instead of fs2.util.NonFatal. I'm fine with this but both Paul and Daniel supported using a custom notion of NonFatal that didn't catch ControlException and some others.

Oh, I forgot about that! I'll look into the differences.

• Probably should think of a new name for IO#ensuring given the ensuring from Predef

Eh… There's no namespace conflict, since it's being called on an IO instance, and I can't think of anyone other than Bill Venners who uses ensuring. I actually forgot that it even existed until recently.

Do you have a better name in mind though? I'm not married to ensuring, it was just there.

• To make sure I've got it right, Task.delay is now IO.apply and Task.apply doesn't exist, right?

Yep.

IO is missing the equivalent of Task#async, which moves a computation to an execution context. I'd also like to get start somehow (without pulling in Ref).

I wanted to steer clear of any concurrency support in IO itself. Both the functions you're referencing can be implemented in terms of what's here. The forked async constructor is by far the most tempting for me, since most of the time users want to move back onto the main thread pool after a callback. I would be pretty leery of start though.

Maybe the "strict concurrency avoidance" plan is a bad one, I'm not sure. I wanted to do that both to avoid stepping on fs2/Monix, and because I thought the separation of concerns could be beneficial.

[mpilquist]

No strong opinions on ensuring -- I forgot about it until recently too.

I can live without start on IO but I would like a way to shift a synchronous computation to an execution context. How about this?

final def async(implicit ec: ExecutionContext): IO[A] = IO.async { cb => 
  ec.execute(new Runnable { def run = unsafeRunAsync(cb) })
}

[djspiewak]

I'm ok with that, though the name seems a bit weird. How does fork or shift strike you?

[mpilquist]

Agreed on name being weird. I'm fine with either of your suggestions.

Are you okay with both IO.fork and IO#fork existing? The constructor variant would match the implementation you have in the ScalaDoc for async. Or perhaps IO.fork constructor should exist like you have in the docs while IO#shift would match the example above. Either way, I find both of these operations common enough that I'd like them to exist with first class syntax -- i.e., I don't want to have to teach folks how to derive these each time this question comes up.

[djspiewak]

I have an IO.fork constructor in the docs?

[mpilquist]

https://github.com/djspiewak/cats-effect/blob/c5024ed3b2345d481a51c0b947f920cbfaf65b41/shared/src/main/scala/cats/effect/IO.scala#L330-L337

[djspiewak]

Oh right, lol. The constructor variant is basically Task.apply. Is that really something you do so regularly that IO(action).shift is worse than IO.fork(action)? I actually almost never use Task.apply.

[mpilquist]

Eh that's fair. OK I'm happy with just IO(action).shift then. :)

[djspiewak]

Alrighty. :-) So we're loosening the "no concurrency" restriction to "no concurrency, but thread pool manipulation isn't really concurrency, so it's ok," which actually seems pretty reasonable to me.

[djspiewak]

So to summarize, we've got the following actions:

• Add IO#shift
• Remove StdIO
• classify IO
• Investigate NonFatal
• Remove Attempt

Thoughts on my unsafeRunAsync response?

[mpilquist]

What would awakeEvery look like?

https://github.com/functional-streams-for-scala/fs2/blob/20208de46e9ab968726f2a9699b781a6f8da0c49/core/shared/src/main/scala/fs2/time/time.scala#L47

I guess it would just be:

F.runAsync(signal.set(d))(_ => IO(running.set(false))).unsafeRunSync

[djspiewak]

Yep. Conceptually here, the typeclass is "imbuing" IO with special properties. It's effectively saying that IO is the only parametrically valid root of an effect stack, in that any side-effect managing type constructor should be able to asynchronously interpret to it. The function serves the dual purpose of allowing a safe unsafeRunSync on JavaScript, since the resulting IO is guaranteed to have only synchronous actions.

[djspiewak]

@mpilquist Implementing shift, I realized that it is probably best if we implement both versions of it:

  /**
   * Shifts the computation of any prefix contiguous synchronous actions into the implicitly
   * specified `ExecutionContext`.  Asynchronous actions and continuations will be unshifted
   * and will remain on whatever thread they are associated with.  This should be used if
   * you want to evaluate a given `IO` action on a specific thread pool when it is eventually
   * run.
   */
  final def shiftPrefix(implicit EC: ExecutionContext): IO[A] = {
    IO async { cb =>
      EC.execute(new Runnable {
        def run() = self.unsafeRunAsync(cb)
      })
    }
  }

  /**
   * Shifts the continuation of the action into the implicitly specified `ExecutionContext`.
   * The thread pool association will be observed in any actions which are flatMapped onto
   * the result of this function.  That is to say, the *continuation* of the `IO`.  All of
   * the effects within the current `IO` will retain their pre-existing thread affinities,
   * if any.
   *
   * This function is most useful on asynchronous actions which require thread-shifting back
   * onto some other thread pool (e.g. off of an event dispatch thread).
   */
  final def shiftSuffix(implicit EC: ExecutionContext): IO[A] = {
    attempt.flatMap { e =>
      IO async { (cb: Either[Throwable, A] => Unit) =>
        EC.execute(new Runnable {
          def run() = cb(e)
        })
      }
    }
  }

[djspiewak]

Closing this now since I've implemented all the things. Please feel free to open more for other desired changes!