From c4b6fb3fd2fbd5349290a57c1a2d257955827064 Mon Sep 17 00:00:00 2001
From: zerbina <100542850+zerbina@users.noreply.github.com>
Date: Tue, 12 Mar 2024 19:43:01 +0000
Subject: [PATCH 01/14] specify that hooks must not raise exceptions

The beginning of a language specification test category dedicated to
hooks is added. At the moment, only the exception-effect-related
behaviour is covered.
---
 tests/lang/s02_core/s99_hooks/README.md       | 21 +++++++++++++
 .../s99_hooks/s99_escaping_defects/README.md  |  3 ++
 .../s99_escaping_defects/t01_copy_hook.nim    | 17 +++++++++++
 .../s99_escaping_defects/t02_sink_hook.nim    | 17 +++++++++++
 .../s99_escaping_defects/t03_destroy_hook.nim | 17 +++++++++++
 .../s99_escaping_defects/t04_trace_hook.nim   | 17 +++++++++++
 .../t05_deepcopy_hook.nim                     | 16 ++++++++++
 .../s02_core/s99_hooks/t99_cannot_raise.nim   | 30 +++++++++++++++++++
 8 files changed, 138 insertions(+)
 create mode 100644 tests/lang/s02_core/s99_hooks/README.md
 create mode 100644 tests/lang/s02_core/s99_hooks/s99_escaping_defects/README.md
 create mode 100644 tests/lang/s02_core/s99_hooks/s99_escaping_defects/t01_copy_hook.nim
 create mode 100644 tests/lang/s02_core/s99_hooks/s99_escaping_defects/t02_sink_hook.nim
 create mode 100644 tests/lang/s02_core/s99_hooks/s99_escaping_defects/t03_destroy_hook.nim
 create mode 100644 tests/lang/s02_core/s99_hooks/s99_escaping_defects/t04_trace_hook.nim
 create mode 100644 tests/lang/s02_core/s99_hooks/s99_escaping_defects/t05_deepcopy_hook.nim
 create mode 100644 tests/lang/s02_core/s99_hooks/t99_cannot_raise.nim

diff --git a/tests/lang/s02_core/s99_hooks/README.md b/tests/lang/s02_core/s99_hooks/README.md
new file mode 100644
index 00000000000..786bff439da
--- /dev/null
+++ b/tests/lang/s02_core/s99_hooks/README.md
@@ -0,0 +1,21 @@
+## What belongs here
+
+This section contains tests related to hook procedures, that is, procedures:
+- to which calls are statically inserted by the compiler
+- that are invoked by the runtime at run-time
+
+This should cover:
+- syntax
+- restrictions on the routine definitions
+- restrictions on the run-time behaviour (if any)
+- where the hooks are injected
+
+## Assumptions
+
+- nothing beyond a single module/file
+- assertions may still be built-ins
+- user-defined types are supported and work
+- procedures and calls thereof work
+- `var T` is supported as a parameter's type
+- raising and catching exceptions work
+- tag effect tracking works
\ No newline at end of file
diff --git a/tests/lang/s02_core/s99_hooks/s99_escaping_defects/README.md b/tests/lang/s02_core/s99_hooks/s99_escaping_defects/README.md
new file mode 100644
index 00000000000..1538b39ff42
--- /dev/null
+++ b/tests/lang/s02_core/s99_hooks/s99_escaping_defects/README.md
@@ -0,0 +1,3 @@
+If a Defect is raised from a hook routine at run-time, the program immediately
+terminates (i.e., panics) and an unhandled exception is reported. This is the
+case for both automatic and explicit calls of the hooks.
\ No newline at end of file
diff --git a/tests/lang/s02_core/s99_hooks/s99_escaping_defects/t01_copy_hook.nim b/tests/lang/s02_core/s99_hooks/s99_escaping_defects/t01_copy_hook.nim
new file mode 100644
index 00000000000..968edc82d00
--- /dev/null
+++ b/tests/lang/s02_core/s99_hooks/s99_escaping_defects/t01_copy_hook.nim
@@ -0,0 +1,17 @@
+discard """
+  description: "`=copy` hooks panic when a defect escapes"
+  outputsub: "Error: unhandled exception:  [Defect]"
+  exitcode: 1
+"""
+
+type Type = object
+
+proc `=copy`(a: var Type, b: Type) =
+  raise (ref Defect)()
+
+try:
+  var x: Type
+  `=copy`(x, Type())
+finally:
+  # finally sections are not reached and no cleanup is performed
+  doAssert false
diff --git a/tests/lang/s02_core/s99_hooks/s99_escaping_defects/t02_sink_hook.nim b/tests/lang/s02_core/s99_hooks/s99_escaping_defects/t02_sink_hook.nim
new file mode 100644
index 00000000000..6fecdf046d5
--- /dev/null
+++ b/tests/lang/s02_core/s99_hooks/s99_escaping_defects/t02_sink_hook.nim
@@ -0,0 +1,17 @@
+discard """
+  description: "`=sink` hooks panic when a defect escapes"
+  outputsub: "Error: unhandled exception:  [Defect]"
+  exitcode: 1
+"""
+
+type Type = object
+
+proc `=sink`(a: var Type, b: Type) =
+  raise (ref Defect)()
+
+try:
+  var x: Type
+  `=sink`(x, Type())
+finally:
+  # finally sections are not reached and no cleanup is performed
+  doAssert false
diff --git a/tests/lang/s02_core/s99_hooks/s99_escaping_defects/t03_destroy_hook.nim b/tests/lang/s02_core/s99_hooks/s99_escaping_defects/t03_destroy_hook.nim
new file mode 100644
index 00000000000..e924242560a
--- /dev/null
+++ b/tests/lang/s02_core/s99_hooks/s99_escaping_defects/t03_destroy_hook.nim
@@ -0,0 +1,17 @@
+discard """
+  description: "`=destroy` hooks panic when a defect escapes"
+  outputsub: "Error: unhandled exception:  [Defect]"
+  exitcode: 1
+"""
+
+type Type = object
+
+proc `=destroy`(a: var Type) =
+  raise (ref Defect)()
+
+try:
+  var x: Type
+  `=destroy`(x)
+finally:
+  # finally sections are not reached and no cleanup is performed
+  doAssert false
diff --git a/tests/lang/s02_core/s99_hooks/s99_escaping_defects/t04_trace_hook.nim b/tests/lang/s02_core/s99_hooks/s99_escaping_defects/t04_trace_hook.nim
new file mode 100644
index 00000000000..10f7988a1e3
--- /dev/null
+++ b/tests/lang/s02_core/s99_hooks/s99_escaping_defects/t04_trace_hook.nim
@@ -0,0 +1,17 @@
+discard """
+  description: "`=trace` hooks panic when a defect escapes"
+  outputsub: "Error: unhandled exception:  [Defect]"
+  exitcode: 1
+"""
+
+type Type = object
+
+proc `=trace`(a: var Type, p: pointer) =
+  raise (ref Defect)()
+
+try:
+  var x: Type
+  `=trace`(x, nil)
+finally:
+  # finally sections are not reached and no cleanup is performed
+  doAssert false
diff --git a/tests/lang/s02_core/s99_hooks/s99_escaping_defects/t05_deepcopy_hook.nim b/tests/lang/s02_core/s99_hooks/s99_escaping_defects/t05_deepcopy_hook.nim
new file mode 100644
index 00000000000..b5b33bab1ac
--- /dev/null
+++ b/tests/lang/s02_core/s99_hooks/s99_escaping_defects/t05_deepcopy_hook.nim
@@ -0,0 +1,16 @@
+discard """
+  description: "`=copy` hooks panic when a defect escapes"
+  outputsub: "Error: unhandled exception:  [Defect]"
+  exitcode: 1
+"""
+
+type Type = ref object
+
+proc `=deepcopy`(a: Type): Type =
+  raise (ref Defect)()
+
+try:
+  var x = `=deepcopy`(Type())
+finally:
+  # finally sections are not reached and no cleanup is performed
+  doAssert false
diff --git a/tests/lang/s02_core/s99_hooks/t99_cannot_raise.nim b/tests/lang/s02_core/s99_hooks/t99_cannot_raise.nim
new file mode 100644
index 00000000000..0232aeed375
--- /dev/null
+++ b/tests/lang/s02_core/s99_hooks/t99_cannot_raise.nim
@@ -0,0 +1,30 @@
+discard """
+  description: '''
+    Hook routines are not allowed to raise exception. If they're inferred to
+    raise, a compile-time error is reported.
+  '''
+  action: reject
+  matrix: "--errorMax:5"
+"""
+
+type Type = object
+
+proc `=copy`(a: var Type, b: Type) =
+  raise (ref CatchableError)() #[tt.Error
+  ^ a hook routine is not allowed to raise. (ref CatchableError)]#
+
+proc `=sink`(a: var Type, b: Type) =
+  raise (ref CatchableError)() #[tt.Error
+  ^ a hook routine is not allowed to raise. (ref CatchableError)]#
+
+proc `=destroy`(a: var Type) =
+  raise (ref CatchableError)() #[tt.Error
+  ^ a hook routine is not allowed to raise. (ref CatchableError)]#
+
+proc `=trace`(a: var Type, env: pointer) =
+  raise (ref CatchableError)() #[tt.Error
+  ^ a hook routine is not allowed to raise. (ref CatchableError)]#
+
+proc `=deepCopy`(a: ref Type): ref Type =
+  raise (ref CatchableError)() #[tt.Error
+  ^ a hook routine is not allowed to raise. (ref CatchableError)]#

