Error handling (Exception vs Result)

[dryajov]

After implementing #32 and experimenting a little bit with Result and questionable, I wanted to leave my current impressions and gather some feedback around whats the best way forward.

Our current situation is pretty dire, inadvertently we've ended up with two half working error handling mechanisms - exceptions which don't support raises with async code and result which is also partially broken with async, specially ? doesn't work in async transformations. Above all, we've ended up with an inconsistent implementation which is what worries me the most at this point - consistency is key when it comes to error handling and it's the one things we aren't right now.

The debate around Exception vs Result tends to focus on safety, performance and ergonomics. I want to give my perspective on each of this points, but bare in mind that this are nuanced topics and it's easy to be subjective when treating them, so don't take this as an attempt to "convert" anyone, in fact I'm not dead set on anything at all, this is just my current perspective and I really do want to understand what's the best way of writing software in our specific case.

With that said, it's probably no secret that so far I have been unconvinced by the supposed advantages of Result over Exception. Practically, exceptions are less secure because of historical reasons - Nim bugs, lack of integration with async, etc... However, they have been or are in the process of being resolved.

Assuming that exceptions work as promised and we can eventually use raises consistently across sync and async code, I don't see any advantage of Result over Exception. Semantically and by extension safety wise, they are about the same!

The difference is then in the syntax (i.e. ergonomics) and performance.

Performance wise, exceptions might be a tad more expensive, but the interesting thing is that the performance penalty comes from the way exceptions have traditionally been implemented, but it doesn't have to be like that. In fact, it's possible to implement exceptions on top of Results and this is what some languages like Haskel and Swift have done.

At this point, I treat the performance overhead as an implementation detail and not something inherent to the exceptions mechanism proper. It's also worth asking wether the supposed overhead is of any significance to the type of software we're writing? I'm inclined to say that overwhelmingly it isn't.

Let me now move on to my major beef with Result - ergonomics. IMO, Result sacrifices ergonomics for all the wrong reasons. In part this is because of how Result is historically used, i.e. this isn't inherent to Result, in part it is.

One issues which I and others bring up is chaining (a().b().c()) - Result complicates this. Surprisingly (or unsurprisingly), this isn't inherent to Result itself, rather the fact that traditionally it's used with simple types (enums, bools, strings, etc...), the lack of a polymorphic type hierarchy forces the constant use of mapErr and similar to map across the different errors. If Result is used with a proper error hierarchy, most of this issues go away, similar to how questionable does it by relying on the existent CatchableError hierarchy.

Some would argue that this is wrong, but is it? What's better, a hierarchy of polymorphic types that can be propagated up the stack with ? without any invocation of mapErr or endlessly mapping enums to string and vice-versa? To me the former is safer, because there isn't any loss of information or context.

This brings me to ?, which is essentially Result's way of saying "I don't care about the error". The counterargument I've heard here is that "you can't simply return the error with ? without handling it", but IMO this is far from true, ?/mapErr combo is the usual escape hatch, albeit with a lot more ceremony and loss of information in the process.

What happens if ? is gone and you're left with the bare Result api? Well, in 99% cases you'll end up emulating ? one way or another, just because writing an if/else every other line is nuts!

So back to my initial argument of Result and checked Exceptions being equivalent - I strongly believe that Result brings little to the table (if at all) at the expense of worst overall UX and safety.

Let me illustrate:

Exception:

proc findPeers(self: Store, id: Cid, timeout: uint64): Future[seq[PeerInfo]] {.async, raises: [AsyncTimeout].} =
  ## Discover peers holding id, if peer(s) is already in routing table, return imediatelly
  ##
  ## If no peers where discovered, returns empty seq
  ## If no peer responds within `timeout`, raise a `Timeout` exception
  ##
  ...

proc download(self: Node, id: Cid): Future[BufferStream] {.async, raises: [CodexError].} =
  ## Attempt to download content identified by `id`
  ##

  var retry = 5
  if id notin self.store:
    while true:
      try:
        (await self
        .findPeers(id))
        .mapIt(
          self.switch.connect(it)
        )
      catch AsyncTimeout as exc:
        trace "Couldn't find peers, retrying", retry
        if retry > 0:
          dec(retry)
          continue
        raise (ref CodexError)(msg: "Failed to find peers for Cid!")

  let stream = BufferStream.new()
  proc getAsync() {.async.} =
    try:
      let block = await self.store.getBlock(cid)
      await stream.pushData(block.data)
    except CatchableError as exc:
      trace "Unable to get block", exc = exc.msg

  asyncSpawn getAsync()
  return stream

Result:

proc findPeers(self: Store, id: Cid, timeout: uint64): Future[Result[seq[PeerInfo], DiscoveryError]] {.async.} =
  ## Discover peers holding ContentId, if peer(s) is already in routing table, return imediatelly
  ##
  ## If no peers where discovered, returns empty seq
  ## If no peer responds within `timeout`, returns DiscoveryError.Timeout
  ##
  ...

