ARC: closure bugfixes (#12677)

* ARC: closure bugfixes
* progress
* ARC closures: create =hooks for captured parameters
* ARC: always destroy constructions like tuples, arrays properly, even in edge cases
* fixes a regression

compiler/injectdestructors.nim

@@ -271,7 +271,7 @@ proc sinkParamIsLastReadCheck(c: var Con, s: PNode) =
      localError(c.graph.config, c.otherRead.info, "sink parameter `" & $s.sym.name.s &
          "` is already consumed at " & toFileLineCol(c. graph.config, s.info))
 
-proc p(n: PNode; c: var Con): PNode
+proc p(n: PNode; c: var Con; consumed = false): PNode
 proc pArg(arg: PNode; c: var Con; isSink: bool): PNode
 proc moveOrCopy(dest, ri: PNode; c: var Con): PNode
 
@@ -366,6 +366,22 @@ template handleNested(n: untyped, processCall: untyped) =
       result.add branch
   else: assert(false)
 
+proc ensureDestruction(arg: PNode; c: var Con): PNode =
+  # it can happen that we need to destroy expression contructors
+  # like [], (), closures explicitly in order to not leak them.
+  if arg.typ != nil and hasDestructor(arg.typ):
+    # produce temp creation for (fn, env). But we need to move 'env'?
+    # This was already done in the sink parameter handling logic.
+    result = newNodeIT(nkStmtListExpr, arg.info, arg.typ)
+    let tmp = getTemp(c, arg.typ, arg.info)
+    var sinkExpr = genSink(c, tmp, arg)
+    sinkExpr.add arg
+    result.add sinkExpr
+    result.add tmp
+    c.destroys.add genDestroy(c, tmp)
+  else:
+    result = arg
+
 proc pArg(arg: PNode; c: var Con; isSink: bool): PNode =
   if isSink:
     if arg.kind in nkCallKinds:
@@ -374,7 +390,7 @@ proc pArg(arg: PNode; c: var Con; isSink: bool): PNode =
       result = copyNode(arg)
       let parameters = arg[0].typ
       let L = if parameters != nil: parameters.len else: 0
-      result.add arg[0]
+      result.add pArg(arg[0], c, isSink = false)
       for i in 1..<arg.len:
         result.add pArg(arg[i], c, i < L and isSinkTypeForParam(parameters[i]))
     elif arg.containsConstSeq:
@@ -392,6 +408,9 @@ proc pArg(arg: PNode; c: var Con; isSink: bool): PNode =
           result[i][1] = pArg(arg[i][1], c, isSink = true)
         else:
           result[i] = pArg(arg[i], c, isSink = true)
+      #if arg.kind == nkClosure:
+      #  result = handleClosureCall(result, c)
+      # Not required here because the nkClosure will be consumed!
     elif arg.kind == nkSym and isSinkParam(arg.sym):
       # Sinked params can be consumed only once. We need to reset the memory
       # to disable the destructor which we have not elided
@@ -442,7 +461,7 @@ proc isCursor(n: PNode): bool =
   else:
     result = false
 
-proc p(n: PNode; c: var Con): PNode =
+proc p(n: PNode; c: var Con; consumed = false): PNode =
   case n.kind
   of nkCallKinds:
     let parameters = n[0].typ
@@ -454,6 +473,8 @@ proc p(n: PNode; c: var Con): PNode =
       if c.graph.config.selectedGC in {gcHooks, gcDestructors}:
         let destroyOld = genDestroy(c, result[1])
         result = newTree(nkStmtList, destroyOld, result)
+    else:
+      result[0] = pArg(result[0], c, isSink = false)
   of nkDiscardStmt: # Small optimization
     if n[0].kind != nkEmpty:
       n[0] = pArg(n[0], c, false)
@@ -464,12 +485,16 @@ proc p(n: PNode; c: var Con): PNode =
       # everything that is passed to an array constructor is consumed,
       # so these all act like 'sink' parameters:
       result[i] = pArg(n[i], c, isSink = true)
+    if not consumed:
+      result = ensureDestruction(result, c)
   of nkObjConstr:
     result = copyTree(n)
     for i in 1..<n.len:
       # everything that is passed to an object constructor is consumed,
       # so these all act like 'sink' parameters:
       result[i][1] = pArg(n[i][1], c, isSink = true)
+    if not consumed:
+      result = ensureDestruction(result, c)
   of nkTupleConstr, nkClosure:
     result = copyTree(n)
     for i in ord(n.kind == nkClosure)..<n.len:
@@ -479,6 +504,8 @@ proc p(n: PNode; c: var Con): PNode =
         result[i][1] = pArg(n[i][1], c, isSink = true)
       else:
         result[i] = pArg(n[i], c, isSink = true)
+    if not consumed:
+      result = ensureDestruction(result, c)
   of nkVarSection, nkLetSection:
     # transform; var x = y to  var x; x op y  where op is a move or copy
     result = newNodeI(nkStmtList, n.info)
@@ -556,11 +583,11 @@ proc p(n: PNode; c: var Con): PNode =
     result = copyTree(n)
     result[1][0] = p(result[1][0], c)
   of nkStmtList, nkStmtListExpr, nkBlockStmt, nkBlockExpr, nkIfStmt, nkIfExpr, nkCaseStmt:
-    handleNested(n): p(node, c)
+    handleNested(n): p(node, c, consumed)
   else:
     result = shallowCopy(n)
     for i in 0..<n.len:
-      result[i] = p(n[i], c)
+      result[i] = p(n[i], c, consumed)
 
 proc moveOrCopy(dest, ri: PNode; c: var Con): PNode =
   # unfortunately, this needs to be kept consistent with the cases
@@ -571,12 +598,12 @@ proc moveOrCopy(dest, ri: PNode; c: var Con): PNode =
   case ri.kind
   of nkCallKinds:
     result = genSink(c, dest, ri)
-    result.add p(ri, c)
+    result.add p(ri, c, consumed = true)
   of nkBracketExpr:
     if ri[0].kind == nkSym and isUnpackedTuple(ri[0].sym):
       # unpacking of tuple: move out the elements
       result = genSink(c, dest, ri)
-      result.add p(ri, c)
+      result.add p(ri, c, consumed = true)
     elif isAnalysableFieldAccess(ri, c.owner) and isLastRead(ri, c):
       # Rule 3: `=sink`(x, z); wasMoved(z)
       var snk = genSink(c, dest, ri)
@@ -584,17 +611,17 @@ proc moveOrCopy(dest, ri: PNode; c: var Con): PNode =
       result = newTree(nkStmtList, snk, genWasMoved(ri, c))
     else:
       result = genCopy(c, dest, ri)
-      result.add p(ri, c)
+      result.add p(ri, c, consumed = true)
   of nkBracket:
     # array constructor
     if ri.len > 0 and isDangerousSeq(ri.typ):
       result = genCopy(c, dest, ri)
     else:
       result = genSink(c, dest, ri)
-    result.add p(ri, c)
+    result.add p(ri, c, consumed = true)
   of nkObjConstr, nkTupleConstr, nkClosure, nkCharLit..nkNilLit:
     result = genSink(c, dest, ri)
-    result.add p(ri, c)
+    result.add p(ri, c, consumed = true)
   of nkSym:
     if isSinkParam(ri.sym):
       # Rule 3: `=sink`(x, z); wasMoved(z)
@@ -610,7 +637,7 @@ proc moveOrCopy(dest, ri: PNode; c: var Con): PNode =
       result = newTree(nkStmtList, snk, genWasMoved(ri, c))
     else:
       result = genCopy(c, dest, ri)
-      result.add p(ri, c)
+      result.add p(ri, c, consumed = true)
   of nkHiddenSubConv, nkHiddenStdConv, nkConv:
     result = moveOrCopy(dest, ri[1], c)
     if not sameType(ri.typ, ri[1].typ):
@@ -633,7 +660,7 @@ proc moveOrCopy(dest, ri: PNode; c: var Con): PNode =
       result = newTree(nkStmtList, snk, genWasMoved(ri, c))
     else:
       result = genCopy(c, dest, ri)
-      result.add p(ri, c)
+      result.add p(ri, c, consumed = true)
 
 proc computeUninit(c: var Con) =
   if not c.uninitComputed:

compiler/lambdalifting.nim

@@ -218,6 +218,10 @@ proc makeClosure*(g: ModuleGraph; prc: PSym; env: PNode; info: TLineInfo): PNode
     if env.skipConv.kind == nkClosure:
       localError(g.config, info, "internal error: taking closure of closure")
     result.add(env)
+  #if isClosureIterator(result.typ):
+  createTypeBoundOps(g, nil, result.typ, info)
+  if tfHasAsgn in result.typ.flags or optSeqDestructors in g.config.globalOptions:
+    prc.flags.incl sfInjectDestructors
 
 proc interestingIterVar(s: PSym): bool {.inline.} =
   # XXX optimization: Only lift the variable if it lives across
@@ -236,8 +240,7 @@ proc liftingHarmful(conf: ConfigRef; owner: PSym): bool {.inline.} =
 proc createTypeBoundOpsLL(g: ModuleGraph; refType: PType; info: TLineInfo; owner: PSym) =
   createTypeBoundOps(g, nil, refType.lastSon, info)
   createTypeBoundOps(g, nil, refType, info)
-  if (tfHasAsgn in refType.flags) or
+  if tfHasAsgn in refType.flags or optSeqDestructors in g.config.globalOptions:
-      optSeqDestructors in g.config.globalOptions:
     owner.flags.incl sfInjectDestructors
 
 proc liftIterSym*(g: ModuleGraph; n: PNode; owner: PSym): PNode =
@@ -604,6 +607,9 @@ proc rawClosureCreation(owner: PSym;
         let fieldAccess = indirectAccess(env, local, env.info)
         # add ``env.param = param``
         result.add(newAsgnStmt(fieldAccess, newSymNode(local), env.info))
+        createTypeBoundOps(d.graph, nil, fieldAccess.typ, env.info)
+        if tfHasAsgn in fieldAccess.typ.flags or optSeqDestructors in d.graph.config.globalOptions:
+          owner.flags.incl sfInjectDestructors
 
   let upField = lookupInRecord(env.typ.skipTypes({tyOwned, tyRef, tyPtr}).n, getIdent(d.graph.cache, upName))
   if upField != nil:
@@ -627,6 +633,7 @@ proc finishClosureCreation(owner: PSym; d: DetectionPass; c: LiftingPass;
     assert unowned != nil
     let nilLit = newNodeIT(nkNilLit, info, unowned.typ)
     res.add newAsgnStmt(unowned, nilLit, info)
+    createTypeBoundOpsLL(d.graph, unowned.typ, info, owner)
 
 proc closureCreationForIter(iter: PNode;
                             d: DetectionPass; c: var LiftingPass): PNode =

compiler/sempass2.nim

@@ -709,11 +709,11 @@ proc track(tracked: PEffects, n: PNode) =
     # p's effects are ours too:
     var a = n.sons[0]
     let op = a.typ
-    if getConstExpr(tracked.ownerModule, n, tracked.graph) != nil:
-      return
     if n.typ != nil:
       if tracked.owner.kind != skMacro and n.typ.skipTypes(abstractVar).kind != tyOpenArray:
         createTypeBoundOps(tracked, n.typ, n.info)
+    if getConstExpr(tracked.ownerModule, n, tracked.graph) != nil:
+      return
     if a.kind == nkCast and a[1].typ.kind == tyProc:
       a = a[1]
     # XXX: in rare situations, templates and macros will reach here after
@@ -883,6 +883,9 @@ proc track(tracked: PEffects, n: PNode) =
     if n.len == 2: track(tracked, n.sons[1])
   of nkObjUpConv, nkObjDownConv, nkChckRange, nkChckRangeF, nkChckRange64:
     if n.len == 1: track(tracked, n.sons[0])
+  of nkBracket:
+    for i in 0 ..< safeLen(n): track(tracked, n.sons[i])
+    createTypeBoundOps(tracked, n.typ, n.info)
   else:
     for i in 0 ..< safeLen(n): track(tracked, n.sons[i])
 

tests/destructor/tlists.nim

@@ -51,7 +51,26 @@ proc tleakingNewStmt =
   for i in 0..10:
     new(x)
 
+iterator infinite(): int {.closure.} =
+  var i = 0
+  while true:
+    yield i
+    inc i
+
+iterator take(it: iterator (): int, numToTake: int): int {.closure.} =
+  var i = 0
+  for x in it():
+    if i >= numToTake:
+      break
+    yield x
+    inc i
+
+proc take3 =
+  for x in infinite.take(3):
+    discard
+
 let startMem = getOccupiedMem()
+take3()
 tlazyList()
 
 mkManyLeaks()