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

WIP: Make IO a bifunctor #1708

Merged
merged 25 commits into from Apr 17, 2018

Conversation

@jdegoes
Member

jdegoes commented Apr 10, 2018

Initial WIP for transforming IO into a Bifunctor, with a precisely-specified error type. This allows a precise, compile-time specification of if and how an IO computation may fail.

Many methods have been generalized. The remaining work involves more precisely specifying error types—for example, ensuring that effects which cannot fail are polymorphic in the error type—as well as updating documentation to reflect the new error model.

@jdegoes jdegoes added the scalaz8 label Apr 10, 2018

private final case object Later extends AsyncReturn[Nothing]
final case class Now[A](value: A) extends AsyncReturn[A]
final case class MaybeLater[A](canceler: Canceler) extends AsyncReturn[A]
private final case object Later extends AsyncReturn[Nothing, Nothing]

This comment has been minimized.

@emilypi

emilypi Apr 11, 2018

Member

Maybe we can just do a quick fix and consider @fommil's example in this PR -

sealed abstract case class Later[E, A] private () extends AsyncReturn[E, A]
object Later {
    // avoids object alloc, whilst allowing exhaustivity checking
    private[this] val value: Later[Void, Void] = new Later[Void, Void] {} // or [Nothing, Nothing] if need be
    def apply[A](): AsyncReturn[Void, A]       = value.asInstanceOf[AsyncReturn[Void, A]]
  }

Or some such variation

This comment has been minimized.

@jdegoes

jdegoes Apr 11, 2018

Member

There is also @TomasMikula's way of encoding these polymorphic values, which I have meant to deploy ever since I learned about it (see also: IO.never, IO.unit, and several other values which could benefit from this transformation).

I'll be sure I clean all this stuff up in a subsequent commit!

