Makes except: panics on Defect (#24821)

implements https://github.com/nim-lang/RFCs/issues/557


It inserts defect handing into a bare except branch

```nim
try:
  raiseAssert "test"
except:
  echo "nope"
```

=>

```nim
try:
  raiseAssert "test"
except:
  # New behaviov, now well-defined: **never** catches the assert, regardless of panic mode
  raiseDefect()
  echo "nope"
```

In this way, `except` still catches foreign exceptions, but panics on
`Defect`. Probably when Nim has `except {.foreign.}`, we can extend
`raiseDefect` to foreign exceptions as well. That's supposed to be a
small use case anyway.

 `--legacy:noPanicOnExcept` is provided for a transition period.

changelog.md

@@ -19,6 +19,8 @@ errors.
 
 - With `-d:nimPreviewAsmSemSymbol`, backticked symbols are type checked in the `asm/emit` statements.
 
+- The bare `except:` now panics on `Defect`. Use `except Exception:` or `except Defect:` to catch `Defect`. `--legacy:noPanicOnExcept` is provided for a transition period.
+
 ## Standard library additions and changes
 
 [//]: # "Additions:"

compiler/options.nim

@@ -248,6 +248,8 @@ type
       ## Useful for libraries that rely on local passC
     jsNoLambdaLifting
       ## Old transformation for closures in JS backend
+    noPanicOnExcept
+      ## don't panic on bare except
 
   SymbolFilesOption* = enum
     disabledSf, writeOnlySf, readOnlySf, v2Sf, stressTest

compiler/transf.nim

@@ -957,6 +957,23 @@ proc transformCall(c: PTransf, n: PNode): PNode =
     else:
       result = s
 
+proc transformBareExcept(c: PTransf, n: PNode): PNode =
+  result = newTransNode(nkExceptBranch, n, 1)
+  if isEmptyType(n[0].typ):
+    result[0] = newNodeI(nkStmtList, n[0].info)
+  else:
+    result[0] = newNodeIT(nkStmtListExpr, n[0].info, n[0].typ)
+  # Generating `raiseDefect()`
+  let raiseDefectCall = callCodegenProc(c.graph, "raiseDefect", n[0].info)
+  result[0].add raiseDefectCall
+  if n[0].kind in {nkStmtList, nkStmtListExpr}:
+    # flattens stmtList
+    for son in n[0]:
+      result[0].add son
+  else:
+    result[0].add n[0]
+  result[0] = transform(c, result[0])
+
 proc transformExceptBranch(c: PTransf, n: PNode): PNode =
   if n[0].isInfixAs() and not isImportedException(n[0][1].typ, c.graph.config):
     let excTypeNode = n[0][1]
@@ -985,6 +1002,9 @@ proc transformExceptBranch(c: PTransf, n: PNode): PNode =
     # Replace the `Exception as foobar` with just `Exception`.
     result[0] = transform(c, n[0][1])
     result[1] = actions
+  elif n.len == 1 and
+      noPanicOnExcept notin c.graph.config.legacyFeatures:
+    result = transformBareExcept(c, n)
   else:
     result = transformSons(c, n)
 

compiler/vmops.nim

@@ -143,6 +143,9 @@ proc getCurrentExceptionMsgWrapper(a: VmArgs) {.nimcall.} =
 proc getCurrentExceptionWrapper(a: VmArgs) {.nimcall.} =
   setResult(a, a.currentException)
 
+proc raiseDefectWrapper(a: VmArgs) {.nimcall.} =
+  discard
+
 proc staticWalkDirImpl(path: string, relative: bool): PNode =
   result = newNode(nkBracket)
   for k, f in walkDir(path, relative):
@@ -263,6 +266,7 @@ proc registerAdditionalOps*(c: PCtx) =
     wrap2si(readLines, ioop)
     systemop getCurrentExceptionMsg
     systemop getCurrentException
+    systemop raiseDefect
     registerCallback c, "stdlib.staticos.staticWalkDir", proc (a: VmArgs) {.nimcall.} =
       setResult(a, staticWalkDirImpl(getString(a, 0), getBool(a, 1)))
     registerCallback c, "stdlib.staticos.staticDirExists", proc (a: VmArgs) {.nimcall.} =

lib/pure/asyncmacro.nim

@@ -46,7 +46,7 @@ template createCb(futTyp, strName, identName, futureVarCompletions: untyped) =
           {.gcsafe.}:
             next.addCallback(cast[proc() {.closure, gcsafe.}](proc =
               identName(fut, it)))
-    except:
+    except Exception:
       futureVarCompletions
       if fut.finished:
         # Take a look at tasyncexceptions for the bug which this fixes.

lib/pure/unittest.nim

@@ -556,15 +556,16 @@ template test*(name, body) {.dirty.} =
       body
       {.pop.}
 
-    except:
+    except Exception:
       let e = getCurrentException()
       let eTypeDesc = "[" & exceptionTypeName(e) & "]"
       checkpoint("Unhandled exception: " & getCurrentExceptionMsg() & " " & eTypeDesc)
-      if e == nil: # foreign
-        fail()
-      else:
-        var stackTrace {.inject.} = e.getStackTrace()
-        fail()
+      var stackTrace {.inject.} = e.getStackTrace()
+      fail()
+
+    except:
+      checkpoint("Unhandled exception: " & getCurrentExceptionMsg() & " [<foreign exception>]")
+      fail()
 
     finally:
       if testStatusIMPL == TestStatus.FAILED:
@@ -760,6 +761,14 @@ macro expect*(exceptions: varargs[typed], body: untyped): untyped =
     expect IOError, OSError, ValueError, AssertionDefect:
       defectiveRobot()
 
+  template expectException(errorTypes, lineInfoLit, body): NimNode {.dirty.} =
+    try:
+      body
+      checkpoint(lineInfoLit & ": Expect Failed, no exception was thrown.")
+      fail()
+    except errorTypes:
+      discard
+
   template expectBody(errorTypes, lineInfoLit, body): NimNode {.dirty.} =
     {.push warning[BareExcept]:off.}
     try:
@@ -770,17 +779,23 @@ macro expect*(exceptions: varargs[typed], body: untyped): untyped =
       fail()
     except errorTypes:
       discard
-    except:
+    except Exception:
       let err = getCurrentException()
       checkpoint(lineInfoLit & ": Expect Failed, " & $err.name & " was thrown.")
       fail()
     {.pop.}
 
   var errorTypes = newNimNode(nnkBracket)
+  var hasException = false
   for exp in exceptions:
+    if exp.strVal == "Exception":
+      hasException = true
     errorTypes.add(exp)
 
-  result = getAst(expectBody(errorTypes, errorTypes.lineInfo, body))
+  if hasException:
+    result = getAst(expectException(errorTypes, errorTypes.lineInfo, body))
+  else:
+    result = getAst(expectBody(errorTypes, errorTypes.lineInfo, body))
 
 proc disableParamFiltering* =
   ## disables filtering tests with the command line params

lib/system.nim

@@ -2312,8 +2312,16 @@ when notJSnotNims and hostOS != "standalone":
     ##
     ## .. warning:: Only use this if you know what you are doing.
     currException = exc
+
+  proc raiseDefect() {.compilerRtl.} =
+    let e = getCurrentException()
+    if e of Defect:
+      reportUnhandledError(e)
+      rawQuit(1)
+
 elif defined(nimscript):
   proc getCurrentException*(): ref Exception {.compilerRtl.} = discard
+  proc raiseDefect*() {.compilerRtl.} = discard
 
 when notJSnotNims:
   {.push stackTrace: off, profiler: off.}

lib/system/embedded.nim

@@ -42,6 +42,9 @@ proc raiseExceptionEx(e: sink(ref Exception), ename, procname, filename: cstring
 proc reraiseException() {.compilerRtl.} =
   sysFatal(ReraiseDefect, "no exception to reraise")
 
+proc raiseDefect() {.compilerRtl.} =
+  sysFatal(ReraiseDefect, "exception handling is not available")
+
 proc writeStackTrace() = discard
 
 proc unsetControlCHook() = discard

lib/system/jssys.nim

@@ -154,6 +154,16 @@ proc raiseException(e: ref Exception, ename: cstring) {.
     e.trace = rawWriteStackTrace()
   {.emit: "throw `e`;".}
 
+proc raiseDefect() {.compilerproc, asmNoStackFrame.} =
+  if isNimException():
+    let e = getCurrentException()
+    if e of Defect:
+      if excHandler == 0:
+        unhandledException(e)
+      when NimStackTrace:
+        e.trace = rawWriteStackTrace()
+      {.emit: "throw `e`;".}
+
 proc reraiseException() {.compilerproc, asmNoStackFrame.} =
   if lastJSError == nil:
     raise newException(ReraiseDefect, "no exception to reraise")

testament/important_packages.nim

@@ -42,7 +42,7 @@ pkg "asyncthreadpool", "nimble test --mm:refc"
 pkg "awk"
 pkg "bigints"
 pkg "binaryheap", "nim c -r binaryheap.nim"
-pkg "BipBuffer"
+pkg "BipBuffer", url = "https://github.com/nim-lang/BipBuffer"
 pkg "bncurve"
 pkg "brainfuck", "nim c -d:release -r tests/compile.nim"
 pkg "c2nim", "nim c testsuite/tester.nim"
@@ -66,7 +66,7 @@ pkg "delaunay"
 pkg "docopt"
 pkg "dotenv"
 pkg "easygl", "nim c -o:egl -r src/easygl.nim", "https://github.com/jackmott/easygl"
-pkg "elvis"
+pkg "elvis", url = "https://github.com/nim-lang/elvis"
 pkg "eth", "nim c -o:common -r tests/common/all_tests"
 pkg "faststreams"
 pkg "fidget"

tests/async/tasynctry.nim

@@ -21,7 +21,7 @@ proc catch() {.async.} =
   # TODO: Create a test for when exceptions are not caught.
   try:
     await foobar()
-  except:
+  except Exception:
     echo("Generic except: ", getCurrentExceptionMsg().splitLines[0])
 
   try:

tests/ccgbugs/t21995.nim

@@ -5,5 +5,5 @@ discard """
 
 try:
   raise
-except:
+except ReraiseDefect:
   echo "Hi!"

tests/ccgbugs/t9286.nim

@@ -1,5 +1,5 @@
 discard """
-  action: run
+  matrix: "--legacy:noPanicOnExcept"
 """
 
 import options

tests/float/tfloatrange.nim

@@ -32,7 +32,7 @@ doAssert(sqrt(x) == 3.0)
 var z = -10.0
 try:
   myoverload(StrictPositive(z))
-except:
+except Exception:
   echo "range fail expected"
 
 
@@ -45,6 +45,6 @@ doAssert(strictOnlyProc(x2))
 try:
   let x4 = 0.0.Positive
   discard strictOnlyProc(x4)
-except:
+except Exception:
   echo "range fail expected"
 

tests/iter/titer_issues.nim

@@ -385,6 +385,9 @@ iterator tryFinally() {.closure.} =
     try:
       echo "trying"
       raise
+    except ReraiseDefect:
+      echo "exception caught"
+      break route
     except:
       echo "exception caught"
       break route

tests/js/tarrayboundscheck.nim

@@ -35,9 +35,9 @@ proc test_arrayboundscheck() =
       let idx = indices[i]
       try:
         echo months[idx]
-      except:
+      except IndexDefect:
         echo "month out of bounds: ", idx
-    except:
+    except IndexDefect:
       echo "idx out of bounds: ", i
   
   # #13966