ADR-10: Add proposal ADR for addressing lack of stack-traces in cardano-api

[palas]

Technical proposal for modifying the error handling system in cardano-api. The goal is to ensure that every error has an associated stack trace and to provide a way to display all layers of errors in a readable format.

TLDR:

Here's a breakdown of the key points:

1. Error types: The proposal splits the existing Error class in two: ErrorContent and Error. ErrorContent has the same functionality as the old Error class, and the new Error class includes an ErrorContent and a stack trace.
2. ErrorWithStack: It suggests a new data type ErrorWithStack for convenience, that wraps ErrorContent and automatically introduces a stack trace, with a smart constructor called mkError.
3. Modifying existing code: The document explains a potential approach to apply the changes to existing code
4. Displaying errors: The proposal suggests using a recursive approach to display all layers of errors in a readable format.

Benefits:

• Every error has an associated stack trace
• All layers of errors can be displayed in a readable format
• Minimal changes are required to existing code

Cons:

• The approach requires a significant refactoring effort
• The approach introduces complexity to the error types.

[Jimbo4350]

What about the following:

data WithCallStack e = WithCallStack e CallStack

class Error e where
  prettyError :: WithCallStack e -> Doc ann

We modify the existing Error class and wrap our error e with WithCallStack. Each error has a callstack included with the original pretty printing functionality. No need to introduce additional type classes and/or methods to capture the notion of a callstack.

[palas]

I think it is a good point that we don't need the classes, we could just use WithCallStack always. But:

1. Having each error print their own stacktrace is unnecessary, because it is done the same way for every error. So, I think it would be equivalent to renaming prettyError to prettyErrorWithoutCallStack and creating a method outside of the class that has the signature prettyError :: Error e => WithCallStack e -> Doc ann that uses prettyErrorWithoutCallStack and appends the stacktrace.
2. There is still the issue of dealing with the nested errors. We could either, unwrap and re-wrap every time we nest (and that would keep the innermost call-stack), or we could keep the cause field in WithCallStack. But we have to make sure that the field in cause is an error WithCallStack and that is were the GADT comes in.

[Jimbo4350]

Having each error print their own stacktrace is unnecessary,

You're not forced to render the CallStack if you don't want to in a given Error instance.

There is still the issue of dealing with the nested errors

What about data WithCallStack e = WithCallStack e (Last CallStack)? We only care about the innermost callstack right? We can recurse and mappend on Last CallStack.

[palas]

You're not forced to render the CallStack if you don't want to in a given Error instance.

We could still decide to not print it by calling prettyErrorWithoutCallStack instead of prettyError (which is probably the time where we want to make the choice anyway). Right?
But also:

• We would save a bunch of boilerplate each time we write an Error.
• We would have much more consistency in the format in which we print errors.

What about data WithCallStack e = WithCallStack e (Last CallStack)?

So, we could have WitCallStack e1 c1 <> WitCallStack e2 c2 = WitCallStack (e1 $ e2) (c1 <> c2).

Nice, I think it may work.

We only care about the innermost callstack right? We can recurse and mappend on Last CallStack.

That is definitely the most important one. It is not always where the issue is, so it can make sense to see the stacktrace of the other errors too. Also, if the stack of the innermost error was long enough, then we wouldn't need anything else, because it includes callers, (but it probably isn't long enough all the time, especially because in Haskell functions are recursive). On the other hand, having several stacktraces clutters the error output a lot, so it may be a good compromise to just have the innermost.

[carbolymer]

Nicely written. I'm not 100% sold on it yet. I was thinking about this alternative (comparing to your example in "Adapting existing code" section):

class Error e where
  prettyError :: e -> Doc ann
  -- | this still enforces mandatory call stack
  getErrorCallStack :: e -> CallStack
  getCause :: e -> Maybe Cause

and

data ProtocolParametersConversionError
  = HasCallStack => PpceOutOfBounds !ProtocolParameterName !Rational
  | HasCallStack => PpceVersionInvalid !ProtocolParameterVersion
  | HasCallStack => PpceInvalidCostModel !CostModel !CostModelApplyError
  | HasCallStack => PpceMissingParameter !ProtocolParameterName
  | HasCallStack => PpceSyntheticChainedPparamError !ProtocolParametersConversionError

deriving instance Eq ProtocolParametersConversionError
deriving instance Show ProtocolParametersConversionError
deriving instance Data ProtocolParametersConversionError

instance Error ProtocolParametersConversionError where
  prettyError = \case
    PpceOutOfBounds name r ->
      "Value for '" <> pretty name <> "' is outside of bounds: " <> pretty (fromRational r :: Double)
    PpceVersionInvalid majorProtVer ->
      "Major protocol version is invalid: " <> pretty majorProtVer
    PpceInvalidCostModel cm err ->
      "Invalid cost model: " <> pretty @Text (display err) <> " Cost model: " <> pshow cm
    PpceMissingParameter name ->
      "Missing parameter: " <> pretty name
    PpceSyntheticChainedPparamError cause ->
      "Chained error: " <> pretty cause

   getErrorCallStack = \case
     PpceOutOfBounds _ _ -> callStack
     PpceVersionInvalid _ -> callStack
     PpceInvalidCostModel _ _ -> callStack
     PpceMissingParameter _ -> callStack
     PpceSyntheticChainedPparamError _ -> callStack
    
   getCause = \case
     PpceSyntheticChainedPparamError cause -> Just $ Cause cause
     _ -> Nothing

But I think you've solved this with less boilerplate in the end. The downside of your solution I see is that it's more convoluted with multiple wrappers so it may be a little confusing when implementing new error types.

I like how you solved the problem of chained errors.

On a side note, in GHC 9.12 there's ExceptionContext which solves similar problem of nesting multiple exceptions and adding multiple backtraces.

[carbolymer]

I don't anticipate a big performance hit from this change because:

• I don't see the point of adding HasCallStacks to (de)serialisation implementations
• expensive computations are done in ledger code most of the time
• most of the time of cardano-api code execution is spent in ledger code or in I/O (networking)
• we should take care to not add HasCallstack in tight loops