proc download(self: Node, id: Cid): Future[Result[BufferStream, cstring]] {.async.} =
  ## Attempt to download content identified by `id`
  ##

  var retry = 5
  if id notin self.store:
    while true:
      var res: Result[seq[PeerInfo], DiscoveryError]
      if(
        res = (await self.findPeers(id)); 
        res.isErr):
        trace "Couldn't find peers, retrying", retry
        if res.error == DiscoveryError.Timeout and retry > 0:
          dec(retry)
          continue

        return err("Failed to find peers for Cid!")
      
      res.get().mapIt( 
        node.switch.connect(it)
      )

  let stream = BufferStream.new()
  proc getAsync() {.async.} =
    try:
      let block = await self.store.getBlock(cid)
      await stream.pushData(block.data)
    except CatchableError as exc:
      trace "Unable to get block", exc = exc.msg

  asyncSpawn getAsync()
  return stream

Note that I haven't compiled this and admittedly this example is a bit contrived and doesn't attempt to be correct as far as the actual implementation goes, but it is close to whatever we end up with.

In a few words on what the code is attempting to do:

• findPeers should discover some peers or return the ones we already know of from our routing table based on the passed id. If we couldn't find/connect to any within some timeout, raise or return an error (we could also pass a minimum number of peers we want, but it's out of scope for this example).

• download will attempt to call findPeers repeatedly until retry == 0 or it succeeds finding peers within the retry limit, it will then attempt to request the block from the store, which should dispatch to the network accordingly, etc...

From the two examples above, the number of lines is actually pretty much the same, what strikes me however is the awkward interleaving of error handling and the execution flow in the results example. IMO, being able to readily understand the intent of the entire sequence is more valuable to the reader than attempting to decipher what type of errors are being handled in the process. The exceptions based example is IMO much easier to follow at a glance than the result one.

Does this make Exception fundamentally better than Result, not necessarily, as I mentioned above, most of the issues come from mapErr and that can be addressed by an error hierarchy, but I do think that overall, even when used more like exceptions, it's still inferior UX wise.

So whats my verdict so far? If we get proper support for raises in Chronos with status-im/nim-chronos#251, then I don't see any immediate advantage of using Result, even if we manage to properly address ? in async transformations.

[arnetheduck]

Semantically and by extension safety wise, they are about the same!

not quite - there are two fundamental differences:

