enable lambda-lifting for the JavaScript target

Summary
=======

Previously, inner closure procedures were not lifted into free-standing
procedures during `transf`, but rather kept as is and mapped to nested
JavaScript functions by `jsgen`. Besides requiring the JavaScript target
to be special-cased in `transf` and `lambdalifting`, this also caused
observable semantic issues: the destructors for captured variables were
called when leaving the scope in which they were defined, not when the
closure got destroyed.

In order to bring the closure semantics in line with the other targets
and as a prerequisite for full ORC support, lambda-lifting is now also
enabled when using the JS target -- the code generator is adjusted to
support the lifted procedures. The code generator changes are also a
preparation for full closure iterator support.

As a side-effect, closures now also work as expected when using the JS
target and executing non-compile-time-only procedures that have inner
closure procedures at compile-time.

Details
=======

- remove the dispatching based on the selected backend from `transf` and
  `lambdalifting`
- remove all code related to the previous closure support from `jsgen`
- implement support for lifted procedures in `jsgen`

The hidden environment parameter of routines using the `.closure`
calling convention is mapped to the this parameter at the JavaScript
level.

When a NimSkull closure object is created, a new function instance is
created via `bind` and the provided environment bound to the `this`
parameter. In addition, the both the function and environment are also
assigned to the respective prc and env properties on the function
object -- this allows for later retrieval.

Compared to using an object, this approach has the benefit that closures
can still be passed to an imported procedure that expects a JavaScript
function object, without requiring any changes to the user code.

compiler/backend/jsgen.nim

@@ -143,25 +143,18 @@ type
     options: TOptions
     module: BModule
     g: PGlobals
-    generatedParamCopies: IntSet
     beforeRetNeeded: bool
     unique: int    # for temp identifier generation
     blocks: seq[TBlock]
     extraIndent: int
-    up: PProc     # up the call chain; required for closure support
     declaredGlobals: IntSet
 
 template config*(p: PProc): ConfigRef = p.module.config
 
 proc indentLine(p: PProc, r: Rope): Rope =
   result = r
-  var p = p
-  while true:
-    for i in 0..<p.blocks.len + p.extraIndent:
-      prepend(result, rope"  ")
-    if p.up == nil or p.up.prc != p.prc.owner:
-      break
-    p = p.up
+  for i in 0..<p.blocks.len + p.extraIndent:
+    prepend(result, rope"  ")
 
 template line(p: PProc, added: string) =
   p.body.add(indentLine(p, rope(added)))
@@ -714,6 +707,13 @@ proc arith(p: PProc, n: PNode, r: var TCompRes, op: TMagic) =
       gen(p, n[1], x)
       gen(p, n[2], y)
       r.res = "($# == $# && $# == $#)" % [x.address, y.address, x.res, y.res]
+  of mEqProc:
+    if skipTypes(n[1].typ, abstractInst).callConv == ccClosure:
+      # a closure itself is a value type, so a dedicated comparision is
+      # required
+      binaryExpr(p, n, r, "cmpClosures", "cmpClosures($1, $2)")
+    else:
+      arithAux(p, n, r, op)
   else:
     arithAux(p, n, r, op)
   r.kind = resExpr
@@ -1027,8 +1027,11 @@ proc genIf(p: PProc, n: PNode, r: var TCompRes) =
     lineF(p, "}$n", [])
   line(p, repeat('}', toClose) & "\L")
 
-proc generateHeader(p: PProc, typ: PType): Rope =
+proc generateHeader(p: PProc, prc: PSym): Rope =
   result = ""
+
+  let typ = prc.typ
+
   for i in 1..<typ.n.len:
     assert(typ.n[i].kind == nkSym)
     var param = typ.n[i].sym
@@ -1041,6 +1044,14 @@ proc generateHeader(p: PProc, typ: PType): Rope =
       result.add(name)
       result.add("_Idx")
 
