[CSL-1842] Write the guidelines for exception handling

[int-index]

Rendered: https:/input-output-hk/cardano-sl/blob/int-index/csl-1842/docs/exceptions.md

[gromakovsky]

I'd add MonadFail to this list.

[gromakovsky]

I'd write that ResourceT should be avoided if possible and only used when bracket is not enough. It was recommended during one of HBP meetings and I totally agree, because we had several bugs with improper usage of ResourceT.

[gromakovsky]

I'd also get rid of Mockable Bracket. Unlike Mockable Throw and Mockable Catch, there is no equivalent class in exceptions (MonadMask is not equivalent, it's strictly more powerful), so there is the reason to have Mockable Bracket, but I think that the advantage of using MonadMask instead (e. g. that foreign libraries like safe-exceptions are aware of MonadMask) is more important.

[gromakovsky]

I personally think that using ExceptT or MaybeT in impure code is sometimes reasonable, as long as it's not exposed by the interface.
Here is a real example where I find it quite useful and convenient:

cardano-sl/lib/src/Pos/Recovery/Instance.hs

Line 30 in d598003

getSyncStatus lagBehindParam =

getSyncStatus has type SlotCount -> m SyncStatus. It doesn't mention IO directly, so it's potentially impure code.
Its implementation is IO-based (it has MonadIO constraint, also MonadThrow constraint via MonadDBRead). There we want to check several conditions and exit immediately when some of them are met.
ExceptT works really nicely in this case.
Note that the interface doesn't expose ExceptT or MonadError.
There are two alternatives:

1. Use throwM and then catch (or another catching function like try) outside main do-block. I don't like it because throwing an exception and catching it within the same function looks weird and because making SyncStatus an Exception is a bit insane (I don't think we can call it an exception in common sense).
2. Use a lot of nested ifs or cases or similar language constructs. I don't like it because then code will look really ugly.

Maybe you didn't intend to prohibit such usages, but it should be explicitly clarified then.

You can also add this example to Code references section below.

[int-index]

I am still not convinced that using ExceptT here is a good idea. I believe that it can be safe when used carefully, but:

• now you don't have MonadMask instance, meaning that you might need to rewrite code if it needs to allocate resources in the future

• now you have to lift through an additional transformer, which might contradict the practices we're establishing in CSL-1823

• the rules for exception usage become more nuanced: is it ok to do this if it's in a helper function? what if I move this helper function to top-level? what if I put it into a separate module for reuse?

I've seen your comment that you want to allow usage like this, but I don't think that it pulls its weight. Are you sure that doing throwM and try wouldn't give you the same thing?

The transformation is mechanical:

        fmap convertRes . runExceptT $ do
-- becomes
        fmap convertRes . try @SyncStatus $ do

                True -> throwError SSDoingRecovery
-- becomes
                True -> throwM SSDoingRecovery

Adding an Exception instance does not sound bad to me, it's derived by GHC so you don't have to write any code.

It looks like we get the same benefits without any disadvantages if we throwM here.

[gromakovsky]

now you don't have MonadMask instance, meaning that you might need to rewrite code if it needs to allocate resources in the future

I still have MonadMask instance, I can just use lift where I need it. The only place where I can't use lift is throwError, but I don't need MonadMask there.

now you have to lift through an additional transformer, which might contradict the practices we're establishing in CSL-1823

It's hard to talk about it until we come to a conclusion about CSL-1823, but I am already a bit concerned about that. If we are going to prohibit all lifting, what will we do with our tests which live in PropertyM? There we use lift in many places. AFAIU, ExceptT case is the same. Maybe it should be discussed in CSL-1823, not here. Maybe I am just missing something.

the rules for exception usage become more nuanced: is it ok to do this if it's in a helper function? what if I move this helper function to top-level? what if I put it into a separate module for reuse?

It's not hard to come up with concrete rules. For example, we can prohibit ExceptT in types of all top-level impure or potentially impure functions. If you have a nested function which has ExceptT in its type and you want to make it top-level, you can change its type to m (Either Patak Sepulka) and convert it to ExceptT where it's used (which is straightforward).

It looks like we get the same benefits without any disadvantages if we throwM here.