From 3e52eaa525dfabcd24a1dfba48efc0d23a1e0bc3 Mon Sep 17 00:00:00 2001
From: zerbina <100542850+zerbina@users.noreply.github.com>
Date: Tue, 12 Mar 2024 22:20:52 +0000
Subject: [PATCH 02/14] sempass2: ensure that hooks don't raise exceptions

The procedure symbol is also marked with the `sfNeverRaises` flag, so
that defects are prevented from escaping at run-time (they're not
tracked at compile-time).

A new report is added: `rsemHooksCannotRaise`.
---
 compiler/ast/report_enums.nim   |  1 +
 compiler/front/cli_reporter.nim |  3 +++
 compiler/sem/sempass2.nim       | 15 +++++++++++++++
 3 files changed, 19 insertions(+)

diff --git a/compiler/ast/report_enums.nim b/compiler/ast/report_enums.nim
index 78a92bcdfd0..1e3bf8cedf3 100644
--- a/compiler/ast/report_enums.nim
+++ b/compiler/ast/report_enums.nim
@@ -422,6 +422,7 @@ type
     rsemCallingConventionMismatch
     rsemHasSideEffects
     rsemCantPassProcvar
+    rsemHookCannotRaise
     rsemUnlistedRaises
     rsemUnlistedEffects
     rsemOverrideSafetyMismatch
diff --git a/compiler/front/cli_reporter.nim b/compiler/front/cli_reporter.nim
index cf89556fe88..7041f776c4e 100644
--- a/compiler/front/cli_reporter.nim
+++ b/compiler/front/cli_reporter.nim
@@ -1303,6 +1303,9 @@ proc reportBody*(conf: ConfigRef, r: SemReport): string =
     of rsemXCannotRaiseY:
       result = "'$1' cannot raise '$2'" % [r.ast.render, r.raisesList.render]
 
+    of rsemHookCannotRaise:
+      result = "a hook routine is not allowed to raise. ($1)" % r.typ.render
+
     of rsemUnlistedRaises, rsemWarnUnlistedRaises:
       result.add("$1 can raise an unlisted exception: " % r.ast.render,
                  r.typ.render)
diff --git a/compiler/sem/sempass2.nim b/compiler/sem/sempass2.nim
index 74b6e2e2c89..d60c2146345 100644
--- a/compiler/sem/sempass2.nim
+++ b/compiler/sem/sempass2.nim
@@ -1788,6 +1788,21 @@ proc trackProc*(c: PContext; s: PSym, body: PNode) =
   else:
     effects[tagEffects] = t.tags
 
