From b8302cdd97ddbb80240dab947fc794a81f790f65 Mon Sep 17 00:00:00 2001
From: ringabout <43030857+ringabout@users.noreply.github.com>
Date: Mon, 10 Mar 2025 18:20:44 +0800
Subject: [PATCH] implements internal sink copy (#24747)

TODO:

- [x] other value types (arrays, strings, seqs, objects)
- [x] replaces https://github.com/nim-lang/Nim/pull/24731
- [x] improve code shape
- [ ] revert https://github.com/nim-lang/Nim/issues/24175
- [x] if possible, revert https://github.com/nim-lang/Nim/pull/23685
- [ ] if possible, revert https://github.com/nim-lang/Nim/pull/22229 and
https://github.com/nim-lang/Nim/pull/23764
- [ ] if possible, remove `if n.containsConstSeq:`
- [ ] if possible, always pass value (arrays, strings, seqs, tuples, or
even objects without custom hooks (?)) sinks by ref because this PR
should ensure these value types are not modified without a copy
- [x] fixes `say a, (b = move a; a)` for potential writes
https://github.com/nim-lang/Nim/pull/24753
---
 compiler/ccgcalls.nim          | 29 ---------------
 compiler/ccgutils.nim          |  9 +----
 compiler/injectdestructors.nim | 64 ++++++++++++++++++++++++++++++++--
 compiler/trees.nim             | 29 +++++++++++++++
 4 files changed, 91 insertions(+), 40 deletions(-)

diff --git a/compiler/ccgcalls.nim b/compiler/ccgcalls.nim
index aaad53b64b..2017f7dffc 100644
--- a/compiler/ccgcalls.nim
+++ b/compiler/ccgcalls.nim
@@ -418,35 +418,6 @@ proc skipTrivialIndirections(n: PNode): PNode =
       result = result[1]
     else: break
 
-proc getPotentialWrites(n: PNode; mutate: bool; result: var seq[PNode]) =
-  case n.kind:
-  of nkLiterals, nkIdent, nkFormalParams: discard
-  of nkSym:
-    if mutate: result.add n
-  of nkAsgn, nkFastAsgn, nkSinkAsgn:
-    getPotentialWrites(n[0], true, result)
-    getPotentialWrites(n[1], mutate, result)
-  of nkAddr, nkHiddenAddr:
-    getPotentialWrites(n[0], true, result)
-  of nkBracketExpr, nkDotExpr, nkCheckedFieldExpr:
-    getPotentialWrites(n[0], mutate, result)
-  of nkCallKinds:
-    case n.getMagic:
-    of mIncl, mExcl, mInc, mDec, mAppendStrCh, mAppendStrStr, mAppendSeqElem,
-        mAddr, mNew, mNewFinalize, mWasMoved, mDestroy:
-      getPotentialWrites(n[1], true, result)
-      for i in 2..<n.len:
-        getPotentialWrites(n[i], mutate, result)
-    of mSwap:
-      for i in 1..<n.len:
-        getPotentialWrites(n[i], true, result)
-    else:
-      for i in 1..<n.len:
-        getPotentialWrites(n[i], mutate, result)
-  else:
-    for s in n:
-      getPotentialWrites(s, mutate, result)
-
 proc getPotentialReads(n: PNode; result: var seq[PNode]) =
   case n.kind:
   of nkLiterals, nkIdent, nkFormalParams: discard
diff --git a/compiler/ccgutils.nim b/compiler/ccgutils.nim
index 1b75906909..2811aa7a35 100644
--- a/compiler/ccgutils.nim
+++ b/compiler/ccgutils.nim
@@ -90,9 +90,6 @@ proc ccgIntroducedPtr*(conf: ConfigRef; s: PSym, retType: PType): bool =
     if s.typ.sym != nil and sfForward in s.typ.sym.flags:
       # forwarded objects are *always* passed by pointers for consistency!
       result = true
-    elif s.typ.kind == tySink and conf.selectedGC notin {gcArc, gcAtomicArc, gcOrc, gcHooks}:
-      # bug #23354:
-      result = false
     elif (optByRef in s.options) or (getSize(conf, pt) > conf.target.floatSize * 3):
       result = true           # requested anyway
     elif (tfFinal in pt.flags) and (pt[0] == nil):
@@ -101,11 +98,7 @@ proc ccgIntroducedPtr*(conf: ConfigRef; s: PSym, retType: PType): bool =
       result = true           # ordinary objects are always passed by reference,
                               # otherwise casting doesn't work
   of tyTuple:
-    if s.typ.kind == tySink:
-      # it's a sink, so we pass it by value
-      result = false
-    else:
-      result = (getSize(conf, pt) > conf.target.floatSize*3) or (optByRef in s.options)
+    result = (getSize(conf, pt) > conf.target.floatSize*3) or (optByRef in s.options)
   else:
     result = false
   # first parameter and return type is 'lent T'? --> use pass by pointer
diff --git a/compiler/injectdestructors.nim b/compiler/injectdestructors.nim
index 013ab1c506..be2df8c1f2 100644
--- a/compiler/injectdestructors.nim
+++ b/compiler/injectdestructors.nim
@@ -17,7 +17,7 @@ import
   ast, astalgo, msgs, renderer, magicsys, types, idents,
   options, lowerings, modulegraphs,
   lineinfos, parampatterns, sighashes, liftdestructors, optimizer,
-  varpartitions, aliasanalysis, dfa, wordrecg
+  varpartitions, aliasanalysis, dfa, wordrecg, trees
 
 import std/[strtabs, tables, strutils, intsets]
 
@@ -1244,6 +1244,55 @@ when false:
       for i in 0..<n.safeLen:
         injectDefaultCalls(n[i], c)
 
+proc replaceSinkParam(n: PNode, mapping: Table[int, PSym]): PNode =
+  case n.kind
+  of nkSym:
+    if n.sym.id in mapping:
+      result = newSymNode(mapping[n.sym.id])
+    else:
+      result = n
+  of nkVarSection, nkLetSection:
+    result = copyNode(n)
+    newSons(result, n.len)
+    for i in 0..<n.len:
+      result[i] = copyNode(n[i])
+      for j in 0..<n[i].len-1:
+        result[i].add n[i][j]
+      result[i].add replaceSinkParam(n[i][^1], mapping)
+  of {nkNone..nkNilLit}-{nkSym}, nkTypeSection, nkProcDef, nkConverterDef,
+      nkMethodDef, nkIteratorDef, nkMacroDef, nkTemplateDef, nkLambda, nkDo,
+      nkFuncDef, nkConstSection, nkConstDef, nkIncludeStmt, nkImportStmt,
+      nkExportStmt, nkPragma, nkCommentStmt, nkBreakState,
+      nkTypeOfExpr, nkMixinStmt, nkBindStmt:
+    result = n
+  else:
+    result = copyNode(n)
+    for i in 0..<n.len:
+      result.add replaceSinkParam(n[i], mapping)
+
+proc addSinkCopy(c: var Con; s: var Scope; sinkParams: seq[PSym]; n: PNode): PNode =
+  result = newNodeI(nkStmtList, n.info)
+  var mapping = initTable[int, PSym]()
+
+  var mutated = newSeq[PNode]()
+  getPotentialWrites(n, false, mutated)
+  var mutatedSet = initIntSet()
+  for m in mutated:
+    mutatedSet.incl m.sym.id
+  for param in sinkParams:
+    if param.id in mutatedSet:
+      let newSym = newSym(skTemp, getIdent(c.graph.cache, "sinkCopy"), c.idgen, param.owner, n.info)
+      newSym.flags.incl sfFromGeneric
+      newSym.typ = param.typ.elementType
+      mapping[param.id] = newSym
+      let v = newNodeI(nkVarSection, n.info)
+      v.addVar(newSymNode(newSym), newSymNode(param))
+      result.add v
+  if mapping.len > 0:
+    result.add replaceSinkParam(n, mapping)
+  else:
+    result = n
+
 proc injectDestructorCalls*(g: ModuleGraph; idgen: IdGenerator; owner: PSym; n: PNode): PNode =
   when toDebug.len > 0:
     shouldDebug = toDebug == owner.name.s or toDebug == "always"
@@ -1257,15 +1306,24 @@ proc injectDestructorCalls*(g: ModuleGraph; idgen: IdGenerator; owner: PSym; n:
   var scope = Scope(body: n)
   let body = p(n, c, scope, normal)
 
+  var sinkParams = newSeq[PSym]()
+
   if owner.kind in {skProc, skFunc, skMethod, skIterator, skConverter}:
     let params = owner.typ.n
     for i in 1..<params.len:
       let t = params[i].sym.typ
-      if isSinkTypeForParam(t) and hasDestructor(c, t.skipTypes({tySink})):
-        scope.final.add c.genDestroy(params[i])
+      if isSinkTypeForParam(t):
+        let baseType = t.skipTypes({tySink})
+        if baseType.kind in {tyString, tySequence, tyArray, tyTuple, tyObject}:
+          sinkParams.add params[i].sym
+        if hasDestructor(c, baseType):
+          scope.final.add c.genDestroy(params[i])
   #if optNimV2 in c.graph.config.globalOptions:
   #  injectDefaultCalls(n, c)
   result = optimize processScope(c, scope, body)
+  if sinkParams.len > 0:
+    result = addSinkCopy(c, scope, sinkParams, result)
+
   dbg:
     echo ">---------transformed-to--------->"
     echo renderTree(result, {renderIds})
diff --git a/compiler/trees.nim b/compiler/trees.nim
index da58878f82..6d85f920c8 100644
--- a/compiler/trees.nim
+++ b/compiler/trees.nim
@@ -243,3 +243,32 @@ proc isRunnableExamples*(n: PNode): bool =
 
 proc skipAddr*(n: PNode): PNode {.inline.} =
   result = if n.kind in {nkAddr, nkHiddenAddr}: n[0] else: n
+
+proc getPotentialWrites*(n: PNode; mutate: bool; result: var seq[PNode]) =
+  case n.kind:
+  of nkLiterals, nkIdent, nkFormalParams: discard
+  of nkSym:
+    if mutate: result.add n
+  of nkAsgn, nkFastAsgn, nkSinkAsgn:
+    getPotentialWrites(n[0], true, result)
+    getPotentialWrites(n[1], mutate, result)
+  of nkAddr, nkHiddenAddr:
+    getPotentialWrites(n[0], true, result)
+  of nkBracketExpr, nkDotExpr, nkCheckedFieldExpr:
+    getPotentialWrites(n[0], mutate, result)
+  of nkCallKinds:
+    case n.getMagic:
+    of mIncl, mExcl, mInc, mDec, mAppendStrCh, mAppendStrStr, mAppendSeqElem,
+        mAddr, mNew, mNewFinalize, mWasMoved, mDestroy:
+      getPotentialWrites(n[1], true, result)
+      for i in 2..<n.len:
+        getPotentialWrites(n[i], mutate, result)
+    of mSwap:
+      for i in 1..<n.len:
+        getPotentialWrites(n[i], true, result)
+    else:
+      for i in 1..<n.len:
+        getPotentialWrites(n[i], mutate, result)
+  else:
+    for s in n:
+      getPotentialWrites(s, mutate, result)