typelevel / cats-effect

The pure asynchronous runtime for Scala
https://typelevel.org/cats-effect/
Apache License 2.0
2.04k stars 523 forks source link

Effect capture with Eval #78

Closed durban closed 7 years ago

durban commented 7 years ago

I think the scaladoc of IO.fromFuture says that it can be used for effect capture. For example like this:

def sideEffectingAsyncMethod(): Future[Unit] = ...
val pureValue: IO[Unit] = IO.fromFuture(Eval.always(sideEffectingAsyncMethod()))

However, using Eval for effect capture seems ... questionable. Shouldn't fromFuture accept a by-name Future (similarly to IO.apply)?

alexandru commented 7 years ago

What would be the difference between Eval.always and a by-name parameter?

Eval is meant to control evaluation, being a replacement for by-name parameters. Its advantage is that it can also have strict behavior, or that of a lazy val.

fromFuture had a by-name parameter in its original version by Daniel. My objection was that I wanted a strict fromFuture that does not suspend side effects. The Eval is meant to appease both use cases.

durban commented 7 years ago

Of course, with the by-name it would behave the same. And yes, with using a by-name, we'd lose strict or lazy val-like behavior. But:

  1. The same could be said about IO.apply, which currently uses a by-name. (I see now, that there is also an IO.eval, which seems to be the same as fromFuture in this regard.)

  2. I feel that using Eval for effect capture (both in IO.eval and fromFuture) is problematic (with it having a Comonad instance and similar things).

alexandru commented 7 years ago

IO also has IO.pure. If we'd take the Eval out, I would actually prefer to have fromFuture be strict, because Future is strict and Cats is strict by default as a design philosophy.

That Eval is a Comonad isn't a problem, as this behaves like you'd expect it to:

IO.fromFuture(Eval.always(throw ex)) <-> IO.raiseError(ex)
mpilquist commented 7 years ago

fromFuture takes an Eval to prevent bugs like this, where folks intend for the overall expression to be RT:

val f: Future[X] = ...
IO.fromFuture(f)

I'm sympathetic to the argument that such bugs are par for the course when using Future but I've seen these types of bugs too many times when folks start using IO. The Eval signature acts a speed bump, hopefully encouraging folks to think more about the evaluation semantics in creating the Future.

durban commented 7 years ago

My problem is that the intended use of these APIs (IO.eval and fromFuture) forces the user to abuse cats.Eval, since any expression of the form Eval.always(<some non-pure expression>) is illegal (or it should be). (Maybe there could be a different datatype for reifying strict/lazy/by-name evaluation, one which could be used for effect capture?)

durban commented 7 years ago

How about something like this?

def fromFuture[A](f: IO[Future[A]])(implicit ec: ExecutionContext): IO[A]

Then we could do IO.fromFuture(IO { sideEffectingAsyncMethod() }) instead of the current IO.fromFuture(Eval.always { sideEffectingAsyncMethod() }). IO.fromFuture(IO.pure(f)) would work for pure Futures. (And there could be something like IO.memoize for using instead of Eval.later.) This would still encourage users to think about how they're creating the Future.

