Raise tracking

[Menduist]

EDIT:
This PR adds the possibility to do check exceptions in async procedures. More info from the updated README:

---

By specifying a asyncraises list to an async procedure, you can check which
exceptions can be thrown by it.

proc p1(): Future[void] {.async, asyncraises: [IOError].} =
  assert not (compiles do: raise newException(ValueError, "uh-uh"))
  raise newException(IOError, "works") # Or any child of IOError

Under the hood, the return type of p1 will be rewritten to another type,
which will convey raises informations to await.

proc p2(): Future[void] {.async, asyncraises: [IOError].} =
  await p1() # Works, because await knows that p1
             # can only raise IOError

---

[arnetheduck]

This is indeed one of the options we've discussed in the past, to annotate future with an error type akin to Result, and make it customizable.

However, it would indeed be nice to reuse the raises instead, and this should be doable eventually - the way forward here should be that we move any given raises annotations to the generated iterator such that the compiler enforces them - what remains is to introspect code snippets with the effectsOf pragma from future nim versions, to transport type information about caught exceptions to the continuation site.

There are smaller incremental steps we can take however: one is an {.atask.} replacement for {.async.} where the only difference is that it acts as a full exception barrier, enforcing the use of try/catch - its primary use case would be asyncSpawn. A strengthening of strict exceptions, this would change the raises of the generated iterator to raises: [Defect] - this is already a requirement that we enforce at runtime.

Finally, there's already a few places experimenting with Future[Result... - one pragmatic way forward thus becomes to combine atask with special Result support in the library to ensure that no exceptions leak when using Result.

[zah]

This is one of the two possible approaches to solve this problem. Besides changing the Future type, it's also possible to introduce an additional wrapper type that will appear in place of the T in the Future instantiation. This has several potential benefits:

• Less disruption, existing code continues to compile as it is
• Ability to introduce gradually in selected modules first
• The wrapper type can enforce the error handling at the call site if desired

I've written about the design and implemented the key parts of it here:

https://github.com/status-im/nim-stew/blob/b3a6b52c7764da0b351e748b1080127677c734eb/docs/error_handling.md
https://github.com/status-im/nim-stew/pull/26/files

Having that said, changing the Future type is quite reasonable as well.

[arnetheduck]

it's also possible to introduce an additional wrapper type that will appear in place

