addQuitProc now works with closures, and c, js(node/browser) backend; fix some bugs in testament (#14342)

* make addQuitProc great again

* fix bugs in testament

* fix test

* change 2016 => 2020

* addQuitProc => addExitProc + locks

* move to std/exitprocs

lib/core/locks.nim

@@ -9,6 +9,10 @@
 
 ## This module contains Nim's support for locks and condition vars.
 
+#[
+for js, for now we treat locks as noop's to avoid pushing `when defined(js)`
+in client code that uses locks.
+]#
 
 when not compileOption("threads") and not defined(nimdoc):
   when false: # fix #12330
@@ -26,7 +30,8 @@ type
 
 proc initLock*(lock: var Lock) {.inline.} =
   ## Initializes the given lock.
-  initSysLock(lock)
+  when not defined(js):
+    initSysLock(lock)
 
 proc deinitLock*(lock: var Lock) {.inline.} =
   ## Frees the resources associated with the lock.
@@ -38,11 +43,13 @@ proc tryAcquire*(lock: var Lock): bool =
 
 proc acquire*(lock: var Lock) =
   ## Acquires the given lock.
-  acquireSys(lock)
+  when not defined(js):
+    acquireSys(lock)
 
 proc release*(lock: var Lock) =
   ## Releases the given lock.
-  releaseSys(lock)
+  when not defined(js):
+    releaseSys(lock)
 
 
 proc initCond*(cond: var Cond) {.inline.} =

lib/pure/quitprocs.nim

@@ -1,28 +0,0 @@
-#
-#
-#            Nim's Runtime Library
-#        (c) Copyright 2016 Andreas Rumpf
-#
-#    See the file "copying.txt", included in this
-#    distribution, for details about the copyright.
-#
-
-## ``system.addQuitProc`` is nice and very useful but due to its C-based
-## implementation it doesn't support closures which limits its usefulness.
-## This module fixes this. Later versions of this module will also
-## support the JavaScript backend.
-
-var
-  gClosures: seq[proc () {.closure.}]
-
-proc callClosures() {.noconv.} =
-  for i in countdown(gClosures.len-1, 0):
-    gClosures[i]()
-
-proc addQuitClosure*(cl: proc () {.closure.}) =
-  ## Like ``system.addQuitProc`` but it supports closures.
-  if gClosures.len == 0:
-    addQuitProc(callClosures)
-    gClosures = @[cl]
-  else:
-    gClosures.add(cl)

lib/std/exitprocs.nim

@@ -0,0 +1,65 @@
+#
+#
+#            Nim's Runtime Library
+#        (c) Copyright 2020 Andreas Rumpf
+#
+#    See the file "copying.txt", included in this
+#    distribution, for details about the copyright.
+#
+
+import locks
+
+type
+  FunKind = enum kClosure, kNoconv # extend as needed
+  Fun = object
+    case kind: FunKind
+    of kClosure: fun1: proc () {.closure.}
+    of kNoconv: fun2: proc () {.noconv.}
+
+var
+  gFunsLock: Lock
+  gFuns: seq[Fun]
+
+initLock(gFunsLock)
+
+when defined(js):
+  proc addAtExit(quitProc: proc() {.noconv.}) =
+    when defined(nodejs):
+      asm """
+        process.on('exit', `quitProc`);
+      """
+    elif defined(js):
+      asm """
+        window.onbeforeunload = `quitProc`;
+      """
+else:
+  proc addAtExit(quitProc: proc() {.noconv.}) {.
+    importc: "atexit", header: "<stdlib.h>".}
+
+proc callClosures() {.noconv.} =
+  withLock gFunsLock:
+    for i in countdown(gFuns.len-1, 0):
+      let fun = gFuns[i]
+      case fun.kind
+      of kClosure: fun.fun1()
+      of kNoconv: fun.fun2()
+
+template fun() =
+  if gFuns.len == 0:
+    addAtExit(callClosures)
+
+proc addExitProc*(cl: proc () {.closure.}) =
+  ## Adds/registers a quit procedure. Each call to `addExitProc` registers
+  ## another quit procedure. They are executed on a last-in, first-out basis.
+  # Support for `addExitProc` is done by Ansi C's facilities here.
+  # In case of an unhandled exception the exit handlers should
+  # not be called explicitly! The user may decide to do this manually though.
+  withLock gFunsLock:
+    fun()
+    gFuns.add Fun(kind: kClosure, fun1: cl)
+
+proc addExitProc*(cl: proc() {.noconv.}) =
+  ## overload for `noconv` procs.
+  withLock gFunsLock:
+    fun()
+    gFuns.add Fun(kind: kNoconv, fun2: cl)

lib/system.nim

@@ -1147,8 +1147,8 @@ when defined(nimdoc):
   proc quit*(errorcode: int = QuitSuccess) {.magic: "Exit", noreturn.}
     ## Stops the program immediately with an exit code.
     ##
-    ## Before stopping the program the "quit procedures" are called in the
+    ## Before stopping the program the "exit procedures" are called in the
-    ## opposite order they were added with `addQuitProc <#addQuitProc,proc>`_.
+    ## opposite order they were added with `addExitProc <exitprocs.html#addExitProc,proc>`_.
     ## ``quit`` never returns and ignores any exception that may have been raised
     ## by the quit procedures.  It does *not* call the garbage collector to free
     ## all the memory, unless a quit procedure calls `GC_fullCollect