(BTW, there was a somewhat similar discussion about Eval at #67; unresolved, as far as I can tell.)

mpilquist commented 7 years ago

That looks good to me. 👍

alexandru commented 7 years ago

since any expression of the form Eval.always(<some non-pure expression>) is illegal (or it should be)

I don't understand how usage of Eval.always is any less different from using a by-name parameter.

If we cannot use Eval in place of a by-name parameter, whenever we need it, based on some purity contract that cannot be enforced by the language, then Eval as a data type is totally useless and should be removed from cats-core, because controlling evaluation and thus be a replacement for by-name params and for lazy val was its whole raison d'être.

I do not agree with this of course, because in IO.fromFuture(Eval.always(...)) it is not Eval that's managing / suspending the side-effects, but IO. Otherwise this property would not hold:

IO.fromFuture(Eval.always(throw ex)) <-> IO.raiseError(ex)

Also, take a good look at this function signature:

def fromFuture[A](f: IO[Future[A]])(implicit ec: ExecutionContext): IO[A]

If you really, really want to preserve the illusion that IO is the only data type allowed to suspend side effects and that this fromFuture is pure, then you've got an elephant in the room, which is the ExecutionContext.

Which does highlight that this function gets used at FP program boundaries, being part of the FFI, just like IO.unsafeRunAsync — and it's already impure, with or without changing that param type.

alexandru commented 7 years ago

Btw, in the discussion that led to MonadError[Eval] being removed from Cats, I always felt like Eval having a Comonad implementation is a red herring.

def foo(x: Int): Eval[Int] = {
  var y = 0
  Eval.always { y += x; y }
}

def bar(x: Int): () => Int = {
  var y = 0
  () => { y += x; y }
}

Fact of the matter is that these functions are equivalent and pure as they do suspend side effects, now and in the future, regardless of what type-classes Eval implements, now or in the future, because whatever type-classes get implemented for Eval, it does not change Eval's fundamental nature, which is its equivalence to a plain old thunk.

In other words, given the choice between Comonad and MonadError, I would have removed the Comonad and not the MonadError. And even if that boat has sailed, the Comonad implementation of Eval can't be a problem for IO, because IO does not use Eval's Comonad implementation and it does not care whether .value is a total function or not.

/cc @non — I would be interested in what he has to say, as he's the one that added Eval in Cats

durban commented 7 years ago

@alexandru In my opinion, by-name parameters are used for two, quite different purposes: (1) laziness (of pure expressions), and (2) effect capture. I assume we all agree that IO.apply is using it for (2) (but correct me if I'm wrong). I'd argue, that Eval is for (1), as shown by a lot of code in Cats, which assumes that e.value is pure for any e: Eval[A].

Of course, you're right, that technically it is possible to use Eval for (2). Technically Eval is capable of effect capture – just as Function1 is also capable (but if you're using a side-effecting Function1 as an argument to map, you're gonna have a bad time). However, the API of Eval, and its usage in Cats suggests that it's only for (1); if it wouldn't be, .value would be called .unsafeValue. There could exist a data type for general replacement of by-name parameters; I'd even agree that it would be useful. But (currently) Eval isn't that.

So my problem is not with using Eval for implementing fromFuture; my problem is with using it in the API. And I think that this is important exactly because folks new to IO may not be perfectly clear on which calls are doing effect capture. And that's why I think Eval should not be used for effect capture in APIs (since it's inconsistent to pretend that .value is pure and that Eval can capture effects).

(BTW, I still think that fromFuture could simply accept a by-name Future; my proposal for the signature with IO[Future[A]] was in response to the concern, that users should be forced to think about evaluation semantics. Another possibility for solving the same problem would be having different methods, for example:

def fromPureFuture[A](f: Future[A])(implicit ec: ExecutionContext): IO[A]
def fromLazyFuture[A](f: => Future[A])(implicit ec: ExecutionContext): IO[A] // memoization
def fromByNameFuture[A](f: => Future[A])(implicit ec: ExecutionContext): IO[A] // no memoization

)

alexandru commented 7 years ago

There could exist a data type for general replacement of by-name parameters; I'd even agree that it would be useful. But (currently) Eval isn't that.

Well, this is where we disagree, I think Eval is that data type. And its usage in cats-core is pretty consistent for replacing by-name parameters, even when side-effects are involved.

One such example is Applicative.catchNonFatalEval

As to your proposal, I have exactly that in Monix's Task. So we have:

def fromFuture[A](f: Future[A]): Task[A]

def deferFuture[A](fa: => Future[A]): Task[A]

def deferFutureAction[A](f: Scheduler => Future[A]): Task[A]

Me, I want that strict fromFuture because in my mind that is consistent with the Scala language, with what Future is and with the design of Cats and because people can always do this:

IO.suspend {
  IO.fromFuture(someFutureAction())
}

Now, it is true that people don't look at signatures, but I think that a strict fromFuture is less surprising than a lazy fromFuture that's used incorrectly. But there wasn't consensus on that, which is why we ended up with Eval, which I think it's still fine.

Also checkout the discussion we had then — https://github.com/typelevel/cats-effect/pull/35

alexandru commented 7 years ago

Btw, this is my educated opinion, but I'd like consensus on usage of Eval, which is why I'd really like hearing from @non and @djspiewak

djspiewak commented 7 years ago

So I read over this a few times, but it's still possible I missed something. Attempting to reply to each point in turn…

The Eval in IO.fromFuture isn't intended for suspending effects, because Eval is not an appropriate datatype for effect suspension. We can in fact demonstrate this by attempting to define Sync[Eval] and watching the laws break! With that said, all of IO's constructors are a bit dodgy because of the need to capture effects in some way, and fromFuture isn't any different in that it captures the effect of starting a Future (when the Eval.always constructor is used).

Originally, fromFuture took a by-name parameter, just like apply. However, I rather like the current approach better, since it gives users more control and is generally more consistent with how laziness is encoded in Cats. The reason not to make the same transformation on apply is both due to the existence of IO.eval and because apply is simply used so much in common code, and it would be a relatively high syntactic burden to be forced to wrap in Eval every time.

Regarding alternative signatures for fromFuture, the "proper" way to do it would be the following:

def fromFuture[F[_]: Effect](ffa: F[Future[A]]): IO[A]

So, any runnable effect thus would be supported. In practice, IO and all of its equivalents. I'm not entirely averse to this, and I think it's a fair point that this (or even specializing F on IO) is more correct than the current Eval signature, but I don't see a huge win either way. I'm willing to be swayed by overwhelming opinion, but the current signature has preference in my mind simply because it's already there and we're trying to stabilize as Cats moves toward 1.0.

But anyway, to @alexandru's point…  I definitely don't consider effect suspension to be a valid use of Eval. The comonadic nature means that effects are not really safely suspended, and errors cannot be handled soundly. Allowing Eval to suspend effects also opens up a bit of a pandora's box with respect to the many function signatures in Cats which use it (e.g. foldRight). I think Eval makes a lot more sense as just being a better =>, which is generally how Cats uses it.

durban commented 7 years ago

Okay, I've mostly given up on this, I'd just like to summarize my position one last time:

I think that it is inconsistent to say that Eval cannot be used for effect capture and to provide APIs which (if used as intended) require the user to create an effect-capturing Eval instance. I think this is much more of a problem for Eval than for by-names, because Eval is a data type from a library for pure functional programming, while by-names are a language construct of a non-purely-functional programming language.

However, if people generally say, that this is not a problem, or not worth worrying about ... well, I can live with that :smile:

(Just for avoiding any misunderstandings: the APIs I have a problem with are IO.fromFuture, IO.eval and, as it turns out, ApplicativeError.catchNonFatalEval. I wasn't aware that that exists, or I would've opened this issue in the Cats repo. I might still do that ...)

durban commented 7 years ago

BTW, I'm willing to work on a PR for a signature like def fromFuture[F[_]: Effect](ffa: F[Future[A]]): IO[A]. I realize that it would be incompatible, but now it's still possible change it. After 1.0, I guess not ...

djspiewak commented 7 years ago

@durban

I think that it is inconsistent to say that Eval cannot be used for effect capture and to provide APIs which (if used as intended) require the user to create an effect-capturing Eval instance.

You are correct that this is an inconsistent position. I'm not as fussed about it mostly because the only effect that would reasonably be captured by the fromFuture Eval is the effect of starting the Future. Clearly this is an effect, but despite the inconsistency, it doesn't strike me as harmful. But I'm started to be swayed to your point of view…

the APIs I have a problem with are IO.fromFuture, IO.eval and, as it turns out, ApplicativeError.catchNonFatalEval

IMO, catchNonFatalEval just shouldn't exist. I would be in favor of removing that function.

Beyond that, IO.eval isn't intended to capture effects at all. It's intended to be analogous to IO.pure, except for lazy evaluation.

BTW, I'm willing to work on a PR for a signature like def fromFuture[F[_]: Effect](ffa: F[Future[A]]): IO[A]

I've turned this back and forth in my mind a few times. I wonder if it would be better to have def fromFuture(ffa: IO[Future[A]]): IO[A] rather than F[_]: Effect, just for consistency. Nothing else in IO (other than to) is abstracted. Still not convinced this is 100% the right way to go, but you're starting to sway me. :-)

