From 8120d329eeb93213cff9ae44a628d0402a671df7 Mon Sep 17 00:00:00 2001
From: metagn <metagngn@gmail.com>
Date: Thu, 15 May 2025 10:32:10 +0300
Subject: [PATCH] generate `let _ =` to fully unpack partial tuple unpacking
 assignment for arc (#24948)

fixes #24947

When injectdestructors detects that a variable is a tuple unpacking temp
(i.e. it is an `skTemp`, is not a cursor, and has tuple type) it does
not generate a destructor for it and only generates sink/bit assignments
for its components. However the reason it does not generate a destructor
is that it expects it to be fully unpacked, this is true for unpackings
in for loops but not for tuple unpacking assignments which supports `_`
since #22537. Tuple unpacking definitions for `var`/`let`/`const` do not
generate `skTemp` and use the same symbol kind as the definition so they
did not have this problem.

To keep this compatible, the `_` parts of the tuple unpacking
assignments are now not ignored and unpacked into `let _ = ...`, which
generates its own destructor. Another option might be to use `skLet`
instead of `skTemp` but this might cause changes to behavior like
additional copies, I am not sure about this though.

(cherry picked from commit 71c5a4f72c2130184dcf6a6b21bccf756a98dc85)
---
 compiler/injectdestructors.nim        |  9 +++++----
 compiler/semexprs.nim                 | 16 ++++++++++++++--
 tests/arc/tpartialtupleunpacking1.nim | 19 +++++++++++++++++++
 tests/arc/tpartialtupleunpacking2.nim | 18 ++++++++++++++++++
 4 files changed, 56 insertions(+), 6 deletions(-)
 create mode 100644 tests/arc/tpartialtupleunpacking1.nim
 create mode 100644 tests/arc/tpartialtupleunpacking2.nim

diff --git a/compiler/injectdestructors.nim b/compiler/injectdestructors.nim
index 0dc2773a1c..7e8d4250b1 100644
--- a/compiler/injectdestructors.nim
+++ b/compiler/injectdestructors.nim
@@ -185,10 +185,11 @@ proc isCursor(n: PNode): bool =
   else:
     false
 
-template isUnpackedTuple(n: PNode): bool =
+template isFullyUnpackedTuple(n: PNode): bool =
   ## we move out all elements of unpacked tuples,
   ## hence unpacked tuples themselves don't need to be destroyed
   ## except it's already a cursor
+  ## restricted to `skTemp`, tuple temps where not every field is unpacked should not use `skTemp`
   (n.kind == nkSym and n.sym.kind == skTemp and
    n.sym.typ.kind == tyTuple and sfCursor notin n.sym.flags)
 
@@ -275,7 +276,7 @@ proc deepAliases(dest, ri: PNode): bool =
     return aliases(dest, ri) != no
 
 proc genSink(c: var Con; s: var Scope; dest, ri: PNode; flags: set[MoveOrCopyFlag] = {}): PNode =
-  if (c.inLoopCond == 0 and (isUnpackedTuple(dest) or IsDecl in flags or
+  if (c.inLoopCond == 0 and (isFullyUnpackedTuple(dest) or IsDecl in flags or
       (isAnalysableFieldAccess(dest, c.owner) and isFirstWrite(dest, c)))) or
       isNoInit(dest) or IsReturn in flags:
     # optimize sink call into a bitwise memcopy
@@ -559,7 +560,7 @@ proc cycleCheck(n: PNode; c: var Con) =
 proc pVarTopLevel(v: PNode; c: var Con; s: var Scope; res: PNode) =
   # move the variable declaration to the top of the frame:
   s.vars.add v.sym
-  if isUnpackedTuple(v):
+  if isFullyUnpackedTuple(v):
     if c.inLoop > 0:
       # unpacked tuple needs reset at every loop iteration
       res.add newTree(nkFastAsgn, v, genDefaultCall(v.typ, c, v.info))
@@ -1148,7 +1149,7 @@ proc moveOrCopy(dest, ri: PNode; c: var Con; s: var Scope, flags: set[MoveOrCopy
     of nkCallKinds:
       result = c.genSink(s, dest, p(ri, c, s, consumed), flags)
     of nkBracketExpr:
-      if isUnpackedTuple(ri[0]):
+      if isFullyUnpackedTuple(ri[0]):
         # unpacking of tuple: take over the elements
         result = c.genSink(s, dest, p(ri, c, s, consumed), flags)
       elif isAnalysableFieldAccess(ri, c.owner) and isLastRead(ri, c, s):
diff --git a/compiler/semexprs.nim b/compiler/semexprs.nim
index 2a3a4d13d4..5e2a5d0a72 100644
--- a/compiler/semexprs.nim
+++ b/compiler/semexprs.nim
@@ -1923,8 +1923,20 @@ proc makeTupleAssignments(c: PContext; n: PNode): PNode =
 
   for i in 0..<lhs.len:
     if lhs[i].kind == nkIdent and lhs[i].ident.id == ord(wUnderscore):
-      # skip _ assignments if we are using a temp as they are already evaluated
-      discard
+      # tuple unpacking `skTemp` does not generate a destructor and
+      # expects all fields to be unpacked, so instead of skipping,
+      # generate `let _ = temp[i]` which should generate a destructor
+      let utemp = newSym(skLet, lhs[i].ident, c.idgen, getCurrOwner(c), lhs[i].info)
+      utemp.typ = value.typ[i]
+      temp.flags.incl(sfGenSym)
+      var uv = newNodeI(nkLetSection, lhs[i].info)
+      let utempNode = newSymNode(utemp)
+      var uvpart = newNodeI(nkIdentDefs, v.info, 3)
+      uvpart[0] = utempNode
+      uvpart[1] = c.graph.emptyNode
+      uvpart[2] = newTupleAccessRaw(tempNode, i)
+      uv.add uvpart
+      result.add(uv)
     else:
       result.add newAsgnStmt(lhs[i], newTupleAccessRaw(tempNode, i))
 
diff --git a/tests/arc/tpartialtupleunpacking1.nim b/tests/arc/tpartialtupleunpacking1.nim
new file mode 100644
index 0000000000..6eb9d13522
--- /dev/null
+++ b/tests/arc/tpartialtupleunpacking1.nim
@@ -0,0 +1,19 @@
+discard """
+  output: '''
+destroyed
+'''
+"""
+
+# issue #24947
+
+type Foo = object
+
+proc `=destroy`(x: Foo) =
+  echo "destroyed"
+
+proc go(): void =
+  let a = (1,2,3, Foo())
+  var b,c,d: int
+  (b,c,d,_) = a # assignment unpacking
+
+go()
diff --git a/tests/arc/tpartialtupleunpacking2.nim b/tests/arc/tpartialtupleunpacking2.nim
new file mode 100644
index 0000000000..45f6d48f04
--- /dev/null
+++ b/tests/arc/tpartialtupleunpacking2.nim
@@ -0,0 +1,18 @@
+discard """
+  output: '''
+destroyed
+'''
+"""
+
+# issue #24947
+
+type Foo = object
+
+proc `=destroy`(x: Foo) =
+  echo "destroyed"
+
+proc go(): void =
+  let a = (1,2,3, Foo())
+  let (b,c,d,_) = a # let unpacking
+
+go()