fixes #22286; enforce Non-var T destructors by nimPreviewNonVarDestructor (#22975)

fixes #22286
ref https://forum.nim-lang.org/t/10642

For backwards compatibilities, we might need to keep the changes under a
preview compiler flag. Let's see how many packags it break.

**TODO** in the following PRs

- [ ] Turn the `var T` destructors warning into an error with
`nimPreviewNonVarDestructor`

---------

Co-authored-by: Andreas Rumpf <rumpf_a@web.de>

changelog.md

@@ -5,6 +5,7 @@
 
 - `-d:nimStrictDelete` becomes the default. An index error is produced when the index passed to `system.delete` was out of bounds. Use `-d:nimAuditDelete` to mimic the old behavior for backwards compatibility.
 - The default user-agent in `std/httpclient` has been changed to `Nim-httpclient/<version>` instead of `Nim httpclient/<version>` which was incorrect according to the HTTP spec.
+- With `-d:nimPreviewNonVarDestructor`, non-var destructors become the default.
 
 ## Standard library additions and changes
 

compiler/liftdestructors.nim

@@ -1082,7 +1082,7 @@ proc symDupPrototype(g: ModuleGraph; typ: PType; owner: PSym; kind: TTypeAttache
   incl result.flags, sfGeneratedOp
 
 proc symPrototype(g: ModuleGraph; typ: PType; owner: PSym; kind: TTypeAttachedOp;
-              info: TLineInfo; idgen: IdGenerator): PSym =
+              info: TLineInfo; idgen: IdGenerator; isDiscriminant = false): PSym =
   if kind == attachedDup:
     return symDupPrototype(g, typ, owner, kind, info, idgen)
 