+  # ensure that user-provided hooks have no effects and don't raise
+  if sfOverriden in s.flags:
+    # if raising was explicitly disabled (i.e., via ``.raises: []``),
+    # exceptions, if any, were already reported; don't report errors again in
+    # that case
+    if raisesSpec.isNil or raisesSpec.len > 0:
+      let newSpec = newNodeI(nkArgList, s.info)
+      checkRaisesSpec(g, rsemHookCannotRaise, newSpec,
+                      t.exc, hints=off, nil)
+      # override the raises specification to prevent cascading errors:
+      effects[exceptionEffects] = newSpec
+
+    # enforce that no defects escape the routine at run-time:
+    s.flags.incl sfNeverRaises
+
   var mutationInfo = MutationInfo()
   var hasMutationSideEffect = false
   if {strictFuncs, views} * c.features != {}:

From 65adcae6b8067a4c65cd2a046c91905f745a4096 Mon Sep 17 00:00:00 2001
From: zerbina <100542850+zerbina@users.noreply.github.com>
Date: Tue, 12 Mar 2024 22:20:52 +0000
Subject: [PATCH 03/14] liftdestructors: remove "raises" tracking

Lifting/synthesizing hooks no longer needs to track whether an
exception is possible.
---
 compiler/sem/liftdestructors.nim | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/compiler/sem/liftdestructors.nim b/compiler/sem/liftdestructors.nim
index 7493f01316d..8c66b425416 100644
--- a/compiler/sem/liftdestructors.nim
+++ b/compiler/sem/liftdestructors.nim
@@ -57,7 +57,6 @@ type
     asgnForType: PType
     recurse: bool
     addMemReset: bool    # add wasMoved() call after destructor call
-    canRaise: bool
     filterDiscriminator: PSym  # we generating destructor for case branch
     c: PContext # c can be nil, then we are called from lambdalifting!
     idgen: IdGenerator
@@ -152,8 +151,6 @@ proc genContainerOf(c: var TLiftCtx; objType: PType, field, x: PSym): PNode =
 proc destructorCall(c: var TLiftCtx; op: PSym; x: PNode): PNode =
   var destroy = newTreeIT(nkCall, x.info, op.typ[0]):
     [newSymNode(op), genAddr(c, x)]
-  if sfNeverRaises notin op.flags:
-    c.canRaise = true
   if c.addMemReset:
     result = newTree(nkStmtList):
       [destroy, genBuiltin(c, mWasMoved,  "wasMoved", x)]
@@ -303,8 +300,6 @@ proc newHookCall(c: var TLiftCtx; op: PSym; x, y: PNode): PNode =
   #  localReport(c.config, x.info, "usage of '$1' is a user-defined error" % op.name.s)
   result = newNodeI(nkCall, x.info)
   result.add newSymNode(op)
-  if sfNeverRaises notin op.flags:
-    c.canRaise = true
   if op.typ.sons[1].kind == tyVar:
     result.add genAddr(c, x)
   else:
@@ -322,8 +317,6 @@ proc newHookCall(c: var TLiftCtx; op: PSym; x, y: PNode): PNode =
 proc newOpCall(c: var TLiftCtx; op: PSym; x: PNode): PNode =
   result = newTreeIT(nkCall, x.info, op.typ[0]):
     [newSymNode(op), x]
-  if sfNeverRaises notin op.flags:
-    c.canRaise = true
 
 proc newDeepCopyCall(c: var TLiftCtx; op: PSym; x, y: PNode): PNode =
   result = newAsgnStmt(x, newOpCall(c, op, y))