Yes, I agree that there no obvious disadvantages in using throwM. I never had good arguments why ExceptT is preferable in this case. It's mostly based on my personal feeling/intuition, rather than some fact that make ExceptT definite winner.

1. For me SyncStatus conceptually is not an exception, that's why I find it a bit counterintuitive to throw it as runtime exception. And defining its Exception instance. It's not about writing extra code, it's just about intuition behind this. ExceptT name also suggests something exceptional, but I treat it simply as a transformer which provides multiple exit points.
2. ExceptT version is basically just sugar for the version with multiple ifs/cases. And the version with multiple cases is exactly how we want the code to behave. It's really clear how it works if you desugar it. On the other hand, version with throwM involves real runtime exceptions which are implemented somehow.

Anyway, as I said, I don't have and never had solid arguments in favour of ExceptT in this case, it's mostly my personal preference. I can easily discard this preference if others prefer throwM version in this case.

[gromakovsky]

Btw, with throwM approach someone can make joke and throw SSInFuture from e. g. DB.getTipHeader function and we'll catch it and return SSInFuture, though perhaps we shouldn't catch it.
It's something exotic and nobody will do it of course, so you can ignore it, just something came into my mind :)

[int-index]

I still have MonadMask instance, I can just use lift where I need it.

Not quite, you can't do something like this:

runExceptT $ do
  doStuff
  bracket ... $ do
    if b
      then throwError e -- won't work
      else return x

If we are going to prohibit all lifting, what will we do with our tests which live in PropertyM?

I believe we can use it as our base monad, because the only transformer on top that we want is ReaderT and since adding ReaderT is equivalent to adding a parameter to a function, I believe it should be always possible.

If you have a nested function which has ExceptT in its type and you want to make it top-level, you can change its type to m (Either Patak Sepulka)

But returning m (Either Patak Sepulka) isn't recommended either, you should return m Sepulka and throw Patak. The reason is that you wouldn't want to deal with the possibility that Patak might be both thrown and returned in Either. Perhaps we can invent a rule that it's OK to return something as Left if and only if it doesn't have an Exception instance (preferably, it should have an instance like instance TypeError "NOT AN EXC" => Exception Patak to avoid accidents).

It's mostly based on my personal feeling/intuition

I share your feelings, I see value in using ExceptT in this case too. But I'm having a hard time to come up with good, simple and precise rules, to detect these cases, so let's settle for a more uniform (but perhaps not as nice) solution.

[gromakovsky]

It sounds quite sad. So AFAIU in some cases we have to use exceptions for control flow, which is generally considered anti-pattern, at least in some languages. Maybe it's not a big problem for Haskell, though. Or we can write deeply nested code, but it's also sad.
I am not saying that we shouldn't do it, I just don't like this world :/

Regarding your points:

Not quite, you can't do something like this:

Well, there is a workaround to return Either from inside bracket and then have ExceptT $ bracket …. If it's too unpleasant, you can use exceptions instead. Permitting ExceptT in such cases doesn't mean that one must always use ExceptT. It's just another possible approach (which will sometimes be useful).

I believe we can use it as our base monad, because the only transformer on top that we want is ReaderT and since adding ReaderT is equivalent to adding a parameter to a function, I believe it should be always possible.

As I wrote somewhere, having PropertyM in base monad will leave us without MonadMask instance, which is probably too bad. Anyway, perhaps we shouldn't continue discussing it here.

But returning m (Either Patak Sepulka) isn't recommended either, you should return m Sepulka and throw Patak…

1. I think we can prohibit returning Either from potentially impure functions. We can always define an isomorphic ADT. E. g. we have verifyBlocksPrefix :: OldestFirst NE Block -> m (Either VerifyBlocksException (OldestFirst NE Undo, PollModifier)) and we can change its return type to m VerifyBlocksRes where data VerifyBlocksRes = VerifyBlocksSuccess (OldestFirst NE Undo, PollModifier) | VerifyBlocksInvalid VerifyBlocksException. But maybe it would be too strict restriction. Either doesn't always carry something exceptional as its Left. For instance, we have type Block = Either GenesisBlock MainBlock. This rule (the version without «it's OK to return something as Left if and only if it doesn't have» part) would mean we can't return m Block. Yes, we can and should change it do another ADT instead of using Either. But do we want to create an ADT for everything isomorphic to Either?
   Also, let's consider VerifyBlocksRes from above. VerifyBlocksException might have an Exception instance. Should we care about possibility to throwM it? It's not Either, but isomorphic. What if VerifyBlocksRes has 10 constructors only one of which has an Exception instance?
   Alternatively we can change verifyBlocksPrefix's type to OldestFirst NE Block -> m (OldestFirst NE Undo, PollModifier). My intuition suggests that we shouldn't do it, but maybe it's another case where I should ignore intuition.