• the use of Result is enforced at the call site, and techniques exist to reduce the syntactic impact of the checks (there's a learning curve here to learn these techniques, however) - for exceptions, it's the other way around: nothing is enforced at the call site, and techniques exist to enforce it for those that know them - this is the key reason why overall, we've had fewer problems on launch with code that doesn't use Exceptions - it's not the bugs in the implementation of exceptions that has led to the poor quality: it's the bugs in the code that uses exceptions and lacks error handling: we know this from experience of fixing bugs in libp2p in particular - another way to put this is the following: with exceptions, you get buggy code, and you can add some "ceremony" as you call it to deal with it, the end result being "not enough error handling" - with Result, you get non-buggy code by default, and you have to explicitly introduce the bugs by ignoring errors, the end result being "a little bit too much error handling" - when you're making a web page with a reload button, the former model is fine- it's more difficult to motivate when you're building critical infrastructure.
• Exceptions are dynamically typed - Result allows both static and dynamic typing - this is an obvious advantage because in the places where static typing can be used, it limits the scope of analysis to a tractable problem - this is the foundational difference between statically and dynamically typed languages overall, and it applies equally to error handling as to any other code.

I'll also note that annotating all code in dagger with raises [DaggerException] has zero effect on the code-base except adding a lot of noise: the raises annotations together with the dynamically typed nature of exceptions cancel out any safety advantages you get from static analysis, because now, all exceptions bubble up freely through the layers again: if that's the plan, you might as well not uses raises at all - it'll indeed only be ceremony at that point.

[dryajov]

the use of Result is enforced at the call site, and techniques exist to reduce the syntactic impact of the checks

OK, if they do, then I'd love to see them illustrated, so far from what I've seen in the wild is a combination of ?/mapErr and worst, things returning Result[..., string/cstring].

I get the point about making you handle the error at the call site, this is the first thing that result advocates point to, unfortunately, this is not how it's used in the wild and the lack of ergonomics make it's use burdensome to the point where the above mentioned escape hatch becomes the default, giving you the worst of both worlds - loss of information and cluttered flow.

This is a pretty classic example of good defaults but horrible UX, is even worst than bad defaults and good UX - at least with the latter as long as you stick to the correct default, you get to benefit of the UX, no such luck with the former.

Exceptions are dynamically typed

This is true, but like I said is this really that bad? This becomes a problem only when you stick to the worst possible baseline, annotate everything with the base exception type, if you stick to only the absolutely necessary, you minimize this issue, this is both easy to spot and in other languages can be enforced by tooling such as linters, you simply forbid propagating CatchableError or whatever your base exception is. In Nim world, this is unfortunately a fantasy, but it's still a valid argument, for now we just have to be careful to never propagate the base type, push raises: [Defect] already gets us half way there either way.

I'll also note that annotating all code in dagger with raises [DaggerException]

You do this only at the edge of your API, usually whoever is consuming it cares little about what the actual internal error was, but in the rare cases it does, you can definitely list the specific types in the raises - take it as your Result[..., cstring] equivalent.

[emizzle]

Firstly, thanks so much for putting this in a discussion, as it will allow others to weigh in that have not been privy to our internal discussions.

I'm not dead set on anything at all, this is just my current perspective and I really do want to understand what's the best way of writing software in our specific case

I couldn't agree with this more! I, too, am not married to exceptions nor results, and am keen to get the best possible solution implemented.

I spent quite a lot of time migrating exceptions in an existing nim-status codebase using {.push raises:[Defect].} first, then later moving the entire codebase to Result, so my thoughts here come from that experience, which is to say that that particular experience with that particular application by no means translates directly here and by no means makes me right in any way shape or form. I'm simply hoping to pass on my thoughts and hope we find the best solution.

The first thing to address is that the discussion above centers around the idea that the only difference is syntax and performance:

The difference is then in the syntax (i.e. ergonomics) and performance.

When we moved to Results across the entire codebase in nim-status, Results coerced the codebase to consider the architectural layers of the application by way of defining Error types: each layer would have its own Error type. The reason behind this is that the returned errors needed to be considered from the viewpoint of the consumer. For example if we encounter an IO error, eg file not found, we don't necessarily want to bubble that up to the consuming layer as it doesn't quite make sense in the context of the consumer. A consumer should expect an error that is relevant to the layer it called. For example, if we have a database layer, which exposes an api, eg getData which opens the DB then runs a query. Let's say a consumer calls that and the DB file was deleted somehow so there is a file not found IO error. Does it makes sense to the consumer that they receive a file not found IO error when calling getData? Instead, wouldn't it be better that they get an error at the database layer, eg DatabaseError.DbFileMissing? Of course this can be handled using polymorphic exception hierarchy, however using exceptions does not force us to consider this, while using Results across the board does. We could be very careful to do this with exceptions, but because consistency is key, it is likely better to lean on the compiler to aid us and force as such. It's quite difficult to articulate this concept as a whole, but once you try to implement Results in a real world codebase, and try with more than just a simple example, it becomes easier to see.

This brings me to ?, which is essentially Result's way of saying "I don't care about the error".

Respectfully disagreeing, once we start handling errors using results, however, we've already done quite a lot of reasoning that the consumer likely won't need to inspect the error stack to reason any further. As above, sometimes the underlying error doesn't make sense when bubbled up, and instead all we need to do as a consumer is reason about the error we've received from the layer below. Additionally, when mapping the error (mapErr, which can be called before calling ?), we can choose to throw away the error or not, by way of mapErr's predicate parameter. However this becomes complicated as the error type could be static or dynamic. One option here is to convert any error encountered to an exception and add it to the returned error. @michaelsbradleyjr and I are working on an example to show how this can be done, stay tuned.

Readability

As a reader, when trying to reason where exception originated in a chunk of code is nearly impossible without inspecting each and every call inside the block. Take, for example, a snippet from proc store* in dagger/node.nim:

try:
    while (
      let chunk = await chunker.getBytes();
      chunk.len > 0):

      trace "Got data from stream", len = chunk.len
      without blk =? bt.Block.init(chunk):
        return failure("Unable to init block from chunk!")

      blockManifest.put(blk.cid)
      if not (await node.blockStore.putBlock(blk)):
        # trace "Unable to store block", cid = blk.cid
        return failure("Unable to store block " & $blk.cid)

  except CancelledError as exc:
    raise exc
  except CatchableError as exc:
    return failure(exc.msg)
  finally:
    await stream.close()

How can we know where CancelledError originated? Also, how do we know where CatchableError originated, but much more importantly, how do we know that this isn't masking other exceptions that inherit from CatchableError? If this is indeed catching derived exceptions, the compiler won't be able to alert us that there are possible exceptions that haven't been caught.

Downside of Result

As mentioned, there is an ergonomic annoyance with Result that can get in the way. For example, catching simple exceptions like ValueError when using strformat.fmt can be annoying:

{.push raises: [Defect].}
proc getPath(newIdx: int): string =
  ?(catch fmt"{PATH_WALLET_ROOT}/{newIdx}").mapErrTo(PathFormatError)

However, ValueError could simply be appended to the raises pragma on getPath instead, eg:

{.push raises: [Defect].}
proc getPath(newIdx: int): string {.raises: [ValueError].} =
  fmt"{PATH_WALLET_ROOT}/{newIdx}")

But that pushes the problem up the call chain. Even if it's pushed up the call chain, we now at least know explicitly that we have a proc (or procs if pushed up the chain) that could potentially raise ValueError.

[emizzle]

At this point it sounds like this is a preference thing - if you are used to more procedural type of flow and rely little on chaining then a few if's in the middle of the flow makes little difference, but if you want more of a functional flow that is uncluttered by exceptional conditions and clearly conveys the intent of the snippet being examined rather than how it can fail, the result really becomes an annoyance.

