[bugfix] owned closures (#11544)

compiler/ast.nim

@@ -279,6 +279,8 @@ type
     sfNonReloadable   # symbol will be left as-is when hot code reloading is on -
                       # meaning that it won't be renamed and/or changed in any way
     sfGeneratedOp     # proc is a generated '='; do not inject destructors in it
+                      # variable is generated closure environment; requires early
+                      # destruction for --newruntime.
 
 
   TSymFlags* = set[TSymFlag]

compiler/ccgexprs.nim

@@ -360,7 +360,7 @@ proc genAssignment(p: BProc, dest, src: TLoc, flags: TAssignmentFlags) =
     else:
       linefmt(p, cpsStmts, "$1 = $2;$n", [rdLoc(dest), rdLoc(src)])
   of tyPtr, tyPointer, tyChar, tyBool, tyEnum, tyCString,
-     tyInt..tyUInt64, tyRange, tyVar, tyLent:
+     tyInt..tyUInt64, tyRange, tyVar, tyLent, tyNil:
     linefmt(p, cpsStmts, "$1 = $2;$n", [rdLoc(dest), rdLoc(src)])
   else: internalError(p.config, "genAssignment: " & $ty.kind)
 

compiler/dfa.nim

@@ -619,8 +619,8 @@ proc aliases(obj, field: PNode): bool =
       break
   return false
 
-proc instrTargets*(ins: Instr; loc: PNode): bool =
+proc useInstrTargets*(ins: Instr; loc: PNode): bool =
-  assert ins.kind in {def, use}
+  assert ins.kind == use
   if ins.sym != nil and loc.kind == nkSym:
     result = ins.sym == loc.sym
   else:
@@ -633,6 +633,20 @@ proc instrTargets*(ins: Instr; loc: PNode): bool =
     # use x; question does it affect 'x.f'? Yes.
     result = aliases(ins.n, loc) or aliases(loc, ins.n)
 
+proc defInstrTargets*(ins: Instr; loc: PNode): bool =
+  assert ins.kind == def
+  if ins.sym != nil and loc.kind == nkSym:
+    result = ins.sym == loc.sym
+  else:
+    result = ins.n == loc or sameTrees(ins.n, loc)
+  if not result:
+    # We can come here if loc is 'x.f' and ins.n is 'x' or the other way round.
+    # def x.f; question: does it affect the full 'x'? No.
+    # def x; question: does it affect the 'x.f'? Yes.
+    # use x.f;  question: does it affect the full 'x'? No.
+    # use x; question does it affect 'x.f'? Yes.
+    result = aliases(ins.n, loc)
+
 proc isAnalysableFieldAccess*(orig: PNode; owner: PSym): bool =
   var n = orig
   while true:

compiler/injectdestructors.nim

@@ -165,12 +165,12 @@ proc isLastRead(location: PNode; c: var Con; pc, comesFrom: int): int =
   while pc < c.g.len:
     case c.g[pc].kind
     of def:
-      if instrTargets(c.g[pc], location):
+      if defInstrTargets(c.g[pc], location):
         # the path lead to a redefinition of 's' --> abandon it.
         return high(int)
       inc pc
     of use:
-      if instrTargets(c.g[pc], location):
+      if useInstrTargets(c.g[pc], location):
         c.otherRead = c.g[pc].n
         return -1
       inc pc
@@ -349,8 +349,10 @@ proc genSink(c: Con; t: PType; dest, ri: PNode): PNode =
     # we generate a fast assignment in this case:
     result = newTree(nkFastAsgn, dest)
 
-proc genCopy(c: Con; t: PType; dest, ri: PNode): PNode =
+proc genCopy(c: var Con; t: PType; dest, ri: PNode): PNode =
   if tfHasOwned in t.flags:
+    # try to improve the error message here:
+    if c.otherRead == nil: discard isLastRead(ri, c)
     checkForErrorPragma(c, t, ri, "=")
   let t = t.skipTypes({tyGenericInst, tyAlias, tySink})
   result = genOp(c, t, attachedAsgn, dest, ri)
@@ -410,7 +412,7 @@ proc destructiveMoveVar(n: PNode; c: var Con): PNode =
   add(v, vpart)
 
   result.add v
-  result.add genWasMoved(n, c)
+  result.add genWasMoved(skipConv(n), c)
   result.add tempAsNode
 
 proc sinkParamIsLastReadCheck(c: var Con, s: PNode) =
@@ -611,7 +613,7 @@ proc moveOrCopy(dest, ri: PNode; c: var Con): PNode =
   of nkTupleConstr, nkClosure:
     result = genSink(c, dest.typ, dest, ri)
     let ri2 = copyTree(ri)
-    for i in 0..<ri.len:
+    for i in ord(ri.kind == nkClosure)..<ri.len:
       # everything that is passed to an tuple constructor is consumed,
       # so these all act like 'sink' parameters:
       if ri[i].kind == nkExprColonExpr:
@@ -757,7 +759,7 @@ proc p(n: PNode; c: var Con): PNode =
     else:
       result = n
   of nkAsgn, nkFastAsgn:
-    if hasDestructor(n[0].typ) and n[1].kind notin {nkProcDef, nkDo, nkLambda, nkClosure}:
+    if hasDestructor(n[0].typ) and n[1].kind notin {nkProcDef, nkDo, nkLambda}:
       result = moveOrCopy(n[0], n[1], c)
     else:
       result = copyNode(n)

compiler/lambdalifting.nim

@@ -324,6 +324,7 @@ proc asOwnedRef(c: DetectionPass; t: PType): PType =
   if optNimV2 in c.graph.config.globalOptions:
     assert t.kind == tyRef
     result = newType(tyOwned, t.owner)
+    result.flags.incl tfHasOwned
     result.rawAddSon t
   else:
     result = t
@@ -490,6 +491,7 @@ type
     processed: IntSet
     envVars: Table[int, PNode]
     inContainer: int
+    unownedEnvVars: Table[int, PNode] # only required for --newruntime
 
 proc initLiftingPass(fn: PSym): LiftingPass =
   result.processed = initIntSet()
@@ -514,9 +516,9 @@ proc accessViaEnvParam(g: ModuleGraph; n: PNode; owner: PSym): PNode =
   localError(g.config, n.info, "internal error: environment misses: " & s.name.s)
   result = n
 
-proc newEnvVar(cache: IdentCache; owner: PSym; typ: PType): PNode =
+proc newEnvVar(cache: IdentCache; owner: PSym; typ: PType; info: TLineInfo): PNode =
-  var v = newSym(skVar, getIdent(cache, envName), owner, owner.info)
+  var v = newSym(skVar, getIdent(cache, envName), owner, info)
-  incl(v.flags, sfShadowed)
+  v.flags = {sfShadowed, sfGeneratedOp}
   v.typ = typ
   result = newSymNode(v)
   when false:
@@ -528,7 +530,7 @@ proc newEnvVar(cache: IdentCache; owner: PSym; typ: PType): PNode =
       result = newSymNode(v)
 
 proc setupEnvVar(owner: PSym; d: DetectionPass;
-                 c: var LiftingPass): PNode =
+                 c: var LiftingPass; info: TLineInfo): PNode =
   if owner.isIterator:
     return getHiddenParam(d.graph, owner).newSymNode
   result = c.envvars.getOrDefault(owner.id)