2. I like this part:

Perhaps we can invent a rule that it's OK to return something as Left if and only if it doesn't have an Exception instance (preferably, it should have an instance like instance TypeError "NOT AN EXC" => Exception Patak to avoid accidents).

3. Exceptions Best Practices suggests that returning Either is ok. Maybe not ideal but not something to prohibit. At least that's how I treat this phrase:

Generally the solution to the ExceptT IO anti-pattern is to return an Either from more functions and throw an exception for uncommon errors.

[int-index]

Does anyone else have an opinion on this? Should we add a rule about allowing local use of ExceptT in impure code and returning Either, as long as the type has no Exception instance? I do not have a strong opinion, but I'm hesitant to add more complications to the rules.

[chshersh]

I like the rule to use ExceptT only with TypeError exception instances for types. I'm not aware of better solution for early exit from function (I'm aware only of Cont-monad solution but I can't call this solution a better one ;) ).

[gromakovsky]

I wrote few more things which I want to be done. But I admit they might take quite a lot of time and I don't know how much time we can spend on this Epic.
If I had to choose only the most important points from those, I would choose these three 👍
But I think it'd be good to also add other improvements which I suggested in comments to CSL-1842 (avoiding Text for errors, avoiding exceptions leakages due to forks, etc.). Maybe we don't have time to do them within this Epic, but at least it will provide broader picture of the problems we currently have.

[int-index]

avoiding Text for errors, avoiding exceptions leakages due to forks, etc

I included these in the guidelines, but not in the migration plan. I'll make it more explicit and copy these to the migration plan.

[volhovm]

Why not? Sometimes we have someFoo = runExceptT someFooDo because every usecase would anyway run runExceptT on the someFooDo. Or do you mean that returning Text error is bad? It's a reasonable tradeoff, i guess.

[int-index]

This is about Text. Define proper ADTs for your errors.

[volhovm]

I can't see why is it always reasonable.

[int-index]

I answered in the neighboring thread where you made the same comment.

[volhovm]

Again, I disagree. Consider that we're writing a message handler. What impureFunc returns is either data or error message to user. Why would one use adt here? We never catch this error.

listener msg = 
  either respondError respondSuccess =<< impureFunc msg

Rephrasing, sometimes Either A B is not a "way of exception handling", but is a handy way to return a result. One more example: let checkData be a impure function that has possible outcomes:

• Exit with success ~ "data is alright"
• Exit with failure ~ "data is not alright because T"
• Exit with normal error ~ "database became inaccessible (if we communicate w/ it over web)" or "we tried to write a log to file, but it doesn't exist".
• Exit with programmer error ~ "something went really wrong and it's not expected"

What you seem to miss is that sometimes it's easier to represent 1/2 difference with Either Text a instead of a custom ADT like DataVerSuccess | DataVerErrorX | DataVerErrorY | ....

[int-index]

Defining an ADT in Haskell is cheap. You never know what the caller of your function might want to do with the Left case, perhaps it has a way to handle specific cases. You assume that the only thing done to the error is displaying it, which is a strong assumption, and may change with time.

To be clear, this rule applies to top-level functions, where you cannot know what the caller intends to do with the return value. Maybe we want to log the error as JSON rather than text, maybe we want to return it from a HTTP request (and assign an appropriate error code), maybe we want to display it in a different language (Japanese), etc...

Just be nice to the consumers of your API and don't return Text. Instead, return an ADT and a function (or a Buildable instance) to display the error.

[volhovm]

@int-index Ok, I support your point in fact, but I think it should be explicitly stated in the document. It's not that straightforward and clear why do we ban using Either for returning non-exceptional failures.