Again, I'm not married either way. But for the sake of argument, while I prefer Result, it's not because it is more or less ergonomic or functionally/procedurally friendly. It is, as @arnetheduck the pointed out, more prudent to bring error handling as a forethought rather than an afterthought, and using Results forces the writer to do this. While it does "distract" the writer from achieving his/her main goal of the happy path (and the reader from understanding that happy path), it is just as important to consider the unhappy path, and the more this is done upfront, the less painful debugging hard to reach errors will be later on. This transcends preference, in my opinion, as it has a direct impact on the shape of the codebase from a separation of concerns point of view.

[emizzle]

@michaelsbradleyjr and I are working on an example to show how this can be done, stay tuned.

Here is an example of how to retain error information. This is not picking on existing code in dagger at all, rather I used a couple of existing files so the examples are less contrived. Notice the very last test shows the full structure of the retained data:

# ---- common module ----------------------------------------------------------------#
type

  ErrorInfo = object of RootObj
    info: string

  CodexError[T] = object of RootObj
    kind: T
    parent: ErrorInfo

# ---- dagger/chunker.nim -------------------------------------------------------------#
type
  Chunker = ref object
    chunkSize*: int64

  ChunkerErrorType = enum
    InvalidChunkSize = "couldn't fill buffer because chunks size is invalid"

  ChunkerError = object of CodexError[ChunkerErrorType]

  ChunkerResult[T] = Result[T, ChunkerError]

const
  DefaultChunkSize*: int64 = 1024 * 256

func new*(T: type Chunker, chunkSize = DefaultChunkSize): T =
  var chunker = Chunker(chunkSize: chunkSize)
  return chunker

proc getBytes*(c: Chunker): Future[ChunkerResult[seq[byte]]] {.async.} =
  var buff = newSeq[byte](c.chunkSize) # abbrev for illustration

  # we do stuff to fill the buffer...

  # and then encounter an error
  if c.chunkSize == 0:
    # this isn't going to happen, but needed a way to make it fail in the tests
    return err ChunkerError(kind: InvalidChunkSize)

  return ok buff


# ---- dagger/node.nim -------------------------------------------------------------#
type
  NodeRef* = ref object

  NodeErrorType = enum
    StoreInvalidCid   = "couldn't create cid from chunk data"
    StoreInvalidChunk = "couldn't get chunk bytes due to invalid chunk size"

  NodeError = object of CodexError[NodeErrorType]

  NodeResult[T] = Result[T, NodeError]

proc store*(node: NodeRef, chunkSize: int64): Future[NodeResult[Cid]] {.async.} =

  let
    chunker = Chunker.new(chunkSize) # abbreviated for illustration
    chunk = ? chunker
                .getBytes()
                .mapErrTo(NodeError(kind: StoreInvalidChunk))

  let cid = ? Cid.init(
                CIDv0,
                multiCodec("dag-pb"),
                MultiHash.digest("sha2-256", chunk).get())
              .mapErrTo(NodeError(kind: StoreInvalidCid))

  return ok cid

proc new*(
  T: type NodeRef): T =
  T()


# ---- Tests -----------------------------------------------------------------------#
let
  node =  NodeRef.new()

let res = waitFor node.store(0) # set an invalid chunk size and expect an error
assert res.isErr
assert res.error is CodexError[NodeErrorType]
assert res.error.parent != ErrorInfo()

assert res.error == NodeError(
                      kind: NodeErrorType.StoreInvalidChunk,
                      parent: ErrorInfo(info: $ChunkerErrorType.InvalidChunkSize))

This is made possible by a mapErrTo proc:

proc mapErrTo*[T, E1, E2](r: Future[Result[T, E1]], v: E2):
  Future[Result[T, E2]] {.async.} =

  mixin toErrorInfo
  var ve = v

  return (await r).mapErr(
    proc (e: E1): E2 =
      when e is E2:
        return ve
      elif compiles(e.toErrorInfo):
        ve.parent = e.toErrorInfo
      elif e is enum:
        ve.parent.info = $e
      else:
        ve.parent.info = $e.kind
      ve
  )

What is interesting here is that we could create as many toErrorInfo mixins as we want so that we could potentially store particular typed error data as needed in the returned error, eg:

type
  CidErrorInfo = object of ErrorInfo
    cidSpecificProp: string

func toErrorInfo(v: CidError): CidErrorInfo =
  result.cidSpecificProp = v.someProp # doesn't exist, for illustration only

[dryajov]

more prudent to bring error handling as a forethought rather than an afterthought, and using Results forces the writer to do this

This is a bit of a contrived argument - just because you interleave error handling with the flow of your algorithm, it doesn't automatically become a forethought, on the contrary, the code becomes more convoluted, harder to read and overall more fragile. Prime example of this is the presence of ? and similar sigils, simply because it would be nuts to have an if/else check on every other line!

This transcends preference, in my opinion, as it has a direct impact on the shape of the codebase from a separation of concerns point of view.

