Fixes compilation issues in v3 compatibility mode (-d:chronosHandleException)
#545
Conversation
tests/testmacro.nim
Outdated
| test "Cannot raise invalid exception": | ||
| checkNotCompiles: | ||
| proc test3 {.async: (raises: [IOError]).} = raise newException(ValueError, "hey") | ||
| when not chronosHandleException: |
There was a problem hiding this comment.
why does this test need to be disabled? the global handleException default should not affect raises-annotated functions..
There was a problem hiding this comment.
Oh... apparently it does, as this snippet starts to compile when I enable it; i.e., it seems to relax the raises constraint in raises-annotated procs as well. I thought this was intentional.
There was a problem hiding this comment.
OK, running a simple test with the raises-annotated proc above and looking at the expansions I can see that the global flag does indeed modify the exception handlers (adds handlers for Exception and CatchableError, removes handler for IOError).
There was a problem hiding this comment.
Hm... if I understand the implementation right:
proc decodeParams(params: NimNode): AsyncParams =
...
var
raw = false
raises: NimNode = nil
handleException = chronosHandleException
...
if param[0].eqIdent("raises"):
... # doesn't reset handleException to false
elif param[0].eqIdent("handleException"):
handleException = param[1].eqIdent("true")this will indeed simply start from the global value and then override it if handleException is set, but won't set it to false if there's a raises clause (or handle the fact that it was set globally rather than locally in any special way).
There was a problem hiding this comment.
I'm experimenting with fixing this and seeing how it impacts our code.
There was a problem hiding this comment.
OK here goes my attempt at fixing this. I've added it to this branch as it sort of fits the overall "compilation issues in v3 compatibility mode" theme, and I'd need the previous changes to be able to compile Chronos with the flag on anyways. I'm compiling/running Codex with this and it seems fine.
|
Hm, this looks like it's cycling down the wrong path. Let's revert to what handleExceptions is supposed to do: remap |
| @@ -720,7 +720,7 @@ proc newDatagramTransportCommon(cbproc: UnsafeDatagramCallback, | |||
| proc wrap(transp: DatagramTransport, | |||
| remote: TransportAddress) {.async: (raises: []).} = | |||
| try: | |||
| cbproc(transp, remote) | |||
| await cbproc(transp, remote) | |||
|
hm, or .. maybe you're right, maybe it doesn't make sense for the global flag to apply to raises-annotated functions in which case the documentation needs to be updated as well. This makes more sense in the context of upgrading an existing codebase with mixed annotated and non-annotated code - let's go with your proposal, forget my previous comment. |
|
Yeah, even though my proposal actually comes from my understanding of what I thought was your proposal ( 😄 ), I came to realize that being able to get incremental exception checking as I annotate a legacy codebase doesn't seem bad at all. Gonna take a stab at updating the docs. |
|
OK, added a tentative update to the docs. |
|
@arnetheduck ping 🙂 |
fixes #544
This PR adds the missing
awaitcalls which fixes the issue I was experiencing without visible ill effects. I've also added (somewhat naively) the ability to run the test suite in compat mode, though not sure this is a desirable change.