[int-index]

I tried to keep the guidelines to the point, without justifications, so people can use them as a quick reference to make a decision. Good thing that this GH discussion will remain and can be linked to in case there are questions.

[avieth]

Regular error means

In cases when the erroneous scenarios are out of our control, we consider these to be regular errors.

but in a pure program, nothing is out of our control, so does "Pure code, regular errors" make any sense?

[int-index]

Well, technically yes, we do control what our function take as input. But consider these three ways to give a type signature to head:

1. [a] -> a
2. NonEmpty a -> a
3. [a] -> Maybe a

In case (1) we assume that the caller has full control of the passed argument, and passing [] is a programmer mistake. If the provenance of the input list is external (e.g. we've read the list from a file), we (as a caller) need to check whether the list is empty. In case (2) we ensure this invariant on types. But in case (3) we move the check into the function itself, rather than impose it on the caller. You could say that this is not an "error path", but informally you would normally think about this as a failure (we tried to extract the head, but there isn't one).

If we tried to formalize this, I would say in a pure function we can talk about an expected set of codepaths, and we also have checks that ensure that the input satisfies the necessary conditions for this codepath. When the checks fail, we want early exit from the function — this is what I mean by "regular errors in pure code".

I agree that it's not a strict definition, it depends on what you consider to be an error, but that's how we usually reason about code informally.

[avieth]

The lesson here is that head :: [a] -> a is a lie :)

I revise my statement then: in a total pure program, error throwing / handling doesn't make sense. There can be programmer mistakes, sure, but the function is total so in these cases the programmer didn't realize there was a mistake.

[int-index]

Even in total functions, can't we talk about having a "major" codepath and various "error" codepaths? For instance, when we consider parsing, "parse errors" are called "errors" for a reason, no?

[avieth]

You're right it's confusing terminology. It's a parse error, but a parse error is not an error in parsing. Parsing went just fine: we successfully determined that the input string is not in the language. A better terminology would perhaps be parse :: Parser a -> Either Unrecognized a.

[int-index]

Well, I would consider it an error in user input, as opposed to an error in our program. This is the distinction: when input data (or outside world in general) does not satisfy the properties we want (but we have to handle it), I called this a regular error. When our program has a bug, I called this a programmer mistake.

Can you propose better terminology? I do not insist on calling both of these "errors", but there's already a (confusing) tradition to call both of these errors.

[avieth]

I tend to use "error" to mean a programmer mistake i.e. a bug. Something that could be avoided but was not, because we're human. So regarding head :: [a] -> a, yes, the error would be the caller's error, because he was just supposed to know that the input must not be [].

I tend to use "exception" to mean something that the programmer could not possibly have defended against / ruled out and is only capable of handling / responding to. There's no exception in head [], it's a plain error. But reading from a TCP socket that has been reset is an exception. The programmer did nothing wrong, the connection just broke. Failing to handle an exception could be considered an error.

I guess you could also speak of "user error" but that's very different. It doesn't mean any errors in our program. A parse error could be a user error (user gave a malformed config file or something). It can be considered an error to fail to deal with a user error.

[avieth]

Not so sure about this one. f :: forall m . Monad m => m () is pure, even though it can be specialized to IO (). The specialization can never throw an exception / do other side-effects, because forall m . Monad m => m () doesn't permit this.

[int-index]

Well, it is classified as "potentially impure", because it can be instantiated to IO. This means we don't want to use ExceptT there, because it could do IO.

For instance, assume that we have f :: forall m. Monad m => m a -> m a. Suppose we're passing an IO a, and it throws. Inside f we could have used ExceptT that assumed that it was the only way to throw, but now it isn't.

[avieth]

Seems fine to use ExceptT in the definition of f. The resulting forall m . Monad m => m a cannot introduce any of its own throws, so the use of ExceptT is not really adding a new avenue of exception throwing to that term. If f act :: IO a throws an exception we are 100% sure it was thrown by act, and if act has only way exception throwing mechanism then so does f act.

[int-index]

OK, perhaps this definition is overly restrictive. Could we come up with a more precise one?

[avieth]