alexandru commented 7 years ago

Well, I disagree with you both. First of all, I think laziness is capturing side effects as a consequence, whether you want it or not.

So @djspiewak @durban lets be rigorous and mathy about it. Before talking about any changes I want an example that breaks IO.fromFuture as is currently defined, or highlights some sort of problem, or indicates some evolutionary trait of Eval that might pose problems in the future. Just one sample.

Because making changes for the sake of purity without demonstrating that we've got a problem is a no go. It's not even "theoretical" if you can't give some provable theory and we shouldn't succumb to non-falsifiable arguments. I admit that I might be wrong, but I'm willing to be educated 😀

The Eval in IO.fromFuture isn't intended for suspending effects, because Eval is not an appropriate datatype for effect suspension. We can in fact demonstrate this by attempting to define Sync[Eval] and watching the laws break!

I'm pretty sure that we can define a Sync[Eval] that does not break any laws as defined by SyncLaws.

Of course, you can say that Eval is not adequate for capturing side-effects, due to not having good error handling capabilities. In fromFuture however that value gets lifted in the IO context, which makes this point moot.

And I can't stress that enough 😐

When you raised the issue on Cats about Group not being lawful because Eval has a Comonad implementation, that speaks about a Sync[Eval] being incompatible with other type-class implementations of Eval, but those type-classes don't necessarily define what Eval is or what it can do.

