Merge #292

292: Consistently initialize `Exception.name` across back-ends r=saem a=zerbina

Creating an `Exception` without raising it left it's `name` field as nil
on the JS and VM back-ends. For the C targets, the name initialization
was injected during code-gen.

Initializing the `name` field now either happens in `newException` or
when raising the exception and thus works the same across all back-ends.
Creating an exception without `newException` doesn't set the `name`
field on the C back-ends anymore.

In addition, when raising an exception on the JS target, the exception's
name is not overridden if it's non-nil.

This is a breaking change.



Co-authored-by: zerbina <[email protected]>

compiler/backend/cgen.nim

@@ -429,16 +429,6 @@ proc genObjectInit(p: BProc, section: TCProcSection, t: PType, a: var TLoc,
       var r = if mode == constructObj: addrLoc(p.config, a) else: rdLoc(a)
       linefmt(p, section, "#objectInit($1, $2);$n", [r, genTypeInfoV1(p.module, t, a.lode.info)])
 
-  if isException(t):
-    var r = rdLoc(a)
-    if mode == constructRefObj: r = "(*$1)" % [r]
-    var s = skipTypes(t, abstractInst)
-    if not p.module.compileToCpp:
-      while s.kind == tyObject and s[0] != nil and s.sym.magic != mException:
-        r.add(".Sup")
-        s = skipTypes(s[0], skipPtrs)
-    linefmt(p, section, "$1.name = $2;$n", [r, makeCString(t.skipTypes(abstractInst).sym.name.s)])
-
 proc genRefAssign(p: BProc, dest, src: TLoc)
 
 proc isComplexValueType(t: PType): bool {.inline.} =

compiler/vm/vm.nim

@@ -1479,8 +1479,12 @@ proc rawExecute(c: var TCtx, pc: var int, tos: var StackFrameIndex): TFullReg =
         else:
           regs[ra].node
       c.currentExceptionA = raised
-      # Set the `name` field of the exception
-      c.currentExceptionA[2].skipColon.strVal = c.currentExceptionA.typ.sym.name.s
+      if raised[2].skipColon.strVal.len == 0:
+        # XXX: the VM doesn't distinguish between a `nil` cstring and an empty
+        #      `cstring`, leading to the name erroneously being overridden if
+        #      it was explicitly initialized with `""`
+        # Set the `name` field of the exception
+        raised[2].skipColon.strVal = raised.typ.sym.name.s
       c.exceptionInstr = pc
 
       var frame = tos

lib/system.nim

@@ -2032,9 +2032,9 @@ proc debugEcho*(x: varargs[typed, `$`]) {.magic: "Echo", noSideEffect,
 
 template newException*(exceptn: typedesc, message: string;
                        parentException: ref Exception = nil): untyped =
-  ## Creates an exception object of type `exceptn` and sets its `msg` field
-  ## to `message`. Returns the new exception object.
-  (ref exceptn)(msg: message, parent: parentException)
+  ## Creates an exception object of type `exceptn`, initializes it's `name`
+  ## and sets its `msg` field to `message`. Returns the new exception object.
+  (ref exceptn)(name: $exceptn, msg: message, parent: parentException)
 
 when hostOS == "standalone" and defined(nogc):
   proc nimToCStringConv(s: NimString): cstring {.compilerproc, inline.} =

lib/system/jssys.nim

@@ -142,7 +142,8 @@ proc unhandledException(e: ref Exception) {.
 
 proc raiseException(e: ref Exception, ename: cstring) {.
     compilerproc, asmNoStackFrame.} =
-  e.name = ename
+  if e.name.isNil:
+    e.name = ename
   if excHandler == 0:
     unhandledException(e)
   when NimStackTrace:

tests/exception/texception_name.nim

@@ -0,0 +1,56 @@
+discard """
+  description: "Tests to make sure that the Exception.name field is set"
+  targets: "c cpp js"
+  matrix: "--gc:refc; --gc:arc"
+"""
+
+template test(desc, body) {.dirty.} =
+  block:
+    # Don't execute at top level
+    proc wrapper() =
+      body
+
+    # TODO: once it's available in testament, use the VM target instead
+    static: wrapper()
+    wrapper()
+
+
+test "Valid name without raising first":
+  let e1 = IOError.newException("")
+  doAssert e1.name == "IOError"
+
+  let e2 = ValueError.newException("")
+  doAssert e2.name == "ValueError"
+
+
+test "Valid name after raising":
+
+  proc raiseTest[T](typ: typedesc[T], name: string) =
+    try:
+      raise T.newException("")
+    except T as e:
+      doAssert e.name == name
+
+  raiseTest(IOError, "IOError")
+  raiseTest(ValueError, "ValueError")
+
+
+test "Empty name":
+
+  let e = (ref CatchableError)()
+  doAssert e.name == nil
+
+
+test "Name is set on raise if it was unset":
+  try:
+    raise (ref IOError)() # leave `name` as `nil`
+  except IOError as e:
+    doAssert e.name == "IOError"
+
+
+# fails for the VM. See ``texception_name_vm_issue.nim``
+block no_override_empty:
+  try:
+    raise (ref IOError)(name: "") # explicitly set name to an empty string
+  except IOError as e:
+    doAssert e.name == ""

tests/exception/texception_name_vm_issue.nim

@@ -0,0 +1,14 @@
+discard """
+  action: compile
+  description: '''An empty exception name is overridden on raise when run in
+                  the VM'''
+  knownIssue
+"""
+
+# The VM treats `cstring(nil)` and `cstring("")` as the same thing
+
+static:
+  try:
+    raise (ref CatchableError)(name: "")
+  except CatchableError as e:
+    doAssert e.name == ""