There is no separation of concerns whatsoever if you are forced to handle every single error condition where it originates, thus as I've already pointed ? is your usual escape hatch, at which point it's little different from a well supported (TM) raises: [...] annotation, i.e. checked exceptions.

Here is an example of how to retain error information.

Before diving deep, let me just reiterate how little difference there is in constructs like this from allowing the exception to escape (provided we're sticking to the premiss of using stricter exception handling than the defaults - raises: [Exc1, Exc2, ...]).

    chunk = ? chunker
                .getBytes()
                .mapErrTo(NodeError(kind: StoreInvalidChunk))

This is exactly the same as letting an exception bubble up. Now, there isn't anything wrong with this as there isn't anything wrong with allowing known exceptions to escape the current context so that errors can be handled/communicated appropriately across layers.

Regarding the example. I'm not entirely sure how it actually fixes the issue of context/information loss?

For example:

type

  ErrorInfo = object of RootObj
    info: string

  CodexError[T] = object of RootObj
    kind: T
    parent: ErrorInfo

What is it attempting to fix? It doesn't seem to preserve the previous error beyond stringifying it into the parent field - how is this preserving the type and context of the previous error?

Perhaps the intent was something like:

  CodexError[T] = object of RootObj
    kind: T
    info: string
    parent: CodexError[...]

But that wouldn't compile because the compiler cannot reason about the type that will end up stored in the parent field at instantiation.

I'm also unsure of what the mapErrorTo that you cannot already do with mapErr? For example, you could define a new mapeErr() for each concrete error you're attempting to map and as far as I can see you'd have the same effect? What am I missing?

---

As a counter example, this is how your example would look with exceptions:

{.push raises: [Defect].}

import chronos
import libp2p

# ---- common module ----------------------------------------------------------------#
type
  CodexError = object of CatchableError

# ---- dagger/chunker.nim -------------------------------------------------------------#
type
  Chunker = ref object
    chunkSize*: int64

  ChunkerErrorType = object of CodexError
  InvalidChunkSize = object of ChunkerErrorType
    
proc new(T: type InvalidChunkSize): ref InvalidChunkSize =
  (ref InvalidChunkSize)(msg: "couldn't fill buffer because chunks size is invalid")

const
  DefaultChunkSize*: int64 = 1024 * 256

func new*(T: type Chunker, chunkSize = DefaultChunkSize): T =
  var chunker = Chunker(chunkSize: chunkSize)
  return chunker

proc getBytes*(c: Chunker): Future[seq[byte]] {.async, raises: [Defect, ChunkerErrorType].} =
  var buff = newSeq[byte](c.chunkSize) # abbrev for illustration

  # we do stuff to fill the buffer...

  # and then encounter an error
  if c.chunkSize == 0:
    # this isn't going to happen, but needed a way to make it fail in the tests
    raise InvalidChunkSize.new()

  return buff

# ---- dagger/node.nim -------------------------------------------------------------#
type
  NodeRef* = ref object

  NodeErrorType     = object of CodexError
  StoreInvalidCid   = object of NodeErrorType # "couldn't create cid from chunk data"
  StoreInvalidChunk = object of NodeErrorType # "couldn't get chunk bytes due to invalid chunk size"

proc new(T: type StoreInvalidCid, parent: ref Exception = nil): ref StoreInvalidCid =
  (ref StoreInvalidCid)(
    msg: "couldn't create cid from chunk data", 
    parent: parent)

proc new(T: type StoreInvalidChunk, parent: ref Exception = nil): ref StoreInvalidChunk =
  (ref StoreInvalidChunk)(
    msg: "couldn't get chunk bytes due to invalid chunk size",
    parent: parent)

proc store*(node: NodeRef, chunkSize: int64): Future[Cid] {.async, raises: [Defect, NodeErrorType].} =

  let
    chunker = Chunker.new(chunkSize) # abbreviated for illustration
    chunk = try:
      await chunker.getBytes() 
    except InvalidChunkSize as exc: 
      raise StoreInvalidCid.new(exc)
    # chunk = await chunker.getBytes() # With properly working `raises` in async, this would be flagged by the compiler...

  let 
    cid = try:
      Cid.init(
        CIDv0,
        multiCodec("dag-pb"),
        MultiHash.digest("sha2-256", chunk).get())
        .tryGet()
    except CatchableError as exc:
      raise StoreInvalidChunk.new(exc)

  return cid

proc new*(
  T: type NodeRef): T =
  T()

# ---- Tests -----------------------------------------------------------------------#
let
  node =  NodeRef.new()

try:
  discard waitFor node.store(0) # set an invalid chunk size and expect an error
except StoreInvalidCid as exc:
  assert exc of ref StoreInvalidCid
  assert exc.parent of ref InvalidChunkSize

The above compiles and runs as expected, with the caveat that raises simply don't work in async code :(, so even if you try to uncomment the chunk = await chunker.getBytes() line, it wouldn't get flagged by the compiler - for example, store shouldn't allow InvalidChunkSize to escape...

But the point is to show how little difference there is in practical terms with respect to Result once everything is set and done.

That said, I don't think the example illustrates the syntactic issues with Result, it does however, show how the type hierarchy used in exceptions helps preserve context and information about mapped/parent errors.

This is something that can also be done with results, the argument I constantly hear against it is that you can now freely bubble errors up the stack without "handling" them; my counterargument is that, blindly invoking ?/mapErr at each layer and mapping an enum to a string and vice-versa isn't actually handling anything. In other words, if you want to preserve information across layers, you need some type hierarchy.

Phew :-) !!!