As I mentioned then, the users themselves can still define a perfectly lawful ApplicativeError implementation for Eval. Of course, I agreed with your argument that we should not implement things in terms of throw, along with other arguments like the library having to basically pick between Comonad and ApplicativeError because in this case they are incompatible and we want to educate users to do the right thing.

However that line of thought is bringing us conversations like the current one. If we agree that fromFuture(Eval) no longer works for us, this is making it clear that Eval is no longer suitable as a replacement for by-name parameters in Scala.

In which case we should raise a ticket in Cats to talk about extracting Eval from cats-core into another project, because it's not generic enough to warrant inclusion in the core. In which case I'd prefer a decision soon, because cats-effect is not the only project that uses Eval in place of by-name parameters.

djspiewak commented 7 years ago

First of all, I think laziness is capturing side effects as a consequence, whether you want it or not.

It's not though. We can define purity (which is to say, the absence of side-effects) quite rigorously:

f e e ≡ f v v, where e ↦ v

This is in fact, the formal definition. None of this says anything about evaluation semantics. Which is intuitive, since we can write pure programs in strict or lazy languages alike.

Laziness simply refers to evaluation semantics, of which there are three major ones:

There is also a fourth strategy which is almost never used called "β evaluation". Mathematica is a notable exemplar of this strategy.

In a pure and total language, there is no semantic difference between these three strategies: they will all yield the same results, though some may require fewer evaluation steps than the others. In a pure language which is not total (such as Haskell), evaluation strategy matters intuitively because the call graph is not normalizing. Quick intuitive example:

derp = derp

example = test derp 42
  where test _ a = a

If you invoke example, the results will be 42, despite the fact that derp is an infinite recursion. This program is still pure (note that derp doesn't produce a value, so it still satisfies our definition above, albeit trivially so).

Anyway, all of this is to say that evaluation strategy (broadly, strict or lazy) is entirely independent of purity. We can have purity with lazy evaluation, we can have impurity with strict evaluation, and all permutations of the two. Laziness doesn't need to capture side-effects.

Before talking about any changes I want an example that breaks IO.fromFuture as is currently defined

Assuming that people solely use the Eval to capture the effect of starting the Future, then no practical problem can be observed. There are two dangers here. First, the signature is less parametric than it could be, since it sort of implies that the construction of the Future (within the Eval) is pure, when it in fact is not. Second, we're outright relying on two major semantics of Eval and Future: the fact that Eval only evaluates suspensions once (which is not a hard guarantee), and the fact that starting a Future cannot (to my knowledge) produce an error. If either of those semantics change (or are already false), then we're in trouble.

I'm pretty sure that we can define a Sync[Eval] that does not break any laws as defined by SyncLaws.

Here's what happens if you try: https://gist.github.com/eb99856ca34b3dc505d2e4e9f97bdb0e

However that line of thought is bringing us conversations like the current one. If we agree that fromFuture(Eval) no longer works for us, this is making it clear that Eval is no longer suitable as a replacement for by-name parameters in Scala.

Only if you assume that by-name parameters are by definition effect-capturing, which is not a view that I accept. I use laziness all the time for things that have nothing to do with effects (such as short-circuiting, or representing graphs). Eval is perfectly suitable for all of these use-cases, which is its intended target within cats.

SystemFw commented 7 years ago

:+1: to def fromFuture[A](f: IO[Future[A]])(implicit ec: ExecutionContext): IO[A]

I like this signature better. I won't repeat @durban and @djspiewak arguments since I'm in perfect agreement with them, but I'll say that from a pure FP scala perspective, IO.apply (F.delay) is a very, very special method, which I view it as the FFI to the impure side of the language. Therefore, we should keep the surface of this FFI as small as possible. Eval is still great for pure laziness though.

mpilquist commented 7 years ago

Another :+1: for:

def fromFuture[A](f: IO[Future[A]])(implicit ec: ExecutionContext): IO[A]
alexandru commented 7 years ago

It appears I might be in minority so I'm going to back down, deferring this decision to you.