adds recipe CapabilityPatternWithCheckedExceptions

[afcondon]

submission for #261

[JordanMartinez]

Thanks for your work @afcondon. At the same time, I don't think this recipe in its current state is what should be added here.

Sorry, but I'm having difficulty figuring out what about this recipe just seems off to me. Perhaps it's because none of the exceptions raised are "real" exceptions? Perhaps its because one could use Variant to interleave exceptions from native side-effects (e.g. HTTP request) and pure but still monadic computations (e.g. parsing)? Perhaps its because one could handle the errors at various parts in the code (e.g. handle an exception within the program function, within a type class instance, and/or at the end of runAppM)?

[afcondon]

Fair comments - i, personally, think this pattern has a lot of potential but i also feel like i won't know if it turns out to be less alluring when used in real world application structuring.

I mostly wanted to do it to check that it could be done - which i've done to my satisfaction with it where it's at now.

Perhaps - after Miles' maybe has also weighed in? - we could open it up for discussion on Discourse instead and see what some further critical evaluation of it leads to.

Either way, thanks for looking at it.

[milesfrain]

after Miles' maybe has also weighed in?

Diving into this is on my backlog. Hopefully will be able to offer more meaningful review feedback in a week or so.

Edit: It might be most efficient to review this after after your presentation on the topic (if that's still in the works) in next month's meetup.

[afcondon]

Edit: It might be most efficient to review this after after your presentation on the topic (if that's still in the works) in next month's meetup.

That's a good idea. I may well have some hands on experience of trying to use it in an application by then, too.

[JordanMartinez]

@afcondon Any updates on this PR? I know you gave your presentation a while back, so did you get any feedback that might influence how we should proceed here?

[afcondon]

I didn't get any feedback positive or negative on the checked exceptions aspect. I'm sort of inclined to believe that "app skeletons" like this belong in a slightly different collection than the cookbook.

But that said, i think that my example here would at the very least save someone a lot of work figuring out how to scaffold such a design, thus lowering the barrier to starting a project, which - at least to my mind - is really core value of cookbook recipes. So i'd be happy to take feedback to get this into the cookbook for now.

with regard to your original objections, @JordanMartinez, perhaps the we could get someone to write a counterpoint recipe that does as you suggest with interleaving using Variant or perhaps the readme could just list your caveats? Or drop it altogether?

[JordanMartinez]

I decided to checkout your branch locally to more easily navigate through it. Here's what I think.

First, I think this recipe should be merged into the cookbook because the idea is worth it. However, it shouldn't be merged in its present form.

Second, one problem with the recipe that can be fixed is how you currently handle any errors. Right now, you merge an error value with a valid output value rather than keeping the two separate. I think the code should distinguish these two. Let me walk through this in a bit more detail. getUserName returns back a String value that is later printed to the console:

program :: forall m.
    Logger m =>
    GetUserName m =>
    m String
program = do
    log "what is your name?"
    name <- getUserName -- <-
    log $ "Your name is " <> getName name
    pure $ getName name

In the instance for your monad, you handle all possible errors by doing some effects and then "returning" a default String value. When get doesn't produce an error, there isn't an issue. When it does, this error is later returned as name in program above.

instance getUserNameAppExcVM :: GetUserName (AppExcVM var) where
  getUserName = do
    env <- ask

    -- I want to focus on the below two lines
    name <- safe ((getPureScript env.url) `handleError`
        (errorHandlersWithDefault "there was an error!"))

    pure $ Name name

-- Only showing the type signature
getPureScript :: forall m r
  .  Monad m
  => MonadHttp m
  => MonadFs m
  => String -> ExceptV (HttpError + FsError + r) m String

-- Only showing part of the type signature
errorHandlersWithDefault :: forall m a.
  MonadEffect m =>
  a ->
  { fsFileNotFound     :: String -> m a
  , fsPermissionDenied :: Unit -> m a
  , httpNotFound       :: Unit -> m a
  , httpOther          :: { body :: String, status :: Int} -> m a
  , httpServerError    :: String -> m a
  }
  errorHandlersWithDefault defaultValue = {
      httpServerError:    \error -> do
        Console.log $ "Server error:" <> error
        pure defaultValue
    , -- and similarly for the rest of them
    }

Thus, name in name <- getName can be either an error message or an actual name.

Third and most importantly, you've hard-coded the monad to ExceptV rows monad output as the return value for various things (e.g. HTTP.get, FS.write).

get ∷ ∀ r m.
  MonadHttp m ⇒
  String ->
  ExceptV (HttpError + r) m String
get url = -- code

In normal situations, we wouldn't hard-code the monad. Rather, we would use type class constraints via MonadError:

get ∷ ∀ r m.
  MonadHttp m ⇒
  MonadError (Variant (HttpError + r)) m ⇒
  String ->
  m String
get url = -- code

But I think this is where things get nuanced when it comes to type classes. But before I say anything more, I should probably state my understanding on the two kinds of errors we could throw. First, there are some errors you could throw and which shouldn't be "caught" (i.e. unrecoverable errors). As an example, if your program requires the ~/purescript.html to be writable and you can't write to it, crashing the program with an error message is probably the best way to handle that. I believe that kind of error should be thrown in the API layer but handled in the Main layer.

Second, there are some errors you could throw in the API layer and which could be handled in the API layer (i.e. recoverable errors). For example, sending an HTTP request might fail or timeout, but you can retry the request at least 3 times in the API layer. If any one of those requests works, your program can continue. If it fails on the third try, you could throw a different unrecoverable error in the API layer that is handled in the Main layer by crashing the program with a good error message.

With that in mind, here's the nuance I'm not sure about. I think we would want something like this, but I don't think it would compile because MonadError implies MonadThrow:

get ∷ ∀ otherUnrecoverableErrors otherRecoverableErrors m.
  MonadHttp m ⇒

  -- MonadThrow = unrecoverable errors handled in `Main` layer
  MonadThrow (Variant (ServerError + otherUnrecoverableErrors)) m ⇒

  -- MonadError = recoverable errors handled in `API` layer
  MonadError (Variant (TimeoutError + otherRecoverableErrors)) m ⇒

  String ->
  m String
get url =
  -- pseudo code. Try getting the URL 3 times, retrying when it fails with
  -- a recoverable TimeoutError. Otherwise, fail with ServerError,
  -- which will be handled in `Main` layer.
  tryNTimesThenFailWith 3
    (Node.HTTP.get url)
    (throwError (V.inj _serverError $ "Could not connect to " <> url))

I think the only solution to this problem is to remove MonadThrow from being a superclass of MonadError. I'm not sure whether other core contributors would be happy with that change. This approach to writing applications doesn't have a strong backing yet, so they would likely see this as a niche use case that isn't yet worth the cost of the change.

---

Assuming I'm correct about the third point's type class nuance above, what can we do to get this PR merged? Two things.

1. I think you should first distinguish between which errors should be "recoverable" and "unrecoverable." Recoverable errors should have a handler that shows some form of recovery.
2. Unrecoverable errors should be handled in main.

In other words, for 2, this means redefining runApp, so that it no longer handles the Either case. Rather, main will do that. Since I'm not familiar with purescript-checked-exceptions, I'm not sure whether there will be issues with doing this. But I think this is how it should be done.

runApp :: forall a. AppExcVM () a -> Environment -> Aff a
runApp = runAppExcVM (RProxy :: _ ())
  where
    runAppExcVM :: forall var rl.
      RowToList var rl =>
      VariantTags rl =>
      VariantShows rl =>
      RProxy var ->
      AppExcVM var a ->
      Environment ->
      Aff a
    runAppExcVM _ (AppExcVM appExcVM) env =
      runExceptT $ runReaderT appExcVM env

main :: Effect Unit
main = launchAff_ do
  result <- AppExcVM.runApp program { url: "http://www.purescript.org" }
  case result of
    Right _ -> pure unit
    Left e -> handleUnrecoverableError e

  where
  handleUnrecoverableError = V.match
    { httpServerError:    \error -> do
        Console.log $ "Server error:" <> error
    , httpNotFound:       \error -> do
        Console.log "Not found"
    , httpOther:          \error -> do
        Console.log $ "Other: { status: " <> show error.status <> " , body: " <> error.body <> "}"
    , fsFileNotFound:     \error -> do
        Console.log $ "File Not Found" <> error
    , fsPermissionDenied: \error -> do
        Console.log "Permission Denied"
    }

Thoughts?

[afcondon]

That's a wonderfully detailed comment and analysis, @JordanMartinez, i've been meaning to reply for the last couple of days but haven't had time. I'll try and write a response and/or incorporate your ideas above over the weekend or next week.

(btw this might be a good thing to cross-post to Discourse, cause probably there are more people interested in the ideas than those of us subbed to this GitHub issue)

[JordanMartinez]

@afcondon No worries! Whenever you get to it.

We can crosspost once we've discussed this further and have a final summary for things.

[JordanMartinez]

I'm also wondering whether error handling has any similarities with contravariant logging. Rather than logging, we might have contravariant error handling.