allow destructors to accept non var parameters; deprecate proc =destroy(x: var T) (#22130)

* make destructors accept non var parameters
* define nimAllowNonVarDestructor
* add a test case and a changelog
* update documentation and error messages
* deprecate destructors taking 'var T'

changelogs/changelog_2_0_0.md

@@ -258,6 +258,8 @@
 
 - `strutils.split` and `strutils.rsplit` now forbid an empty separator.
 
+- Custom destructors now supports non-var parameters, e.g. `proc =destroy[T: object](x: T)` is valid. `proc =destroy[T: object](x: var T)` is deprecated.
+
 - Relative imports will not resolve to searched paths anymore, e.g. `import ./tables` now reports an error properly.
 
 ## Standard library additions and changes

compiler/condsyms.nim

@@ -155,3 +155,4 @@ proc initDefines*(symbols: StringTableRef) =
   defineSymbol("nimHasDup")
   defineSymbol("nimHasChecksums")
   defineSymbol("nimHasSendable")
+  defineSymbol("nimAllowNonVarDestructor")

compiler/injectdestructors.nim

@@ -213,8 +213,12 @@ proc makePtrType(c: var Con, baseType: PType): PType =
   addSonSkipIntLit(result, baseType, c.idgen)
 
 proc genOp(c: var Con; op: PSym; dest: PNode): PNode =
-  let addrExp = newNodeIT(nkHiddenAddr, dest.info, makePtrType(c, dest.typ))
-  addrExp.add(dest)
+  var addrExp: PNode
+  if op.typ != nil and op.typ.len > 1 and op.typ[1].kind != tyVar:
+    addrExp = dest
+  else:
+    addrExp = newNodeIT(nkHiddenAddr, dest.info, makePtrType(c, dest.typ))
+    addrExp.add(dest)
   result = newTree(nkCall, newSymNode(op), addrExp)
 
 proc genOp(c: var Con; t: PType; kind: TTypeAttachedOp; dest, ri: PNode): PNode =

compiler/liftdestructors.nim

@@ -140,7 +140,10 @@ proc genContainerOf(c: var TLiftCtx; objType: PType, field, x: PSym): PNode =
 proc destructorCall(c: var TLiftCtx; op: PSym; x: PNode): PNode =
   var destroy = newNodeIT(nkCall, x.info, op.typ[0])
   destroy.add(newSymNode(op))
-  destroy.add genAddr(c, x)
+  if op.typ[1].kind != tyVar:
+    destroy.add x
+  else:
+    destroy.add genAddr(c, x)
   if sfNeverRaises notin op.flags:
     c.canRaise = true
   if c.addMemReset:
@@ -1115,7 +1118,7 @@ proc produceSym(g: ModuleGraph; c: PContext; typ: PType; kind: TTypeAttachedOp;
                    fn: result)
 
   let dest = if kind == attachedDup: result.ast[resultPos].sym else: result.typ.n[1].sym
-  let d = if kind == attachedDup: newSymNode(dest) else: newDeref(newSymNode(dest))
+  let d = if result.typ[1].kind == tyVar: newDeref(newSymNode(dest)) else: newSymNode(dest)
   let src = case kind
             of {attachedDestructor, attachedWasMoved}: newNodeIT(nkSym, info, getSysType(g, info, tyPointer))
             of attachedDup: newSymNode(result.typ.n[1].sym)
@@ -1127,7 +1130,8 @@ proc produceSym(g: ModuleGraph; c: PContext; typ: PType; kind: TTypeAttachedOp;
   if kind == attachedSink and destructorOverridden(g, typ):
     ## compiler can use a combination of `=destroy` and memCopy for sink op
     dest.flags.incl sfCursor
-    result.ast[bodyPos].add newOpCall(a, getAttachedOp(g, typ, attachedDestructor), d[0])
+    let op = getAttachedOp(g, typ, attachedDestructor)
+    result.ast[bodyPos].add newOpCall(a, op, if op.typ[1].kind == tyVar: d[0] else: d)
     result.ast[bodyPos].add newAsgnStmt(d, src)
   else:
     var tk: TTypeKind

compiler/semstmts.nim

@@ -1872,7 +1872,7 @@ proc bindTypeHook(c: PContext; s: PSym; n: PNode; op: TTypeAttachedOp) =
   let t = s.typ
   var noError = false
   let cond = case op
-             of {attachedDestructor, attachedWasMoved}:
+             of attachedWasMoved:
                t.len == 2 and t[0] == nil and t[1].kind == tyVar
              of attachedTrace:
                t.len == 3 and t[0] == nil and t[1].kind == tyVar and t[2].kind == tyPointer
@@ -1887,6 +1887,8 @@ proc bindTypeHook(c: PContext; s: PSym; n: PNode; op: TTypeAttachedOp) =
       elif obj.kind == tyGenericInvocation: obj = obj[0]
       else: break
     if obj.kind in {tyObject, tyDistinct, tySequence, tyString}:
+      if op == attachedDestructor and t[1].kind == tyVar:
+        message(c.config, n.info, warnDeprecated, "A custom '=destroy' hook which takes a 'var T' parameter is deprecated; it should take a 'T' parameter")
       obj = canonType(c, obj)
       let ao = getAttachedOp(c.graph, obj, op)
       if ao == s:
@@ -1904,6 +1906,9 @@ proc bindTypeHook(c: PContext; s: PSym; n: PNode; op: TTypeAttachedOp) =
     of attachedTrace:
       localError(c.config, n.info, errGenerated,
         "signature for '=trace' must be proc[T: object](x: var T; env: pointer)")
+    of attachedDestructor:
+      localError(c.config, n.info, errGenerated,
+        "signature for '=destroy' must be proc[T: object](x: var T) or proc[T: object](x: T)")
     else:
       localError(c.config, n.info, errGenerated,
         "signature for '" & s.name.s & "' must be proc[T: object](x: var T)")

doc/destructors.md

@@ -127,16 +127,16 @@ other associated resources. Variables are destroyed via this hook when
 they go out of scope or when the routine they were declared in is about
 to return.
 
-The prototype of this hook for a type `T` needs to be:
+A `=destroy` hook is allowed to have a parameter of a `var T` or `T` type. Taking a `var T` type is deprecated. The prototype of this hook for a type `T` needs to be:
 
   ```nim
-  proc `=destroy`(x: var T)
+  proc `=destroy`(x: T)
   ```
 
 The general pattern in `=destroy` looks like:
 
   ```nim
-  proc `=destroy`(x: var T) =
+  proc `=destroy`(x: T) =
     # first check if 'x' was moved to somewhere else:
     if x.field != nil:
       freeResource(x.field)
@@ -690,11 +690,11 @@ The ability to override a hook leads to a phase ordering problem:
     # error: destructor for 'f' called here before
     # it was seen in this module.
 
-  proc `=destroy`[T](f: var Foo[T]) =
+  proc `=destroy`[T](f: Foo[T]) =
     discard
   ```
 
-The solution is to define ``proc `=destroy`[T](f: var Foo[T])`` before
+The solution is to define ``proc `=destroy`[T](f: Foo[T])`` before
 it is used. The compiler generates implicit
 hooks for all types in *strategic places* so that an explicitly provided
 hook that comes too "late" can be detected reliably. These *strategic places*
@@ -723,7 +723,7 @@ used to specialize the object traversal in order to avoid deep recursions:
   type Tree = object
     root: Node
 
-  proc `=destroy`(t: var Tree) {.nodestroy.} =
+  proc `=destroy`(t: Tree) {.nodestroy.} =
     # use an explicit stack so that we do not get stack overflows:
     var s: seq[Node] = @[t.root]
     while s.len > 0:

lib/impure/re.nim

@@ -65,10 +65,16 @@ type
     ## is raised if the pattern is no valid regular expression.
 
 when defined(gcDestructors):
-  proc `=destroy`(x: var RegexDesc) =
-    pcre.free_substring(cast[cstring](x.h))
-    if not isNil(x.e):
-      pcre.free_study(x.e)
+  when defined(nimAllowNonVarDestructor):
+    proc `=destroy`(x: RegexDesc) =
+      pcre.free_substring(cast[cstring](x.h))
+      if not isNil(x.e):
+        pcre.free_study(x.e)
+  else:
+    proc `=destroy`(x: var RegexDesc) =
+      pcre.free_substring(cast[cstring](x.h))
+      if not isNil(x.e):
+        pcre.free_study(x.e)
 
 proc raiseInvalidRegex(msg: string) {.noinline, noreturn.} =
   var e: ref RegexError

lib/std/tasks.nim

@@ -68,12 +68,20 @@ type
 
 proc `=copy`*(x: var Task, y: Task) {.error.}
 
-proc `=destroy`*(t: var Task) {.inline, gcsafe.} =
-  ## Frees the resources allocated for a `Task`.
-  if t.args != nil:
-    if t.destroy != nil:
-      t.destroy(t.args)
-    deallocShared(t.args)
+when defined(nimAllowNonVarDestructor):
+  proc `=destroy`*(t: Task) {.inline, gcsafe.} =
+    ## Frees the resources allocated for a `Task`.
+    if t.args != nil:
+      if t.destroy != nil:
+        t.destroy(t.args)
+      deallocShared(t.args)
+else:
+  proc `=destroy`*(t: var Task) {.inline, gcsafe.} =
+    ## Frees the resources allocated for a `Task`.
+    if t.args != nil:
+      if t.destroy != nil:
+        t.destroy(t.args)
+      deallocShared(t.args)
 
 proc invoke*(task: Task; res: pointer = nil) {.inline, gcsafe.} =
   ## Invokes the `task`.

lib/std/widestrs.nim

@@ -25,12 +25,20 @@ when not (defined(cpu16) or defined(cpu8)):
         bytes: int
         data: WideCString
 
-    proc `=destroy`(a: var WideCStringObj) =
-      if a.data != nil:
-        when compileOption("threads"):
-          deallocShared(a.data)
-        else:
-          dealloc(a.data)
+    when defined(nimAllowNonVarDestructor):
+      proc `=destroy`(a: WideCStringObj) =
+        if a.data != nil:
+          when compileOption("threads"):
+            deallocShared(a.data)
+          else:
+            dealloc(a.data)
+    else:
+      proc `=destroy`(a: var WideCStringObj) =
+        if a.data != nil:
+          when compileOption("threads"):
+            deallocShared(a.data)
+          else:
+            dealloc(a.data)
 
     proc `=copy`(a: var WideCStringObj; b: WideCStringObj) {.error.}
 

tests/destructor/tnonvardestructor.nim

@@ -0,0 +1,195 @@
+discard """
+  targets: "c cpp"
+  matrix: "--mm:arc; --mm:orc"
+"""
+
+block:
+  type
+    PublicKey = array[32, uint8]
+    PrivateKey = array[64, uint8]
+
+  proc ed25519_create_keypair(publicKey: ptr PublicKey; privateKey: ptr PrivateKey) =
+    publicKey[][0] = uint8(88)
+
+  type
+    KeyPair = object
+      public: PublicKey
+      private: PrivateKey
+
+  proc initKeyPair(): KeyPair =
+    ed25519_create_keypair(result.public.addr, result.private.addr)
+
+  let keys = initKeyPair()
+  doAssert keys.public[0] == 88
+
+
+template minIndexByIt: untyped =
+  var other = 3
+  other
+
+proc bug20303() =
+  var hlibs = @["hello", "world", "how", "are", "you"]
+  let res = hlibs[minIndexByIt()]
+  doAssert res == "are"
+
+bug20303()
+
+proc main() = # todo bug with templates
+  block: # bug #11267
+    var a: seq[char] = block: @[]
+    doAssert a == @[]
+    # 2
+    proc b: seq[string] =
+      discard
+      @[]
+    doAssert b() == @[]
+static: main()
+main()
+
+
+type Obj = tuple
+  value: int
+  arr: seq[int]
+
+proc bug(): seq[Obj] =
+  result.add (value: 0, arr: @[])
+  result[^1].value = 1
+  result[^1].arr.add 1
+
+# bug #19990
+let s = bug()
+doAssert s[0] == (value: 1, arr: @[1])
+
+block: # bug #21974
+  type Test[T] = ref object
+      values : seq[T]
+      counter: int
+
+  proc newTest[T](): Test[T] =
+      result         = new(Test[T])
+      result.values  = newSeq[T](16)
+      result.counter = 0
+
+  proc push[T](self: Test[T], value: T) =
+      self.counter += 1
+      if self.counter >= self.values.len:
+          self.values.setLen(self.values.len * 2)
+      self.values[self.counter - 1] = value
+
+  proc pop[T](self: Test[T]): T =
+      result         = self.values[0]
+      self.values[0] = self.values[self.counter - 1] # <--- This line
+      self.counter  -= 1
+
+
+  type X = tuple
+      priority: int
+      value   : string
+
+  var a = newTest[X]()
+  a.push((1, "One"))
+  doAssert a.pop.value == "One"
+
+# bug #21987
+
+type
+  EmbeddedImage* = distinct Image
+  Image = object
+    len: int
+
+proc imageCopy*(image: Image): Image {.nodestroy.}
+
+proc `=destroy`*(x: Image) =
+  discard
+
+proc `=sink`*(dest: var Image; source: Image) =
+  `=destroy`(dest)
+  wasMoved(dest)
+
+proc `=dup`*(source: Image): Image {.nodestroy.} =
+  result = imageCopy(source)
+
+proc `=copy`*(dest: var Image; source: Image) =
+  dest = imageCopy(source) # calls =sink implicitly
+
+proc `=destroy`*(x: EmbeddedImage) = discard
+
+proc `=dup`*(source: EmbeddedImage): EmbeddedImage {.nodestroy.} = source
+
+proc `=copy`*(dest: var EmbeddedImage; source: EmbeddedImage) {.nodestroy.} =
+  dest = source
+
+proc imageCopy*(image: Image): Image =
+  result = image
+
+proc main2 =
+  block:
+    var a = Image(len: 2).EmbeddedImage
+    var b = Image(len: 1).EmbeddedImage
+    b = a
+    doAssert Image(a).len == 2
+    doAssert Image(b).len == 2
+
+  block:
+    var a = Image(len: 2)
+    var b = Image(len: 1)
+    b = a
+    doAssert a.len == 2
+    doAssert b.len == 0
+
+main2()
+
+type
+  Edge = object
+    neighbor {.cursor.}: Node
+
+  NodeObj = object
+    neighbors: seq[Edge]
+    label: string
+    visited: bool
+  Node = ref NodeObj
+
+  Graph = object
+    nodes: seq[Node]
+
+proc `=destroy`(x: var NodeObj) =
+  `=destroy`(x.neighbors)
+  `=destroy`(x.label)
+
+proc addNode(self: var Graph; label: string): Node =
+  self.nodes.add(Node(label: label))
+  result = self.nodes[^1]
+
+proc addEdge(self: Graph; source, neighbor: Node) =
+  source.neighbors.add(Edge(neighbor: neighbor))
+
+block:
+  proc main =
+    var graph: Graph
+    let nodeA = graph.addNode("a")
+    let nodeB = graph.addNode("b")
+    let nodeC = graph.addNode("c")
+
+    graph.addEdge(nodeA, neighbor = nodeB)
+    graph.addEdge(nodeA, neighbor = nodeC)
+
+  main()
+
+block:
+  type RefObj = ref object
+
+  proc `[]`(val: static[int]) = # works with different name/overload or without static arg
+    discard
+
+  template noRef(T: typedesc): typedesc = # works without template indirection
+    typeof(default(T)[])
+
+  proc `=destroy`(x: noRef(RefObj)) =
+    discard
+
+  proc foo =
+    var x = new RefObj
+    doAssert $(x[]) == "()"
+
+  # bug #11705
+  foo()