From 22d2cf217597468ace8ba540d6990b1f6d8a816a Mon Sep 17 00:00:00 2001
From: metagn <metagngn@gmail.com>
Date: Mon, 16 Sep 2024 08:16:21 +0300
Subject: [PATCH] disable closure iterator changes in #23787 unless
 `-d:nimOptIters` is enabled (#24108)

refs #24094, soft reverts #23787

#23787 turned out to cause issues as described in #24094, but the
changes are still positive, so it is now only enabled if compiling with
`-d:nimOptIters`. Unfortunately the changes are really interwoven with
each other so the checks for this switch in the code are a bit messy,
but searching for `nimOptIters` should give the necessary clues to
remove the switch properly later on.

Locally tested that nimlangserver works but others can also check.
---
 compiler/closureiters.nim                     | 128 ++++++++++++++----
 compiler/lambdalifting.nim                    |  36 ++++-
 compiler/transf.nim                           |  16 ++-
 tests/destructor/tuse_ownedref_after_move.nim |   2 +-
 tests/iter/tyieldintry.nim                    |   4 +-
 5 files changed, 150 insertions(+), 36 deletions(-)

diff --git a/compiler/closureiters.nim b/compiler/closureiters.nim
index 9ee394111ba26..8bdd04ca78e19 100644
--- a/compiler/closureiters.nim
+++ b/compiler/closureiters.nim
@@ -161,6 +161,10 @@ type
                     # is their finally. For finally it is parent finally. Otherwise -1
     idgen: IdGenerator
     varStates: Table[ItemId, int] # Used to detect if local variable belongs to multiple states
+    stateVarSym: PSym # :state variable. nil if env already introduced by lambdalifting
+      # remove if -d:nimOptIters is default, treating it as always nil
+    nimOptItersEnabled: bool # tracks if -d:nimOptIters is enabled
+      # should be default when issues are fixed, see #24094
 
 const
   nkSkip = {nkEmpty..nkNilLit, nkTemplateDef, nkTypeSection, nkStaticStmt,
@@ -170,8 +174,11 @@ const
   localRequiresLifting = -2
 
 proc newStateAccess(ctx: var Ctx): PNode =
-  result = rawIndirectAccess(newSymNode(getEnvParam(ctx.fn)),
+  if ctx.stateVarSym.isNil:
+    result = rawIndirectAccess(newSymNode(getEnvParam(ctx.fn)),
       getStateField(ctx.g, ctx.fn), ctx.fn.info)
+  else:
+    result = newSymNode(ctx.stateVarSym)
 
 proc newStateAssgn(ctx: var Ctx, toValue: PNode): PNode =
   # Creates state assignment:
@@ -189,12 +196,22 @@ proc newEnvVar(ctx: var Ctx, name: string, typ: PType): PSym =
   result.flags.incl sfNoInit
   assert(not typ.isNil, "Env var needs a type")
 
-  let envParam = getEnvParam(ctx.fn)
-  # let obj = envParam.typ.lastSon
-  result = addUniqueField(envParam.typ.elementType, result, ctx.g.cache, ctx.idgen)
+  if not ctx.stateVarSym.isNil:
+    # We haven't gone through labmda lifting yet, so just create a local var,
+    # it will be lifted later
+    if ctx.tempVars.isNil:
+      ctx.tempVars = newNodeI(nkVarSection, ctx.fn.info)
+      addVar(ctx.tempVars, newSymNode(result))
+  else:
+    let envParam = getEnvParam(ctx.fn)
+    # let obj = envParam.typ.lastSon
+    result = addUniqueField(envParam.typ.elementType, result, ctx.g.cache, ctx.idgen)
 
 proc newEnvVarAccess(ctx: Ctx, s: PSym): PNode =
-  result = rawIndirectAccess(newSymNode(getEnvParam(ctx.fn)), s, ctx.fn.info)
+  if ctx.stateVarSym.isNil:
+    result = rawIndirectAccess(newSymNode(getEnvParam(ctx.fn)), s, ctx.fn.info)
+  else:
+    result = newSymNode(s)
 
 proc newTempVarAccess(ctx: Ctx, s: PSym): PNode =
   result = newSymNode(s, ctx.fn.info)
@@ -246,12 +263,20 @@ proc newTempVarDef(ctx: Ctx, s: PSym, initialValue: PNode): PNode =
     v = ctx.g.emptyNode
   newTree(nkVarSection, newTree(nkIdentDefs, newSymNode(s), ctx.g.emptyNode, v))
 
+proc newEnvVarAsgn(ctx: Ctx, s: PSym, v: PNode): PNode
+
 proc newTempVar(ctx: var Ctx, typ: PType, parent: PNode, initialValue: PNode = nil): PSym =
-  result = newSym(skVar, getIdent(ctx.g.cache, ":tmpSlLower" & $ctx.tempVarId), ctx.idgen, ctx.fn, ctx.fn.info)
+  if ctx.nimOptItersEnabled:
+    result = newSym(skVar, getIdent(ctx.g.cache, ":tmpSlLower" & $ctx.tempVarId), ctx.idgen, ctx.fn, ctx.fn.info)
+  else:
+    result = ctx.newEnvVar(":tmpSlLower" & $ctx.tempVarId, typ)
   inc ctx.tempVarId
   result.typ = typ
   assert(not typ.isNil, "Temp var needs a type")
-  parent.add(ctx.newTempVarDef(result, initialValue))
+  if ctx.nimOptItersEnabled:
+    parent.add(ctx.newTempVarDef(result, initialValue))
+  elif initialValue != nil:
+    parent.add(ctx.newEnvVarAsgn(result, initialValue))
 
 proc hasYields(n: PNode): bool =
   # TODO: This is very inefficient. It traverses the node, looking for nkYieldStmt.
@@ -430,13 +455,24 @@ proc newTempVarAsgn(ctx: Ctx, s: PSym, v: PNode): PNode =
     result = newTree(nkFastAsgn, ctx.newTempVarAccess(s), v)
     result.info = v.info
 
+proc newEnvVarAsgn(ctx: Ctx, s: PSym, v: PNode): PNode =
+  # unused with -d:nimOptIters
+  if isEmptyType(v.typ):
+    result = v
+  else:
+    result = newTree(nkFastAsgn, ctx.newEnvVarAccess(s), v)
+    result.info = v.info
+
 proc addExprAssgn(ctx: Ctx, output, input: PNode, sym: PSym) =
+  var input = input
   if input.kind == nkStmtListExpr:
     let (st, res) = exprToStmtList(input)
     output.add(st)
-    output.add(ctx.newTempVarAsgn(sym, res))
-  else:
+    input = res
+  if ctx.nimOptItersEnabled:
     output.add(ctx.newTempVarAsgn(sym, input))
+  else:
+    output.add(ctx.newEnvVarAsgn(sym, input))
 
 proc convertExprBodyToAsgn(ctx: Ctx, exprBody: PNode, res: PSym): PNode =
   result = newNodeI(nkStmtList, exprBody.info)
@@ -565,7 +601,11 @@ proc lowerStmtListExprs(ctx: var Ctx, n: PNode, needsSplit: var bool): PNode =
         else:
           internalError(ctx.g.config, "lowerStmtListExpr(nkIf): " & $branch.kind)
 
-      if isExpr: result.add(ctx.newTempVarAccess(tmp))
+      if isExpr:
+        if ctx.nimOptItersEnabled:
+          result.add(ctx.newTempVarAccess(tmp))
+        else:
+          result.add(ctx.newEnvVarAccess(tmp))
 
   of nkTryStmt, nkHiddenTryStmt:
     var ns = false
@@ -595,7 +635,10 @@ proc lowerStmtListExprs(ctx: var Ctx, n: PNode, needsSplit: var bool): PNode =
           else:
             internalError(ctx.g.config, "lowerStmtListExpr(nkTryStmt): " & $branch.kind)
         result.add(n)
-        result.add(ctx.newTempVarAccess(tmp))
+        if ctx.nimOptItersEnabled:
+          result.add(ctx.newTempVarAccess(tmp))
+        else:
+          result.add(ctx.newEnvVarAccess(tmp))
 
   of nkCaseStmt:
     var ns = false
@@ -627,7 +670,10 @@ proc lowerStmtListExprs(ctx: var Ctx, n: PNode, needsSplit: var bool): PNode =
           else:
             internalError(ctx.g.config, "lowerStmtListExpr(nkCaseStmt): " & $branch.kind)
         result.add(n)
-        result.add(ctx.newTempVarAccess(tmp))
+        if ctx.nimOptItersEnabled:
+          result.add(ctx.newTempVarAccess(tmp))
+        else:
+          result.add(ctx.newEnvVarAccess(tmp))
       elif n[0].kind == nkStmtListExpr:
         result = newNodeI(nkStmtList, n.info)
         let (st, ex) = exprToStmtList(n[0])
@@ -660,7 +706,11 @@ proc lowerStmtListExprs(ctx: var Ctx, n: PNode, needsSplit: var bool): PNode =
         let tmp = ctx.newTempVar(cond.typ, result, cond)
         # result.add(ctx.newTempVarAsgn(tmp, cond))
 
-        var check = ctx.newTempVarAccess(tmp)
+        var check: PNode
+        if ctx.nimOptItersEnabled:
+          check = ctx.newTempVarAccess(tmp)
+        else:
+          check = ctx.newEnvVarAccess(tmp)
         if n[0].sym.magic == mOr:
           check = ctx.g.newNotCall(check)
 
@@ -670,12 +720,18 @@ proc lowerStmtListExprs(ctx: var Ctx, n: PNode, needsSplit: var bool): PNode =
           let (st, ex) = exprToStmtList(cond)
           ifBody.add(st)
           cond = ex
-        ifBody.add(ctx.newTempVarAsgn(tmp, cond))
+        if ctx.nimOptItersEnabled:
+          ifBody.add(ctx.newTempVarAsgn(tmp, cond))
+        else:
+          ifBody.add(ctx.newEnvVarAsgn(tmp, cond))
 
         let ifBranch = newTree(nkElifBranch, check, ifBody)
         let ifNode = newTree(nkIfStmt, ifBranch)
         result.add(ifNode)
-        result.add(ctx.newTempVarAccess(tmp))
+        if ctx.nimOptItersEnabled:
+          result.add(ctx.newTempVarAccess(tmp))
+        else:
+          result.add(ctx.newEnvVarAccess(tmp))
       else:
         for i in 0..<n.len:
           if n[i].kind == nkStmtListExpr:
@@ -686,7 +742,10 @@ proc lowerStmtListExprs(ctx: var Ctx, n: PNode, needsSplit: var bool): PNode =
           if n[i].kind in nkCallKinds: # XXX: This should better be some sort of side effect tracking
             let tmp = ctx.newTempVar(n[i].typ, result, n[i])
             # result.add(ctx.newTempVarAsgn(tmp, n[i]))
-            n[i] = ctx.newTempVarAccess(tmp)
+            if ctx.nimOptItersEnabled:
+              n[i] = ctx.newTempVarAccess(tmp)
+            else:
+              n[i] = ctx.newEnvVarAccess(tmp)
 
         result.add(n)
 
@@ -1284,6 +1343,13 @@ proc wrapIntoStateLoop(ctx: var Ctx, n: PNode): PNode =
   result.info = n.info
 
   let localVars = newNodeI(nkStmtList, n.info)
+  if not ctx.stateVarSym.isNil:
+    let varSect = newNodeI(nkVarSection, n.info)
+    addVar(varSect, newSymNode(ctx.stateVarSym))
+    localVars.add(varSect)
+
+    if not ctx.tempVars.isNil:
+      localVars.add(ctx.tempVars)
 
   let blockStmt = newNodeI(nkBlockStmt, n.info)
   blockStmt.add(newSymNode(ctx.stateLoopLabel))
@@ -1486,11 +1552,21 @@ proc liftLocals(c: var Ctx, n: PNode): PNode =
       n[i] = liftLocals(c, n[i])
 
 proc transformClosureIterator*(g: ModuleGraph; idgen: IdGenerator; fn: PSym, n: PNode): PNode =
-  var ctx = Ctx(g: g, fn: fn, idgen: idgen)
-
-  # The transformation should always happen after at least partial lambdalifting
-  # is performed, so that the closure iter environment is always created upfront.
-  doAssert(getEnvParam(fn) != nil, "Env param not created before iter transformation")
+  var ctx = Ctx(g: g, fn: fn, idgen: idgen,
+    # should be default when issues are fixed, see #24094:
+    nimOptItersEnabled: isDefined(g.config, "nimOptIters"))
+
+  if getEnvParam(fn).isNil:
+    if ctx.nimOptItersEnabled:
+      # The transformation should always happen after at least partial lambdalifting
+      # is performed, so that the closure iter environment is always created upfront.
+      doAssert(false, "Env param not created before iter transformation")
+    else:
+      # Lambda lifting was not done yet. Use temporary :state sym, which will
+      # be handled specially by lambda lifting. Local temp vars (if needed)
+      # should follow the same logic.
+      ctx.stateVarSym = newSym(skVar, getIdent(ctx.g.cache, ":state"), idgen, fn, fn.info)
+      ctx.stateVarSym.typ = g.createClosureIterStateType(fn, idgen)
 
   ctx.stateLoopLabel = newSym(skLabel, getIdent(ctx.g.cache, ":stateLoop"), idgen, fn, fn.info)
   var pc = PreprocessContext(finallys: @[], config: g.config, idgen: idgen)
@@ -1516,9 +1592,10 @@ proc transformClosureIterator*(g: ModuleGraph; idgen: IdGenerator; fn: PSym, n:
   let caseDispatcher = newTreeI(nkCaseStmt, n.info,
       ctx.newStateAccess())
 
-  # Lamdalifting will not touch our locals, it is our responsibility to lift those that
-  # need it.
-  detectCapturedVars(ctx)
+  if ctx.nimOptItersEnabled:
+    # Lamdalifting will not touch our locals, it is our responsibility to lift those that
+    # need it.
+    detectCapturedVars(ctx)
 
   for s in ctx.states:
     let body = ctx.transformStateAssignments(s.body)
@@ -1527,7 +1604,8 @@ proc transformClosureIterator*(g: ModuleGraph; idgen: IdGenerator; fn: PSym, n:
   caseDispatcher.add newTreeI(nkElse, n.info, newTreeI(nkReturnStmt, n.info, g.emptyNode))
 
   result = wrapIntoStateLoop(ctx, caseDispatcher)
-  result = liftLocals(ctx, result)
+  if ctx.nimOptItersEnabled:
+    result = liftLocals(ctx, result)
 
   when false:
     echo "TRANSFORM TO STATES: "
diff --git a/compiler/lambdalifting.nim b/compiler/lambdalifting.nim
index 2a72c3846d0a1..2d38d9a69b405 100644
--- a/compiler/lambdalifting.nim
+++ b/compiler/lambdalifting.nim
@@ -150,7 +150,7 @@ template isIterator*(owner: PSym): bool =
 
 proc createEnvObj(g: ModuleGraph; idgen: IdGenerator; owner: PSym; info: TLineInfo): PType =
   result = createObj(g, idgen, owner, info, final=false)
-  if owner.isIterator:
+  if owner.isIterator or not isDefined(g.config, "nimOptIters"):
     rawAddField(result, createStateField(g, owner, idgen))
 
 proc getClosureIterResult*(g: ModuleGraph; iter: PSym; idgen: IdGenerator): PSym =
@@ -228,6 +228,13 @@ proc makeClosure*(g: ModuleGraph; idgen: IdGenerator; prc: PSym; env: PNode; inf
   if tfHasAsgn in result.typ.flags or optSeqDestructors in g.config.globalOptions:
     prc.flags.incl sfInjectDestructors
 
+proc interestingIterVar(s: PSym): bool {.inline.} =
+  # unused with -d:nimOptIters
+  # XXX optimization: Only lift the variable if it lives across
+  # yield/return boundaries! This can potentially speed up
+  # closure iterators quite a bit.
+  result = s.kind in {skResult, skVar, skLet, skTemp, skForVar} and sfGlobal notin s.flags
+
 template liftingHarmful(conf: ConfigRef; owner: PSym): bool =
   ## lambda lifting can be harmful for JS-like code generators.
   let isCompileTime = sfCompileTime in owner.flags or owner.kind == skMacro
@@ -274,6 +281,16 @@ proc liftIterSym*(g: ModuleGraph; n: PNode; idgen: IdGenerator; owner: PSym): PN
   createTypeBoundOpsLL(g, env.typ, n.info, idgen, owner)
   result.add makeClosure(g, idgen, iter, env, n.info)
 
+proc freshVarForClosureIter*(g: ModuleGraph; s: PSym; idgen: IdGenerator; owner: PSym): PNode =
+  # unused with -d:nimOptIters
+  let envParam = getHiddenParam(g, owner)
+  let obj = envParam.typ.skipTypes({tyOwned, tyRef, tyPtr})
+  let field = addField(obj, s, g.cache, idgen)
+
+  var access = newSymNode(envParam)
+  assert obj.kind == tyObject
+  result = rawIndirectAccess(access, field, s.info)
+
 # ------------------ new stuff -------------------------------------------
 
 proc markAsClosure(g: ModuleGraph; owner: PSym; n: PNode) =
@@ -322,7 +339,7 @@ proc getEnvTypeForOwner(c: var DetectionPass; owner: PSym;
   result = c.ownerToType.getOrDefault(owner.id)
   if result.isNil:
     let env = getEnvParam(owner)
-    if env.isNil or not owner.isIterator:
+    if env.isNil or not owner.isIterator or not isDefined(c.graph.config, "nimOptIters"):
       result = newType(tyRef, c.idgen, owner)
       let obj = createEnvObj(c.graph, c.idgen, owner, info)
       rawAddSon(result, obj)
@@ -443,6 +460,17 @@ proc detectCapturedVars(n: PNode; owner: PSym; c: var DetectionPass) =
       if owner.isIterator:
         c.somethingToDo = true
         addClosureParam(c, owner, n.info)
+        if not isDefined(c.graph.config, "nimOptIters") and interestingIterVar(s):
+          if not c.capturedVars.contains(s.id):
+            if not c.inTypeOf: c.capturedVars.incl(s.id)
+            let obj = getHiddenParam(c.graph, owner).typ.skipTypes({tyOwned, tyRef, tyPtr})
+            #let obj = c.getEnvTypeForOwner(s.owner).skipTypes({tyOwned, tyRef, tyPtr})
+
+            if s.name.id == getIdent(c.graph.cache, ":state").id:
+              obj.n[0].sym.flags.incl sfNoInit
+              obj.n[0].sym.itemId = ItemId(module: s.itemId.module, item: -s.itemId.item)
+            else:
+              discard addField(obj, s, c.graph.cache, c.idgen)
     # direct or indirect dependency:
     elif innerClosure or interested:
       discard """
@@ -746,6 +774,8 @@ proc liftCapturedVars(n: PNode; owner: PSym; d: var DetectionPass;
     elif s.id in d.capturedVars:
       if s.owner != owner:
         result = accessViaEnvParam(d.graph, n, owner)
+      elif owner.isIterator and not isDefined(d.graph.config, "nimOptIters") and interestingIterVar(s):
+        result = accessViaEnvParam(d.graph, n, owner)
       else:
         result = accessViaEnvVar(n, owner, d, c)
   of nkEmpty..pred(nkSym), succ(nkSym)..nkNilLit, nkComesFrom,
@@ -863,7 +893,7 @@ proc liftLambdas*(g: ModuleGraph; fn: PSym, body: PNode; tooEarly: var bool;
     # ignore forward declaration:
     result = body
     tooEarly = true
-    if fn.isIterator:
+    if fn.isIterator and isDefined(g.config, "nimOptIters"):
       var d = initDetectionPass(g, fn, idgen)
       addClosureParam(d, fn, body.info)
   else:
diff --git a/compiler/transf.nim b/compiler/transf.nim
index 52b5fc1ffb45b..0a4579171d198 100644
--- a/compiler/transf.nim
+++ b/compiler/transf.nim
@@ -98,7 +98,10 @@ proc newTemp(c: PTransf, typ: PType, info: TLineInfo): PNode =
   r.typ = typ #skipTypes(typ, {tyGenericInst, tyAlias, tySink})
   incl(r.flags, sfFromGeneric)
   let owner = getCurrOwner(c)
-  result = newSymNode(r)
+  if owner.isIterator and not c.tooEarly and not isDefined(c.graph.config, "nimOptIters"):
+    result = freshVarForClosureIter(c.graph, r, c.idgen, owner)
+  else:
+    result = newSymNode(r)
 
 proc transform(c: PTransf, n: PNode): PNode
 
@@ -174,10 +177,13 @@ proc transformSym(c: PTransf, n: PNode): PNode =
 
 proc freshVar(c: PTransf; v: PSym): PNode =
   let owner = getCurrOwner(c)
-  var newVar = copySym(v, c.idgen)
-  incl(newVar.flags, sfFromGeneric)
-  newVar.owner = owner
-  result = newSymNode(newVar)
+  if owner.isIterator and not c.tooEarly and not isDefined(c.graph.config, "nimOptIters"):
+    result = freshVarForClosureIter(c.graph, v, c.idgen, owner)
+  else:
+    var newVar = copySym(v, c.idgen)
+    incl(newVar.flags, sfFromGeneric)
+    newVar.owner = owner
+    result = newSymNode(newVar)
 
 proc transformVarSection(c: PTransf, v: PNode): PNode =
   result = newTransNode(v)
diff --git a/tests/destructor/tuse_ownedref_after_move.nim b/tests/destructor/tuse_ownedref_after_move.nim
index 34d5be65a4137..69348d53033bc 100644
--- a/tests/destructor/tuse_ownedref_after_move.nim
+++ b/tests/destructor/tuse_ownedref_after_move.nim
@@ -1,6 +1,6 @@
 discard """
   cmd: '''nim c --newruntime $file'''
-  errormsg: "'=copy' is not available for type <owned Button>; requires a copy because it's not the last read of ':envAlt.b0'; routine: main"
+  errormsg: "'=copy' is not available for type <owned Button>; requires a copy because it's not the last read of ':envAlt.b1'; routine: main"
   line: 48
 """
 
diff --git a/tests/iter/tyieldintry.nim b/tests/iter/tyieldintry.nim
index 04409795b0763..e51ab7f0d59a1 100644
--- a/tests/iter/tyieldintry.nim
+++ b/tests/iter/tyieldintry.nim
@@ -1,5 +1,5 @@
 discard """
-  matrix: "; --experimental:strictdefs"
+  matrix: "; --experimental:strictdefs; -d:nimOptIters"
   targets: "c cpp"
 """
 
@@ -505,7 +505,7 @@ block: # void iterator
       discard
   var a = it
 
-block: # Locals present in only 1 state should be on the stack
+if defined(nimOptIters): # Locals present in only 1 state should be on the stack
   proc checkOnStack(a: pointer, shouldBeOnStack: bool) =
     # Quick and dirty way to check if a points to stack
     var dummy = 0