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.
2026-04-19 14:00:35 +00:00 · 2024-09-16 08:16:21 +03:00
parent ecc6a1d92b
commit 22d2cf2175
5 changed files with 149 additions and 35 deletions
--- 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)
+  var ctx = Ctx(g: g, fn: fn, idgen: idgen,
+    # should be default when issues are fixed, see #24094:
+    nimOptItersEnabled: isDefined(g.config, "nimOptIters"))

-  # 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")
+  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: "
--- 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:
--- 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)
--- 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
 """

--- 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