fixes #24719; improves order of destruction (#25060)

fixes #24719

Author: ringabout

compiler/liftdestructors.nim

@@ -218,23 +218,38 @@ proc fillBodyObj(c: var TLiftCtx; n, body, x, y: PNode; enforceDefaultOp: bool,
       fillBodyObj(c, n[0], body, x, y, enforceDefaultOp = false)
     c.filterDiscriminator = oldfilterDiscriminator
   of nkRecList:
-    for t in items(n): fillBodyObj(c, t, body, x, y, enforceDefaultOp, enforceWasMoved)
+    # destroys in reverse order #24719
+    if c.kind == attachedDestructor:
+      for i in countdown(n.len-1, 0):
+        fillBodyObj(c, n[i], body, x, y, enforceDefaultOp, enforceWasMoved)
+    else:
+      for t in items(n): fillBodyObj(c, t, body, x, y, enforceDefaultOp, enforceWasMoved)
   else:
     illFormedAstLocal(n, c.g.config)
 
 proc fillBodyObjTImpl(c: var TLiftCtx; t: PType, body, x, y: PNode) =
-  if t.baseClass != nil:
-    let dest = newNodeIT(nkHiddenSubConv, c.info, t.baseClass)
-    dest.add newNodeI(nkEmpty, c.info)
-    dest.add x
-    var src = y
-    if c.kind in {attachedAsgn, attachedDeepCopy, attachedSink}:
-      src = newNodeIT(nkHiddenSubConv, c.info, t.baseClass)
-      src.add newNodeI(nkEmpty, c.info)
-      src.add y
-
-    fillBody(c, skipTypes(t.baseClass, abstractPtrs), body, dest, src)
-  fillBodyObj(c, t.n, body, x, y, enforceDefaultOp = false)
+  template fillBase =
+    if t.baseClass != nil:
+      let dest = newNodeIT(nkHiddenSubConv, c.info, t.baseClass)
+      dest.add newNodeI(nkEmpty, c.info)
+      dest.add x
+      var src = y
+      if c.kind in {attachedAsgn, attachedDeepCopy, attachedSink}:
+        src = newNodeIT(nkHiddenSubConv, c.info, t.baseClass)
+        src.add newNodeI(nkEmpty, c.info)
+        src.add y
+
+      fillBody(c, skipTypes(t.baseClass, abstractPtrs), body, dest, src)
+  template fillFields =
+    fillBodyObj(c, t.n, body, x, y, enforceDefaultOp = false)
+
+  if c.kind == attachedDestructor:
+    # destroys in reverse order #24719
+    fillFields()
+    fillBase()
+  else:
+    fillBase()
+    fillFields()
 
 proc fillBodyObjT(c: var TLiftCtx; t: PType, body, x, y: PNode) =
   var hasCase = isCaseObj(t.n)

tests/arc/tdestructor_order.nim

@@ -0,0 +1,39 @@
+discard """
+  output: '''
+destroying d
+destroying c
+destroying a 2
+destroying d
+destroying c
+destroying a 1
+'''
+joinable: false
+"""
+
+type
+  Aaaa {.inheritable.} = object
+    vvvv: int
+  Bbbb = object of Aaaa
+    c: Cccc
+    d: Dddd
+  Cccc = object
+  Dddd = object
+
+  Holder = object
+    member: ref Aaaa
+
+proc `=destroy`(v: Cccc) =
+  echo "destroying c"
+
+proc `=destroy`(v: Dddd) =
+  echo "destroying d"
+
+proc `=destroy`(v: Aaaa) =
+  echo "destroying a ", v.vvvv
+
+func makeHolder(vvvv: int): ref Holder =
+  (ref Holder)(member: (ref Bbbb)(vvvv: vvvv))
+
+block:
+  var v = makeHolder(1)
+  var v2 = makeHolder(2)

tests/destructor/tdestructor.nim

@@ -1,5 +1,6 @@
 discard """
-  output: '''----1
+  output: '''
+----1
 myobj constructed
 myobj destroyed
 ----2
@@ -14,8 +15,8 @@ mygeneric3 constructed
 mygeneric1 destroyed
 ----5
 mydistinctObj constructed
-myobj destroyed
 mygeneric2 destroyed
+myobj destroyed
 ------------------8
 mygeneric1 destroyed
 ----6