final def write(a: A): IO[Unit] = IO.sync(this.value = a)
final def modify[E](f: A => A): IO[E, A] = IO.sync {
var loop = true
var next : A = null.asInstanceOf[A]

This comment has been minimized.

@emilypi

This comment has been minimized.

@jdegoes

jdegoes Apr 11, 2018

Member

Hehe. The other alternative is to @tailcall it and inspect the generated bytecode.

Unfortunately any unnecessary boxing in IORef will significantly decrease performance given how low-level the construct is...

This comment has been minimized.

@emilypi

emilypi Apr 11, 2018

Member

I hear you and trust the motivation 😄

}
object IORef {
/**
* Creates a new `IORef` with the specified value.
*/
final def apply[A](a: A): IO[IORef[A]] = IO.sync(new IORef[A](a))
final def apply[E, A](a: A): IO[E, IORef[A]] = IO.sync(new IORef[A](new AtomicReference(a)))

This comment has been minimized.

@emilypi

emilypi Apr 11, 2018

Member

@jmcardon 😄 you were spot on!

@@ -15,28 +18,104 @@ package scalaz.effect
* } yield ()
* }}}
*/
final class IORef[A] private (@volatile private var value: A) {
final class IORef[A] private (private val value : AtomicReference[A]) extends AnyVal {

This comment has been minimized.

@jmcardon

jmcardon Apr 11, 2018

Member

LOL. I made this exact same change 2 days ago which was intended to be PR'd to scalaz 7.2.x, but then ioeffect was decided to be a separate module.

I was thinking of adding lazySet and something similar to modify from fs2 as well. You beat me to it 👍

This comment has been minimized.

@jdegoes

jdegoes Apr 11, 2018

Member

Superb! Great minds and all that. 😄

The main benefit to AtomicReference is that a concurrent-safe version of modify can be implemented (and has been, here), which means you can use IORef as a single-variable STM.

Surprisingly there's also a precedent for this functionality in Haskell's IORef, although I can't recall exactly what it is (some atomic compare-and-swap variant).

@jmcardon

👍

One thing that might be useful is explicit syntax for widening E.

That is, when working with IO[E1, A] and IO[E, B] where E1 <: E, attempting to compose these two won't work out of the box without a cast since IO is invariant, despite it being correct.

Something like myIO.widen[E] would make that situation easier I think.

@jdegoes

This comment has been minimized.

Member

jdegoes commented Apr 11, 2018

@jmcardon

That is, when working with IO[E1, A] and IO[E, B] where E1 <: E, attempting to compose these two won't work out of the box without a cast since IO is invariant, despite it being correct.

This makes sense for performance reasons (of course you can leftMap to widen). I wonder if we should use @alexknvl's Liskov or actually use subtyping. Probably the latter would make for an easier-to-use API.

@alexknvl

This comment has been minimized.

Contributor

alexknvl commented Apr 11, 2018

I think they are for the most part equivalent. You can go from E1 <: E to E1 <~< E and back (that's a bit trickier). But if you do, use <~< rather than Liskov (it is for substitutions in bounded type constructors F[_ >: L <: U]).

jdegoes added some commits Apr 11, 2018

@fommil

This comment has been minimized.

Contributor

fommil commented Apr 11, 2018

how about renaming this IO to GIO (for General) and then IO[a] = GIO[Throwable, a]

It's going to be a pain to migrate codebases that are using IO already for dealing with legacy java apis.

@jdegoes

This comment has been minimized.

Member

jdegoes commented Apr 11, 2018

@fommil

screenshot_945

@@ -17,8 +17,9 @@ object AsyncReturn {
object Later {
// Trick thanks to Sam Halliday. We eliminate allocation overhead but also

This comment has been minimized.

@fommil

fommil Apr 11, 2018

Contributor

it's really @xuwei-k's trick... mine was cleaner but broke exhaustivity checking. And @hrhino came up with the fix for the dead code thing.

BTW, can we please make @hrhino a member of the org so he can approve PRs?

This comment has been minimized.

@hrhino

hrhino Apr 11, 2018

Member

I think I am a member? so I'm confused.

Also I think I can approve them. I see the implication that I can't / shouldn't disapprove of them, though.

This comment has been minimized.

@fommil

fommil Apr 11, 2018

Contributor

@hrhino oh, when I see your reviews they are often grayed out... whereas others come through with a green tick.

This comment has been minimized.

@jdegoes

jdegoes Apr 12, 2018

Member

Yep he's a member already.

@@ -39,7 +39,7 @@ import scalaz.effect.{IO, SafeApp}
import scalaz.effect.console._
object MyApp extends SafeApp {
def run(args: IList[String]): IO[Unit] =
def run(args: IList[String]): IO[E, Unit] =

This comment has been minimized.

@Blaisorblade

Blaisorblade Apr 11, 2018

Contributor

Where's type variable E bound? Or is it a top-level type named E? Or a _meta_variable E which isn't introduced yet? Your examples seem to use Error instead.

@@ -53,15 +53,15 @@ object MyApp extends SafeApp {
You can lift pure values into `IO` with `IO.point`:
```scala
val liftedString: IO[String] = IO.point("Hello World")
val liftedString: IO[E, String] = IO.point("Hello World")

This comment has been minimized.

@Blaisorblade

Blaisorblade Apr 11, 2018

Contributor

here too whence E

@@ -117,12 +117,12 @@ val contacts: IO[IList[Contact]] =
You can create `IO` actions that describe failure with `IO.fail`:
```scala
val io: IO[String] = IO.fail(new Error("Oh noes!"))
val io: IO[E, String] = IO.fail(new Error("Oh noes!"))

This comment has been minimized.

@Blaisorblade

Blaisorblade Apr 11, 2018

Contributor

all introduced E until here seem unbound

* An `IO[A]` ("Eye-Oh of A") is an immutable data structure that describes an
* effectful action that may throw an exception (`Throwable`), run forever, or
* produce a single `A` at some point in the future.
* An `IO[E, A]` ("Eye-Oh of A") is an immutable data structure that describes an

This comment has been minimized.

@Blaisorblade

Blaisorblade Apr 11, 2018

Contributor

Still read "Eye-Oh of A"?

This comment has been minimized.

@hrhino

hrhino Apr 11, 2018

Member

"Eye-Oh Ee of Aye", you.

This comment has been minimized.

@fommil

fommil Apr 11, 2018

Contributor

and bingo was his name o.

```scala
def sqrt(io: IO[Double]): IO[Double] =
def sqrt(io: IO[E, Double]): IO[E, Double] =
IO.absolve(

This comment has been minimized.

@Blaisorblade

Blaisorblade Apr 11, 2018

Contributor

If absolve only accepts IO[Void, E \/ A], how does this code typecheck?

This comment has been minimized.

@hepin1989

hepin1989 Apr 11, 2018

I do like the Done one.

* produce a single `A` at some point in the future.
* An `IO[E, A]` ("Eye-Oh of A") is an immutable data structure that describes an
* effectful action that may fail with an `E`, run forever, or produce a single
* `A` at some point in the future.

This comment has been minimized.

@Blaisorblade

Blaisorblade Apr 11, 2018

Contributor

Looking forward to more documentation on if/when this guarantee fails. Quoting what I asked on Gitter:

does anybody plan to state "formally" under what conditions you get other exceptions? Not sure Scalazzi is enough
it seems E behaves basically like a phantom type parameter at the edges

  1. so you need to import Java functions with the correct spec
  2. you have "async. exceptions" from the runtime, you need to consider them
  3. if you listen to Java checked exception specs, you need to consider unchecked exceptions as "async"
* Widens the error type to any supertype. While `leftMap` suffices for this
* purpose, this method is significantly faster for this purpose.
*/
final def widen[E2 >: E]: IO[E2, A] = self.asInstanceOf[IO[E2, A]]

This comment has been minimized.

@fommil

fommil Apr 11, 2018

Contributor

no Liskov?

This comment has been minimized.

@jdegoes

jdegoes Apr 11, 2018

Member

If I use <~<, then the user will have to construct it explicitly, as these things are not automatically summoned into scope for subtyping relationships. Or am I missing something, @alexknvl?

This comment has been minimized.

@fommil

fommil Apr 11, 2018

Contributor

maybe it's different in scalaz8 but in 7.2 they are summoned

This comment has been minimized.

@alexknvl

alexknvl Apr 11, 2018

Contributor

implicit will work in Scalaz8. https://github.com/scalaz/scalaz/blob/series/8.0.x/base/shared/src/test/scala/AsResolutionTest.scala

EDIT: It will infer in every single case where A <: B works. However, it does not handle composition (p: A <~< B, q: B <~< C, infer A <~< C) automatically since that would require modifications to the compiler's implicit inference:

def f[A <: B] = implicitly[A <~< B] // will work
def f[A <: B, B <: C, C] = implicitly[A <~< C] // will work
def f[A: ? <~< B, B: ? <~< C, C] = implicitly[A <~< C] // won't work

Converting A <: B to A <~< B is trivial. Converting A <~< B to A <: B is not.

def f[A <: B](): Unit = g()
def g[A, B]()(implicit ev: A <~< B): Unit = f() // this won't work, 
// but it is possible with some extra machinery from leibniz

This comment has been minimized.

@jdegoes

jdegoes Apr 12, 2018

Member

@alexknvl Unfortunately, I cannot use it, since the method I require (substCv) requires variance in the type parameter E.

Well, I can require it as an implicit and ignore it, performing the coercion, but then scalac will fail due to an unused implicit. 😄

This comment has been minimized.

@alexknvl

alexknvl Apr 12, 2018

Contributor

You can un-ignore something using

val _ = ev

But regardless, I don't feel particularly strongly about this issue :)

This comment has been minimized.

@jdegoes

jdegoes Apr 12, 2018

Member

I could go either way, too. I do feel like we need a general solution to this problem since many other classes in Scalaz 8 are invariant.

This comment has been minimized.

@alexknvl

alexknvl Apr 12, 2018

Contributor

Is there any way to create IsCovariant[IO] instance?

EDIT: nvm, there is not. I would implement widen(implicit ev: A <~< B) simply because I have never found a single case where bounds actually improved anything 😄

This comment has been minimized.

@TomasMikula

TomasMikula Apr 12, 2018

Member

You could have IsCovariant for IO without coercions if IO was an opaque type implemented by a covariant IOImpl[+E, +A]. 😃 (That's how IsCovariant is done for Maybe, because it is just an alias for the covariant Option.)

/**
* Imports a synchronous effect into a pure `IO` value. If the thunk returns
* a null value, this will throw a `NullPointerException` inside `IO`.
*
* {{{
* def putStrLn(line: String): IO[Unit] = IO.sync(println(line))
* def putStrLn(line: String): IO[Throwable, Unit] = IO.partialSync(println(line))

This comment has been minimized.

@pjrt

pjrt Apr 12, 2018

This scaladoc is for sync, yet it mentions partialSync.

This comment has been minimized.

@jdegoes

jdegoes Apr 12, 2018

Member

Fixed, thanks!

@@ -39,31 +43,31 @@ import scalaz.data.Maybe
* values, see the default interpreter in `RTS` or the safe main function in
* `SafeApp`.
*/
sealed abstract class IO[A] { self =>
sealed abstract class IO[E, A] { self =>

This comment has been minimized.

@pjrt

pjrt Apr 12, 2018

Not really sure what it is that we expect E to be. From my point of view, E can't really be anything but some E <: Exception. If we are considering E to be any error type, like String for example, then how is that different than EitherT[IO, E, A]? (where IO is the current IO).

Now, one cool thing that can happen from exposing this E is that we can now have IO[Exception with NoStackTrace, A]. This will allow your code to use exceptions without the stacktrace issue (and even control it). Not sure how impure code will convert from their Exception/Throwable into a NoStrackTrace one, but maybe there's way.

This comment has been minimized.

@jdegoes

jdegoes Apr 12, 2018

Member

There is no requirement on E. It can be some ADT, unrelated to exceptions, or even just String if all you care about is error messages; Void / Nothing if you don't want any errors; or Unit if you want to treat an IO as F[Option[A]].

You can leftMap over E to "lift" an IO from one error type to another, which lets you change your error types in different parts of your application.

This comment has been minimized.

@pjrt

pjrt Apr 12, 2018

Then I'm confused: what's the difference between IO[E, A] and EitherT[IO, E, A]?

Edit: and how does E interface with flat out thrown exceptions?

This comment has been minimized.

@jbgi

jbgi Apr 12, 2018

Member

what's the difference between IO[E, A] and EitherT[IO, E, A]

considering an IO monad that never catch exceptions, there are isomorphic (I think), the difference is better performance and ease of use (no/less monad transformers).

and how does E interface with flat out thrown exceptions?

See partialSnyc. The strategy here is to deffer constraints on type parameters until you actually need them, to maximize possible usages. It is similar to how one does not need an Order[A] to construct a ISet.empty[A].

This comment has been minimized.

@jdegoes

jdegoes Apr 12, 2018

Member

@pjrt

The type EitherT[IO, E, A] — for Scalaz 7, Monix Task, Cats Effect, Future, etc. — has two separate error channels, and two MonadError instances (one is fixed to Throwable, and one varies as E).

You can and probably should think of EitherT[IO, E, A] for these effect monads as being equivalent to EitherT[EitherT[Unexceptional, Throwable, ?], E, A] (for some unexceptional effect monad Unexceptional), which is a nesting of 2 layers of Either. Such values may "fail" — in the functional programming sense — and be recovered from in two distinct ways: the inner Either (with Throwable), or the outer Either (with E).

In contrast, IO[E, A] has a single MonadError instance, and should be thought of as equivalent to EitherT[Unexceptional, E, A]. There is only one way it may "fail" — in the functional programming sense — and be recovered from: with E.

With the older effect monads, because there are two separate error channels, and because one of them is fixed at compile-time to Throwable, it limits the precision with which you can describe effects. You cannot describe effects that cannot fail, nor can you describe effects that can fail in only one of n distinct ways, because the bundling of the inner Either with its Left case fixed to Throwable prevents such descriptions.

Now, another way to formulate the Scalaz 8 IO bifunctor is as EitherT[Unexceptional, E, A]. Arguably, this is a better design, practical considerations aside. However, monad transformers are a terrible fit for Scala, and can easily destroy performance and limit applications of effect monads. Instead of taking this approach, Scalaz 8 IO bakes in the EitherT so it has no runtime overhead.

This comment has been minimized.

@pjrt

pjrt Apr 12, 2018

considering an IO monad that never catch exceptions

Is such an IO even useful? IO is meant for those kinds of things.

the difference is better performance and ease of use (no/less monad transformers).

Then it is just another form of EitherT[IO, E, A]? That's a bit confusing, does EitherT have any meaning at that point? Should it be removed?

See partialSnyc. The strategy here is to deffer constraints on type parameters until you actually need them, to maximize possible usages. It is similar to how one does not need an Order[A] to construct a ISet.empty[A].

I get that, but the idea of turning Throwable => E sounds like "My error ADT E will always need a SomeThrowable(t: Throwable) case". Which feel wrong.

This comment has been minimized.

@pjrt

pjrt Apr 12, 2018

@jdegoes

However, monad transformers are a terrible fit for Scala, and can easily destroy performance and limit applications of effect monads. Instead of taking this approach, Scalaz 8 IO bakes in the EitherT so it has no runtime overhead.

That's actually fair (better performance), even though it makes it bit confusing. Maybe make a mention about EitherT on the scaladoc? I can see other people becoming confused about its relationship with EitherT.

This comment has been minimized.

@jbgi

jbgi Apr 12, 2018

Member

Is such an IO even useful? IO is meant for those kinds of things.

IO as APIs to catch exception but the methods of the IO monad (ie. map/bind) do not catch exception implicitly, as a matter of obeying laws.

This comment has been minimized.

@pjrt

pjrt Apr 12, 2018

@jbgi Ah, I see what you mean now.

@jdegoes

This comment has been minimized.

Member

jdegoes commented Apr 12, 2018

@Blaisorblade All right, updated the docs (mostly) to match the current state of the code. Previously, they were result of search & replace (IO[A] => IO[E, A]) which is why they didn't make sense. Let me know if you spot anything!

jdegoes added some commits Apr 12, 2018

@pjrt

This comment has been minimized.

pjrt commented Apr 12, 2018

I'll second @fommil 's concern. Calling this "generic" IO IO will not only require somewhat large refactors on projects that are using the current IO, but it will also confuse people who are used to the original IO definition. Sure, type Task[A] = IO[Throwable, A] but people aren't going to be looking for "Task".

@jdegoes

This comment has been minimized.

Member

jdegoes commented Apr 12, 2018

@pjrt It is well known that almost no one uses Scalaz 7 IO. Almost all Scalaz projects use Task exclusively. I don't think this will be a major issue in practice.

Also in order to cope with the name and semantic changes for many methods, it will be necessary to deploy a ScalaFix rewrite... or at least come up with a facade. Either of which help address the migration problem.

@jmcardon

This comment has been minimized.

Member

jmcardon commented Apr 12, 2018

@pjrt there are no projects using the current IO because no one has published it yet.

@fommil

This comment has been minimized.

Contributor

fommil commented Apr 12, 2018

I was thinking more about refactoring cats IO to this ;-)

Doing a global rename is trivial with scalafix. So I'm less concerned. I think I'd rather be typing IO than Task, though, to be honest.

@pjrt

This comment has been minimized.

pjrt commented Apr 12, 2018

Opps, sorry, lost the context there. I was also thinking about cats-effects IO. My main concern is still people looking for IO and being introduced to IO[E,A], not what I would expect. But the scaladoc was updated to explain it, so that should be good (at least for me).

@Milyardo

This comment has been minimized.

Contributor

Milyardo commented Apr 13, 2018

How about the nameCompute instead of Unexceptional?

@fommil

This comment has been minimized.

Contributor

fommil commented Apr 13, 2018

Hmm, I really hate to keep harping on about the name of things (it's such a waste of time to bike-shed about names), but since we are in general consensus that scalaz8 will use Haskell names where there is precedent (even if we think the haskell name is bad), there is definitely precedent for IO to have a single type parameter, so I think this IO should be renamed to GIO or something and type IO[a] = GIO[Throwable, a], perhaps also type Task[a] = GIO[Throwable, a] for people using the scalaz7 Task.

I shall yield my objection if there is a very strong desire for IO to have a bifunctor, but I reserve my right to complain forever about having to type 4 characters, and take a finger off the middle DVORAK row, instead of 2 everywhere I am interacting with legacy code 😄

build.sbt Outdated
@@ -36,8 +36,8 @@ lazy val benchmarks = project.module
"org.scala-lang" % "scala-reflect" % scalaVersion.value,
"org.scala-lang" % "scala-compiler" % scalaVersion.value % "provided",
"org.scalaz" %% "scalaz-core" % "7.2.7",
"io.monix" %% "monix" % "3.0.0-M1",
"org.typelevel" %% "cats-effect" % "0.4"
"io.monix" %% "monix" % "3.0.0-M3",

This comment has been minimized.

@barambani

barambani Apr 13, 2018

this is currently at 3.0.0-RC1 I think

@pjrt

This comment has been minimized.

pjrt commented Apr 13, 2018

I'm with @fommil on this. I understand the desire to make it a bifunctor, but the fact that it is now more akin to EitherT makes me think it should have a different name.

@jdegoes

This comment has been minimized.

Member

jdegoes commented Apr 13, 2018

Scalaz has a strong historical history of (a) improving upon the mistakes of Haskell, (b) incentivizing users to do the correct thing, even when unfamiliar or inconvenient.

Personally, I feel the default in any new code base should always be IO[E, A], never IO[A]. The latter only has a place in legacy code in the process (perhaps indefinite) of being migrated.

The old IO and Task were already "broken bifunctors" in the sense they bundled the Either and fixed the left type to Throwable. This defect has been rectified in this design. This is exactly IO, but without fixing the left type parameter to Throwable.

So my preference would be to keep the name IO, because this is the construct that you should use for effects in new code bases, not a type synonym such as Task. Simultaneously, we could introduce a new 2-letter synonym for IO[Throwable, A], such as Ef[A] or OI[A], to ease typing.

If it feels strange, it's only because it's new. People complained about fork / join, fibers, and interruption, and now even cats effect has simple versions. This is the shape of all future effect monads in Scala, it's just going to take a little while (and some benchmarking) for the others to catch up.

However, I don't have a terribly strong preference (names are no hill for me to die on!). If we have a vote and most contributors think we should change the name, I'm OK with that.

@jdegoes

This comment has been minimized.

Member

jdegoes commented Apr 13, 2018

@Milyardo The problem with that is IO[Void, A] can represent actual effectful code, just not code that fails. Think of things like new AtomicInteger() or System.nanoTime(), which cannot fail, but are still effectful. It's not "pure compute".

@Milyardo

This comment has been minimized.

Contributor

Milyardo commented Apr 13, 2018

Compute doesn't imply purity to me, but I can see how it does to some. To say something is computable says that a computation eventually terminates much more than than it says it can't fail. So I guess the name is a bit off still.

@jdegoes jdegoes merged commit cc3dd5b into scalaz:series/8.0.x Apr 17, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jdegoes jdegoes deleted the jdegoes:series/scalaz8-io-bifunctor branch Apr 17, 2018

@emilypi

This comment has been minimized.

Member

emilypi commented Apr 17, 2018

BIG MERGE NICE JOB EVERYBODY

@jdegoes

This comment has been minimized.

Member

jdegoes commented Apr 17, 2018

👯‍♂️ 🎉

@jmcardon

This comment has been minimized.

Member

jmcardon commented Apr 17, 2018

@emilypi emilypi referenced this pull request Apr 17, 2018

Merged

Backport Bifunctorial IO #18

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