forbid hook routines raising exceptions

compiler/ast/report_enums.nim

@@ -422,6 +422,7 @@ type
     rsemCallingConventionMismatch
     rsemHasSideEffects
     rsemCantPassProcvar
+    rsemHookCannotRaise
     rsemUnlistedRaises
     rsemUnlistedEffects
     rsemOverrideSafetyMismatch

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)

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

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
 

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) =

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 != {}:

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)

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) =

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))

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)

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.}

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.

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

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") {
@@ -139,6 +142,29 @@ proc unhandledException(e: ref Exception) {.
   }
   """.}
 
+proc nimUnhandledException() {.compilerproc, asmNoStackFrame.} =
+  # |NimSkull| exceptions are turned into JavaScript errors for the purpose
+  # of better error messages
+  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.} =
   if e.name.isNil:

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()

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"))