+  if typ.callConv == ccClosure:
+    # generate the name for the hidden environment parameter. When creating a
+    # closure object, the environment is bound to the ``this`` argument of the
+    # function, hence using ``this`` for the name
+    let hidden = prc.ast[paramsPos].lastSon
+    assert hidden.kind == nkSym, "the hidden parameter is missing"
+    hidden.sym.loc.r = "this"
+
 const
   nodeKindsNeedNoCopy = {nkCharLit..nkInt64Lit, nkStrLit..nkTripleStrLit,
     nkFloatLit..nkFloat64Lit, nkPar, nkStringToCString,
@@ -1050,7 +1061,7 @@ const
 
 proc needsNoCopy(p: PProc; y: PNode): bool =
   return y.kind in nodeKindsNeedNoCopy or
-        ((mapType(y.typ) != etyBaseIndex or (y.kind == nkSym and y.sym.kind == skParam)) and
+        ((mapType(y.typ) != etyBaseIndex) and
           (skipTypes(y.typ, abstractInst).kind in
             {tyRef, tyPtr, tyLent, tyVar, tyCstring, tyProc} + IntegralTypes))
 
@@ -1390,26 +1401,7 @@ proc attachProc(p: PProc; s: PSym) =
 
 proc genProcForSymIfNeeded(p: PProc, s: PSym) =
   if not p.g.generatedSyms.containsOrIncl(s.id):
-    let newp = genProc(p, s)
-    var owner = p
-    while owner != nil and owner.prc != s.owner:
-      owner = owner.up
-    if owner != nil: owner.locals.add(newp)
-    else: attachProc(p, newp, s)
-
-proc genCopyForParamIfNeeded(p: PProc, n: PNode) =
-  let s = n.sym
-  if p.prc == s.owner or needsNoCopy(p, n):
-    return
-  var owner = p.up
-  while true:
-    p.config.internalAssert(owner != nil, n.info, "couldn't find the owner proc of the closed over param: " & s.name.s)
-    if owner.prc == s.owner:
-      if not owner.generatedParamCopies.containsOrIncl(s.id):
-        let copy = "$1 = nimCopy(null, $1, $2);$n" % [s.loc.r, genTypeInfo(p, s.typ)]
-        owner.locals.add(owner.indentLine(copy))
-      return
-    owner = owner.up
+    attachProc(p, s)
 
 proc genVarInit(p: PProc, v: PSym, n: PNode)
 
@@ -1420,8 +1412,6 @@ proc genSym(p: PProc, n: PNode, r: var TCompRes) =
     p.config.internalAssert(s.loc.r != "", n.info, "symbol has no generated name: " & s.name.s)
     if sfCompileTime in s.flags:
       genVarInit(p, s, if s.ast != nil: s.ast else: newNodeI(nkEmpty, s.info))
-    if s.kind == skParam:
-      genCopyForParamIfNeeded(p, n)
     let k = mapType(p, s.typ)
     if k == etyBaseIndex:
       r.typ = etyBaseIndex
@@ -2169,6 +2159,8 @@ proc genMagic(p: PProc, n: PNode, r: var TCompRes) =
     r.kind = resExpr
   of mMove:
     genMove(p, n, r)
+  of mAccessEnv:
+    unaryExpr(p, n, r, "accessEnv", "accessEnv($1)")
   else:
     genCall(p, n, r)
     #else internalError(p.config, e.info, 'genMagic: ' + magicToStr[op]);
@@ -2385,11 +2377,10 @@ proc genProc(oldProc: PProc, prc: PSym): Rope =
   #if gVerbosity >= 3:
   #  echo "BEGIN generating code for: " & prc.name.s
   var p = newProc(oldProc.g, oldProc.module, prc.ast, prc.options)
-  p.up = oldProc
   var returnStmt = ""
   var resultAsgn = ""
   var name = mangleName(p.module, prc)
-  let header = generateHeader(p, prc.typ)
+  let header = generateHeader(p, prc)
   if prc.typ[0] != nil and sfPure notin prc.flags:
     resultSym = prc.ast[resultPos].sym
     let mname = mangleName(p.module, resultSym)
@@ -2562,7 +2553,13 @@ proc gen(p: PProc, n: PNode, r: var TCompRes) =
       genInfixCall(p, n, r)
     else:
       genCall(p, n, r)
-  of nkClosure: gen(p, n[0], r)
+  of nkClosure:
+    useMagic(p, "makeClosure")
+    var tmp1, tmp2: TCompRes
+    gen(p, n[0], tmp1)
+    gen(p, n[1], tmp2)
+    r.res = "makeClosure($1, $2)" % [tmp1.rdLoc, tmp2.rdLoc]
+    r.kind = resExpr
   of nkCurly: genSetConstr(p, n, r)
   of nkBracket: genArrayConstr(p, n, r)
   of nkPar, nkTupleConstr: genTupleConstr(p, n, r)
@@ -2595,13 +2592,6 @@ proc gen(p: PProc, n: PNode, r: var TCompRes) =
   of nkStringToCString: convStrToCStr(p, n, r)
   of nkCStringToString: convCStrToStr(p, n, r)
   of nkEmpty: discard
-  of nkLambdaKinds:
-    let s = n[namePos].sym
-    discard mangleName(p.module, s)
-    r.res = s.loc.r
-    if lfNoDecl in s.loc.flags or s.magic notin {mNone, mIsolate}: discard
-    elif not p.g.generatedSyms.containsOrIncl(s.id):
-      p.locals.add(genProc(p, s))
   of nkType: r.res = genTypeInfo(p, n.typ)
   of nkStmtList, nkStmtListExpr:
     # this shows the distinction is nice for backends and should be kept

compiler/sem/lambdalifting.nim

@@ -252,11 +252,6 @@ proc interestingIterVar(s: PSym): bool {.inline.} =
 template isIterator*(owner: PSym): bool =
   owner.kind == skIterator and owner.typ.callConv == ccClosure
 
-proc liftingHarmful(conf: ConfigRef; owner: PSym): bool {.inline.} =
-  ## lambda lifting can be harmful for JS-like code generators.
-  let isCompileTime = sfCompileTime in owner.flags or owner.kind == skMacro
-  result = conf.backend == backendJs and not isCompileTime
-
 proc createTypeBoundOpsLL(g: ModuleGraph; refType: PType; info: TLineInfo; idgen: IdGenerator; owner: PSym) =
   if owner.kind != skMacro:
     createTypeBoundOps(g, nil, refType.lastSon, info, idgen)
@@ -266,7 +261,6 @@ proc createTypeBoundOpsLL(g: ModuleGraph; refType: PType; info: TLineInfo; idgen
 
 proc liftIterSym*(g: ModuleGraph; n: PNode; idgen: IdGenerator; owner: PSym): PNode =
   # transforms  (iter)  to  (let env = newClosure[iter](); (iter, env))
-  if liftingHarmful(g.config, owner): return n
   let iter = n.sym
   assert iter.isIterator
 
@@ -777,14 +771,7 @@ func dontLift(prc: PSym): bool {.inline.} =
 
 proc liftLambdas*(g: ModuleGraph; fn: PSym, body: PNode; tooEarly: var bool;
                   idgen: IdGenerator): PNode =
-  # XXX backend == backendJs does not suffice! The compiletime stuff needs
-  # the transformation even when compiling to JS ...
-
-  # However we can do lifting for the stuff which is *only* compiletime.
-  let isCompileTime = sfCompileTime in fn.flags or fn.kind == skMacro
-
   if body.kind == nkEmpty or # forward declaration
-     (g.config.backend == backendJs and not isCompileTime) or
      dontLift(fn):
     # HACK: anonymous procedures defined outside of procedures (i.e. owned by
     #       a module), need a hidden environment parameter too, even though
@@ -863,7 +850,6 @@ proc liftForLoop*(g: ModuleGraph; body: PNode; idgen: IdGenerator; owner: PSym):
           break
         ...
     """
-  if liftingHarmful(g.config, owner): return body
   if not (body.kind == nkForStmt and body[^2].kind in nkCallKinds):
     localReport(g.config, body, reportSem rsemIgnoreInvalidForLoop)
     return body

compiler/sem/transf.nim

@@ -472,7 +472,6 @@ proc generateThunk(c: PTransf; prc: PNode, dest: PType): PNode =
 
   # we cannot generate a proper thunk here for GC-safety reasons
   # (see internal documentation):
-  if c.graph.config.backend == backendJs: return prc
   c.graph.config.internalAssert(prc.kind != nkClosure, prc.info, "closure to closure created")
 
   let conv = newTreeIT(nkHiddenSubConv, prc.info, dest):

lib/system/jssys.nim

@@ -800,3 +800,40 @@ if (!Math.trunc) {
 }
 """
 when not defined(nodejs): {.emit: jsMathTrunc .}
+
+proc makeClosure(f: JSRef, env: JSRef) {.compilerproc, asmNoStackFrame.} =
+  ## Used by the code generator to create a |NimSkull| closure object. A
+  ## JavaScript function is used as the base object, so that passing
+  ## |NimSkull| closures through the FFI where the other side expects a
+  ## functions object works.
+  asm """
+    var cl = `f`.bind(`env`);
+    cl.prc = `f`;
+    cl.env = `env`;
+    return cl;
+  """
+
+proc accessEnv(cl: JSRef): JSRef {.compilerproc, asmNoStackFrame.} =
+  ## Used by the code generator to access the environment of a closure object
+  ## (`cl`).
+  ##
+  ## The closure object might not be a proper one if it's provided from the
+  ## JavaScript side via the FFI. In order to also support such usage for
+  ## now, `null` is returned in that case.
+  asm """
+    (`cl`== null || `cl`.env == undefined ? null : `cl`.env)
+  """
+
+proc cmpClosures(a, b: JSRef): bool {.compilerproc, asmNoStackFrame.} =
+  ## Compares two closure for value equality. If at least one of them is not a
+  ## real closure (because an JavaScript object passed through the FFI
+  ## incorrectly uses the ``.closure`` calling convention), the default
+  ## equality operator is used.
+  asm """
+    if (`a` != null && `a`.env != undefined &&
+        `b` != null && `b`.env != undefined) {
+      return `a`.prc == `b`.prc && `a`.env == `b`.env;
+    } else {
+      return `a` == `b`;
+    }
+  """

tests/lang_callable/closure/tclosure.nim

@@ -36,10 +36,7 @@ block tclosure:
   doAssert output == [1, 3, 6, 11, 20]
 
 
-# knownIssue: lambda-lifting related transformation are disabled when using
-#             the JS target, violating the expectation of ``vmgen`` and thus
-#             crashing the VM
-test array_of_procs, {c, vm}:
+block array_of_procs:
   # bug https://github.com/nim-lang/nim/issues/5015
 
   type Mutator = proc(matched: string): string {.noSideEffect, gcsafe, locks: 0.}

tests/lang_callable/closure/tclosure_issues.nim

@@ -62,8 +62,8 @@ proc foo(): proc =
 
 doAssert foo()() == (999, 0)
 
-# knownIssue: the JS code generator emits doesn't detect the inner procedures
-#             created by the lambda expressions as such
+# knownIssue: `i` is treated as a global defined at the procedure level, which
+#             are not properly supported by the JS backend yet
 test tissue7104, {c, vm}:
   var output: seq[int]