From d303c289fa823db150ef8afe819e2d448787d737 Mon Sep 17 00:00:00 2001
From: metagn <metagngn@gmail.com>
Date: Fri, 25 Oct 2024 23:13:22 +0300
Subject: [PATCH] consider calls as complex openarray assignment to iterator
 params (#24333)

fixes #13417, fixes #19703

When passing an expression to an `openarray` iterator parameter: If the
expression is a statement list (considered "complex"), it's assigned in
a non-deep-copying way to a temporary variable first, then this variable
is used as a parameter. If it's not a statement list, i.e. a call or a
symbol, the parameter is substituted directly with the given expression.
In the case of calls, this results in the call potentially being
executed more than once, or can cause redefined variables in the
codegen.

To fix this, calls are also considered as "complex" assignments to
openarrays, as long as the return type of the call is not `openarray` as
the generated assignment in that case has issues/is unimplemented
(caused a segfault [here in
datamancer](https://github.com/SciNim/Datamancer/blob/47ba4d81bf240a7755b73bc48c1cec9b638d18ae/src/datamancer/dataframe.nim#L1580)).

As for why creating a temporary isn't the default only with exceptions
for things like `nkSym`, the "non-deep-copying" way of assignment
apparently still causes arrays to be copied according to a comment in
the code. I'm not sure to what extent this is true: if it still happens
on ARC/ORC, if it happens for every array length, or if we can fix it by
passing arrays by reference. Otherwise, a more general way to assign to
openarrays might be needed, but I'm not sure if the compiler can easily
do this.
---
 compiler/transf.nim           |  8 +++++++-
 tests/iter/titeropenarray.nim | 32 ++++++++++++++++++++++++++++++++
 2 files changed, 39 insertions(+), 1 deletion(-)
 create mode 100644 tests/iter/titeropenarray.nim

diff --git a/compiler/transf.nim b/compiler/transf.nim
index 3db861481a7f..bd1a9cc4ca1f 100644
--- a/compiler/transf.nim
+++ b/compiler/transf.nim
@@ -637,6 +637,12 @@ proc putArgInto(arg: PNode, formal: PType): TPutArgInto =
     case arg.kind
     of nkStmtListExpr:
       return paComplexOpenarray
+    of nkCall:
+      if skipTypes(arg.typ, abstractInst).kind in {tyOpenArray, tyVarargs}:
+        # XXX incorrect, causes #13417 when `arg` has side effects.
+        return paDirectMapping
+      else:
+        return paComplexOpenarray
     of nkBracket:
       return paFastAsgnTakeTypeFromArg
     else:
@@ -803,7 +809,7 @@ proc transformFor(c: PTransf, n: PNode): PNode =
       stmtList.add(newAsgnStmt(c, nkFastAsgn, temp, addrExp, true))
       newC.mapping[formal.itemId] = newDeref(temp)
     of paComplexOpenarray:
-      # arrays will deep copy here (pretty bad).
+      # XXX arrays will deep copy here (pretty bad).
       var temp = newTemp(c, arg.typ, formal.info)
       addVar(v, temp)
       stmtList.add(newAsgnStmt(c, nkFastAsgn, temp, arg, true))
diff --git a/tests/iter/titeropenarray.nim b/tests/iter/titeropenarray.nim
new file mode 100644
index 000000000000..918195b16168
--- /dev/null
+++ b/tests/iter/titeropenarray.nim
@@ -0,0 +1,32 @@
+block: # issue #13417
+  var s: seq[int] = @[]
+  proc p1(): seq[int] =
+    s.add(3)
+    @[1,2]
+
+  iterator ip1(v: openArray[int]): auto =
+    for x in v:
+      yield x
+
+  for x in ip1(p1()):
+    s.add(x)
+
+  doAssert s == @[3, 1, 2]
+
+import std / sequtils
+
+block: # issue #19703
+  iterator combinations[T](s: seq[T], r: Positive): seq[T] =
+    yield @[s[0], s[1]]
+
+  iterator pairwise[T](s: openArray[T]): seq[T] =
+    yield @[s[0], s[0]]
+
+  proc checkSpecialSubset5(s: seq[int]): bool =
+    toSeq(
+      toSeq(
+        s.combinations(2)
+      ).map(proc(a: auto): int = a[0]).pairwise()
+    ).any(proc(a: auto): bool = a == @[s[0], s[0]])
+
+  doAssert checkSpecialSubset5 @[1, 2]