@@ -1092,7 +1092,8 @@ proc symPrototype(g: ModuleGraph; typ: PType; owner: PSym; kind: TTypeAttachedOp
   let src = newSym(skParam, getIdent(g.cache, if kind == attachedTrace: "env" else: "src"),
                    idgen, result, info)
 
-  if kind == attachedDestructor and typ.kind in {tyRef, tyString, tySequence} and g.config.selectedGC in {gcArc, gcOrc, gcAtomicArc}:
+  if kind == attachedDestructor and g.config.selectedGC in {gcArc, gcOrc, gcAtomicArc} and
+     ((g.config.isDefined("nimPreviewNonVarDestructor") and not isDiscriminant) or typ.kind in {tyRef, tyString, tySequence}):
     dest.typ = typ
   else:
     dest.typ = makeVarType(typ.owner, typ, idgen)
@@ -1185,7 +1186,8 @@ proc produceSym(g: ModuleGraph; c: PContext; typ: PType; kind: TTypeAttachedOp;
 proc produceDestructorForDiscriminator*(g: ModuleGraph; typ: PType; field: PSym,
                                         info: TLineInfo; idgen: IdGenerator): PSym =
   assert(typ.skipTypes({tyAlias, tyGenericInst}).kind == tyObject)
-  result = symPrototype(g, field.typ, typ.owner, attachedDestructor, info, idgen)
+  # discrimantor assignments needs pointers to destroy fields; alas, we cannot use non-var destructor here
+  result = symPrototype(g, field.typ, typ.owner, attachedDestructor, info, idgen, isDiscriminant = true)
   var a = TLiftCtx(info: info, g: g, kind: attachedDestructor, asgnForType: typ, idgen: idgen,
                    fn: result)
   a.asgnForType = typ

compiler/nim.cfg

@@ -9,6 +9,7 @@ define:nimPreviewSlimSystem
 define:nimPreviewCstringConversion
 define:nimPreviewProcConversion
 define:nimPreviewRangeDefault
+define:nimPreviewNonVarDestructor
 threads:off
 
 #import:"$projectpath/testability"

lib/system.nim

@@ -366,19 +366,24 @@ proc arrPut[I: Ordinal;T,S](a: T; i: I;
 const arcLikeMem = defined(gcArc) or defined(gcAtomicArc) or defined(gcOrc)
 
 
-when defined(nimAllowNonVarDestructor) and arcLikeMem:
-  proc `=destroy`*(x: string) {.inline, magic: "Destroy".} =
+when defined(nimAllowNonVarDestructor) and arcLikeMem and defined(nimPreviewNonVarDestructor):
+  proc `=destroy`*[T](x: T) {.inline, magic: "Destroy".} =
+    ## Generic `destructor`:idx: implementation that can be overridden.
+    discard
+else:
+  proc `=destroy`*[T](x: var T) {.inline, magic: "Destroy".} =
+    ## Generic `destructor`:idx: implementation that can be overridden.
     discard
 
-  proc `=destroy`*[T](x: seq[T]) {.inline, magic: "Destroy".} =
-    discard
+  when defined(nimAllowNonVarDestructor) and arcLikeMem:
+    proc `=destroy`*(x: string) {.inline, magic: "Destroy".} =
+      discard
 
-  proc `=destroy`*[T](x: ref T) {.inline, magic: "Destroy".} =
-    discard
+    proc `=destroy`*[T](x: seq[T]) {.inline, magic: "Destroy".} =
+      discard
 
-proc `=destroy`*[T](x: var T) {.inline, magic: "Destroy".} =
-  ## Generic `destructor`:idx: implementation that can be overridden.
-  discard
+    proc `=destroy`*[T](x: ref T) {.inline, magic: "Destroy".} =
+      discard
 
 when defined(nimHasDup):
   proc `=dup`*[T](x: T): T {.inline, magic: "Dup".} =

tests/arc/thard_alignment.nim

@@ -1,9 +1,11 @@
 discard """
 disabled: "arm64"
-cmd: "nim c --gc:arc $file"
+cmd: "nim c --mm:arc -u:nimPreviewNonVarDestructor $file"
 output: "y"
 """
 
+# TODO: fixme: investigate why it failed with non-var destructors
+
 {.passC: "-march=native".}
 
 proc isAlignedCheck(p: pointer, alignment: int) = 

tests/arc/topt_no_cursor.nim

@@ -97,7 +97,7 @@ try:
         finally:
           `=destroy`(splitted)
 finally:
-  `=destroy`(lan_ip)
+  `=destroy_1`(lan_ip)
 -- end of expandArc ------------------------
 --expandArc: mergeShadowScope
 

tests/ccgbugs/tforward_decl_only.nim

@@ -1,6 +1,5 @@
 discard """
-ccodecheck: "\\i !@('struct tyObject_MyRefObject'[0-z]+' {')"
-ccodecheck: "\\i !@('mymoduleInit')"
+ccodecheck: "\\i !@('struct tyObject_MyRefObject'[0-z]+' _')"
 output: "hello"
 """
 

tests/config.nims

@@ -40,6 +40,7 @@ switch("define", "nimPreviewFloatRoundtrip")
 switch("define", "nimPreviewJsonutilsHoleyEnum")
 switch("define", "nimPreviewHashRef")
 switch("define", "nimPreviewRangeDefault")
+switch("define", "nimPreviewNonVarDestructor")
 
 switch("warningAserror", "UnnamedBreak")
 switch("legacy", "verboseTypeMismatch")

tests/converter/tconverter_unique_ptr.nim

@@ -22,12 +22,11 @@ proc `$`(x: MyLen): string {.borrow.}
 proc `==`(x1, x2: MyLen): bool {.borrow.}
 
 
-proc `=destroy`*(m: var MySeq) {.inline.} =
+proc `=destroy`*(m: MySeq) {.inline.} =
   if m.data != nil:
     deallocShared(m.data)
-    m.data = nil
 
-proc `=`*(m: var MySeq, m2: MySeq) =
+proc `=copy`*(m: var MySeq, m2: MySeq) =
   if m.data == m2.data: return
   if m.data != nil:
     `=destroy`(m)
@@ -77,13 +76,12 @@ converter literalToLen*(x: int{lit}): MyLen =
 # Unique pointer implementation
 #-------------------------------------------------------------
 
-proc `=destroy`*[T](p: var UniquePtr[T]) =
+proc `=destroy`*[T](p: UniquePtr[T]) =
   if p.val != nil:
     `=destroy`(p.val[])
     dealloc(p.val)
-    p.val = nil
 
-proc `=`*[T](dest: var UniquePtr[T], src: UniquePtr[T]) {.error.}
+proc `=copy`*[T](dest: var UniquePtr[T], src: UniquePtr[T]) {.error.}
 
 proc `=sink`*[T](dest: var UniquePtr[T], src: UniquePtr[T]) {.inline.} =
   if dest.val != nil and dest.val != src.val:
@@ -118,13 +116,12 @@ type
     ## as it returns only `lent T`
     val: ptr T
 
-proc `=destroy`*[T](p: var ConstPtr[T]) =
+proc `=destroy`*[T](p: ConstPtr[T]) =
   if p.val != nil:
     `=destroy`(p.val[])
     dealloc(p.val)
-    p.val = nil
 
-proc `=`*[T](dest: var ConstPtr[T], src: ConstPtr[T]) {.error.}
+proc `=copy`*[T](dest: var ConstPtr[T], src: ConstPtr[T]) {.error.}
 
 proc `=sink`*[T](dest: var ConstPtr[T], src: ConstPtr[T]) {.inline.} =
   if dest.val != nil and dest.val != src.val:

tests/destructor/const_smart_ptr.nim

@@ -2,13 +2,12 @@ type
   ConstPtr*[T] = object
     val: ptr T
 
-proc `=destroy`*[T](p: var ConstPtr[T]) =
+proc `=destroy`*[T](p: ConstPtr[T]) =
   if p.val != nil:
     `=destroy`(p.val[])
     dealloc(p.val)
-    p.val = nil
 
-proc `=`*[T](dest: var ConstPtr[T], src: ConstPtr[T]) {.error.}
+proc `=copy`*[T](dest: var ConstPtr[T], src: ConstPtr[T]) {.error.}
 
 proc `=sink`*[T](dest: var ConstPtr[T], src: ConstPtr[T]) {.inline.} =
   if dest.val != nil and dest.val != src.val:
@@ -31,12 +30,11 @@ type
     len: int
     data: ptr UncheckedArray[float]
 
-proc `=destroy`*(m: var MySeqNonCopyable) {.inline.} =
+proc `=destroy`*(m: MySeqNonCopyable) {.inline.} =
   if m.data != nil:
     deallocShared(m.data)
-    m.data = nil
 
-proc `=`*(m: var MySeqNonCopyable, m2: MySeqNonCopyable) {.error.}
+proc `=copy`*(m: var MySeqNonCopyable, m2: MySeqNonCopyable) {.error.}
 
 proc `=sink`*(m: var MySeqNonCopyable, m2: MySeqNonCopyable) {.inline.} =
   if m.data != m2.data:

tests/destructor/tnonvardestructor.nim

@@ -152,7 +152,7 @@ type
   Graph = object
     nodes: seq[Node]
 
-proc `=destroy`(x: var NodeObj) =
+proc `=destroy`(x: NodeObj) =
   `=destroy`(x.neighbors)
   `=destroy`(x.label)
 
@@ -210,7 +210,7 @@ block: # bug #22197
   ## If this does not exist, it also works!
   proc newFileID(): FileID = FileID(H5Id())
 
-  proc `=destroy`(grp: var H5GroupObj) =
+  proc `=destroy`(grp: H5GroupObj) =
     ## Closes the group and resets all references to nil.
     if cast[pointer](grp.fileId) != nil:
       `=destroy`(grp.file_id)
@@ -218,3 +218,30 @@ block: # bug #22197
   var grp = H5Group()
   reset(grp.file_id)
   reset(grp)
+
+import std/tables
+
+block: # bug #22286
+  type
+    A = object
+    B = object
+      a: A
+    C = object
+      b: B
+
+  proc `=destroy`(self: A) =
+    echo "destroyed"
+
+  proc `=destroy`(self: C) =
+    `=destroy`(self.b)
+
+  var c = C()
+
+block: # https://forum.nim-lang.org/t/10642
+  type AObj = object
+    name: string
+    tableField: Table[string, string]
+
+  proc `=destroy`(x: AObj) =
+    `=destroy`(x.name)
+    `=destroy`(x.tableField)

tests/destructor/tv2_cast.nim

@@ -20,8 +20,8 @@ data =
     :tmpD_2))
   :tmpD
 `=destroy`(:tmpD_2)
-`=destroy`(:tmpD_1)
-`=destroy`(data)
+`=destroy_1`(:tmpD_1)
+`=destroy_1`(data)
 -- end of expandArc ------------------------
 --expandArc: main1
 
@@ -37,8 +37,8 @@ data =
     :tmpD_1))
   :tmpD
 `=destroy`(:tmpD_1)