this is good for experiments experiments and proof-of-concepts, but I think ultimately we will want to land in a "native" solution for nim-chronos where we don't pay the EH and wrapper overhead (cognitive and implementation-wise) in general, except where those features are used - right now, the flow with exceptions across await barriers is quite expensive because we need to catch exceptions and repropagate them even in code that doesn't raise - better would be that code that doesn't use EH doesn't have to pay the cost either, in any way (not even in the Future object - for that to work, we'd need a more fine-grained Future implementation - one that also maybe disconnects writers from readers akin to C++) - basically, if code can be deduced not to raise, it doesn't need to catch, then store, then reraise, and this should be part of the final implementation.

Having that said, changing the Future type is quite reasonable as well.

does it even work to do Future[T, E = CatchableError] with a default E? I wonder how much stuff breaks then.

[Menduist]

does it even work to do Future[T, E = CatchableError] with a default E?

Couldn't make it work. However, I think we could do

type
  EFuture[T, E] =... 
  Future[T] = EFuture[T, (CatchableError)]

And then, the async pragma could automatically transform the return Future into a EFuture using the raises as E

However, it would indeed be nice to reuse the raises instead

Issue is, iirc, the push pragma is applied after async transformation, so we don't know the raises coming from push yet.

transport type information about caught exceptions to the continuation site.

To my current knowledge, the only way to do this is how I did it here
We need to know, at compile time, which exceptions can be thrown by a Future given to await. Meaning, from the future type, we must be able to get the exception list

[arnetheduck]

To my current knowledge, the only way to do this is how I did it here

that's what effectsOf does, in future nim versions - though I anticipate that it will work poorly, given that it has to fully resolve and deduce the effects of the called code, something that nim tends to do quite poorly.

[arnetheduck]

so we don't know the raises coming from push yet.

a starting point might be to get explicit {.async, raises ...} to work

[zah]

"native" solution

This is a false dilemma in general. You can implement exactly the same specializations of the Future type and the surrounding code using the additional wrapper type.

[arnetheduck]

You can

I'm saying that we must, compared to the proposal in nim-stew, if we're going to actually land it in something like nim-chronos that's used for real.. it's not so much a dilemma as a requirement so as to turn the error handling proposal into a pragmatic and real solution

[zah]

that's what effectsOf does, in future nim versions?

effectsOf has limited utility to the problem at hand here. It can be only useful if await is defined as macro that looks at the call expression being awaited. In other words, await foo() would discover that foo() can raise certain exceptions and it would modify the call-site accordingly. The usefulness is lost when you allow the result of foo() to be assigned to a future variable before being awaited and thus it cannot deliver a complete solution. On the other hand, a "compete" solution based on modifying the Future type as it's done here doesn't need to use effectsOf in any way because the same information is already encoded in the result type.

[zah]

I'm saying that we must, compared to the proposal in nim-stew, if we're going to actually land it in something like nim-chronos that's used for real.. it's not so much a dilemma as a requirement so as to turn the error handling proposal into a pragmatic and real solution

I don't understand any bit of this sentence. What I said is that you can define the error-information-carrying wrapper type at a layer that is above Chronos (or within Chronos) and you can still implement features such as "A Future for an async op that can't raise an exception doesn't have an error field". This renders the argument claiming that there are certain "optimisation" benefits of the Future[T, E] approach invalid and I just wanted to clarify that in the interest of having a fair discussion.

[Menduist]

I did a V2:

  Future*[T] = ref object of FutureBase ## Typed future.
    [..]
    
  FuturEx*[T, E] = Future[T]

This allows it to be backward compatible, you can use a Future directly but loose raises info, or use FuturEx to have it

The async macro will read the raises pragma and put info in the return type:

proc test1(): Future[void] {.async, raises: [ValueError].} =
  raise newException(ValueError, "Value")

Will become

proc test1(): FuturEx[void, (CancelledError, ValueError)] {.stackTrace: off,
    raises: [], gcsafe.} =
  iterator test1_436207619(chronosInternalRetFuture: Future[void]): FutureBase {.
      closure, gcsafe, raises: [CancelledError, ValueError].} =
    block:
      raise newException(ValueError, "Value")
    complete(chronosInternalRetFuture)

  var resultFuture = newFuture[void]("test1")
  resultFuture.closure = test1_436207619
  futureContinue(resultFuture)
  return resultFuture

If no raises are found, CatchableError is added for back. compatibility

Works:

proc test1(): Future[void] {.async, raises: [ValueError].} =
  raise newException(ValueError, "Value")

proc test2() {.async, raises: [].} =
  await sleepAsync(100.milliseconds)
  let toto = test1()
  try:
    await toto
    echo "Mhh?"
  except ValueError as exc:
    echo "Works"

(and also shows why the errors must be stored in the future type, and effectsOf may not help us if we keep our current syntax)

Doesn't compile:

proc test1(): Future[void] {.async, raises: [ValueError].} =
  raise newException(ValueError, "Value")

proc test2() {.async, raises: [].} =
  await test1()

Doesn't compile:

proc test1(): Future[void] {.async, raises: [ValueError].} =
  raise newException(ValueError, "Value")

proc test2() {.async, raises: [ValueError].} =
  let fut: Future[void] = test1() # We lost raise info here
  await fut # can raise an unlisted exception: CatchableError

[Menduist]

one is an {.atask.} replacement for {.async.} where the only difference is that it acts as a full exception barrier

You would still need to inform the caller that you can't raise, otherwise eg

proc test1() {.atask.} =
  discard

proc test2() {.atask.} =
  await test1() # Can't compile, can raise unlisted exceptions

So the usefulness would be very limited (only useful for asyncSpawn)

[Menduist]

The branches I had to create to make this work with nimbus:
vacp2p/nim-libp2p@7eb3225
status-im/nim-websock@b31ff99
[same issues in json_rpc and nim-presto, but didn't make a branch for theses one yet]

With theses branches, everything compiles correctly

As you can see, the major issue is people (including me) putting wrong raises in async
We could use a different pragma in async context, to avoid this issue and keep backward compatibility, but not sure it make total sense

There is also a weird issue with libp2p on nim devel, need to investigate
(libp2p/stream/connection.nim(91, 1) Error: Defect can raise an unlisted exception: Defect ?)
(EDIT: it's actually because we push raises: [Defect] everywhere in libp2p :/ )

wdyt?

[dryajov]

Overall this looks really promising! I'd love to see this moving forward and it would be invaluable given the current status-quo!

[arnetheduck]

Thinking about the Futurex type, one of the goals I had for the future type family was that it should carry no exceptions for code that doesn't raise - raises has the weakness that it forces dynamic type on the user which prevents several forms of static analysis, and code that doesn't raise shouldn't have to pay this price.

There are two ways to go forward here, I think - either a base type or allowing E to be void - this is also one of the reasons Result is more fundamentally more powerful as a vehicle for transporting error information: it allows both static and dynamic typing to be used, with the former allowing more powerful static analysis down the line - in a base layer library / type structure like Future, it's an important flexibility to have.

We have to anticipate that whatever type we add here will end up being used in user code as well - although the majority of code uses the macros, there are special cases to consider where the transformation ends up generating poor code and manually written Future code simply is better.

[arnetheduck]

As you can see, the major issue is people (including me) putting wrong raises in async

putting raises on async in the current version is wrong indeed - however, push raises should not interfere - at least it hasn't interfered prior to this branch.

[Menduist]

Bumped this to be on top of #449

Also, chronos doesn't compile anymore with nim 1.2 (even before this PR), so updated the nimble file

[Menduist]

Upstream regression: nim-lang/Nim#22765
This prevent us from doing:

  test "Can create callbacks":
    proc test1 {.async, asyncraises: [ValueError].} = raise newException(ValueError, "hey")
    let callback: proc {.async, asyncraises: [ValueError].} = test1

on nim 2.0

Workaround is:

  test "Can create callbacks":
    proc test1 {.async, asyncraises: [ValueError].} = raise newException(ValueError, "hey")
    let callback: proc: Future[void] {.async, asyncraises: [ValueError].} = test1

[arnetheduck]

This is now looking ok:ish - since it was easy, we also no longer generate try/finally at all for asyncraises: [] eliminating a significant amount of bloat :)

[arnetheduck]

This version of the PR is sufficient for enabling experimentation with the feature, as the changes are additive (should not significantly change code that doesn't use it) - merging so as to simplify further work on the feature while testing it in real-life code