From d44b0b186943b3187e6dbc8bc5d304f402d978dc Mon Sep 17 00:00:00 2001
From: ringabout <43030857+ringabout@users.noreply.github.com>
Date: Fri, 26 Jan 2024 13:06:08 +0800
Subject: [PATCH] fixes #22597; avoid side effects for call returning openArray
 types (#23257)

fixes #22597

```nim
proc autoToOpenArray*[T](s: Slice[T]): openArray[T] =
  echo "here twice"
  result = toOpenArray(s.p, s.first, s.last)
```
For functions returning openarray types, `fixupCall` creates a temporary
variable to store the return value: `let tmp = autoToOpenArray()`. But
`genOpenArrayConv` cannot handle openarray assignements with side
effects. It should have stored the right part of the assignment first
instead of calling the right part twice.
---
 compiler/ccgcalls.nim   |  8 ++++++--
 compiler/ccgexprs.nim   | 15 +++++++++++----
 compiler/cgen.nim       |  1 +
 tests/views/tviews1.nim | 19 +++++++++++++++++++
 4 files changed, 37 insertions(+), 6 deletions(-)

diff --git a/compiler/ccgcalls.nim b/compiler/ccgcalls.nim
index 5dc6b39a6b..607f6d51e6 100644
--- a/compiler/ccgcalls.nim
+++ b/compiler/ccgcalls.nim
@@ -84,6 +84,10 @@ proc fixupCall(p: BProc, le, ri: PNode, d: var TLoc,
   # getUniqueType() is too expensive here:
   var typ = skipTypes(ri[0].typ, abstractInst)
   if typ.returnType != nil:
+    var flags: TAssignmentFlags = {}
+    if typ.returnType.kind in {tyOpenArray, tyVarargs}:
+      # perhaps generate no temp if the call doesn't have side effects
+      flags.incl needTempForOpenArray
     if isInvalidReturnType(p.config, typ):
       if params.len != 0: pl.add(", ")
       # beware of 'result = p(result)'. We may need to allocate a temporary:
@@ -128,13 +132,13 @@ proc fixupCall(p: BProc, le, ri: PNode, d: var TLoc,
         assert(d.t != nil)        # generate an assignment to d:
         var list = initLoc(locCall, d.lode, OnUnknown)
         list.r = pl
-        genAssignment(p, d, list, {}) # no need for deep copying
+        genAssignment(p, d, list, flags) # no need for deep copying
         if canRaise: raiseExit(p)
       else:
         var tmp: TLoc = getTemp(p, typ.returnType, needsInit=true)
         var list = initLoc(locCall, d.lode, OnUnknown)
         list.r = pl
-        genAssignment(p, tmp, list, {}) # no need for deep copying
+        genAssignment(p, tmp, list, flags) # no need for deep copying
         if canRaise: raiseExit(p)
         genAssignment(p, d, tmp, {})
   else:
diff --git a/compiler/ccgexprs.nim b/compiler/ccgexprs.nim
index 17e0da5755..66a4d9a930 100644
--- a/compiler/ccgexprs.nim
+++ b/compiler/ccgexprs.nim
@@ -285,15 +285,22 @@ proc genGenericAsgn(p: BProc, dest, src: TLoc, flags: TAssignmentFlags) =
     linefmt(p, cpsStmts, "#genericAssign((void*)$1, (void*)$2, $3);$n",
             [addrLoc(p.config, dest), addrLoc(p.config, src), genTypeInfoV1(p.module, dest.t, dest.lode.info)])
 
-proc genOpenArrayConv(p: BProc; d: TLoc; a: TLoc) =
+proc genOpenArrayConv(p: BProc; d: TLoc; a: TLoc; flags: TAssignmentFlags) =
   assert d.k != locNone
   #  getTemp(p, d.t, d)
 
   case a.t.skipTypes(abstractVar).kind
   of tyOpenArray, tyVarargs:
     if reifiedOpenArray(a.lode):
-      linefmt(p, cpsStmts, "$1.Field0 = $2.Field0; $1.Field1 = $2.Field1;$n",
-        [rdLoc(d), a.rdLoc])
+      if needTempForOpenArray in flags:
+        var tmp: TLoc = getTemp(p, a.t)
+        linefmt(p, cpsStmts, "$2 = $1; $n",
+                [a.rdLoc, tmp.rdLoc])
+        linefmt(p, cpsStmts, "$1.Field0 = $2.Field0; $1.Field1 = $2.Field1;$n",
+          [rdLoc(d), tmp.rdLoc])
+      else:
+        linefmt(p, cpsStmts, "$1.Field0 = $2.Field0; $1.Field1 = $2.Field1;$n",
+          [rdLoc(d), a.rdLoc])
     else:
       linefmt(p, cpsStmts, "$1.Field0 = $2; $1.Field1 = $2Len_0;$n",
         [rdLoc(d), a.rdLoc])
@@ -391,7 +398,7 @@ proc genAssignment(p: BProc, dest, src: TLoc, flags: TAssignmentFlags) =
     # open arrays are always on the stack - really? What if a sequence is
     # passed to an open array?
     if reifiedOpenArray(dest.lode):
-      genOpenArrayConv(p, dest, src)
+      genOpenArrayConv(p, dest, src, flags)
     elif containsGarbageCollectedRef(dest.t):
       linefmt(p, cpsStmts,     # XXX: is this correct for arrays?
            "#genericAssignOpenArray((void*)$1, (void*)$2, $1Len_0, $3);$n",
diff --git a/compiler/cgen.nim b/compiler/cgen.nim
index b18ddb63cb..3a6d9a75ab 100644
--- a/compiler/cgen.nim
+++ b/compiler/cgen.nim
@@ -406,6 +406,7 @@ proc rdCharLoc(a: TLoc): Rope =
 type
   TAssignmentFlag = enum
     needToCopy
+    needTempForOpenArray
   TAssignmentFlags = set[TAssignmentFlag]
 
 proc genObjConstr(p: BProc, e: PNode, d: var TLoc)
diff --git a/tests/views/tviews1.nim b/tests/views/tviews1.nim
index 6662e3e5a8..1cc9fcdec3 100644
--- a/tests/views/tviews1.nim
+++ b/tests/views/tviews1.nim
@@ -105,3 +105,22 @@ block: # bug #22117
     result = aref
 
   doAssert main() == 10
+
+type
+  Slice*[T] = object
+    first, last: int
+    p: ptr UncheckedArray[T]
+
+var i = 0
+
+converter autoToOpenArray*[T](s: Slice[T]): openArray[T] =
+  inc i
+  result = toOpenArray(s.p, s.first, s.last)
+
+proc acceptOpenArray(s: openArray[byte]) = discard
+
+proc bug22597 = # bug #22597
+  acceptOpenArray(Slice[byte]())
+  doAssert i == 1
+
+bug22597()