@@ -536,8 +538,13 @@ proc setupEnvVar(owner: PSym; d: DetectionPass;
     let envVarType = d.ownerToType.getOrDefault(owner.id)
     if envVarType.isNil:
       localError d.graph.config, owner.info, "internal error: could not determine closure type"
-    result = newEnvVar(d.graph.cache, owner, asOwnedRef(d, envVarType))
+    result = newEnvVar(d.graph.cache, owner, asOwnedRef(d, envVarType), info)
     c.envVars[owner.id] = result
+    if optNimV2 in d.graph.config.globalOptions:
+      var v = newSym(skVar, getIdent(d.graph.cache, envName & "Alt"), owner, info)
+      v.flags = {sfShadowed, sfGeneratedOp}
+      v.typ = envVarType
+      c.unownedEnvVars[owner.id] = newSymNode(v)
 
 proc getUpViaParam(g: ModuleGraph; owner: PSym): PNode =
   let p = getHiddenParam(g, owner)
@@ -550,20 +557,34 @@ proc getUpViaParam(g: ModuleGraph; owner: PSym): PNode =
       result = rawIndirectAccess(result, upField, p.info)
 
 proc rawClosureCreation(owner: PSym;
-                        d: DetectionPass; c: var LiftingPass): PNode =
+                        d: DetectionPass; c: var LiftingPass;
+                        info: TLineInfo): PNode =
   result = newNodeI(nkStmtList, owner.info)
 
   var env: PNode
   if owner.isIterator:
     env = getHiddenParam(d.graph, owner).newSymNode
   else:
-    env = setupEnvVar(owner, d, c)
+    env = setupEnvVar(owner, d, c, info)
     if env.kind == nkSym:
       var v = newNodeI(nkVarSection, env.info)
       addVar(v, env)
       result.add(v)
+      if optNimV2 in d.graph.config.globalOptions:
+        let unowned = c.unownedEnvVars[owner.id]
+        assert unowned != nil
+        addVar(v, unowned)
+
     # add 'new' statement:
     result.add(newCall(getSysSym(d.graph, env.info, "internalNew"), env))