A Left ParseError from parsing or a Nothing from a map lookup is not an error / exception, it's completely normal. If I get a parse error, nothing has gone wrong, the parser simply decided that the input was not in the language. When I get Nothing from a map lookup, nothing has gone wrong, the thing just wasn't in the map... completely normal.

[int-index]

That's why this is classified as "regular errors", everything is normal, we just have to handle these cases, which are not the main (desired) flow of events.

[avieth]

Right my issue is with the term "error". They're not errors, they are part of the main desired flow of events.

[int-index]

What you consider an error depends on your expectations. When you readFile and the file does not exist, is it an error? No, we should've expected this. When you allocate a chunk of memory and the heap is full, is this an error? No, we are aware that the events can take this turn. If you think this way, nothing is an error.

[avieth]

When you readFile and the file does not exist, is it an error? ... When you allocate a chunk of memory and the heap is full, is this an error?

Yes and yes. Strictly speaking I'd call them exceptions but let's not get hung up on semantics of error vs. exception.

What you consider an error depends on your expectations

You're absolutely right. When I parse something I expect that the input string may not be recognized. When I lookup a key in a map I expect that it may not be in the map. Nothing went wrong in these cases. But with readFile, and with IO in general, things may go wrong for reasons outside of the programmer's control (reasons external to the program itself), and there's no obvious one-size-fits-all response to it, so we resort to exception handling.

[gromakovsky]

If one uses MonadError or MonadThrow, code stops being pure (because m can be e. g. ExceptT () IO or just IO for MonadThrow).

[int-index]

You can write code in a concrete pure monad but use methods from MonadError and MonadThrow, that's what is meant here. For instance, when you write code for CatchT, you use methods from MonadCatch.

[avieth]

Yes and prefer withAsync / concurrently / race wherever possible over async itself, because that one will not automatically kill the child if the parent dies. withAsync basically does a bracket so don't use it in a tail-recursive loop like loop = waitForData >>= \it -> withAsync (process it) >> loop, and be aware of link in case you don't plan to wait for the Async.

[int-index]

Yes, that's the way to use async that I had in mind. I added a clarification.

[chshersh]

Well, it might be even better to use LiquidHaskell for this. I think this is exactly the use case for LiquidHaskell — verify some invariants on compilation state.

[int-index]

Sure, we could experiment with LiquidHaskell in the future. Right now it does not seem mature enough for a project on the scale of Cardano, but I'd like to be proven wrong.

[chshersh]

I don't mind adding bug = impureThrow to universum. With HasCallStack => of course.
You're welcome to open issue here: https:/serokell/universum/issues

[int-index]

Ok, created serokell/universum#100

[chshersh]

Is there any particular reason to use prisms exactly instead of affine traversals? I understand that word prism is much more clear for people. But usage of Prism type class enforces to use lens library instead of some cheaper alternative, like microlens-platform. As for me. even for such big project like cardano-sl package microlens-platform should be enough. Though, I guess there're a lot of dependencies which use lens package by themselves so we can't optimize dependencies here much...

[int-index]

Yes, we want Prism to have both construction and matching.

[chshersh]

I like the rule to use ExceptT only with TypeError exception instances for types. I'm not aware of better solution for early exit from function (I'm aware only of Cont-monad solution but I can't call this solution a better one ;) ).

[chshersh]

Could you please clarify what is non-linear resource? It's not a common terminology... I'm aware of Linear Types and I can imagine which resources can be called linear but still want to make sure that terminology is the same for everybody.

[gromakovsky]

@int-index please clarify.

[int-index]

Non-linear is anything that doesn't fit into the "allocate, use, deallocate" pattern.

[gromakovsky]

1. What's wrong with Maybe?
2. AFAIU, you mean that Either shouldn't be used to represent regular errors, but it's fine to return Either SomethingThatIsNotAnError SomethingElseThatIsNotAnError , is it correct?

[gromakovsky]

Example: is it ok to return Block? Currently Block is Either GenesisBlock MainBlock. Of course it can be changed to a new type which is not Either, but suppose we don't want to do it for some reason. For example, because it's not trivial and will take some time, but one may want to return Block from a function right now.

[int-index]