[emizzle]

What is it attempting to fix? It doesn't seem to preserve the previous error beyond stringifying it into the parent field - how is this preserving the type and context of the previous error?

I agree that storing the error type information as it was given is not possible in this context, which is not great. We could, however, provide an inherited ErrorInfo if needed using the mixins and store that on the parent property. We could also translate the error to be stored (exception or error type) in to an exception (even using a toException mixin) and storing that object, which borrows the error-storing abilities of exception system. Or, we could even serialise the error along with its type information in the error and deserialise if needed at the call site. In most cases, with errors handled at each layer, almost all callers won't need to inspect the error stack as each error has already been handled, so there's not that much more reasoning that will be needed. This was an example to show that we could store the error information, albeit not statically typed as it arose, if needed.

This is exactly the same as letting an exception bubble up. Now, there isn't anything wrong with this as there isn't anything wrong with allowing known exceptions to escape the current context so that errors can be handled/communicated appropriately across layers.

I believe this highlights one of the main sticking points when deciding between Result and Exceptions: should some exceptions be allowed to bubble? Results doesn't really allow it (without mapping of course), and that is why it forces the writer to consider all exceptions up front. Exceptions allows it, and allows the writer to let the less important exceptions bubble without having to handle it upfront. With this in mind, to my point about this being more than just syntax differences, when Results handles all exceptions, the calling layer gets context-specific errors that have been mapped, or "translated" from even lower layers in to a layer-specific error that the caller can reason about. And I do agree, that this can become cumbersome to handle every single error, and of course requires us to lean on ? significantly. It is indeed a hard choice!

blindly invoking ?/mapErr at each layer and mapping an enum to a string and vice-versa isn't actually handling anything

I also agree, it's not being handled except to make Results happy, but the error that is returned in the Result is "translated" in such a way that the caller knows what the error was in the context of the layer below. There could be a FileNotFound error that bubbles its way up 5 layers to an api that deals with inserting a Cid in to a DHT (contrived example). The context was lost along the way and now the caller will have to inspect the exception stack to figure out where it originated. If Results forced it to be handled at every layer, then the call site will get an error that is specific that layer and will understand, ok, there was a problem with my cipher password on the database (or something like that). But yes, i totally get it, handling at every layer is a complete PITA 😂

I'm also unsure of what the mapErrorTo that you cannot already do with mapErr? For example, you could define a new mapErr() for each concrete error you're attempting to map and as far as I can see you'd have the same effect? What am I missing?

mapErrTo is just a generic helper of mapErr that helps to store error information if its possible. This was kind a "default" for all types that could be modified per type, using toErrorInfo mixins. But yes, there could also be error-specific mapErr's that do the same thing, but that just felt like a lot of boilerplate.

All-in-all, i'm really glad to have had the discussion, i've definitely learned a lot about the effects system. It sounds like once we get raises working inside of async procs, and we are careful to craft exception type hierarchies per module, I think we'll be a good shape to use exceptions if that is the preference going forward.

[arnetheduck]

By the way, these any many other points are covered in https://status-im.github.io/nim-style-guide/errors.html -

Keep in mind that deep exception hierarchies are complex beasts - just like the compiler, you as a developer will have a hard time reasoning about them for the same reasons that a "bubbling-up" exception is hard to reason about: it comes from a different abstraction level and has not benefited from the shift in detail and focus that a translation brings: it has not been thought about at the abstraction level that it originates from.

One thing that keeps coming up in these discussions is how important it is to have full information from the very bottom of the call stack in order to understand where something went wrong: if you think carefully about the reason why, you'll quickly see that the only time it happens is when you haven't thought about error handling up front, and now uncontrolled exceptions are bubbling up the stack and you need desperate measures to be able to debug - if this is the scenario you're optimising your error handling strategy for, you're baking in the expectation that it's a frequently occuring phenomenon that deserves expensive measures such as dynamic typing, call stacks and so on - your development style becomes one of "let's write the happy path and see what bubbles up".

With Result, it is in practice very rarely the case that you actually need a call stack - the only time you need one is when there is a logic fault, and these are raised already in the form of Defect - ie the EH mechanism is great for panics, where your regular thinking about the both the happy and unhappy path has failed.

?/mapErr at each layer

This is "handling" it, very strongly so: you're translating the error to the proper level of abstraction and/or doing something about it - that's one way of handling errors - "handling" is a concept that involves agency and a deliberate choice: you choose an intervention based on what happened in the layer below, and sometimes, that intervention is a decision to report it to the user of your api, at the right level of abstraction.