+    if optNimV2 in d.graph.config.globalOptions:
+      let unowned = c.unownedEnvVars[owner.id]
+      assert unowned != nil
+      let env2 = copyTree(env)
+      env2.typ = unowned.typ
+      result.add newAsgnStmt(unowned, env2, env.info)
+      createTypeBoundOps(d.graph, nil, unowned.typ, env.info)
+
     # add assignment statements for captured parameters:
     for i in 1..<owner.typ.n.len:
       let local = owner.typ.n[i].sym
@@ -585,9 +606,16 @@ proc rawClosureCreation(owner: PSym;
       localError(d.graph.config, env.info, "internal error: cannot create up reference")
   # we are not in the sem'check phase anymore! so pass 'nil' for the PContext
   # and hope for the best:
-  when false:
+  if optNimV2 in d.graph.config.globalOptions:
-    if optNimV2 in d.graph.config.globalOptions:
+    createTypeBoundOps(d.graph, nil, env.typ, owner.info)
-      createTypeBoundOps(d.graph, nil, env.typ, owner.info)
+
+proc finishClosureCreation(owner: PSym; d: DetectionPass; c: LiftingPass;
+                           info: TLineInfo; res: PNode) =
+  if optNimV2 in d.graph.config.globalOptions:
+    let unowned = c.unownedEnvVars[owner.id]
+    assert unowned != nil
+    let nilLit = newNodeIT(nkNilLit, info, unowned.typ)
+    res.add newAsgnStmt(unowned, nilLit, info)
 
 proc closureCreationForIter(iter: PNode;
                             d: DetectionPass; c: var LiftingPass): PNode =
@@ -610,7 +638,7 @@ proc closureCreationForIter(iter: PNode;
 
   let upField = lookupInRecord(v.typ.skipTypes({tyOwned, tyRef}).n, getIdent(d.graph.cache, upName))
   if upField != nil:
-    let u = setupEnvVar(owner, d, c)
+    let u = setupEnvVar(owner, d, c, iter.info)
     if u.typ.skipTypes({tyOwned, tyRef}) == upField.typ.skipTypes({tyOwned, tyRef}):
       result.add(newAsgnStmt(rawIndirectAccess(vnode, upField, iter.info),
                  u, iter.info))
@@ -620,7 +648,9 @@ proc closureCreationForIter(iter: PNode;
 
 proc accessViaEnvVar(n: PNode; owner: PSym; d: DetectionPass;
                      c: var LiftingPass): PNode =
-  let access = setupEnvVar(owner, d, c)
+  var access = setupEnvVar(owner, d, c, n.info)
+  if optNimV2 in d.graph.config.globalOptions:
+    access = c.unownedEnvVars[owner.id]
   let obj = access.typ.skipTypes({tyOwned, tyRef})
   let field = getFieldFromObj(obj, n.sym)
   if field != nil:
@@ -646,7 +676,7 @@ proc symToClosure(n: PNode; owner: PSym; d: DetectionPass;
     result = closureCreationForIter(n, d, c)
   elif s.skipGenericOwner == owner:
     # direct dependency, so use the outer's env variable:
-    result = makeClosure(d.graph, s, setupEnvVar(owner, d, c), n.info)
+    result = makeClosure(d.graph, s, setupEnvVar(owner, d, c, n.info), n.info)
   else:
     let available = getHiddenParam(d.graph, owner)
     let wanted = getHiddenParam(d.graph, s).typ
@@ -679,7 +709,8 @@ proc liftCapturedVars(n: PNode; owner: PSym; d: DetectionPass;
         if c.envvars.getOrDefault(s.id).isNil:
           s.transformedBody = body
         else:
-          s.transformedBody = newTree(nkStmtList, rawClosureCreation(s, d, c), body)
+          s.transformedBody = newTree(nkStmtList, rawClosureCreation(s, d, c, n.info), body)
+          finishClosureCreation(s, d, c, n.info, s.transformedBody)
         c.inContainer = oldInContainer
 
       if s.typ.callConv == ccClosure:
@@ -816,7 +847,8 @@ proc liftLambdas*(g: ModuleGraph; fn: PSym, body: PNode; tooEarly: var bool;
       result = liftCapturedVars(body, fn, d, c)
       # echo renderTree(result, {renderIds})
       if c.envvars.getOrDefault(fn.id) != nil:
-        result = newTree(nkStmtList, rawClosureCreation(fn, d, c), result)
+        result = newTree(nkStmtList, rawClosureCreation(fn, d, c, body.info), result)
+        finishClosureCreation(fn, d, c, body.info, result)
     else:
       result = body
     #if fn.name.s == "get2":

compiler/liftdestructors.nim

@@ -363,7 +363,13 @@ proc weakrefOp(c: var TLiftCtx; t: PType; body, x, y: PNode) =
     body.add genIf(c, x, callCodegenProc(c.g, "nimDecWeakRef", c.info, x))
     body.add newAsgnStmt(x, y)
   of attachedDestructor:
-    body.add genIf(c, x, callCodegenProc(c.g, "nimDecWeakRef", c.info, x))
+    # it's better to prepend the destruction of weak refs in order to
+    # prevent wrong "dangling refs exist" problems:
+    let des = genIf(c, x, callCodegenProc(c.g, "nimDecWeakRef", c.info, x))
+    if body.len == 0:
+      body.add des
+    else:
+      body.sons.insert(des, 0)
   of attachedDeepCopy: assert(false, "cannot happen")
 
 proc ownedRefOp(c: var TLiftCtx; t: PType; body, x, y: PNode) =
@@ -413,7 +419,11 @@ proc closureOp(c: var TLiftCtx; t: PType; body, x, y: PNode) =
       body.add genIf(c, xx, callCodegenProc(c.g, "nimDecWeakRef", c.info, xx))
       body.add newAsgnStmt(x, y)
     of attachedDestructor:
-      body.add genIf(c, xx, callCodegenProc(c.g, "nimDecWeakRef", c.info, xx))
+      let des = genIf(c, xx, callCodegenProc(c.g, "nimDecWeakRef", c.info, xx))
+      if body.len == 0:
+        body.add des
+      else:
+        body.sons.insert(des, 0)
     of attachedDeepCopy: assert(false, "cannot happen")
 
 proc ownedClosureOp(c: var TLiftCtx; t: PType; body, x, y: PNode) =

compiler/lowerings.nim

@@ -155,6 +155,7 @@ proc rawAddField*(obj: PType; field: PSym) =
   assert field.kind == skField
   field.position = sonsLen(obj.n)
   addSon(obj.n, newSymNode(field))
+  propagateToOwner(obj, field.typ)
 
 proc rawIndirectAccess*(a: PNode; field: PSym; info: TLineInfo): PNode =
   # returns a[].field as a node
@@ -205,6 +206,7 @@ proc addField*(obj: PType; s: PSym; cache: IdentCache) =
   let t = skipIntLit(s.typ)
   field.typ = t
   assert t.kind != tyTyped
+  propagateToOwner(obj, t)
   field.position = sonsLen(obj.n)
   addSon(obj.n, newSymNode(field))
 
@@ -217,6 +219,7 @@ proc addUniqueField*(obj: PType; s: PSym; cache: IdentCache): PSym {.discardable
     let t = skipIntLit(s.typ)
     field.typ = t
     assert t.kind != tyTyped
+    propagateToOwner(obj, t)
     field.position = sonsLen(obj.n)
     addSon(obj.n, newSymNode(field))
     result = field

lib/core/runtime_v2.nim

@@ -78,6 +78,13 @@ proc nimRawDispose(p: pointer) {.compilerRtl.} =
 proc nimDestroyAndDispose(p: pointer) {.compilerRtl.} =
   let d = cast[ptr PNimType](p)[].destructor
   if d != nil: cast[DestructorProc](d)(p)
+  when false:
+    cstderr.rawWrite cast[ptr PNimType](p)[].name
+    cstderr.rawWrite "\n"
+    if d == nil:
+      cstderr.rawWrite "bah, nil\n"
+    else:
+      cstderr.rawWrite "has destructor!\n"
   nimRawDispose(p)
 
 proc isObj(obj: PNimType, subclass: cstring): bool {.compilerproc.} =

tests/destructor/tuse_ownedref_after_move.nim

@@ -1,6 +1,6 @@
 discard """
   cmd: '''nim c --newruntime $file'''
-  errormsg: "'=' is not available for type <owned Widget>; requires a copy because it's not the last read of ':env.b1()'; another read is done here: tuse_ownedref_after_move.nim(53, 4)"
+  errormsg: "'=' is not available for type <owned Widget>; requires a copy because it's not the last read of ':envAlt.b1()'; another read is done here: tuse_ownedref_after_move.nim(53, 4)"
   line: 49
 """
 

tests/destructor/twidgets.nim

@@ -63,7 +63,7 @@ proc main =
   var b = newButton("button", nil)
   let u: Button = b
   b.onclick = proc () =
-    b.caption = "clicked!"
+    u.caption = "clicked!"
   w.add b
 
   w.draw()

tests/destructor/twidgets_unown.nim

@@ -2,7 +2,7 @@ discard """
   cmd: '''nim c --newruntime $file'''
   output: '''button
 clicked!
-3 3  alloc/dealloc pairs: 0'''
+6 6  alloc/dealloc pairs: 0'''
 """
 
 import core / allocators
@@ -52,8 +52,10 @@ proc main =
 
   var b = newButton("button", nil)
   let u = unown b
+  var clicked = "clicked"
   b.onclick = proc () =
-    b.caption = "clicked!"
+    clicked.add "!"
+    u.caption = clicked
   w.add b
 
   w.draw()