make sink operator optional (#13068)

* make sink operator optional

* bug fix, add changelog entry

* Trigger build

* fix one regression

* fix test

* Trigger build

* fix typos

changelog.md

@@ -75,6 +75,10 @@
 - An `align` pragma can now be used for variables and object fields, similar
   to the `alignas` declaration modifier in C/C++.
 
+- `=sink` type bound operator is now optional. Compiler can now use combination
+  of `=destroy` and `copyMem` to move objects efficiently.
+
+
 ## Language changes
 
 - Unsigned integer operators have been fixed to allow promotion of the first operand.

compiler/injectdestructors.nim

@@ -230,6 +230,10 @@ proc genOp(c: Con; t: PType; kind: TTypeAttachedOp; dest, ri: PNode): PNode =
   addrExp.add(dest)
   result = newTree(nkCall, newSymNode(op), addrExp)
 
+proc genDestroy(c: Con; dest: PNode): PNode =
+  let t = dest.typ.skipTypes({tyGenericInst, tyAlias, tySink})
+  result = genOp(c, t, attachedDestructor, dest, nil)
+
 when false:
   proc preventMoveRef(dest, ri: PNode): bool =
     let lhs = dest.typ.skipTypes({tyGenericInst, tyAlias, tySink})
@@ -248,18 +252,17 @@ proc canBeMoved(c: Con; t: PType): bool {.inline.} =
 
 proc genSink(c: var Con; dest, ri: PNode): PNode =
   if isFirstWrite(dest, c): # optimize sink call into a bitwise memcopy
-    result = newTree(nkFastAsgn, dest)
+    result = newTree(nkFastAsgn, dest, ri)
   else:
     let t = dest.typ.skipTypes({tyGenericInst, tyAlias, tySink})
-    let k = if t.attachedOps[attachedSink] != nil: attachedSink
-            else: attachedAsgn
-    if t.attachedOps[k] != nil:
-      result = genOp(c, t, k, dest, ri)
+    if t.attachedOps[attachedSink] != nil:
+      result = genOp(c, t, attachedSink, dest, ri)
+      result.add ri
     else:
-      # in rare cases only =destroy exists but no sink or assignment
-      # (see Pony object in tmove_objconstr.nim)
-      # we generate a fast assignment in this case:
-      result = newTree(nkFastAsgn, dest)
+      # the default is to use combination of `=destroy(dest)` and
+      # and copyMem(dest, source). This is efficient.
+      let snk = newTree(nkFastAsgn, dest, ri)
+      result = newTree(nkStmtList, genDestroy(c, dest), snk)
 
 proc genCopyNoCheck(c: Con; dest, ri: PNode): PNode =
   let t = dest.typ.skipTypes({tyGenericInst, tyAlias, tySink})
@@ -273,10 +276,6 @@ proc genCopy(c: var Con; dest, ri: PNode): PNode =
     checkForErrorPragma(c, t, ri, "=")
   result = genCopyNoCheck(c, dest, ri)
 
-proc genDestroy(c: Con; dest: PNode): PNode =
-  let t = dest.typ.skipTypes({tyGenericInst, tyAlias, tySink})
-  result = genOp(c, t, attachedDestructor, dest, nil)
-
 proc addTopVar(c: var Con; v: PNode) =
   c.topLevelVars.add newTree(nkIdentDefs, v, c.emptyNode, c.emptyNode)
 
@@ -298,8 +297,6 @@ proc genDefaultCall(t: PType; c: Con; info: TLineInfo): PNode =
 
 proc destructiveMoveVar(n: PNode; c: var Con): PNode =
   # generate: (let tmp = v; reset(v); tmp)
-  # XXX: Strictly speaking we can only move if there is a ``=sink`` defined
-  # or if no ``=sink`` is defined and also no assignment.
   if not hasDestructor(n.typ):
     result = copyTree(n)
   else:
@@ -440,9 +437,7 @@ proc ensureDestruction(arg: PNode; c: var Con): PNode =
     # 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 genSink(c, tmp, arg)
     result.add tmp
     c.destroys.add genDestroy(c, tmp)
   else:
@@ -598,8 +593,7 @@ proc p(n: PNode; c: var Con; mode: ProcessMode): PNode =
             if ri.kind == nkEmpty and c.inLoop > 0:
               ri = genDefaultCall(v.typ, c, v.info)
             if ri.kind != nkEmpty:
-              let r = moveOrCopy(v, ri, c)
-              result.add r
+              result.add moveOrCopy(v, ri, c)
         else: # keep the var but transform 'ri':
           var v = copyNode(n)
           var itCopy = copyNode(it)
@@ -667,8 +661,7 @@ proc moveOrCopy(dest, ri: PNode; c: var Con): PNode =
     if isUnpackedTuple(dest):
       result = newTree(nkFastAsgn, dest, p(ri, c, consumed))
     else:
-      result = genSink(c, dest, ri)
-      result.add p(ri, c, consumed)
+      result = genSink(c, dest, p(ri, c, consumed))
   of nkBracketExpr:
     if isUnpackedTuple(ri[0]):
       # unpacking of tuple: take over elements
@@ -677,7 +670,6 @@ proc moveOrCopy(dest, ri: PNode; c: var Con): PNode =
         not aliases(dest, ri):
       # Rule 3: `=sink`(x, z); wasMoved(z)
       var snk = genSink(c, dest, ri)
-      snk.add ri
       result = newTree(nkStmtList, snk, genWasMoved(ri, c))
     else:
       result = genCopy(c, dest, ri)
@@ -686,24 +678,21 @@ proc moveOrCopy(dest, ri: PNode; c: var Con): PNode =
     # array constructor
     if ri.len > 0 and isDangerousSeq(ri.typ):
       result = genCopy(c, dest, ri)
+      result.add p(ri, c, consumed)
     else:
-      result = genSink(c, dest, ri)
-    result.add p(ri, c, consumed)
+      result = genSink(c, dest, p(ri, c, consumed))
   of nkObjConstr, nkTupleConstr, nkClosure, nkCharLit..nkNilLit:
-    result = genSink(c, dest, ri)
-    result.add p(ri, c, consumed)
+    result = genSink(c, dest, p(ri, c, consumed))
   of nkSym:
     if isSinkParam(ri.sym):
       # Rule 3: `=sink`(x, z); wasMoved(z)
       sinkParamIsLastReadCheck(c, ri)
-      var snk = genSink(c, dest, ri)
-      snk.add ri
+      let snk = genSink(c, dest, ri)
       result = newTree(nkStmtList, snk, genWasMoved(ri, c))
     elif ri.sym.kind != skParam and ri.sym.owner == c.owner and
         isLastRead(ri, c) and canBeMoved(c, dest.typ):
       # Rule 3: `=sink`(x, z); wasMoved(z)
-      var snk = genSink(c, dest, ri)
-      snk.add ri
+      let snk = genSink(c, dest, ri)
       result = newTree(nkStmtList, snk, genWasMoved(ri, c))
     else:
       result = genCopy(c, dest, ri)
@@ -716,8 +705,7 @@ proc moveOrCopy(dest, ri: PNode; c: var Con): PNode =
         copyRi[1] = result[^1]
         result[^1] = copyRi
     else:
-      result = genSink(c, dest, ri)
-      result.add p(ri, c, sinkArg)
+      result = genSink(c, dest, p(ri, c, sinkArg))
   of nkObjDownConv, nkObjUpConv:
     when false:
       result = moveOrCopy(dest, ri[0], c)
@@ -725,16 +713,14 @@ proc moveOrCopy(dest, ri: PNode; c: var Con): PNode =
       copyRi[0] = result[^1]
       result[^1] = copyRi
     else:
-      result = genSink(c, dest, ri)
-      result.add p(ri, c, sinkArg)
+      result = genSink(c, dest, p(ri, c, sinkArg))
   of nkStmtListExpr, nkBlockExpr, nkIfExpr, nkCaseStmt:
     handleNested(ri): moveOrCopy(dest, node, c)
   else:
     if isAnalysableFieldAccess(ri, c.owner) and isLastRead(ri, c) and
         canBeMoved(c, dest.typ):
       # Rule 3: `=sink`(x, z); wasMoved(z)
-      var snk = genSink(c, dest, ri)
-      snk.add ri
+      let snk = genSink(c, dest, ri)
       result = newTree(nkStmtList, snk, genWasMoved(ri, c))
     else:
       result = genCopy(c, dest, ri)

compiler/liftdestructors.nim

@@ -775,18 +775,26 @@ proc produceSym(g: ModuleGraph; c: PContext; typ: PType; kind: TTypeAttachedOp;
   # register this operation already:
   typ.attachedOps[kind] = result
 
-  var tk: TTypeKind
-  if g.config.selectedGC in {gcArc, gcOrc, gcHooks}:
-    tk = skipTypes(typ, {tyOrdinal, tyRange, tyInferred, tyGenericInst, tyStatic, tyAlias, tySink}).kind
+
+  if kind == attachedSink and typ.attachedOps[attachedDestructor] != nil and
+       sfOverriden in typ.attachedOps[attachedDestructor].flags:
+    ## compiler can use a combination of `=destroy` and memCopy for sink op
+    dest.flags.incl sfCursor
+    body.add newOpCall(typ.attachedOps[attachedDestructor], d[0])
+    body.add newAsgnStmt(d, newSymNode(src))
   else:
-    tk = tyNone # no special casing for strings and seqs
-  case tk
-  of tySequence:
-    fillSeqOp(a, typ, body, d, newSymNode(src))
-  of tyString:
-    fillStrOp(a, typ, body, d, newSymNode(src))
-  else:
-    fillBody(a, typ, body, d, newSymNode(src))
+    var tk: TTypeKind
+    if g.config.selectedGC in {gcArc, gcOrc, gcHooks}:
+      tk = skipTypes(typ, {tyOrdinal, tyRange, tyInferred, tyGenericInst, tyStatic, tyAlias, tySink}).kind
+    else:
+      tk = tyNone # no special casing for strings and seqs
+    case tk
+    of tySequence:
+      fillSeqOp(a, typ, body, d, newSymNode(src))
+    of tyString:
+      fillStrOp(a, typ, body, d, newSymNode(src))
+    else:
+      fillBody(a, typ, body, d, newSymNode(src))
 
   var n = newNodeI(nkProcDef, info, bodyPos+1)
   for i in 0..<n.len: n[i] = newNodeI(nkEmpty, info)

doc/destructors.rst

@@ -53,7 +53,8 @@ written as:
         a.data[i] = b.data[i]
 
   proc `=sink`*[T](a: var myseq[T]; b: myseq[T]) =
-    # move assignment
+    # move assignment, optional.
+    # Compiler is using `=destroy` and `copyMem` when not provided
     `=destroy`(a)
     a.len = b.len
     a.cap = b.cap
@@ -130,7 +131,10 @@ A `=sink` hook moves an object around, the resources are stolen from the source
 and passed to the destination. It is ensured that source's destructor does
 not free the resources afterwards by setting the object to its default value
 (the value the object's state started in). Setting an object ``x`` back to its
-default value is written as ``wasMoved(x)``.
+default value is written as ``wasMoved(x)``. When not provided the compiler
+is using a combination of `=destroy` and `copyMem` instead. This is efficient
+hence users rarely need to implement their own `=sink` operator, it is enough to
+provide `=destroy` and `=`, compiler will take care about the rest.
 
 The prototype of this hook for a type ``T`` needs to be:
 

tests/destructor/smart_ptr.nim

@@ -20,12 +20,6 @@ proc `=`*[T](dest: var SharedPtr[T], src: SharedPtr[T]) {.inline.} =
       discard atomicInc(src.val[].atomicCounter)
     dest.val = src.val
 
-proc `=sink`*[T](dest: var SharedPtr[T], src: SharedPtr[T]) {.inline.} =
-  if dest.val != src.val:
-    if dest.val != nil:
-      `=destroy`(dest)
-    dest.val = src.val
-
 proc newSharedPtr*[T](val: sink T): SharedPtr[T] =
   result.val = cast[type(result.val)](allocShared0(sizeof(result.val[])))
   result.val.atomicCounter = 1

tests/destructor/tdestructor.nim

@@ -20,14 +20,10 @@ mygeneric2 destroyed
 ----
 ----
 myobj destroyed
-myobj destroyed
-myobj destroyed
-myobj destroyed
 mygeneric1 destroyed
 ---
 myobj destroyed
 myobj destroyed
-myobj destroyed
 '''
 """
 
@@ -37,8 +33,10 @@ type
     p: pointer
 
 proc `=destroy`(o: var TMyObj) =
-  if o.p != nil: dealloc o.p
-  echo "myobj destroyed"
+  if o.p != nil: 
+    dealloc o.p
+    o.p = nil
+    echo "myobj destroyed"
 
 type
   TMyGeneric1[T] = object