ADR 8 - RIO monad in cardano cli

[Jimbo4350]

Re-added ADR 8 to easily access discussions regarding the ADR

[carbolymer]

Needs more details how do we go about it

[carbolymer]

This ADR does not explain how this type should be used. You can't fromException other exceptions into CustomCliExceptions, so it will be useful only when explicitly thrown.

Is this a temporary solution to bridge between ExceptT errors and exceptions, or a final design?

[Jimbo4350]

You can't fromException other exceptions into CustomCliExceptions

I don't understand this objection. Can you clarify?

Is this a temporary solution to bridge between ExceptT errors and exceptions, or a final design?

This is a suggestion of a final design.

The purpose of CustomCliException is to represent explicitly thrown, structured errors that are meaningful to our application. For other exceptions (e.g., runtime errors or unexpected failures), we rely on the Nothing case in our handler.

[carbolymer]

I don't understand this objection. Can you clarify?

You've answered my question in the next paragraph of your comment. 👍🏻

This is a suggestion of a final design.

The purpose of CustomCliException is to represent explicitly thrown, structured errors that are meaningful to our application. For other exceptions (e.g., runtime errors or unexpected failures), we rely on the Nothing case in our handler.

Ok. The thing is, that explanation should be in the ADR itself, so that it's clear for the next person reading it.

[carbolymer]

This is a suggestion of a final design.

This does not sound like a suggestion to me, more like a strict rule:

All errors are caught as CustomCliException.

[Jimbo4350]

It will be a strict rule when the ADR gets merged. I.e I want the team's buy in.

[carbolymer]

All errors are caught as CustomCliException.

This is nowhere demonstrated.

[Jimbo4350]

[carbolymer]

#60 (comment)

[carbolymer]

1. ADR 7 says we should log to stderr
2. I'm not quite seeing the point of differentiation between two cases: CustomCliException and the other one. You're doing displayException in either case.

[Jimbo4350]

1. We can log to stderr.

You're doing displayException in either case.

Look at the instance Exception CustomCliException. It requires an Error e instance for pretty rendering and includes a call stack for debugging.

I'm not quite seeing the point of differentiation between two cases

This ensures we capture all exceptions. We handle CustomCliException explicitly for structured, user-facing errors, while the Nothing case acts as a safety net for unexpected exceptions.

[carbolymer]

This ensures we capture all exceptions.

Catching SomeException would do the same here.

We handle CustomCliException explicitly for structured, user-facing errors, while the Nothing case acts as a safety net for unexpected exceptions.

Again, this separation is not shown anywhere in the code and is not explained in the text. A next person reading this ADR will have no clue what is the role of CustomCliException.

[Jimbo4350]

Catching SomeException would do the same here.

Are you saying to explicitly pattern match on this? I don't understand your point here.

[carbolymer]

Are you saying to explicitly pattern match on this? I don't understand your point here.

Yes, SomeException is a catch-all for all exceptions (asynchronous as well).

[Jimbo4350]

How is this different that the Nothing case? It's going to handle all other exceptions via displayException.

[carbolymer]

Exactly, it is not - so what's specific about CustomCliException to pattern match it in the handler?

[Jimbo4350]

#60 (comment)

[carbolymer]

Suggested change

	- Loss of Specificity: Existential quantification erases concrete error types, preventing pattern-matching on specific errors.
	- 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.

[carbolymer]

Converted how? This is not explained in the examples.

[Jimbo4350]

https://github.com/input-output-hk/cardano-node-wiki/pull/60/files#diff-c83af4b681074eae4a9a579fb6a393539599918fd6e7c9e069d8f2422590b48aR100

[carbolymer]

Linking to a whole PR from an ADR is not ideal:

1. That PR is too big to illustrate how it should be used. Now a reader has to go through that PR, sift through non-relevant parts, read it, and figure out what's the idea here.
2. A PR until it's merged is a living thing, can still change.
3. There's a potential bitrot possibility in case yet another github organisation migration happens, github.com dissapearance etc.

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 Except e m a here?

[carbolymer]

1. The purpose is still not explained. Why do we need CustomCliException, and not just use fromEitherIO on our ExceptT err m a functions? You'd need to just provide instance Exception err and that should be enough I think.

2. Why the handler needs to pattern match exceptions? They're rendered and logged in the same way.

