Ported IO#ensuring (and renamed to avoid mass confusion)

[djspiewak]

This function generalizes finally in the try/catch/finally triumvirate. Very much open to suggestions on the name. As a random note, is ensure even a useful function?

Also closes #1630

[codecov-io]

Codecov Report

Merging #1662 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1662      +/-   ##
==========================================
+ Coverage   93.88%   93.88%   +<.01%     
==========================================
  Files         246      246              
  Lines        4088     4091       +3     
  Branches      154      150       -4     
==========================================
+ Hits         3838     3841       +3     
  Misses        250      250

Impacted Files	Coverage Δ
core/src/main/scala/cats/syntax/monadError.scala	100% <100%> (ø)	⬆️
core/src/main/scala/cats/MonadError.scala	100% <100%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1931c93...dac34b9. Read the comment docs.

[johnynek]

can we add a test of some kind to this?

I mean, even adding a reimplementation of this code to the laws for MonadError along with an example exercising it.

[kailuowang]

👍 on adding to law. maybe also add quick doctest as an example and to provide coverage for the syntax code.

[djspiewak]

A law would be useless, since it would over-constrain implementations. The only useful overrides of guarantee would be for implementations which have it as a structural primitive, but in that case guarantee would not be equivalent to flatMap composed with attempt. So it would be a very round-about way of making the function final and running a ton of redundant tests.

The right thing to do is add a test, but unlike MonadTest, MonadErrorTest doesn't currently exist. I'm all in favor of adding it, but this falls into the category of things that are somewhat harder to work on (since I have to figure out how all that stuff works). Incoming sometime in the next few days, hopefully.

[kailuowang]

I added the MonadTests, it's basically just a vanilla scalatests suite. It should be trivial to create a MonadErrorTest class and just write regular scalatests tests for guarantee in it.

Update: actually there is already a MonadErrorTest it's called MonadErrorSuite, we should probably rename it.

[edmundnoble]

@djspiewak If guarantee is a structural primitive, it must still behave as flatMap composed with attempt, no? I don't see how anything otherwise could be the case. What is the difference between a law and a test in this case?

[djspiewak]

I was thinking of a situation where a datatype adds extra guarantees to guarantee, meaning that it would have distinct semantics from flatMap composed with attempt. I certainly can't prove that this is impossible, so I wouldn't want to rule it out completely with a law.

In general, I feel like concrete convenience functions (like this one) should be tested with tests, not laws, especially when their behavior follows trivially from other laws.

[edmundnoble]

I need to see tests here specified in terms of the "bracket problem" (see http://hackage.haskell.org/package/layers-0.1/docs/Documentation-Layers-Overview.html#g:9 and the next couple sections).

I am not convinced this belongs on MonadError. As we've discussed previously, a monad layer being able to return an error has nothing to do with the rest of the effects in the stack being able to halt the computation, which is what guarantee exists to guarantee semantics for.

[ceedubs]

@djspiewak this won't guarantee the execution of the finalizer in the presence of an exception being thrown unless F catches exceptions in its flatMap implementation, right? I have no problem with that limitation, but I think that we should explicitly call it out in the ScalaDoc comment, since this is being compared to finally.

[djspiewak]

@ceedubs It doesn't guarantee anything about exceptions. In fact, most datatypes that form a MonadError do not catch exceptions and thus will not guarantee anything about finalizers. Just like handleErrorWith will not receive exceptions that are thrown unless the underlying datatype implements that behavior.

[ceedubs]

@djspiewak I guess that I just think that guarantee is a pretty strong name for something that isn't...guaranteed...to execute if an exception is thrown. I think that at a minimum attention should be drawn to this in the ScalaDoc comment. Furthermore, I wonder if there's a better name. onFinish comes to mind, but I don't know whether that's a particularly good one.

[edmundnoble]

I think guarantee is reasonable. MonadError has nothing to do with exceptions. If we want a MonadTry, which is guaranteed to catch exceptions, that might be able to deal with them. There's precedent in Haskell.

[ceedubs]

@edmundnoble @djspiewak just to be clear, I agree that this shouldn't try to catch exceptions. I'm just saying we should help users realize that that's the case :)

[djspiewak]

@ceedubs I'm ok with either onFinish or guarantee; neither seems like a particularly good name. :-)

[djspiewak]

@edmundnoble @johnynek @kailuowang Should be ready for a re-review. Tests added.

[djspiewak]

@edmundnoble or @johnynek: Would one of you mind reviewing the current state of this PR and giving me a 👍 or 👎? I'd like to land this soon.

[LukaJCB]

I think this is pretty valuable and worthy of adding to the 1.0 milestone. What do you guys think?

[johnynek]

If we can add a test it seems fine to add to me.

On Fri, Nov 17, 2017 at 16:48 LukaJCB ***@***.***> wrote: I think this is pretty valuable and worthy of adding to the 1.0 milestone. What do you guys think? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1662 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEJdvsyiaFsRiefYMdMlpcxM4zhH8Lwks5s3ilPgaJpZM4NYo0M> .

-- P. Oscar Boykin, Ph.D. | http://twitter.com/posco | http://pobox.com/~boykin

[tpolecat]

This doesn't work with OptionT because the finalizing bind never happens.

scala> val fin = OptionT.liftF(IO(Console.println("hi")))
fin: cats.data.OptionT[cats.effect.IO,Unit] = OptionT(IO$1731525909)

scala> (OptionT(IO(Option(1))) guarantee fin).value.unsafeRunSync
hi
res20: Option[Int] = Some(1)

scala> (OptionT(IO(Option.empty[Int])) guarantee fin).value.unsafeRunSync
res21: Option[Int] = None

So I think we can't offer this combinator. I'm pretty convinced that it has to be a primitive.

[LukaJCB]

I see, that seems pretty reasonable. Maybe it could be part of the cats-effect type class hierarchy.

[ceedubs]

@tpolecat thanks, I think that your counterexample is a really important one.

I'm going to go ahead and close this out, because I don't think that it satisfied the use-case that it was intended for. It seems like the discussion has continued in typelevel/cats-effect#88 . @djspiewak let me know if you disagree and think that this should be reconsidered.