@@ -1426,8 +1426,8 @@ proc toBiggestInt*(f: BiggestFloat): BiggestInt {.noSideEffect.} =
   ## Same as `toInt <#toInt,float>`_ but for ``BiggestFloat`` to ``BiggestInt``.
   if f >= 0: BiggestInt(f+0.5) else: BiggestInt(f-0.5)
 
-proc addQuitProc*(quitProc: proc() {.noconv.}) {.
+proc addQuitProc*(quitProc: proc() {.noconv.}) {. 
-  importc: "atexit", header: "<stdlib.h>".}
+  importc: "atexit", header: "<stdlib.h>", deprecated: "use exitprocs.addExitProc".} 
   ## Adds/registers a quit procedure.
   ##
   ## Each call to ``addQuitProc`` registers another quit procedure. Up to 30
@@ -1440,7 +1440,6 @@ proc addQuitProc*(quitProc: proc() {.noconv.}) {.
 # In case of an unhandled exception the exit handlers should
 # not be called explicitly! The user may decide to do this manually though.
 
-
 proc swap*[T](a, b: var T) {.magic: "Swap", noSideEffect.}
   ## Swaps the values `a` and `b`.
   ##

testament/categories.nim

@@ -269,8 +269,8 @@ proc debuggerTests(r: var TResults, cat: Category, options: string) =
 
 proc jsTests(r: var TResults, cat: Category, options: string) =
   template test(filename: untyped) =
-    testSpec r, makeTest(filename, options & " -d:nodejs", cat), {targetJS}
+    testSpec r, makeTest(filename, options, cat), {targetJS}
-    testSpec r, makeTest(filename, options & " -d:nodejs -d:release", cat), {targetJS}
+    testSpec r, makeTest(filename, options & " -d:release", cat), {targetJS}
 
   for t in os.walkFiles("tests/js/t*.nim"):
     test(t)

testament/specs.nim

@@ -87,6 +87,13 @@ const
   targetToExt*: array[TTarget, string] = ["nim.c", "nim.cpp", "nim.m", "js"]
   targetToCmd*: array[TTarget, string] = ["c", "cpp", "objc", "js"]
 
+proc defaultOptions*(a: TTarget): string =
+  case a
+  of targetJS: "-d:nodejs"
+    # once we start testing for `nim js -d:nimbrowser` (eg selenium or similar),
+    # we can adapt this logic; or a given js test can override with `-u:nodejs`.
+  else: ""
+
 when not declared(parseCfgBool):
   # candidate for the stdlib:
   proc parseCfgBool(s: string): bool =

testament/testament.nim

@@ -135,7 +135,10 @@ proc nimcacheDir(filename, options: string, target: TTarget): string =
 
 proc prepareTestArgs(cmdTemplate, filename, options, nimcache: string,
                      target: TTarget, extraOptions = ""): seq[string] =
-  let options = options & " " & quoteShell("--nimCache:" & nimcache) & " " & extraOptions
+  var options = target.defaultOptions & " " & options
+  # improve pending https://github.com/nim-lang/Nim/issues/14343
+  if nimcache.len > 0: options.add " " & ("--nimCache:" & nimcache).quoteShell
+  options.add " " & extraOptions
   result = parseCmdLine(cmdTemplate % ["target", targetToCmd[target],
                       "options", options, "file", filename.quoteShell,
                       "filedir", filename.getFileDir()])
@@ -192,9 +195,7 @@ proc callCompiler(cmdTemplate, filename, options, nimcache: string,
 
 proc callCCompiler(cmdTemplate, filename, options: string,
                   target: TTarget): TSpec =
-  let c = parseCmdLine(cmdTemplate % ["target", targetToCmd[target],
+  let c = prepareTestArgs(cmdTemplate, filename, options, nimcache = "", target)
-                       "options", options, "file", filename.quoteShell,
-                       "filedir", filename.getFileDir()])
   var p = startProcess(command="gcc", args=c[5 .. ^1],
                        options={poStdErrToStdOut, poUsePath})
   let outp = p.outputStream
@@ -254,7 +255,8 @@ proc addResult(r: var TResults, test: TTest, target: TTarget,
   # test.name is easier to find than test.name.extractFilename
   # A bit hacky but simple and works with tests/testament/tshould_not_work.nim
   var name = test.name.replace(DirSep, '/')
-  name.add " " & $target & test.options
+  name.add " " & $target
+  if test.options.len > 0: name.add " " & test.options
 
   let duration = epochTime() - test.startTime
   let success = if test.spec.timeout > 0.0 and duration > test.spec.timeout: reTimeout

tests/converter/tor_in_converter.nim

@@ -4,7 +4,7 @@ test'''
 """
 # bug #4537
 
-# nim js --d:nodejs
+# nim js -d:nodejs
 
 type
   Str = distinct string

tests/stdlib/texitprocs.nim

@@ -0,0 +1,21 @@
+discard """
+targets: "c cpp js"
+output: '''
+ok4
+ok3
+ok2
+ok1
+'''
+"""
+
+import std/exitprocs
+
+proc fun1() {.noconv.} = echo "ok1"
+proc fun2() = echo "ok2"
+proc fun3() {.noconv.} = echo "ok3"
+proc fun4() = echo "ok4"
+
+addExitProc(fun1)
+addExitProc(fun2)
+addExitProc(fun3)
+addExitProc(fun4)