-`=destroy`(data)
-`=destroy`(s)
+`=destroy_1`(data)
+`=destroy_1`(s)
 -- end of expandArc ------------------------
 --expandArc: main2
 
@@ -54,7 +54,7 @@ data =
     :tmpD_1))
   :tmpD
 `=destroy`(:tmpD_1)
-`=destroy`(data)
+`=destroy_1`(data)
 `=destroy`(s)
 -- end of expandArc ------------------------
 --expandArc: main3
@@ -73,7 +73,7 @@ data =
   :tmpD
 `=destroy`(:tmpD_2)
 `=destroy`(:tmpD_1)
-`=destroy`(data)
+`=destroy_1`(data)
 -- end of expandArc ------------------------
 '''
 """

tests/gc/closureleak.nim

@@ -11,9 +11,19 @@ var foo_counter = 0
 var alive_foos = newseq[int](0)
 
 when defined(gcDestructors):
-  proc `=destroy`(some: var TFoo) =
+  proc `=destroy`(some: TFoo) =
     alive_foos.del alive_foos.find(some.id)
-    `=destroy`(some.fn)
+    # TODO: fixme: investigate why `=destroy` requires `some.fn` to be `gcsafe`
+    # the debugging info below came from `symPrototype` in the liftdestructors
+    # proc (){.closure, gcsafe.}, {tfThread, tfHasAsgn, tfCheckedForDestructor, tfExplicitCallConv}
+    # var proc (){.closure, gcsafe.}, {tfHasGCedMem}
+    # it worked by accident with var T destructors because in the sempass2
+    #
+    # let argtype = skipTypes(a.typ, abstractInst) # !!! it does't skip `tyVar`
+    # if argtype.kind == tyProc and notGcSafe(argtype) and not tracked.inEnforcedGcSafe:
+    #   localError(tracked.config, n.info, $n & " is not GC safe")
+    {.cast(gcsafe).}:
+      `=destroy`(some.fn)
 
 else:
   proc free*(some: ref TFoo) =