From a8d87c041c1a7c62388e0450b60dbced4beacca7 Mon Sep 17 00:00:00 2001
From: metagn <metagngn@gmail.com>
Date: Tue, 15 Apr 2025 20:29:46 +0300
Subject: [PATCH] don't traverse inner procs to lift locals in closure iters
 (#24876)

fixes #24863, refs #23787 and #24316

Working off the minimized example, my understanding of the issue is: `n`
captures `r` as `:envP.r1` where `:envP` is the environment of `b`, then
`proc () = n()` does the lambda lifting of `n` again (which isn't done
if the `proc ()` is marked `{.closure.}`, hence the workaround) which
then captures the `:envP` as another field inside the `:envP`, so it
generates `:envP.:envP_2.r1` but the `.:envP_2` field is `nil`, so it
causes a segfault.

The problem is that the capture of `r` in `n` is done inside
`detectCapturedVars` for the surrounding closure iterator: inner procs
are not special cased and traversed as regular nodes, so it thinks it's
inside the iterator and generates a field access of `:envP` freely. The
lambda lifting version of `detectCapturedVars` ignores inner procs and
works off of symbol uses (anonymous iterator and lambda declarations
pretend their symbol is used).

As a naive solution, closure iterators now also ignore inner proc
declarations same as `lambdalifting.detectCapturedVars`, but unlike it
they also don't do anything for the inner proc symbols. Lambdalifting
seems to properly handle the lifted variables but in the worst case we
can also make sure `closureiters.detectCapturedVars` traverses inner
procs by marking every local of the closure iter used in them as needing
lifting (but not doing the lifting). This does not seem necessary for
now so it's not done (was done and reverted in [this
commit](https://github.com/nim-lang/Nim/pull/24876/commits/9bb39a9259ecf7d93c64a096138f8a2d108333d5)),
but regressions are still possible

(cherry picked from commit c06bb6cc03f1a42d515949967c0c9f267e971d04)
---
 compiler/closureiters.nim      | 14 ++++++++++++++
 compiler/lambdalifting.nim     |  2 +-
 tests/iter/t24863.nim          | 30 ++++++++++++++++++++++++++++++
 tests/iter/tnestedclosures.nim | 20 ++++++++++++++++++++
 4 files changed, 65 insertions(+), 1 deletion(-)
 create mode 100644 tests/iter/t24863.nim

diff --git a/compiler/closureiters.nim b/compiler/closureiters.nim
index 7e0f54b12e..835cbf0ca7 100644
--- a/compiler/closureiters.nim
+++ b/compiler/closureiters.nim
@@ -1451,6 +1451,14 @@ proc detectCapturedVars(c: var Ctx, n: PNode, stateIdx: int) =
       detectCapturedVars(c, n[0][1], stateIdx)
     else:
       detectCapturedVars(c, n[0], stateIdx)
+  of nkEmpty..pred(nkSym), succ(nkSym)..nkNilLit,
+     nkTemplateDef, nkTypeSection, nkProcDef, nkMethodDef,
+     nkConverterDef, nkMacroDef, nkFuncDef, nkCommentStmt,
+     nkTypeOfExpr, nkMixinStmt, nkBindStmt:
+    discard
+  of nkLambdaKinds, nkIteratorDef:
+    if n.typ != nil:
+      detectCapturedVars(c, n[namePos], stateIdx)
   else:
     for i in 0 ..< n.safeLen:
       detectCapturedVars(c, n[i], stateIdx)
@@ -1481,6 +1489,12 @@ proc liftLocals(c: var Ctx, n: PNode): PNode =
       n[0][1] = liftLocals(c, n[0][1])
     else:
       n[0] = liftLocals(c, n[0])
+  of nkEmpty..pred(nkSym), succ(nkSym)..nkNilLit,
+     nkTemplateDef, nkTypeSection, nkProcDef, nkMethodDef,
+     nkConverterDef, nkMacroDef, nkFuncDef, nkCommentStmt,
+     nkTypeOfExpr, nkMixinStmt, nkBindStmt,
+     nkLambdaKinds, nkIteratorDef:
+    discard
   else:
     for i in 0 ..< n.safeLen:
       n[i] = liftLocals(c, n[i])
diff --git a/compiler/lambdalifting.nim b/compiler/lambdalifting.nim
index 640bb4b2f8..c8c5acf974 100644
--- a/compiler/lambdalifting.nim
+++ b/compiler/lambdalifting.nim
@@ -199,7 +199,7 @@ proc interestingVar(s: PSym): bool {.inline.} =
 proc illegalCapture(s: PSym): bool {.inline.} =
   result = classifyViewType(s.typ) != noView or s.kind == skResult
 
-proc isInnerProc(s: PSym): bool =
+proc isInnerProc*(s: PSym): bool =
   if s.kind in {skProc, skFunc, skMethod, skConverter, skIterator} and s.magic == mNone:
     result = s.skipGenericOwner.kind in routineKinds
   else:
diff --git a/tests/iter/t24863.nim b/tests/iter/t24863.nim
new file mode 100644
index 0000000000..96ddbcef7c
--- /dev/null
+++ b/tests/iter/t24863.nim
@@ -0,0 +1,30 @@
+# issue #24863
+
+type M = object
+  p: iterator(): M {.gcsafe.}
+
+template h(f: M): int =
+  yield f
+  456
+
+proc s(): M =
+  iterator g(): M {.closure.} = discard
+  let v = M(p: g)
+  doAssert(not isNil(v.p))
+  discard v.p()
+  v
+
+proc c(): M =
+  iterator b(): M {.closure.} =
+    let r = h(s())
+    doAssert r == 456
+    proc n(): M =
+      iterator y(): M {.closure.} =
+        let _ = r
+      let _ = y
+    let _ = proc () = discard n()
+  let j = M(p: b)
+  doAssert(not isNil(j.p))
+  discard j.p()
+
+let _ = c()
diff --git a/tests/iter/tnestedclosures.nim b/tests/iter/tnestedclosures.nim
index e23fa1355f..f2dc7a51d4 100644
--- a/tests/iter/tnestedclosures.nim
+++ b/tests/iter/tnestedclosures.nim
@@ -25,6 +25,9 @@ Test 7:
 0
 1
 2
+Test 8:
+123
+456
 '''
 """
 
@@ -156,3 +159,20 @@ block: # issue #12487
     doAssert s == @["something"]
 
   main()
+
+block: # minimized issue #24863
+  echo "Test 8:"
+  proc c() =
+    iterator b(): int {.closure.} =
+      let r = 456
+      yield 123
+      proc n() =
+        echo r
+      let a = proc () = n()
+      a()
+
+    let j = b
+    echo j()
+    discard j()
+
+  c()