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'

10 files changed, 261 insertions, 28 deletions

diff --git a/changelogs/changelog_2_0_0.md b/changelogs/changelog_2_0_0.md
index 5707cf955..b56a3e71c 100644
--- a/changelogs/changelog_2_0_0.md
+++ b/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
diff --git a/compiler/condsyms.nim b/compiler/condsyms.nim
index 9d9ba1605..4ff588852 100644
--- a/compiler/condsyms.nim
+++ b/compiler/condsyms.nim
@@ -155,3 +155,4 @@ proc initDefines*(symbols: StringTableRef) =
   defineSymbol("nimHasDup")
   defineSymbol("nimHasChecksums")
   defineSymbol("nimHasSendable")
+  defineSymbol("nimAllowNonVarDestructor")
diff --git a/compiler/injectdestructors.nim b/compiler/injectdestructors.nim
index f684aba5c..c5920f4b3 100644
--- a/compiler/injectdestructors.nim
+++ b/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 =
diff --git a/compiler/liftdestructors.nim b/compiler/liftdestructors.nim
index 865c18692..e40193481 100644
--- a/compiler/liftdestructors.nim
+++ b/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
diff --git a/compiler/semstmts.nim b/compiler/semstmts.nim
index cf3c03039..34df83f46 100644
--- a/compiler/semstmts.nim
+++ b/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)")
diff --git a/doc/destructors.md b/doc/destructors.md
index ddf581efb..789cb93d6 100644
--- a/doc/destructors.md
+++ b/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:
diff --git a/lib/impure/re.nim b/lib/impure/re.nim
index f125f9bcd..f97a31d21 100644
--- a/lib/impure/re.nim
+++ b/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
diff --git a/lib/std/tasks.nim b/lib/std/tasks.nim
index dadb2bc97..923cb2e99 100644
--- a/lib/std/tasks.nim
+++ b/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`.
diff --git a/lib/std/widestrs.nim b/lib/std/widestrs.nim
index 8973579e1..0bf50be45 100644
--- a/lib/std/widestrs.nim
+++ b/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.}
 
diff --git a/tests/destructor/tnonvardestructor.nim b/tests/destructor/tnonvardestructor.nim
new file mode 100644
index 000000000..e482ba137
--- /dev/null
+++ b/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()
\ No newline at end of file