Remove ApplicativeError#catchNonFatalEval

[durban]

ApplicativeError#catchNonFatalEval uses Eval for effect capture. However, as far as I can tell, Eval is not for effect capture. For related discussion, see typelevel/cats-effect#78 (possibly that discussion should be continued, instead of starting a parallel one here).

/cc @djspiewak @alexandru

[tpolecat]

I'm usually "that guy" but in this case I don't see a problem. These methods are for lifting expressions whose evaluation is partial but deterministic, like "foo".toInt; i.e., computations that are morally pure, but due to bad API design signal failure via throwing. This restriction allows us to consider the inner catch to be pure when it would otherwise be a side-effect. This operation is referentially transparent (up to termination, as is always the case) so all is well. If you're passing a side-effect then you're in trouble since nothing in cats-core has anything to say about it.

So I think the case here is distinct from effect-capturing operations in cate-effect. I haven't had time to read the thread carefully so I won't comment on its use there.

[durban]

My problem with the signature of catchNonFatalEval is that it sends the message, that creating Eval instances, where calling .value throws an exception is okay. I explained in the other thread why I think that simple by-names should be used for things like this (like in catchNonFatal).

[alexandru]

In my opinion creating Eval instances where .value throws is perfectly OK.

val e = Eval.always { 1 / 0 }

And then:

try e.value catch { case _ => 0 }

See, the world didn't implode? Which brings us back to my pet peeve that we still don't have a lazy Applicative.pure alternative.

[SystemFw]

try e.value catch { case _ => 0 }

Well, the world didn't implode because you "cheated" by using try/catch, at which point you could as well remove catchNonFatal and catchNonFatalEval altogether. I think catchNonFatalEval could be removed with zero consequences, tbh (happy to be corrected ofc), especially if typelevel/cats-effect#78 goes through (which I think it should)

[johnynek]

I added it (I think) so maybe no surprise: I’m negative on it but it is not a huge deal. You can always recover it by doing catchNonFatal(e.value).

I hate to speak for @non and he can correct me, but knowing him I would be very surprised if he had a notion that Eval should never cause an effect to happen. I’m not really sure why we think this.

I’m mostly negative on removing methods because we fear they encourage something someone doesn’t like. My position is to never throw except where there is a logic error (code believed to be unreachable but difficult or expensive to prove to be unreachable at the type level). Yet, other people don’t always follow this rule and I thought the method was a convenience, but like I say, a very minor one.

[alexandru]

@SystemFw using try / catch is not a cheat because it is part of the language and in the case of ApplicativeError.catchNonFatal we are talking about lifting that Eval into an F[_] applicative context that can handle errors just fine.

This is exactly the same situation with the cats-effect/78 issue — we are talking about lifting an Eval into an IO context that can handle side effects and errors. It's not the Eval itself that's supposed to handle errors, it's not the Eval by itself that's supposed to handle side effects.

My question there for @durban, for which I did not receive a satisfactory answer is this — what makes by-name parameters special and why can't Eval.always be a replacement for by-name parameters in 100% of the use cases, as it was sold actually?

And to get it out of my chest, I also think that Eval shouldn't have implemented Comonad, a mistake for which we are now paying.

So here's why I strongly disagree:

• I was led to believe that Eval is a complete replacement for by-name parameters — by @non actually in one of his presentations ; which is why I ended up investing in it, for example adding support for it in Monix
• I was led to believe that Cats is a pragmatic alternative to Scalaz, an alternative that plays well with the Scala language, playing to Scala's strengths — e.g. by preferring strictness by default and by not reinventing data types from the standard library and recognizing that Scala is not Haskell
• I considered Eval to be one of the core Cats innovations due to tackling the problem of the evaluation model in a strict language

Of course, priorities and mentalities change, this might be a sign of maturity, or it might be a sign of mismanaging expectations, however this also constitutes breakage for users in a very bad sense.

To get this far in the development process without a clear understanding of what Eval should be used for and to consider removing or changing signatures or use cases that people have grown to depend on — that would be a sign that Cats 1.0 isn't mature enough for production use.
We should really not do that.

But anyway, if you really declare that my understanding of Eval is wrong, then:

1. we should consider removing cats.Eval from cats-core completely, into a separate package maybe, because we should not expose data types with marginal use-cases; cats-core is mainly about type classes and utilities, data types are for the standard library or for sub-projects like cats-effect
2. let me know that conclusion quickly, because I'll need to remove traces of Eval from Monix before releasing version 3.0

Thanks,

[johnynek]

Removing Eval is a non-starter in my view. A huge amount of cats code uses it and people who have adopted cats use it a lot. It is not marginal and baked into many of the typeclasses (for instance where we want lazy return values).

[tpolecat]

I guess I mostly see the use of Eval here as a consistency thing, since we have decided to use it rather than (or in addition to) by-name args in our APIs generally.

Re: @johnynek's point there's nothing stopping you from using Eval as a simple kind of IO … the IOLite type in doobie 0.4 is basically that, so it's certainly capable and is mostly equivalent to scalaz IO. The practical issue though is that it's not used with that possibility in mind: the method in question here calls .value for you, which you would never do with IO. And I think you could probably use a side-effecting Eval to observe the order of traversal for folds, which is something you usually want to keep private to the implementation. So I think Eval's niche is reasonable.

[SystemFw]

...notion that Eval should never cause an effect to happen. I’m not really sure why we think this.

@johnynek I think this because, unlike when Eval was introduced, we now have something to talk about this precisely: cats-effect.

Things that are able to suspend side-effects lawfully should be instances of cats-effect Sync, and things that are not instances of it should not be used to suspend side-effects.

That being said, I feel way less strongly about this than I feel about typelevel/cats-effect#78 , because like @tpolecat I expect this to be used for stuff like "foo".toInt, and in any case I don't think Eval should be removed since it has plenty of use cases with no relation to side effects.

P.S. @alexandru I haven't investigated that, so I can't say that I agree or disagree, but if we were to remove Eval Comonad instance and make it an instance of Sync, my points would be moot.

[durban]

@alexandru

My question there for @durban, for which I did not receive a satisfactory answer is this — what makes by-name parameters special and why can't Eval.always be a replacement for by-name parameters in 100% of the use cases, as it was sold actually?

As I've already tried to explain over at typelevel/cats-effect#78, my position is that for that, Eval would have to be SyncIO (or IOLite, or whatever). A data type like that would make sense. Even Eval could be that data type. However, in my opinion, as currently Eval is defined and used, it is not that. For it to become SyncIO, value would have to be something like unsafeValue (and in a lot of places, it mustn't be called), a few typeclass instances would have to be removed (e.g., Comonad), and maybe a few instances would have to be added (mostly Sync).

[ceedubs]

My 2 cents:

I don't think that I've seen an argument for removing this that's compelling enough to break compatibility. I don't see anyone arguing against the existence of a method similar to this one. It seems that the main argument against this method is that it should use a by-name parameter instead of an Eval parameter. I don't have particularly strong feelings about this, but using Eval makes it more consistent with evaluation control in other parts of Cats; and someone implementing an IO-like type could conceivably want to override this to respect the difference between Always and Later. I think that there is a slight argument for keeping it the way that it is, and even in the case of a tie I think that not making a breaking change should win out.

[durban]

Then how about making it protected[ApplicativeError]? That would preserve backwards binary compatibility.

[ceedubs]

@durban preserving backwards binary compatibility is only one component. There still doesn't seem to be consensus that catchNonFatal shouldn't exist.

[ceedubs]

The majority of people who have weighed in on this vote to keep it the way that it is. Since this seems to have stagnated, I'm going to go ahead and close it out.