Like with any code examples, of course it is possible to construct one that "looks bad" with either exceptions or Result:s - it's a skill like any other to be able to efficiently factor code in a way that sympathises with the error delivery mechanism and therefore is a pleasure to read - this has very little to do with whether you use exceptions or not, as good-looking examples exist for both styles.

In such a scenario, it's useful to look at the technical side of things, and there, there's no denying that Result is more powerful and flexible: in addition to being able to express everything that an exception can (via dynamic types / putting an Exception in E), you can also use the help that the compiler can give you when it comes to compile-time type analysis via static types - this is huge when it comes to critical infrastructure-level code.

One outcome, or balance, that can help decide which one to choose is to look at where in the component tree the code lies:

• is it part of a critical core library that sits at the heart of the application? Pick Result, because it is more flexible and can easily be translated to an exception (via toException/tryGet), and the users of your library will thank you for having been forced to think about errors as part of your abstract API, without dynamic types and open type hierarchies.
• is it part of the glue part of the applications where error handling is less common or matters less, and you're merely putting components together? Roll with exceptions and save yourself the bother of having to think about errors up-front - the obvious ones will quickly emerge in UX testing.
• Is it a glue library? Provide excellent support for both, being mindful not to lose information (such as type information) along the way

[michaelsbradleyjr]

consistency is key when it comes to error handling

Agreed. I think we can live with Exception or Result, as long as we're consistent. The sooner we make a well-informed and well-reasoned decision, the better, in my opinion (seems we're very close). And then we just need to stick to it.

---

That being said, I tend to prefer Result:

• With a strong top-level {.raises.} policy and Result encapsulation of CatchableError when interfacing with modules we don't control, the only exceptions will be ones that crash the process, i.e. we don't have to spend much time or effort thinking about exceptional control flow¹.
• Successes and errors are all values and working with them involves normal control flow.
• Moving beyond string and enum errors will require some thought and care, but in a good way (I think): my gut instinct is that we'd end up with code that's easier to reason about and refactor.
• Verbosity and tedium can be addressed with judicious use of macros/templates and helper functions that tidy up common flows.

Those are my personal impressions: feel free to disagree strongly.

As Eric mentioned earlier, he and I put our heads together yesterday and came up with some ideas. Below I'll show what I have in mind. Note that autoErr doesn't exist yet, I'm still working it out.

First, let's wrap around stew/results' ? to make it async (nim-chronos) friendly, per what I posted in discord chat yesterday (with small tweaks):

import chronos except `?`
import stew/results except `?`

template `?`*[T, E](self: Future[Result[T, E]]): auto =
  # h/t @emizzle 🎉
  when not declared(chronosInternalRetFuture):
    {.fatal: "can only be used within the body of a chronos {.async.} proc!".}
  let v = (self)
  var rv = await v
  if rv.isErr:
    when typeof(result) is typeof(rv):
      chronosInternalRetFuture.complete(rv)
    else:
      when E is void:
        chronosInternalRetFuture.complete(err(typeof(rv)))
      else:
        chronosInternalRetFuture.complete(err(typeof(rv), rv.error))
    return chronosInternalRetFuture
  when not(T is void):
    rv.get

template `?`*[T, E](self: Result[T, E]): auto =
  let v = (self)
  when declared(chronosInternalRetFuture):
    let av = proc(): Future[Result[T, E]] {.async.} = return v
    ? av()
  else:
    results.`?` v

Those templates could be provided by e.g. (hypothetical) questionable/async/results.

I've been experimenting with them: so far they seem to work fine. If I've missed something important, i.e. they won't work robustly, I'm all ears.

Some error kinds:

type
  DiscoveryErrors {.pure.} = enum
    Default = "Discovery error"
    NoPeersFound = "No peers found"
    Other = "Some other discovery error"
    Timeout = "Discovery timed out"
    
  DownloadErrors {.pure.} = enum
    Default = "Download error"
    DiscoveryFailed = "Peer discovery failed"
    Other = "Some other download error"

Some error types:

type
  PolyError[T] = object of CatchableError
    kind*: T
    
  DiscoveryError = PolyError[DiscoveryErrors]
  
  DownloadError = PolyError[DownloadErrors]

Here's the original Result example refactored to use async friendly ?, etc.

proc findPeers(self: Store, id: Cid, timeout: uint64): Future[Result[seq[PeerInfo], DiscoveryError]] {.async.} =
  ...

proc findPeersRetry(self: Store, id: Cid, retries: Natural = 0): Future[Result[seq[PeerInfo], DiscoveryError]] {.async.} =
  var remaining = retries
  while true:
    let res = await self.findPeers(id)
    if res.isOk or res.error.kind != Timeout:
      return res
    elif remaining > 0:
      dec remaining
      trace "Couldn't find peers, retrying", retry=(retries - remaining)
      continue
    else:
      break

  return autoErr(NoPeersFound)

proc download(self: Node, id: Cid): Future[Result[BufferStream, DownloadError]] {.async.} =
  if id notin self.store:
    let peers = ? findPeersRetry(5).autoErr(DiscoveryFailed)
    peers.mapIt(node.switch.connect(it))

  let stream = BufferStream.new()
  ...
  return stream

