-
Notifications
You must be signed in to change notification settings - Fork 7
ADR 8 - RIO monad in cardano cli #60
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,186 @@ | ||||||||
# Status | ||||||||
- [x] Adopted 2025/02/10 | ||||||||
|
||||||||
|
||||||||
# Required Reading | ||||||||
|
||||||||
- [ReaderT design pattern](https://tech.fpcomplete.com/blog/2017/06/readert-design-pattern/). | ||||||||
- [RIO](https://tech.fpcomplete.com/haskell/library/rio/) | ||||||||
|
||||||||
# Context | ||||||||
In `cardano-cli` we are using `ExceptT someError IO someResult` pattern 292 times in function types in our codebase for different error types. | ||||||||
|
||||||||
 | ||||||||
|
||||||||
The vast majority of these errors are not used to recover, they are propagated and reported to the user. | ||||||||
```haskell | ||||||||
main :: IO () | ||||||||
main = toplevelExceptionHandler $ do | ||||||||
envCli <- getEnvCli | ||||||||
co <- Opt.customExecParser pref (opts envCli) | ||||||||
orDie (docToText . renderClientCommandError) $ runClientCommand co | ||||||||
|
||||||||
... | ||||||||
|
||||||||
runClientCommand :: ClientCommand -> ExceptT ClientCommandErrors IO () | ||||||||
runClientCommand = \case | ||||||||
AnyEraCommand cmds -> | ||||||||
firstExceptT (CmdError (renderAnyEraCommand cmds)) $ runAnyEraCommand cmds | ||||||||
AddressCommand cmds -> | ||||||||
firstExceptT AddressCmdError $ runAddressCmds cmds | ||||||||
NodeCommands cmds -> | ||||||||
runNodeCmds cmds | ||||||||
& firstExceptT NodeCmdError | ||||||||
ByronCommand cmds -> | ||||||||
firstExceptT ByronClientError $ runByronClientCommand cmds | ||||||||
CompatibleCommands cmd -> | ||||||||
firstExceptT (BackwardCompatibleError (renderAnyCompatibleCommand cmd)) $ | ||||||||
runAnyCompatibleCommand cmd | ||||||||
... | ||||||||
``` | ||||||||
- As a result we have a lot of errors wrapped in errors which makes the code unwieldly and difficult to compose with other code blocks. See image below of incidences of poor composability where we use `firstExceptT` (and sometimes `first`) to wrap errors in other errors. | ||||||||
|
||||||||
 | ||||||||
|
||||||||
- The ExceptT IO is a known anti-pattern for these reasons and others as per: | ||||||||
- https://github.com/haskell-effectful/effectful/blob/master/transformers.md | ||||||||
- https://tech.fpcomplete.com/blog/2016/11/exceptions-best-practices-haskell/ | ||||||||
|
||||||||
|
||||||||
# Proposed Solution | ||||||||
|
||||||||
I propose to replace `ExceptT someError IO a` with [RIO env a](https://hackage.haskell.org/package/rio-0.1.22.0/docs/RIO.html#t:RIO). | ||||||||
|
||||||||
|
||||||||
### Example | ||||||||
|
||||||||
Below is how `cardano-cli` is currently structured. `ExceptT` with errors wrapping errors. However the errors ultimately end up being rendered at the top level to the user. | ||||||||
|
||||||||
```haskell | ||||||||
|
||||||||
-- TOP LEVEL -- | ||||||||
|
||||||||
data ExampleClientCommand = ClientCommandTransactions ClientCommandTransactions | ||||||||
|
||||||||
data ExampleClientCommandErrors | ||||||||
= CmdError CmdError | ||||||||
-- | ByronClientError ByronClientCmdError | ||||||||
-- | AddressCmdError AddressCmdError | ||||||||
-- ... | ||||||||
data CmdError | ||||||||
= ExampleTransactionCmdError ExampleTransactionCmdError | ||||||||
-- | AddressCommand AnyEraCommand | ||||||||
-- | ByronCommand AddressCmds | ||||||||
-- ... | ||||||||
|
||||||||
topLevelRunCommand :: ExampleClientCommand -> ExceptT ExampleClientCommandErrors IO () | ||||||||
topLevelRunCommand (ClientCommandTransactions txsCmd) = | ||||||||
firstExceptT (CmdError . ExampleTransactionCmdError) $ runClientCommandTransactions txsCmd | ||||||||
|
||||||||
-- SUB LEVEL -- | ||||||||
|
||||||||
data ClientCommandTransactions = DummyClientCommandToRun | ||||||||
|
||||||||
data ExampleTransactionCmdError | ||||||||
= TransactionWriteFileError !(FileError ()) | ||||||||
|
||||||||
runClientCommandTransactions | ||||||||
:: () | ||||||||
=> ClientCommandTransactions | ||||||||
-> ExceptT ExampleTransactionCmdError IO () | ||||||||
runClientCommandTransactions DummyClientCommandToRun = | ||||||||
... | ||||||||
left $ | ||||||||
TransactionWriteFileError $ | ||||||||
FileError "dummy.file" () | ||||||||
``` | ||||||||
|
||||||||
Proposed change: | ||||||||
|
||||||||
```haskell | ||||||||
data ClientCommandTransactions = DummyClientCommandToRun | ||||||||
|
||||||||
data ExampleClientCommand = ClientCommandTransactions ClientCommandTransactions | ||||||||
|
||||||||
topLevelRunCommand :: ExampleClientCommand -> RIO () () | ||||||||
topLevelRunCommand (ClientCommandTransactions txsCmd) = | ||||||||
runClientCommandTransactions txsCmd | ||||||||
|
||||||||
runClientCommandTransactions | ||||||||
:: HasCallStack | ||||||||
=> ClientCommandTransactions | ||||||||
-> RIO () () | ||||||||
runClientCommandTransactions DummyClientCommandToRun = | ||||||||
... | ||||||||
throwIO $ | ||||||||
CustomCliException $ | ||||||||
FileError "dummy.file" () | ||||||||
``` | ||||||||
|
||||||||
We have eliminated `data ExampleClientCommandErrors` and `data CmdError` and improved the composability of our code. | ||||||||
|
||||||||
### Pros | ||||||||
- Additional [logging functionality](https://hackage.haskell.org/package/rio-0.1.22.0/docs/RIO.html#g:5) | ||||||||
- Explicit environment dependencies e.g `logError :: HasLogFunc env => Text -> RIO env ()` | ||||||||
Jimbo4350 marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||
- Better composability i.e no more errors that wrap errors (see above). | ||||||||
Jimbo4350 marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the point above already captures this |
||||||||
|
||||||||
### Cons | ||||||||
- `RIO` is hardcoded to IO so we cannot add additional transformer layers e.g `RIO Env (StateT Int IO) a` | ||||||||
- Implicit error flow. Errors are thrown via GHC exceptions in `IO`. See exception handling below. | ||||||||
|
||||||||
## Exception handling | ||||||||
|
||||||||
### Exception type | ||||||||
```haskell | ||||||||
data CustomCliException where | ||||||||
CustomCliException | ||||||||
:: (Show error, Typeable error, Error error, HasCallStack) | ||||||||
=> error -> CustomCliException | ||||||||
|
||||||||
deriving instance Show CustomCliException | ||||||||
|
||||||||
instance Exception CustomCliException where | ||||||||
displayException (CustomCliException e) = | ||||||||
unlines | ||||||||
[ show (prettyError e) | ||||||||
, prettyCallStack callStack | ||||||||
] | ||||||||
|
||||||||
throwCliError :: MonadIO m => CustomCliException -> m a | ||||||||
throwCliError = throwIO | ||||||||
``` | ||||||||
|
||||||||
The purpose of `CustomCliException` is to represent explicitly thrown, structured errors that are meaningful to our application. | ||||||||
|
||||||||
### Exception Handling Mechanism: Pros & Cons | ||||||||
|
||||||||
#### Pros | ||||||||
|
||||||||
1. **Unified Exception Type** | ||||||||
|
||||||||
- Simplifies Top-Level Handling: All errors are caught as `CustomCliException`. | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This ADR does not explain how this type should be used. You can't Is this a temporary solution to bridge between There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I don't understand this objection. Can you clarify?
This is a suggestion of a final design. The purpose of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
You've answered my question in the next paragraph of your comment. 👍🏻
Ok. The thing is, that explanation should be in the ADR itself, so that it's clear for the next person reading it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This does not sound like a suggestion to me, more like a strict rule:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It will be a strict rule when the ADR gets merged. I.e I want the team's buy in. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This is nowhere demonstrated. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||||||||
- Consistent Reporting: Ensures all errors are formatted uniformly via `prettyError`. | ||||||||
|
||||||||
2. **CallStack Inclusion** | ||||||||
|
||||||||
- Embeds CallStack to trace error origins, aiding debugging. | ||||||||
|
||||||||
3. **Polymorphic Error Support** | ||||||||
|
||||||||
- Flexibility: Wraps any error type so long as the required instances exist. | ||||||||
|
||||||||
#### Cons | ||||||||
1. **Type Erasure** | ||||||||
|
||||||||
- Loss of Specificity: Existential quantification erases concrete error types, preventing pattern-matching on specific errors. That may make specific error recovery logic harder to implement. | ||||||||
|
||||||||
# Decision | ||||||||
- Error handling: All errors will be converted to exceptions that will be caught by a single exception handler at the top level. | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Converted how? This is not explained in the examples. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Linking to a whole PR from an ADR is not ideal:
Could you provide a self-contained example what do you mean here? Additionally that PR does not show any errors' conversion. Do you mean using exceptions as a replacement for |
||||||||
- Top level monad: [RIO](https://hackage.haskell.org/package/rio-0.1.22.0/docs/RIO.html#t:RIO). | ||||||||
- We agree not to catch `CustomCliException` except in the top-level handler. If the need for catching an exception arises, we locally use an `Either` or `ExceptT` pattern instead. | ||||||||
- `CustomCliException` should only be thrown within the `RIO` monad. Pure code is still not allowed to throw exceptions. | ||||||||
|
||||||||
# Consequences | ||||||||
- This should dramatically improve our code's composability and remove many unnecessary error types. | ||||||||
- Readability concerning what errors can be thrown will be negatively impacted. However, `ExceptT` already lies about what exceptions can be thrown because it is not limited to the error type stated in `ExceptT`'s type signature. In other words, `IO` can implicitly throw other `Exception`s. | ||||||||
- Initially, this will be adopted under the "compatible" group of commands so `cardano-cli` will have a design split temporarily. Once we are happy with the result we will propagate to the rest of `cardano-cli` |
Uh oh!
There was an error while loading. Please reload this page.