From 7630f394718ebcdb8577e36faacd78cb7a0b7dd6 Mon Sep 17 00:00:00 2001 From: Giuliano Mega Date: Mon, 10 Jun 2024 05:18:42 -0300 Subject: [PATCH] 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 --- chronos.nimble | 8 +++++++ chronos/internal/asyncmacro.nim | 8 ++++++- chronos/transports/datagram.nim | 2 +- chronos/transports/stream.nim | 2 +- docs/src/error_handling.md | 39 ++++++++++++++++++++++++--------- tests/testmacro.nim | 15 +++++++++++++ 6 files changed, 61 insertions(+), 13 deletions(-) diff --git a/chronos.nimble b/chronos.nimble index 490a086..e8334ce 100644 --- a/chronos.nimble +++ b/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 = @[ diff --git a/chronos/internal/asyncmacro.nim b/chronos/internal/asyncmacro.nim index 4ece9f0..e416e1e 100644 --- a/chronos/internal/asyncmacro.nim +++ b/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.} = diff --git a/chronos/transports/datagram.nim b/chronos/transports/datagram.nim index fdb406b..1423d76 100644 --- a/chronos/transports/datagram.nim +++ b/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 diff --git a/chronos/transports/stream.nim b/chronos/transports/stream.nim index 9992543..391ff0a 100644 --- a/chronos/transports/stream.nim +++ b/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 diff --git a/docs/src/error_handling.md b/docs/src/error_handling.md index 54c1236..2b03dc2 100644 --- a/docs/src/error_handling.md +++ b/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,11 +124,12 @@ 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]).} = @@ -136,14 +137,32 @@ proc raiseException() {.async: (handleException: true, raises: [AsyncExceptionEr 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. diff --git a/tests/testmacro.nim b/tests/testmacro.nim index 335e2ee..ba1f691 100644 --- a/tests/testmacro.nim +++ b/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)