[#153] Control NonFatal Throwables

[kartoffelsup]

As discussed in https://kotlinlang.slack.com/archives/C5UPMM0A0/p1550319070097800
Port of the Scala NonFatal.

I've hopefully found all places where it should be applied. I've searched with catch \(.*:\s+Throwable\) in all Production Files using IntelliJ.

Closes #153

[raulraja]

This is great. thanks so much @kartoffelsup ! 🎉
I left some comments to improve the boilerplate an enrich the MonadThrow type class so all data types befit from these changes individually as projected extensions and you can remove the boilerplate in abstract places where you are an instance of MonadThrow already.

[raulraja]

We can encapsulate this pattern For All MonadThrow<F> which constitutes most of the changes in this PR.
If you place this function as part of the MonadThrow type class

fun <F, A> Throwable.raiseNonFatal(): Kind<F, A> =
  if (NonFatal(this)) raiseError<A>(this) else throw e

This functions is scoped to the MonadThrow receiver and will be available in all instances of all the data types as extension functions and would allow you to remove the if else boilerplate in most cases below.

[kartoffelsup]

Thanks for the review and the great suggestion. :) I've applied it to all but IOBracket, it didn't work there (I guess because it's not running in the MonadThrow context?)

There is also a AbstractMethodError in IOTest MonadDefer bind: binding failure that I can't quite figure out.

[pakoito]

Change the genError code to throw something else instead, maybe that's the issue. I'd add a new generator that throws one of the fatal errors and test for that.

[kartoffelsup]

After updating graalvm to rc12 (my jdk), the test no longer fails.
Should I add the fatal error test to MonadDefer laws?

[pakoito]

I'd add a MonadThrow laws altogether that have all MonadErrorLaws, put that test in them, and make MonadDefer do MonadThrowLaws instead of MonadErrorLaws.

That way we have tests where the functions are defined in the hierarchy :D

[kartoffelsup]

Sorry, I'm kind of lost now. :D I've added the suggested function raiseNonFatal to MonadThrow.
What else can I do? (and how can I/should I test it as MonadThrow Law?)

[raulraja]

@kartoffelsup this should cover it:

• override the two functions above using raiseNonFatal to implement them in MonadThrow
• Add a new law in the arrow.test.laws.MonadErrorLaws

Monad Error Laws: NonFatal is caught

• Add a new la in the arrow.test.laws.MonadErrorLaws

Monad Error Laws: Fatal errors are thrown

• Add kdoc and a kotlin:ank:playground snippet similar to the one in Functor that illustrates the usage of NonFatal in the NonFatal object.

That should do it.

[kartoffelsup]

@raulraja thanks for the guidance!
Regarding the catch overrides, atm I've already overridden them in ApplicativeError like so:

 fun <A> catch(recover: (Throwable) -> E, f: () -> A): Kind<F, A> =
    try {
      just(f())
    } catch (t: Throwable) {
      if (NonFatal(t)) {
        raiseError<A>(recover(t))
      } else {
        throw t
      }
    }

  fun <A> ApplicativeError<F, Throwable>.catch(f: () -> A): Kind<F, A> =
    catch(::identity, f)

Do we really need the override in MonadThrow?
Also, it looks to me that the catch(recover: (Throwable) -> E, f: () -> A) variant can't be done with raiseNonFatal as I am unable to call #map on it for the recover function.

[raulraja]

no, if you already did it there it's fine how you did it.

[kartoffelsup]

I've applied the changes you requested. Please let me know if there is anything else I can do. Thanks again for the help!

[raulraja]

e.raiseNonFatal()

[raulraja]

Looks like reactor Flowable is not stack safe https://app.bitrise.io/build/73c812fbc6ef9620.
Can you decrease that number? Not sure why this is showing up here now though.
You can check the docs run by running ./gradlew clean dokka :arrow-docs:runAnk

[raulraja]

I'll rerun the build and if it passes we will leave that for another PR

[raulraja]

@pakoito any idea what is going on here and why this has not surfaced before? https://app.bitrise.io/build/288ec840d3cdc0ca

[pakoito]

None of the Rx constructs are stack-safe unless you introduce an asynchronous jump. It was surfaced, I opened a ticket and it got closed and all ReactiveX/RxJava#6322

[kartoffelsup]

Looks like that is intended there? https://arrow-kt.io/docs/integrations/rx2/#stack-safety, the comment above states this is what's supposed to happen. But the StackOverflow was caught previously.

@raulraja I've annotated the StackoverFlow sample with ank:fail, all green now.