Fixes compilation issues in v3 compatibility mode (-d:chronosHandleException) (#545)

* add missing calls to await

* add test run in v3 compatibility

* fix semantics for chronosHandleException so it does not override local raises/handleException annotations

* distinguish between explicit override and default setting; fix test

* re-enable wrongly disabled check

* make implementation simpler/clearer

* update docs

* reflow long line

* word swap

Author: gmega

chronos.nimble

@@ -60,6 +60,14 @@ task test, "Run all tests":
       run args & " --mm:refc", "tests/testall"
     run args, "tests/testall"
 
+task test_v3_compat, "Run all tests in v3 compatibility mode":
+  for args in testArguments:
+    if (NimMajor, NimMinor) > (1, 6):
+      # First run tests with `refc` memory manager.
+      run args & " --mm:refc -d:chronosHandleException", "tests/testall"
+
+    run args & " -d:chronosHandleException", "tests/testall"
+
 task test_libbacktrace, "test with libbacktrace":
   if platform != "x86":
     let allArgs = @[

chronos/internal/asyncmacro.nim

@@ -219,12 +219,14 @@ proc decodeParams(params: NimNode): AsyncParams =
   var
     raw = false
     raises: NimNode = nil
-    handleException = chronosHandleException
+    handleException = false
+    hasLocalAnnotations = false
 
   for param in params:
     param.expectKind(nnkExprColonExpr)
 
     if param[0].eqIdent("raises"):
+      hasLocalAnnotations = true
       param[1].expectKind(nnkBracket)
       if param[1].len == 0:
         raises = makeNoRaises()
@@ -236,10 +238,14 @@ proc decodeParams(params: NimNode): AsyncParams =
       # boolVal doesn't work in untyped macros it seems..
       raw = param[1].eqIdent("true")
     elif param[0].eqIdent("handleException"):
+      hasLocalAnnotations = true
       handleException = param[1].eqIdent("true")
     else:
       warning("Unrecognised async parameter: " & repr(param[0]), param)
 
+  if not hasLocalAnnotations:
+    handleException = chronosHandleException
+
   (raw, raises, handleException)
 
 proc isEmpty(n: NimNode): bool {.compileTime.} =

chronos/transports/datagram.nim

@@ -720,7 +720,7 @@ proc newDatagramTransportCommon(cbproc: UnsafeDatagramCallback,
   proc wrap(transp: DatagramTransport,
             remote: TransportAddress) {.async: (raises: []).} =
     try:
-      cbproc(transp, remote)
+      await cbproc(transp, remote)
     except CatchableError as exc:
       raiseAssert "Unexpected exception from stream server cbproc: " & exc.msg
 

chronos/transports/stream.nim

@@ -2197,7 +2197,7 @@ proc createStreamServer*(host: TransportAddress,
   proc wrap(server: StreamServer,
             client: StreamTransport) {.async: (raises: []).} =
     try:
-      cbproc(server, client)
+      await cbproc(server, client)
     except CatchableError as exc:
       raiseAssert "Unexpected exception from stream server cbproc: " & exc.msg
 

docs/src/error_handling.md

@@ -110,7 +110,7 @@ sometimes lead to compile errors around forward declarations, methods and
 closures as Nim conservatively asssumes that any `Exception` might be raised
 from those.
 
-Make sure to excplicitly annotate these with `{.raises.}`:
+Make sure to explicitly annotate these with `{.raises.}`:
 
 ```nim
 # Forward declarations need to explicitly include a raises list:
@@ -124,26 +124,45 @@ proc myfunction() =
 
 let closure: MyClosure = myfunction
 ```
+## Compatibility modes
 
-For compatibility, `async` functions can be instructed to handle `Exception` as
-well, specifying `handleException: true`. `Exception` that is not a `Defect` and
-not a `CatchableError` will then be caught and remapped to
-`AsyncExceptionError`:
+**Individual functions.** For compatibility, `async` functions can be instructed
+to handle `Exception` as well, specifying `handleException: true`. Any
+`Exception` that is not a `Defect` and not a `CatchableError` will then be
+caught and remapped to `AsyncExceptionError`:
 
 ```nim
 proc raiseException() {.async: (handleException: true, raises: [AsyncExceptionError]).} =
   raise (ref Exception)(msg: "Raising Exception is UB")
 
 proc callRaiseException() {.async: (raises: []).} =
   try:
-    raiseException()
+    await raiseException()
   except AsyncExceptionError as exc:
     # The original Exception is available from the `parent` field
     echo exc.parent.msg
 ```
 
-This mode can be enabled globally with `-d:chronosHandleException` as a help
-when porting code to `chronos` but should generally be avoided as global
-configuration settings may interfere with libraries that use `chronos` leading
-to unexpected behavior.
+**Global flag.**  This mode can be enabled globally with
+`-d:chronosHandleException` as a help when porting code to `chronos`. The
+behavior in this case will be that:
 
+1. old-style functions annotated with plain `async` will behave as if they had
+   been annotated with `async: (handleException: true)`.
+
+   This is functionally equivalent to
+   `async: (handleException: true, raises: [CatchableError])` and will, as
+   before, remap any `Exception` that is not `Defect` into
+   `AsyncExceptionError`, while also allowing any `CatchableError` (including
+   `AsyncExceptionError`) to get through without compilation errors.
+
+2. New-style functions with `async: (raises: [...])` annotations or their own
+   `handleException` annotations will not be affected.
+
+The rationale here is to allow one to incrementally introduce exception
+annotations and get compiler feedback while not requiring that every bit of
+legacy code is updated at once.
+
+This should be used sparingly and with care, however, as global configuration
+settings may interfere with libraries that use `chronos` leading to unexpected
+behavior.

tests/testmacro.nim

@@ -8,6 +8,7 @@
 import std/[macros, strutils]
 import unittest2
 import ../chronos
+import ../chronos/config
 
 {.used.}
 
@@ -586,6 +587,20 @@ suite "Exceptions tracking":
 
     waitFor(callCatchAll())
 
+  test "Global handleException does not override local annotations":
+    when chronosHandleException:
+      proc unnanotated() {.async.} = raise (ref CatchableError)()
+
+      checkNotCompiles:
+        proc annotated() {.async: (raises: [ValueError]).} = 
+          raise (ref CatchableError)()
+
+      checkNotCompiles:
+        proc noHandleException() {.async: (handleException: false).} =
+          raise (ref Exception)()
+    else:
+      skip()
+
   test "Results compatibility":
     proc returnOk(): Future[Result[int, string]] {.async: (raises: []).} =
       ok(42)