@@ -948,7 +941,7 @@ proc produceSym(g: ModuleGraph; c: PContext; typ: PType; kind: TTypeAttachedOp;
         # bug #19205: Do not forget to also copy the hidden type field:
         genTypeFieldCopy(a, typ, result.ast[bodyPos], d, src)
 
-  if not a.canRaise: incl result.flags, sfNeverRaises
+  incl result.flags, sfNeverRaises
   completePartialOp(g, idgen.module, typ, kind, result)
 
 
@@ -972,7 +965,7 @@ proc produceDestructorForDiscriminator*(g: ModuleGraph; typ: PType; field: PSym,
   result.ast[bodyPos].add v
   let placeHolder = newNodeIT(nkSym, info, getSysType(g, info, tyPointer))
   fillBody(a, typ, result.ast[bodyPos], d, placeHolder)
-  if not a.canRaise: incl result.flags, sfNeverRaises
+  incl result.flags, sfNeverRaises
 
 
 template liftTypeBoundOps*(c: PContext; typ: PType; info: TLineInfo) =

From 7786e11988802579fa36081518f7091fc2cd9a09 Mon Sep 17 00:00:00 2001
From: zerbina <100542850+zerbina@users.noreply.github.com>
Date: Tue, 12 Mar 2024 22:20:53 +0000
Subject: [PATCH 04/14] injectdestructors: always use unchecked calls for hooks

They're guaranteed to not require exception propagation, so a normal
`mnkCall` can always be used.

The comment detailing the now-resolved problem with hooks that raise is
also removed.
---
 compiler/sem/injectdestructors.nim | 34 +-----------------------------
 1 file changed, 1 insertion(+), 33 deletions(-)

diff --git a/compiler/sem/injectdestructors.nim b/compiler/sem/injectdestructors.nim
index b51ff3643cb..8c38b1a2ec8 100644
--- a/compiler/sem/injectdestructors.nim
+++ b/compiler/sem/injectdestructors.nim
@@ -63,31 +63,6 @@
 ## subsequently turning the assignment into a move and thus making the
 ## assertion fail with an ``IndexDefect``.
 
-# XXX: there exists an effect-related problem with the lifetime-tracking hooks
-#      (i.e. ``=copy``, ``=sink``, ``=destroy``). The assignment rewriting and,
-#      to some degree, the destructor injection can be seen as a
-#      refinement/expansion/lowering and should thus not introduce (observable)
-#      side-effects (mutation of global state, exceptional control-flow, etc.) --
-#      it also violates the MIR specification. All three hooks are currently
-#      allowed to have side-effects, which violates the aforementioned rules.
-#      It also causes the concrete issue of cyclic dependencies: for example,
-#      the move analyser uses data-flow analysis (which requires a control-flow
-#      graph) in order to decide where to move and where to copy. If whether a
-#      copy or move is used affects the control-flow graph, the move analyser
-#      depends on its own output, which while possible to make work, would
-#      likely introduce a large amount of complexity.
-#      There are two possible solutions:
-#      1. disallow lifetime-tracking hooks from having any side-effects
-#      2. at least for the ``=copy`` and ``=sink`` hooks, each assignment
-#         could be said to have the union of the effects from both hooks.
-#         Those can be computed when generating the MIR code, as types and
-#         their type-bound operations are already figured out at that point.
-#         It's more complicated for ``=destroy`` hooks, since they are
-#         injected rather than being the result of an expansion. The current
-#         plan is to introduce the MIR concept of dedicated "scope finalizers",
-#         which could be used to attach the effects gathered from all possible
-#         destructor calls to
-
 # XXX: not being able to rewrite an assignment into a call to the copy hook
 #      because it is disabled is a semantic error, meaning that it should
 #      be detected and reported during semantic analysis, not as part of
@@ -527,14 +502,7 @@ template buildVoidCall*(bu: var MirBuilder, env: var MirEnv, p: PSym,
                        body: untyped) =
   let prc = p # prevent multi evaluation
   bu.subTree mnkVoid:
-    let kind =
-      if canRaise(optPanics in graph.config.globalOptions, prc.ast[namePos]):
-        mnkCheckedCall
-      else:
-        mnkCall
-
-    # XXX: injected procedures should not introduce new control-flow paths
-    bu.subTree MirNode(kind: kind, typ: getVoidType(graph)):
+    bu.subTree MirNode(kind: mnkCall, typ: getVoidType(graph)):
       bu.use toValue(env.procedures.add(prc), prc.typ)
       body
 

From e493a2a25759849bb9fdb88ffb03d4094a383c06 Mon Sep 17 00:00:00 2001
From: zerbina <100542850+zerbina@users.noreply.github.com>
Date: Tue, 12 Mar 2024 22:20:53 +0000
Subject: [PATCH 05/14] mirgen: implement "never raises" handling

If a procedure is flagged with `sfNeverRaises`, the body is wrapped in
a catch-all exception handler, where the new `nimUnhandledException`
runtime procedure is invoked from the handler. If the handler is not
actually used, `cgirgen`'s unreachable code elimination automatically
eliminates it again.

Implementing this in `mirgen` ensures consistent behaviour across all
backends.
---
 compiler/mir/mirgen.nim | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/compiler/mir/mirgen.nim b/compiler/mir/mirgen.nim
index 572b334956e..cbbfd5e1860 100644
--- a/compiler/mir/mirgen.nim
+++ b/compiler/mir/mirgen.nim
@@ -2058,6 +2058,10 @@ proc generateCode*(graph: ModuleGraph, env: var MirEnv, owner: PSym,
 
   c.scopeDepth = 1
   c.add MirNode(kind: mnkScope)
+  if sfNeverRaises in owner.flags:
+    c.add MirNode(kind: mnkTry, len: 1)
+    c.add MirNode(kind: mnkStmtList)
+
   if owner.kind in routineKinds:
     # add a 'def' for each ``sink`` parameter. This simplifies further
     # processing and analysis
@@ -2070,6 +2074,21 @@ proc generateCode*(graph: ModuleGraph, env: var MirEnv, owner: PSym,
           c.add MirNode(kind: mnkNone)
 
   gen(c, body)
+
+  if sfNeverRaises in owner.flags:
+    # if it's enforced that the procedure never raises, exceptions escaping
+    # the procedure terminate the program. This is achieved by wrapping the
+    # body in a catch-all exception handler
+    c.add endNode(mnkStmtList)
+    c.subTree MirNode(kind: mnkExcept, len: 1):
+      c.subTree mnkBranch:
+        c.subTree mnkVoid:
+          let p = c.graph.getCompilerProc("nimUnhandledException")
+          c.builder.buildCall c.env.procedures.add(p), p.typ,
+                              typeOrVoid(c, p.typ[0]):
+            discard
+    c.add endNode(mnkTry)
+
   c.add endNode(mnkScope)
 
   swap(c.env, env) # swap back

From 8110d6a96953dd4d2a3109ec833755a9a4fca285 Mon Sep 17 00:00:00 2001
From: zerbina <100542850+zerbina@users.noreply.github.com>
Date: Tue, 12 Mar 2024 22:20:54 +0000
Subject: [PATCH 06/14] implement `nimUnhandledException` for C and JavaScript

For the VM, it's not as simple as for the other two, so the
implementation is still missing.
---
 lib/system/excpt.nim |  6 ++++++
 lib/system/jssys.nim | 11 +++++++++++
 2 files changed, 17 insertions(+)

diff --git a/lib/system/excpt.nim b/lib/system/excpt.nim
index 8603fce28b7..961d02da475 100644
--- a/lib/system/excpt.nim
+++ b/lib/system/excpt.nim
@@ -427,6 +427,12 @@ when true:
       currException = nil
       quit(1)
 
+proc nimUnhandledException() {.compilerproc, noreturn.} =
+  ## Called from generated code when propgation of an exception crosses a
+  ## routine boundary it shouldn't.
+  reportUnhandledError(currException)
+  quit(1)
+
 proc pushActiveException(e: sink(ref Exception)) =
   e.up = activeException
   activeException = e
diff --git a/lib/system/jssys.nim b/lib/system/jssys.nim
index 86b749c47ad..3677b4b0e60 100644
--- a/lib/system/jssys.nim
+++ b/lib/system/jssys.nim
@@ -139,6 +139,17 @@ proc unhandledException(e: ref Exception) {.
   }
   """.}
 
+proc nimUnhandledException() {.compilerproc, asmNoStackFrame.} =
+  # |NimSkull| exceptions are turned into JavaScript errors for the purpose
+  # of better error messages
+  asm """
+    if (lastJSError.m_type !== undefined) {
+      `unhandledException`(lastJSError);
+    } else {
+      throw lastJSError;
+    }
+  """
+
 proc prepareException(e: ref Exception, ename: cstring) {.
     compilerproc, asmNoStackFrame.} =
   if e.name.isNil:

From e8a94e70a41b524675f4c7727eb2259dc41bfd62 Mon Sep 17 00:00:00 2001
From: zerbina <100542850+zerbina@users.noreply.github.com>
Date: Tue, 12 Mar 2024 22:20:55 +0000
Subject: [PATCH 07/14] orc: `GC_fullCollect` doesn't raise

The trace hooks were considered to be able to raise exceptions and have
effects, but this is no longer the case, so the pointer can be safely
cast to procedural values without.
---
 lib/system/orc.nim | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/system/orc.nim b/lib/system/orc.nim
index 38da71d9cda..ee44add02a0 100644
--- a/lib/system/orc.nim
+++ b/lib/system/orc.nim
@@ -27,7 +27,7 @@ const
   logOrc = defined(nimArcIds)
 
 type
-  TraceProc = proc (p, env: pointer) {.nimcall, benign.}
+  TraceProc = proc (p, env: pointer) {.nimcall, benign, raises: [], tags: [].}
   DisposeProc = proc (p: pointer) {.nimcall, benign.}
 
 template color(c): untyped = c.rc and colorMask
@@ -472,7 +472,7 @@ proc GC_prepareOrc*(): int {.inline.} = roots.len
 proc GC_partialCollect*(limit: int) =
   partialCollect(limit)
 
-proc GC_fullCollect* =
+proc GC_fullCollect*() {.raises: [].} =
   ## Forces a full garbage collection pass. With `--gc:orc` triggers the cycle
   ## collector. This is an alias for `GC_runOrc`.
   collectCycles()

From 6482de51b1660b20e7129c8675e5e9e530bd13c3 Mon Sep 17 00:00:00 2001
From: zerbina <100542850+zerbina@users.noreply.github.com>
Date: Tue, 12 Mar 2024 22:20:55 +0000
Subject: [PATCH 08/14] tasks: mark `destroy` callback as not raising

The callback is only used internally, so this does not constitute a
breaking change of the `Task` API.
---
 lib/std/tasks.nim | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/std/tasks.nim b/lib/std/tasks.nim
index e2ea5377f43..ac18862228e 100644
--- a/lib/std/tasks.nim
+++ b/lib/std/tasks.nim
@@ -61,7 +61,7 @@ type
   Task* = object ## `Task` contains the callback and its arguments.
     callback: proc (args: pointer) {.nimcall, gcsafe.}
     args: pointer
-    destroy: proc (args: pointer) {.nimcall, gcsafe.}
+    destroy: proc (args: pointer) {.nimcall, gcsafe, raises: [].}
 
 
 proc `=copy`*(x: var Task, y: Task) {.error.}

From 0cb2a244d631a4e337cceba89902c1a19e2fb65b Mon Sep 17 00:00:00 2001
From: zerbina <100542850+zerbina@users.noreply.github.com>
Date: Tue, 12 Mar 2024 22:20:56 +0000
Subject: [PATCH 09/14] tests: adjust tests

Three individual tests need to be adjusted to the language change:
* `tnew` and `gctest use `debugEcho` instead of `write` (the former
  has no effects)
* `tarcmisc` has to cast away the raise effects of `Stream.close` for
  now
---
 tests/arc/tarcmisc.nim | 3 ++-
 tests/gc/gctest.nim    | 3 +--
 tests/misc/tnew.nim    | 3 +--
 3 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/tests/arc/tarcmisc.nim b/tests/arc/tarcmisc.nim
index 0e1dda7fb33..3747f624097 100644
--- a/tests/arc/tarcmisc.nim
+++ b/tests/arc/tarcmisc.nim
@@ -112,7 +112,8 @@ type
     x: int
 
 proc `=destroy`(x: var AObj) =
-  close(x.io)
+  {.cast(raises: []).}:
+    close(x.io)
   echo "closed"
 
 var x = B(io: newStringStream("thestream"))
diff --git a/tests/gc/gctest.nim b/tests/gc/gctest.nim
index f241bfaf2f0..36138ce1f1d 100644
--- a/tests/gc/gctest.nim
+++ b/tests/gc/gctest.nim
@@ -60,8 +60,7 @@ proc caseTree(lvl: int = 0): PCaseNode =
 
 proc `=destroy`(n: var TNode) =
   assert(addr(n) != nil)
-  write(stdout, "finalizing: ")
-  writeLine(stdout, "not nil")
+  debugEcho "finalizing: not nil"
 
 var
   id: int = 1
diff --git a/tests/misc/tnew.nim b/tests/misc/tnew.nim
index 318fa644be3..1c44bc7b457 100644
--- a/tests/misc/tnew.nim
+++ b/tests/misc/tnew.nim
@@ -20,8 +20,7 @@ type
   TStressTest = ref array[0..45, array[1..45, TNode]]
 
 proc `=destroy`(n: var TNode) =
-  write(stdout, n.data)
-  write(stdout, " is now freed\n")
+  debugEcho n.data, " is now freed"
 
 proc newNode(data: int, le, ri: PNode): PNode =
   new(result)

From 9b22cf48f8cb82b77135389109570fd8ef5dd691 Mon Sep 17 00:00:00 2001
From: zerbina <100542850+zerbina@users.noreply.github.com>
Date: Tue, 12 Mar 2024 22:20:56 +0000
Subject: [PATCH 10/14] tests: update `--expandArc`-using tests

Hooks not being considered as potentially raising is visible in the
pretty-printed MIR output.
---
 tests/arc/topt_no_cursor.nim | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/tests/arc/topt_no_cursor.nim b/tests/arc/topt_no_cursor.nim
index 398a0fe593f..491fc9aefd3 100644
--- a/tests/arc/topt_no_cursor.nim
+++ b/tests/arc/topt_no_cursor.nim
@@ -33,17 +33,17 @@ scope:
   def_cursor _0: Node = target[]
   def_cursor _1: Node = _0[].parent
   def sibling: Node
-  =copy(name sibling, arg _1[].left) (raises)
+  =copy(name sibling, arg _1[].left)
   def_cursor _2: Node = sibling
   def saved: Node
-  =copy(name saved, arg _2[].right) (raises)
+  =copy(name saved, arg _2[].right)
   def_cursor _3: Node = sibling
   def_cursor _4: Node = saved
   def_cursor _6: Node = _4[].left
-  =copy(name _3[].right, arg _6) (raises)
+  =copy(name _3[].right, arg _6)
   def_cursor _5: Node = sibling
-  =sink(name _5[].parent, arg saved) (raises)
-  =destroy(name sibling) (raises)
+  =sink(name _5[].parent, arg saved)
+  =destroy(name sibling)
 -- end of expandArc ------------------------
 --expandArc: p1
 
@@ -130,7 +130,7 @@ scope:
 scope:
   try:
     def shadowScope: Scope
-    =copy(name shadowScope, arg c[].currentScope) (raises)
+    =copy(name shadowScope, arg c[].currentScope)
     rawCloseScope(arg c) (raises)
     scope:
       def_cursor _0: Scope = shadowScope
@@ -157,7 +157,7 @@ scope:
                   addInterfaceDecl(arg c, consume _6) (raises)
                 i = addI(arg i, arg 1) (raises)
   finally:
-    =destroy(name shadowScope) (raises)
+    =destroy(name shadowScope)
 -- end of expandArc ------------------------
 --expandArc: treturn
 

From 47250959fcf55cbd20455d9bc911c228300dddb7 Mon Sep 17 00:00:00 2001
From: zerbina <100542850+zerbina@users.noreply.github.com>
Date: Fri, 15 Mar 2024 01:36:45 +0000
Subject: [PATCH 11/14] tests: fix specification tests for JS

Reporting of unhandled exceptions doesn't include a trailing ':' in the
displayed message if there's no exception message. A message is now set
on the raised defect, which, besides resulting in consistent output,
also helps makes sure that the correct defect is reported.
---
 .../s99_hooks/s99_escaping_defects/t01_copy_hook.nim        | 4 ++--
 .../s99_hooks/s99_escaping_defects/t02_sink_hook.nim        | 4 ++--
 .../s99_hooks/s99_escaping_defects/t03_destroy_hook.nim     | 4 ++--
 .../s99_hooks/s99_escaping_defects/t04_trace_hook.nim       | 4 ++--
 .../s99_hooks/s99_escaping_defects/t05_deepcopy_hook.nim    | 6 +++---
 5 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/tests/lang/s02_core/s99_hooks/s99_escaping_defects/t01_copy_hook.nim b/tests/lang/s02_core/s99_hooks/s99_escaping_defects/t01_copy_hook.nim
index 968edc82d00..af708b10886 100644
--- a/tests/lang/s02_core/s99_hooks/s99_escaping_defects/t01_copy_hook.nim
+++ b/tests/lang/s02_core/s99_hooks/s99_escaping_defects/t01_copy_hook.nim
@@ -1,13 +1,13 @@
 discard """
   description: "`=copy` hooks panic when a defect escapes"
-  outputsub: "Error: unhandled exception:  [Defect]"
+  outputsub: "Error: unhandled exception: error [Defect]"
   exitcode: 1
 """
 
 type Type = object
 
 proc `=copy`(a: var Type, b: Type) =
-  raise (ref Defect)()
+  raise (ref Defect)(msg: "error")
 
 try:
   var x: Type
diff --git a/tests/lang/s02_core/s99_hooks/s99_escaping_defects/t02_sink_hook.nim b/tests/lang/s02_core/s99_hooks/s99_escaping_defects/t02_sink_hook.nim
index 6fecdf046d5..e6f7af55f62 100644
--- a/tests/lang/s02_core/s99_hooks/s99_escaping_defects/t02_sink_hook.nim
+++ b/tests/lang/s02_core/s99_hooks/s99_escaping_defects/t02_sink_hook.nim
@@ -1,13 +1,13 @@
 discard """
   description: "`=sink` hooks panic when a defect escapes"
-  outputsub: "Error: unhandled exception:  [Defect]"
+  outputsub: "Error: unhandled exception: error [Defect]"
   exitcode: 1
 """
 
 type Type = object
 
 proc `=sink`(a: var Type, b: Type) =
-  raise (ref Defect)()
+  raise (ref Defect)(msg: "error")
 
 try:
   var x: Type
diff --git a/tests/lang/s02_core/s99_hooks/s99_escaping_defects/t03_destroy_hook.nim b/tests/lang/s02_core/s99_hooks/s99_escaping_defects/t03_destroy_hook.nim
index e924242560a..5433c981d39 100644
--- a/tests/lang/s02_core/s99_hooks/s99_escaping_defects/t03_destroy_hook.nim
+++ b/tests/lang/s02_core/s99_hooks/s99_escaping_defects/t03_destroy_hook.nim
@@ -1,13 +1,13 @@
 discard """
   description: "`=destroy` hooks panic when a defect escapes"
-  outputsub: "Error: unhandled exception:  [Defect]"
+  outputsub: "Error: unhandled exception: error [Defect]"
   exitcode: 1
 """
 
 type Type = object
 
 proc `=destroy`(a: var Type) =
-  raise (ref Defect)()
+  raise (ref Defect)(msg: "error")
 
 try:
   var x: Type
diff --git a/tests/lang/s02_core/s99_hooks/s99_escaping_defects/t04_trace_hook.nim b/tests/lang/s02_core/s99_hooks/s99_escaping_defects/t04_trace_hook.nim
index 10f7988a1e3..65d62b289fc 100644
--- a/tests/lang/s02_core/s99_hooks/s99_escaping_defects/t04_trace_hook.nim
+++ b/tests/lang/s02_core/s99_hooks/s99_escaping_defects/t04_trace_hook.nim
@@ -1,13 +1,13 @@
 discard """
   description: "`=trace` hooks panic when a defect escapes"
-  outputsub: "Error: unhandled exception:  [Defect]"
+  outputsub: "Error: unhandled exception: error [Defect]"
   exitcode: 1
 """
 
 type Type = object
 
 proc `=trace`(a: var Type, p: pointer) =
-  raise (ref Defect)()
+  raise (ref Defect)(msg: "error")
 
 try:
   var x: Type
diff --git a/tests/lang/s02_core/s99_hooks/s99_escaping_defects/t05_deepcopy_hook.nim b/tests/lang/s02_core/s99_hooks/s99_escaping_defects/t05_deepcopy_hook.nim
index b5b33bab1ac..a4196256bc9 100644
--- a/tests/lang/s02_core/s99_hooks/s99_escaping_defects/t05_deepcopy_hook.nim
+++ b/tests/lang/s02_core/s99_hooks/s99_escaping_defects/t05_deepcopy_hook.nim
@@ -1,13 +1,13 @@
 discard """
-  description: "`=copy` hooks panic when a defect escapes"
-  outputsub: "Error: unhandled exception:  [Defect]"
+  description: "`=deepcopy` hooks panic when a defect escapes"
+  outputsub: "Error: unhandled exception: error [Defect]"
   exitcode: 1
 """
 
 type Type = ref object
 
 proc `=deepcopy`(a: Type): Type =
-  raise (ref Defect)()
+  raise (ref Defect)(msg: "error")
 
 try:
   var x = `=deepcopy`(Type())

From ef88fcc2b133cd1efe5211a14c3d9790ecfb5f65 Mon Sep 17 00:00:00 2001
From: zerbina <100542850+zerbina@users.noreply.github.com>
Date: Fri, 15 Mar 2024 01:36:45 +0000
Subject: [PATCH 12/14] implement `nimUnhandledException` for the VM

The procedure is implemented as a syscall (`vmops`), which raises the
existing `vmEvtUnhandledException` VM event, thus terminating the VM
right away.

Since syscall implementations are currently unable to create a stack-
trace, no stack-trace is passed along, which
`compilerbridge.buildError` now accounts for (by creating the stack-
trace itself). This is somewhat hack-y.
---
 compiler/vm/compilerbridge.nim |  4 +++-
 compiler/vm/vm.nim             | 18 ++----------------
 compiler/vm/vmdeps.nim         | 10 ++++++++++
 compiler/vm/vmops.nim          | 12 ++++++++++++
 lib/system.nim                 |  3 +++
 5 files changed, 30 insertions(+), 17 deletions(-)

diff --git a/compiler/vm/compilerbridge.nim b/compiler/vm/compilerbridge.nim
index 6e147734924..27ee3988b7a 100644
--- a/compiler/vm/compilerbridge.nim
+++ b/compiler/vm/compilerbridge.nim
@@ -225,7 +225,9 @@ proc buildError(c: TCtx, thread: VmThread, event: sink VmEvent): ExecErrorReport
   ## Creates an `ExecErrorReport` with the `event` and a stack-trace for
   ## `thread`
   let stackTrace =
-    if event.kind == vmEvtUnhandledException:
+    if event.kind == vmEvtUnhandledException and event.trace.len > 0:
+      # HACK: an unhandled exception can be reported without providing a trace.
+      #       Ideally, that shouldn't happen
       createStackTrace(c, event.trace)
     else:
       createStackTrace(c, thread)
diff --git a/compiler/vm/vm.nim b/compiler/vm/vm.nim
index 8ec5a6b354b..fdbc65e1604 100644
--- a/compiler/vm/vm.nim
+++ b/compiler/vm/vm.nim
@@ -286,22 +286,8 @@ template toException(x: DerefFailureCode): untyped =
 
 proc reportException(c: TCtx; trace: sink VmRawStackTrace, raised: LocHandle) =
   ## Reports the exception represented by `raised` by raising a `VmError`
-
-  let name = $raised.getFieldHandle(1.fpos).deref().strVal
-  let msg = $raised.getFieldHandle(2.fpos).deref().strVal
-
-  # The reporter expects the exception as a deserialized PNode-tree. Only the
-  # 2nd (name) and 3rd (msg) field are actually used, so instead of running
-  # full deserialization (which is also not possible due to no `PType` being
-  # available), we just create the necessary parts manually
-
-  # TODO: the report should take the two strings directly instead
-  let empty = newNode(nkEmpty)
-  let ast = newTree(nkObjConstr,
-                    empty, # constructor type; unused
-                    empty, # unused
-                    newStrNode(nkStrLit, name),
-                    newStrNode(nkStrLit, msg))
+  let ast = toExceptionAst($raised.getFieldHandle(1.fpos).deref().strVal,
+                           $raised.getFieldHandle(2.fpos).deref().strVal)
   raiseVmError(VmEvent(kind: vmEvtUnhandledException, exc: ast, trace: trace))
 
 func cleanUpReg(r: var TFullReg, mm: var VmMemoryManager) =
diff --git a/compiler/vm/vmdeps.nim b/compiler/vm/vmdeps.nim
index 59918cdb2af..95318902f0d 100644
--- a/compiler/vm/vmdeps.nim
+++ b/compiler/vm/vmdeps.nim
@@ -428,3 +428,13 @@ proc errorReportToString*(c: ConfigRef, error: Report): string =
               # the report, so need to add `"Error: "`
               # manally to stay consistent with the old
               # output.
+
+proc toExceptionAst*(name, msg: sink string): PNode =
+  ## Creates the AST as for an exception object as expected by the report.
+  # TODO: the report should take the two strings directly instead
+  let empty = newNode(nkEmpty)
+  newTree(nkObjConstr,
+          empty, # constructor type; unused
+          empty, # unused
+          newStrNode(nkStrLit, name),
+          newStrNode(nkStrLit, msg))
diff --git a/compiler/vm/vmops.nim b/compiler/vm/vmops.nim
index 70201fd21ae..3d7db173587 100644
--- a/compiler/vm/vmops.nim
+++ b/compiler/vm/vmops.nim
@@ -170,6 +170,17 @@ proc prepareExceptionWrapper(a: VmArgs) {.nimcall.} =
                  deref(a.getHandle(1)).strVal,
                  a.mem.allocator)
 
+proc nimUnhandledExceptionWrapper(a: VmArgs) {.nimcall.} =
+  # setup the exception AST:
+  let
+    exc = a.heap[].tryDeref(a.currentException, noneType).value()
+    ast = toExceptionAst($exc.getFieldHandle(1.fpos).deref().strVal,
+                         $exc.getFieldHandle(2.fpos).deref().strVal)
+  # report the unhandled exception:
+  # XXX: the current stack-trace should be passed along, but we don't
+  #      have access to it here
+  raiseVmError(VmEvent(kind: vmEvtUnhandledException, exc: ast))
+
 proc prepareMutationWrapper(a: VmArgs) {.nimcall.} =
   discard "no-op"
 
@@ -247,6 +258,7 @@ iterator basicOps*(): Override =
   systemop(getCurrentExceptionMsg)
   systemop(getCurrentException)
   systemop(prepareException)
+  systemop(nimUnhandledException)
   systemop(prepareMutation)
   override("stdlib.system.closureIterSetupExc",
            setCurrentExceptionWrapper)
diff --git a/lib/system.nim b/lib/system.nim
index 550b60ac13c..72927a6b123 100644
--- a/lib/system.nim
+++ b/lib/system.nim
@@ -2348,6 +2348,9 @@ elif isNimVmTarget:
   proc prepareException(e: ref Exception, ename: cstring) {.compilerproc.} =
     discard
 
+  proc nimUnhandledException() {.compilerproc.} =
+    discard
+
   proc closureIterSetupExc(e: ref Exception) {.compilerproc, inline.} =
     ## Used by the closure transformation pass for preparing for exception
     ## handling. Implemented as a callback.

From dd4db85699d916dad7aed68a82e8c60fa8b37ff0 Mon Sep 17 00:00:00 2001
From: zerbina <100542850+zerbina@users.noreply.github.com>
Date: Fri, 15 Mar 2024 19:59:53 +0000
Subject: [PATCH 13/14] tests: make sure a `.raises: []` error takes precedence

---
 tests/errmsgs/tprefer_raise_spec_error.nim | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)
 create mode 100644 tests/errmsgs/tprefer_raise_spec_error.nim

diff --git a/tests/errmsgs/tprefer_raise_spec_error.nim b/tests/errmsgs/tprefer_raise_spec_error.nim
new file mode 100644
index 00000000000..4d92f4de3f0
--- /dev/null
+++ b/tests/errmsgs/tprefer_raise_spec_error.nim
@@ -0,0 +1,16 @@
+discard """
+  description: '''
+    An error for violating the explicit `.raises` specification is preferred
+    over the error that hooks cannot raise
+  '''
+  errormsg: "doRaise() can raise an unlisted exception: ref CatchableError"
+  line: 16
+"""
+
+type Obj = object
+
+proc doRaise() =
+  raise CatchableError.newException("")
+
+proc `=copy`(a: var Obj, b: Obj) {.raises: [].} =
+  doRaise()

From 0643d4d1e78550e83c0ea6f6bd4a7084f1f3b540 Mon Sep 17 00:00:00 2001
From: zerbina <100542850+zerbina@users.noreply.github.com>
Date: Fri, 15 Mar 2024 20:58:30 +0000
Subject: [PATCH 14/14] jssys: properly terminate when using node.js

While not possible directly with JavaScript, node.js provides
`process.exit`, which allows terminating in case of an unhandled
exception.
---
 lib/system/jssys.nim | 35 +++++++++++++++++++++++++----------
 1 file changed, 25 insertions(+), 10 deletions(-)

diff --git a/lib/system/jssys.nim b/lib/system/jssys.nim
index 3677b4b0e60..363d20a976a 100644
--- a/lib/system/jssys.nim
+++ b/lib/system/jssys.nim
@@ -115,8 +115,7 @@ proc writeStackTrace() =
 proc getStackTrace*(): string = rawWriteStackTrace()
 proc getStackTrace*(e: ref Exception): string = e.trace
 
-proc unhandledException(e: ref Exception) {.
-    compilerproc, asmNoStackFrame.} =
+proc unhandledExceptionString(e: ref Exception): string =
   var buf = ""
   if e.msg.len != 0:
     add(buf, "Error: unhandled exception: ")
@@ -128,7 +127,11 @@ proc unhandledException(e: ref Exception) {.
   add(buf, "]\n")
   when NimStackTrace:
     add(buf, rawWriteStackTrace())
-  let cbuf = cstring(buf)
+  result = buf
+
+proc unhandledException(e: ref Exception) {.
+    compilerproc, asmNoStackFrame.} =
+  let cbuf = cstring(unhandledExceptionString(e))
   framePtr = nil
   {.emit: """
   if (typeof(Error) !== "undefined") {
@@ -142,13 +145,25 @@ proc unhandledException(e: ref Exception) {.
 proc nimUnhandledException() {.compilerproc, asmNoStackFrame.} =
   # |NimSkull| exceptions are turned into JavaScript errors for the purpose
   # of better error messages
-  asm """
-    if (lastJSError.m_type !== undefined) {
-      `unhandledException`(lastJSError);
-    } else {
-      throw lastJSError;
-    }
-  """
+  when defined(nodejs):
+    {.emit: """
+      if (lastJSError.m_type !== undefined) {
+        console.log(`toJSStr`(`unhandledExceptionString`(`lastJSError`)));
+      } else {
+        console.log('Error: unhandled exception: ', `lastJSError`)
+      }
+      process.exit(1);
+    """.}
+  else:
+    # it's currently not possible to truly panic (abort excution) for non-
+    # node.js JavaScript
+    {.emit: """
+      if (lastJSError.m_type !== undefined) {
+        `unhandledException`(lastJSError);
+      } else {
+        throw lastJSError;
+      }
+    """.}
 
 proc prepareException(e: ref Exception, ename: cstring) {.
     compilerproc, asmNoStackFrame.} =