[Jimbo4350]

1. We need a callstack and the Error error constraint enforces a pretty rendering for the exception.
2. Fair point it is also of the type SomeException.

[carbolymer]

1. We need a callstack and the Error error constraint enforces a pretty rendering for the exception.

I see. But that will only give you a call stack up to the place where the CustomCliException was used to wrap our current error type.

As an alternative I was thinking about this solution:

1. Adding HasCallStack to each of our error constructors
2. Add hand-written Exception instance to render callstack

-- | Our standard error clas
class Error e where
  prettyError :: e -> Text

-- | our typical CLI error type
data CmdError
  = HasCallStack => CmdOtherError Text

-- | Needs standalone deriving instance
deriving instance Show CmdError

-- | already existing Error instance
instance Error CmdError where
  prettyError (CmdOtherError e) = "CmdError: " <> e

-- | Needs manually written Exception instance to print callstack
instance Exception CmdError where
  displayException ce@(CmdOtherError _) = 
    T.unpack (prettyError ce) <> "\n" <> prettyCallStack callStack

The benefit is that you can write

  -- topLevelRunCommand :: ExampleClientCommand 
  --                    -> ExceptT ExampleClientCommandErrors IO ()
  fromEitherIO . runExceptT $ 
    topLevelRunCommand cliCmd
...
  throwIO $
    FileError "dummy.file" ()

instead of

  -- topLevelRunCommand :: ExampleClientCommand 
  --                    -> ExceptT ExampleClientCommandErrors IO ()
  fromEitherIO . runExceptT . firstExceptT CustomCliException $ 
    topLevelRunCommand cliCmd
...
  throwIO $
    CustomCliException $
      FileError "dummy.file" ()

which is a bit simpler and still conveys the same error information.

The downside is that it requires a lot more changes through the codebase. Eventually we can get to that point, but as an interim solution your CustomCliException is fine here. 👍🏻

[Jimbo4350]

which is a bit simpler and still conveys the same error information.

I'm happy to go with this. The idea of the CustomCliException type was to remove the possibility of forgetting to implement an Error instance or including a HasCallStack constraint. However we are big boys and what you proposed works for me.

[palas]

Looks good overall. Just a few suggestions for improving clarity and typos

[palas]

I would hide the env parameter of the RIO now that it is easy, unless we are sure it is going to remain (), and even if we are. Maybe with a type synonym. Something like: type CIO = RIO (). That way if we eventually want to add an environment we won't have to change every type signature

[Jimbo4350]

Something like type EmptyEnvironment = ()? We will eventually use it in logging.

[carbolymer]

I wouldn't bother with hiding it. I think we can just write function types for any environment e.g. RIO e a.

I'm wondering if maybe we could put stuff like LocalNodeConnectInfo for online commands, or environment variables into environment. So an online command using environment variables would have type like for example:

doSomeCliCommand 
  :: Has LocalNodeConnectInfo e
  => Has EnvCli e
  => HasCallStack
  => Foo -> Bar -> RIO e ()

class Has a e where
  obtain :: e -> a

But that's out of scope for this ADR.

Besides that, maybe it be useful to have a type synonym providing HasCallStack everywhere? e.g.

-- CIO, like Cardano IO?
type CIO e a = HasCallStack => RIO e a

and use it instead of RIO? It's a slight inconvenience to remember to put it everywhere.

[Jimbo4350]

I'm wondering if maybe we could put stuff like LocalNodeConnectInfo for online commands, or environment variables into environment.

100%. That is the point of the Has classes 👍 .

besides that, maybe it be useful to have a type synonym providing HasCallStack everywhere?

Good suggestion, yes we should do this.

[palas]

Ah, so we have traits. That works too

[palas]

Just from the point of the document structure. I notice there are essentially three pros and cons sections. One in Proposed Solution, one in Exception Handling Mechanism:, and one disguised as Consequences. Would it make sense to unify them?

[carbolymer]

Nice work, LGTM. Can you let me know what do you think of:

1. #60 (comment)
2. #60 (comment)

[smelc]

Suggested change

	- Better composability i.e no more errors that wrap errors (see above).
	- Better composability i.e no more errors that wrap errors (see above).
	- Less bookeping required: no more wrapping low-level errors into high-level errors

[Jimbo4350]

I think the point above already captures this