From b06c460197bd5258b363a3b9d2fa54fdfb9d17b4 Mon Sep 17 00:00:00 2001 From: Vladislav Zavialov Date: Thu, 7 Dec 2017 03:28:20 +0300 Subject: [PATCH] [CSL-1842] Write the guidelines for exception handling --- docs/exceptions.md | 246 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 246 insertions(+) create mode 100644 docs/exceptions.md diff --git a/docs/exceptions.md b/docs/exceptions.md new file mode 100644 index 00000000000..d8780bd266b --- /dev/null +++ b/docs/exceptions.md @@ -0,0 +1,246 @@ +# Guidelines for Exception Handling in Cardano SL + +To determine the preferred way to handle exceptions in a piece of code, first +answer these questions: + +1. Is this a programmer mistake or a regular error? +2. Is the code 100% pure or potentially impure? + +Depending on the answers, an appropriate approach can be selected. + +## Is this a programmer mistake or a regular error? + +### Programmer mistake + +When a particular codepath is not meant to be ever executed, but happens to be +executed anyway, this is a programmer mistake. + +For instance, consider this piece of code: + +``` +-- Precondition (unchecked): input lists are the same length. +zipSameLen :: [a] -> [b] -> [(a, b)] +zipSameLen [] [] = [] +zipSameLen (a:as) (b:bs) = (a,b) : zipSameLen as bs +zipSameLen _ _ = error "Lists of different length, precondition violated!" +``` + +The `zipSameLen` function is meant to work on two lists of the same length, and +calling it with lists of different length is a mistake on the programmer's part. +Code like `zipSameLen [] [1]` must be rejected at code review stage. However, +the function still must have the `zipSameLen _ _` case to have exhaustive +pattern matching. + +A function that can be used erroneously (has a potential for programmer mistakes) +MUST have a comment that describes its preconditions. When it is called, the +call MUST have an explanation that assures why these preconditions hold. + +For instance, when a programmer calls `Prelude.last xs`, he must add a comment +at this call site that explains why `xs` is guaranteed to be non-empty, even if +it seems obvious. A better option is to use functions and types that don't allow +for errors: instead of `Prelude.last` one can use `NonEmpty.last`. + +In case of `zipSameLen` we could use length-indexed vectors: + +``` +zipSameLen :: Vec n a -> Vec n b -> Vec n (a, b) +``` + +It is always a tradeoff whether to allow programmer mistakes, or do type-level +trickery to avoid them. The decision process for this is out of scope of this +document. + +### Regular error + +Consider a function like `readFile`. It is entirely possible that the path +passed to it may point to a file that does not exist, and the programmer can't +do anything to prevent this. Existence of a file is not a property of our code, +it's the property of the outside world, and we have to deal with all possible +scenarios. + +Another example is parsing user input. We might expect the user to enter a +number, but we have to consider the case that the user enters something else. + +In cases when the erroneous scenarios are out of our control, we consider +these to be regular errors. + +### Identifying error class + +Can the execution of the error codepath be excluded by code review and static +verification (more precise types?) + +* Yes: it is a programmer mistake +* No: it is a regular error + +## Is the code 100% pure or potentially impure? + +### Impure code + +The code is considered impure when it's an `IO` action, a function that returns +an `IO` action, or similar. The code can be _potentially_ impure when it is +written in an abstract monad that can be instantiated to `IO`. + +Definitely impure: + +``` +readFile :: FilePath -> IO String +``` + +Potentially impure: + +``` +lookupThrow :: MonadThrow m => Map k v -> k -> m v +``` + +### Pure code + +Pure code is one that does not use `IO` operations. For example: + +``` +lookupMaybe :: Map k v -> k -> Maybe v +``` + +Notice that while it is conceivable that we have a `Map` of `IO` actions, and +instantiate `lookupMaybe` to `Map k (IO ()) -> k -> Maybe (IO ())`, the +instantiation does not inspect/use these `IO` actions, so it's not considered +potentially impure. + +### Identifying code purity + +Is there an instantiation of type parameters that would mean that there are +`IO` actions inspected/used in the code? + +* Yes: it is potentially impure code +* No: it is 100% pure code + +## Error handling practices + +### Pure code, programmer mistakes + +Do *not*: + +* do this often, as if it's okay +* use `undefined` +* use non-exhaustive pattern matching + +Do: + +* try to use types to avoid the need in the first place +* comment extensively (invariants and precondition, reasoning) +* use the `error` function (or `impureThrow` with custom exception) +* use the `HasCallStack` feature + +Before writing code that allows programmer mistakes, consult with colleagues how +it would be possible to prevent them statically. There might be tricks that +you're not aware of. Always make this trade-off conciously and responsibly. + +(On the other hand, if static guarantees require GADTs or type families, perhaps +it's better to not overcomplicate code. Use your judgement.) + +DISCUSSION: Should we create a synonym `bug = impureThrow` in Universum? This +would make the intention more clear. + +### Pure code, regular errors + +Do *not*: + +* use `error` or `impureThrow` +* return `Either Text` + +Do: + +* return `Either ErrorADT`, `Maybe` +* wrap the underlying (pure!) monad in `ExceptT` or `CatchT` +* use `MonadError` or `MonadThrow` + +Consider parsing: it is pure, but we cannot make assumptions about the input. In +this case we might want to use `ExceptT ParseError`. Or consider a lookup in a +`Map`, where we don't know whether the key is present -- in this case we'd like +to return `Maybe v`. In 100% pure code, use one of these ways to handle errors: + +* `Maybe`, `Either e` +* `MaybeT`, `ExceptT e` +* `CatchT` + +Avoid using `Text` with the error message in place of `e` -- create a proper +ADT. In case creating a proper ADT feels too cumbersome, use `CatchT`, which +is equivalent to `ExceptT SomeException`. + +Be careful not to use `MaybeT`, `ExceptT`, and `CatchT` in potentially impure +code. When in doubt whether the code is potentially impure, use `MonadThrow`. +(The reason we don't want `ExceptT` and co. in potentially impure code is that +they add additional exception mechanisms to the one that `IO` has, and +`catch`/`bracket` don't account for this). + +### Impure code, regular errors + +Do *not*: + +* use `error` or `impureThrow` +* use `ExceptT`, `MaybeT`, or `CatchT` +* use `Maybe` or `Either` +* use `MonadError` +* use `throwIO` + +Do: + +* create a custom exception type +* use `throwM` (`MonadThrow`) + +We disallow the use of `throwIO` only because it is redundant in the presence of +`throwM` and requires a stronger constraint (`MonadIO` rather than +`MonadThrow`). + +Derive prisms for exception types with multiple constructors, so it's convenient +to use them with `catchJust`. + +### Impure code, programmer mistakes + +Use the same techniques as in pure code -- `error` or `impureThrow`. There are +two reasons for this: + +* we would rather catch the error sooner than later, and `impureThrow` explodes + when forced to WHNF, while `throwIO` or `throwM` explode when executed + +* when in an abstract (but potentially impure) monad, using `throwM` might + add an additional constraint + +## Packages and modules + +Do not import `Control.Exception` or `Control.Monad.Catch`! We use the +`safe-exceptions` to deal with asynchronous exceptions gracefully, so import +`Control.Exception.Safe`. + +## Resource handling + +Use `bracket` or `ResourceT` to guarantee the release of resources. In case of +concurrent code, avoid `forkIO` or `forkProcess` in favor of the `async` +package, as it rethrows exceptions from the child threads. + +## Migration + +We should get rid of `Mockable Throw` and `Mockable Catch`, as they buy us +nothing compared to `MonadThrow` and `MonadCatch` but have less ecosystem +support -- for instance, the `safe-exceptions` package doesn't use them. + +We should identify the parts of the code that use `ExceptT` in impure or +potentially impure code and replace them with exceptions. + +We should make sure that no code imports `Control.Exception` or +`Control.Monad.Catch`, and use `Control.Exception.Safe` instead. + +### Code references + +This list is not exhaustive: + +* `TxpGlobalVerifyMode`: https://github.com/input-output-hk/cardano-sl/blob/8507d03ba928e07daea57f9a52f6dc03a9d65779/txp/Pos/Txp/Settings/Global.hs#L37 + -- `MonadError` in potentially impure code +* ... +* TBD + +## Literature + +The following documents were used to create these guidelines: + +* https://www.schoolofhaskell.com/user/commercial/content/exceptions-best-practices +* https://github.com/fpco/safe-exceptions#readme