This is just to get the idea. How many and what enums and error types are warranted (or whether to use a different approach) is where the careful thought process comes in.

The idea with autoErr is to infer the error type from the enclosing proc and thereby cut down on the verbosity of mapErr, while you could always use mapErr instead if what you want to do doesn't fit the autoErr pattern.

My example above hints at autoErr:

• Working in concert with PolyError to set the error kind; default kind could be determined with enum .low, so you can just call autoErr() while working things out.
• Deriving a default msg from the kind; a custom msg could be an optional second arg to autoErr.
• Possibly setting the parent, e.g. above a DiscoveryError could populate the parent field of DownloadError if discovery failed.

That is to say, I'm optimistic re: the above... I think it will work out... I'm still experimenting.

Regardless of whether my idea for autoErr works out, I think async friendly ? plus some helpers (in addition to those already provided by stew/results and questionable) can allow for ergonomic use of Result, if we settle on Result instead of Exception.

Footnotes

1. Granted there will likely be corner cases, and keeping in mind compiler bugs, library difficulties, etc. ↩

[arnetheduck]

One of the next steps steps for Result / ? themselves are waiting somewhere around here: status-im/nim-stew#34 - with this in hand, one can declare canonical mappings between error types - ie that IoError becomes a DatabaseError based on an automatic conversion, taking the thing mapErr out of the equation.

For now, there's an orErr defined for Result which does what mapErr would have done, but without doing mapErr every time:

? readFile().orErr(DatabaseError.X)

[Menduist]

Ok, here are my 2 cents:

I think, as discussed here and elsewhere, exceptions and results are very similar, more than most would admit. After all, chronos is wrapping exception in result, showing that they are almost interchangeable.

So, I want to focus on the actual differences at usage, if we forget for a second the current async issues, performance, and what could be modded easily (for instance as demonstrated by @emizzle and @michaelsbradleyjr)

(goal here is not to say which is better, or which I like best, but to try to make an honest and complete comparison to be able to make a decision)

Operation chaining

Eg:

var something
try:
  something = canFail1().canFail2()
  if condition: something &= canFail3()
except SomeError: something = cannotFail() 
except OtherError: ditto

Results: you have to lift everything and have compatible error types.
Exception: can be done easily

Compatible error re-raising

Eg:

proc r: Result[void, AnError] =
  ? procFailingWithAnError() 
proc e {.raises: [AnError].} =
  procRaisingAnError() #hidden re-raise

Exceptions allows you to be implicit, results forces you to be explicit (with ?). However, if error are not compatible, both forces you to remap to a compatible type explicitly.

Possible error types

Results allows you to use 1 error type, while raises allow as many as you want. Note that is easily circumvented in result by creating an error type which maps to multiple possible errors

Error type

Exception: has to inherit from CatchableError
Results: can be anything you dream of

Multiple errors

Eg

proc a(i: seq[input]): seq[Result[output, AnError]]

Result: can be done fairly easily (you loose some syntactic sugar)
Exceptions: not elegant at all

Syntax (keywords actually)

(? vs try, mapError vs except, etc)
That's a bit up to taste honestly, and easily remapable to something we prefer if wanted, not sure it's worth including here.

Conclusion

First of all, am I missing something?
If not, hopefully this table will help us to make an informed decision:

	Result	Exception
Operation chaining	Possible by lifting every operation	Easy
Compatible error re-raising	Has to be explicit	Implicit
Possible error types	1	N
Error type	Any	Inherits from CatchableError
Seq of errors	Yes	Not pretty

[stefantalpalaru]

it would be nuts to have an if/else check on every other line

Seems to work for Go, which quickly became the most solid language for writing networking software, despite this increase in verbosity: https://go.dev/blog/error-handling-and-go

So do we care more about pretty function call chaining, or robustness?

I'd go further and discourage the use of "?" because it's unreadable when parsing code quickly and it can get dangerous by hiding an early function exit: https://gist.github.com/stefantalpalaru/d96430de60305ce54604d9cf0e601ec5

Verbosity helps comprehension, here.

The "Result" API also got overly large and complex, increasing the cognitive load. Do we really need more than ok(), err(), get() and isErr()? That's all I used in https://github.com/status-im/nim-eth/blob/master/eth/net/nat.nim and https://github.com/status-im/nim-nat-traversal

Yes, it was annoying to handle all those possible errors, but the software is better for it.

[Menduist]

Too much verbosity can make code unreadable
If half the lines are boilerplate error handling, I will loose my mind before the end of the function

Go's strength is goroutines, clearly not error handling

And to be clear, I'm not saying that you should just ignore errors. But if you're most common operation with an error is to bubble it up, it shouldn't take 4 lines every time

[stefantalpalaru]

If half the lines are boilerplate error handling, I will loose my mind before the end of the function

When it's needed, it's needed, and it's better if it's also visible.

most common operation with an error is to bubble it up

That's usually just a way to kick the can down the road and, for that, it's even easier if it's all done under the hood. The end result will rarely be robust software, though.