The idea was to have uniform throwing principle for potentially impure code (i.e. always throw an exception with throwM). Now that we plan to relax this rule to allow Either NonExc NonExc, then allowing Maybe makes sense as well.

[gromakovsky]

So, if my understanding is correct, there are the following concerns which should be addressed before adopting the guidelines from this PR:

1. My new comment. Specifically, I'd like to know what's wrong with Maybe in impure code and some clarifications about Either in impure code.
2. There is a discussion about using ExceptT in impure code in some circumstances. Specifically, we should consider permitting ExceptT in impure code if it's an implementation detail of a function and the first type parameter of ExceptT doesn't have an Exception instance (ideally has instance TypeError "NOT AN EXC" => Exception Patak).
3. @avieth has raised two concerns in two threads: https:/input-output-hk/cardano-sl/pull/2059/files#r156155221 and https:/input-output-hk/cardano-sl/pull/2059/files#r156127346. AFAIU, both of them are only about terminology. Please let me know if they are not only about terminology.
4. Another concern was raised by @avieth here. It's proposed to permit using ExceptT inside f :: Monad m => m () -> m ().
5. @int-index should clarify the meaning of non-linear resource usage.
6. I pushed two small commits recently, someone should review them.

[gromakovsky]

Here is what I propose to do:
0. First of all, I want to notice that we are not writing a static analyzer or something like that, so we don't need to define a strict formal set of rules which everyone must always follow. It's ok to say that some code is not recommended but is sometimes acceptable if there is a good reason to do so. Even if we prohibit some construction, during code review we may notice that it makes perfect sense in some particular case and may want to use it despite being prohibited.
Of course we should try to decrease ambiguity as much as possible, but there is no goal to describe everything formally and completely unambiguously. These are guidelines, not strict rules.

1. Permit returning Maybe from a potentially impure or impure function. Permit returning Either A B, but only if A doesn't have an Exception instance. I like the TypeError trick, I think we should make it recommended, but not strictly mandatory, because writing Exception instance for types like GenesisBlock is insane (at least to me) and hopefully nobody will do it anyway. But if we want to return Either ToilVerFailure a, we should prohibit Exception ToilVerFailure.
   But let's wait for @int-index's reply first.
2. Me, @int-index and @shersh commented on this concern.
   @shersh only says that he likes such usages of ExceptT when instance TypeError => Exception e exists. There no arguments why we should or shouldn't do it.
   @int-index has a feeling that such usages of ExceptT are good, but has reasons not to use it and suggests that we prohibit it.
   I agree with reasons not to use ExceptT and I don't have convincing reasons to use it (and never had, it's only my intuition). So by default I propose to leave it as is (i. e. prohibit ExceptT in potentially impure code completely).
3. I will read terminology concerns again more carefully and will try to improve some words. I don't have anything concrete yet.
4. Both me and @int-index think that ExceptT shouldn't be used in f :: Monad m => m a -> m a. @int-index asked @avieth if he has a better idea (unfortunately we don't).
   By default we'll just leave it as is and prohibit ExceptT there.
   5, 6. Just waiting for @int-index basically.

[shersh]

@gromakovsky I think I'm not who you want to see here =)

[int-index]

@gromakovsky I agree with your plan, will you incorporate the changes into the document? I answered the questions about non-linear resources and Maybe in impure code in the corresponding threads.

[gromakovsky]

I agree with your plan, will you incorporate the changes into the document? I answered the questions about non-linear resources and Maybe in impure code in the corresponding threads.

Ok, will do it after I get quite confident that everyone is fine with that.
Do you agree with changes from my commits?

[int-index]

Do you agree with changes from my commits?

Yes, I reviewed them back when you pushed.

[gromakovsky]

TODO: write that SomeException should be used in pure code only if you are lazy to define a proper ADT with all possible exceptions (because in pure code you know which exceptions can be thrown).

[gromakovsky]

@avieth do I understand correctly that your concerns in https:/input-output-hk/cardano-sl/pull/2059/files#r156155221 and https:/input-output-hk/cardano-sl/pull/2059/files#r156127346 are only about terminology? Do you agree that using Maybe, Either, ExceptT, MaybeandCatchT` to work with e. g. parse errors is reasonable?

If it's only about terminology, can you propose better terminology?