yet another attempt at #25333

compiler/astdef.nim

@@ -319,8 +319,12 @@ type
     nfDefaultRefsParam # a default param value references another parameter
                        # the flag is applied to proc default values and to calls
     nfExecuteOnReload  # A top-level statement that will be executed during reloads
-    nfLastRead  # this node is a last read
     nfFirstWrite # this node is a first write
+    nfLastUse   # this node is a last use (a "last read" that was computed
+                # before lambda lifting; the move analyser trusts it for
+                # captured variables turned into `env[].field` accesses).
+                # Deliberately not serialized through IC: it is a derived
+                # property recomputed each run.
     nfHasComment # node has a comment
     nfSkipFieldChecking # node skips field visable checking
     nfDisabledOpenSym # temporary: node should be nkOpenSym but cannot
@@ -869,7 +873,7 @@ const
                                       nfDotSetter, nfDotField,
                                       nfIsRef, nfIsPtr, nfPreventCg, nfLL,
                                       nfFromTemplate, nfDefaultRefsParam,
-                                      nfExecuteOnReload, nfLastRead,
+                                      nfExecuteOnReload,
                                       nfFirstWrite, nfSkipFieldChecking,
                                       nfDisabledOpenSym, nfLazyType}
   namePos* = 0

compiler/dfa.nim

@@ -489,3 +489,51 @@ proc constructCfg*(s: PSym; body: PNode; root: PSym): ControlFlowGraph =
     shallowCopy(result, c.code)
   when false:
     optimizeJumps result
+
+proc isLastReadOfRoot*(g: ControlFlowGraph; n: PNode; start: int;
+                       otherUsage: var TLineInfo): bool =
+  ## Walks the control flow graph `g` starting at instruction index `start`
+  ## (which must be the index *after* the `use` of `n`) and returns whether
+  ## `n` is the last read of its location. On a conflicting later use/def the
+  ## location of the offending instruction is stored in `otherUsage`.
+  ##
+  ## This is the shared core of the move analyser; it is used both by
+  ## `injectdestructors` (post lambda lifting) and by the pre-lifting marking
+  ## pass in `lambdalifting` that stamps `nfLastUse` on captured variables.
+  var pcs = @[start]
+  var marked = initIntSet()
+  result = true
+  while pcs.len > 0:
+    var pc = pcs.pop()
+    if not marked.contains(pc):
+      let oldPc = pc
+      while pc < g.len:
+        case g[pc].kind
+        of loop:
+          let back = pc + g[pc].dest
+          if not marked.containsOrIncl(back):
+            pc = back
+          else:
+            break
+        of goto:
+          pc = pc + g[pc].dest
+        of fork:
+          if not marked.contains(pc+1):
+            pcs.add pc + 1
+          pc = pc + g[pc].dest
+        of use:
+          if g[pc].n.aliases(n) != no or n.aliases(g[pc].n) != no:
+            otherUsage = g[pc].n.info
+            return false
+          inc pc
+        of def:
+          if g[pc].n.aliases(n) == yes:
+            # the path leads to a redefinition of 's' --> sink 's'.
+            break
+          elif n.aliases(g[pc].n) != no:
+            # only partially writes to 's' --> can't sink 's', so this def reads 's'
+            # or maybe writes to 's' --> can't sink 's'
+            otherUsage = g[pc].n.info
+            return false
+          inc pc
+      marked.incl oldPc

compiler/ic/enum2nif.nim

@@ -1467,12 +1467,12 @@ proc genFlags*(s: set[TNodeFlag]; dest: var string) =
     of nfDefaultParam: dest.add "d1"
     of nfDefaultRefsParam: dest.add "d2"
     of nfExecuteOnReload: dest.add "o"
-    of nfLastRead: dest.add "l0"
     of nfFirstWrite: dest.add "w"
+    of nfLastUse: dest.add "u"
     of nfHasComment: dest.add "h"
     of nfSkipFieldChecking: dest.add "s0"
     of nfDisabledOpenSym: dest.add "d3"
-    of nfLazyType: dest.add "l1"
+    of nfLazyType: dest.add "l0"
 
 
 proc parse*(t: typedesc[TNodeFlag]; s: string): set[TNodeFlag] =
@@ -1513,9 +1513,6 @@ proc parse*(t: typedesc[TNodeFlag]; s: string): set[TNodeFlag] =
     of 'i': result.incl nfIsRef
     of 'l':
       if i+1 < s.len and s[i+1] == '0':
-        result.incl nfLastRead
-        inc i
-      elif i+1 < s.len and s[i+1] == '1':
         result.incl nfLazyType
         inc i
       else: result.incl nfLL
@@ -1533,6 +1530,7 @@ proc parse*(t: typedesc[TNodeFlag]; s: string): set[TNodeFlag] =
         inc i
       else: result.incl nfSem
     of 't': result.incl nfTransf
+    of 'u': result.incl nfLastUse
     of 'w': result.incl nfFirstWrite
     else: discard
     inc i

compiler/injectdestructors.nim

@@ -121,47 +121,7 @@ proc isLastReadImpl(n: PNode; c: var Con; scope: var Scope): bool =
     inc j
   c.otherUsage = unknownLineInfo
   if j < c.g.len:
-    var pcs = @[j+1]
-    var marked = initIntSet()
-    result = true
-    while pcs.len > 0:
-      var pc = pcs.pop()
-      if not marked.contains(pc):
-        let oldPc = pc
-        while pc < c.g.len:
-          dbg:
-            echo "EXEC ", c.g[pc].kind, " ", pc, " ", n
-          when false:
-            inc perfCounters[c.g[pc].kind]
-          case c.g[pc].kind
-          of loop:
-            let back = pc + c.g[pc].dest
-            if not marked.containsOrIncl(back):
-              pc = back
-            else:
-              break
-          of goto:
-            pc = pc + c.g[pc].dest
-          of fork:
-            if not marked.contains(pc+1):
-              pcs.add pc + 1
-            pc = pc + c.g[pc].dest
-          of use:
-            if c.g[pc].n.aliases(n) != no or n.aliases(c.g[pc].n) != no:
-              c.otherUsage = c.g[pc].n.info
-              return false
-            inc pc
-          of def:
-            if c.g[pc].n.aliases(n) == yes:
-              # the path leads to a redefinition of 's' --> sink 's'.
-              break
-            elif n.aliases(c.g[pc].n) != no:
-              # only partially writes to 's' --> can't sink 's', so this def reads 's'
-              # or maybe writes to 's' --> can't sink 's'
-              c.otherUsage = c.g[pc].n.info
-              return false
-            inc pc
-        marked.incl oldPc
+    result = isLastReadOfRoot(c.g, n, j+1, c.otherUsage)
   else:
     result = false
 
@@ -170,8 +130,16 @@ template hasDestructorOrAsgn(c: var Con, typ: PType): bool =
   hasDestructor(c, typ) or (c.graph.config.selectedGC in {gcArc, gcOrc, gcYrc, gcAtomicArc} and
         typ.kind == tyObject and not isTrivial(getAttachedOp(c.graph, typ, attachedAsgn)))
 
+proc isLastUseMarked(n: PNode): bool =
+  ## A captured variable read that the move analyser proved was a last read
+  ## *before* lambda lifting (see `markLastUses` in lambdalifting). After
+  ## lifting it became an `env[].field` access that we can no longer analyse
+  ## here, so we trust the recorded verdict.
+  nfLastUse in skipConvDfa(n).flags
+
 proc isLastRead(n: PNode; c: var Con; s: var Scope): bool =
   if not hasDestructorOrAsgn(c, n.typ): return true
+  if isLastUseMarked(n): return true
 
   let m = skipConvDfa(n)
   result = isLastReadImpl(n, c, s)
@@ -834,7 +802,8 @@ proc p(n: PNode; c: var Con; s: var Scope; mode: ProcessMode; tmpFlags = {sfSing
     elif n.kind in {nkBracket, nkObjConstr, nkTupleConstr, nkClosure, nkNilLit} +
          nkCallKinds + nkLiterals:
       result = p(n, c, s, consumed)
-    elif ((n.kind == nkSym and isSinkParam(n.sym)) or isAnalysableFieldAccess(n, c.owner)) and
+    elif ((n.kind == nkSym and isSinkParam(n.sym)) or isAnalysableFieldAccess(n, c.owner) or
+          isLastUseMarked(n)) and
         isLastRead(n, c, s) and not (n.kind == nkSym and isCursor(n)):
       # Sinked params can be consumed only once. We need to reset the memory
       # to disable the destructor which we have not elided
@@ -1077,7 +1046,7 @@ proc p(n: PNode; c: var Con; s: var Scope; mode: ProcessMode; tmpFlags = {sfSing
       for i in 1 ..< n.len:
         result[i] = n[i]
       if mode == sinkArg and hasDestructor(c, n.typ):
-        if isAnalysableFieldAccess(n, c.owner) and isLastRead(n, c, s):
+        if (isAnalysableFieldAccess(n, c.owner) or isLastUseMarked(n)) and isLastRead(n, c, s):
           s.wasMoved.add c.genWasMoved(n)
         else:
           result = passCopyToSink(result, c, s)
@@ -1278,8 +1247,8 @@ proc moveOrCopy(dest, ri: PNode; c: var Con; s: var Scope, flags: set[MoveOrCopy
 
       if isOwnsData != nil:
         result = moveOrCopy(dest, isOwnsData, c, s, flags)
-      elif isAnalysableFieldAccess(ri, c.owner) and isLastRead(ri, c, s) and
-          canBeMoved(c, dest.typ):
+      elif (isAnalysableFieldAccess(ri, c.owner) or isLastUseMarked(ri)) and
+          isLastRead(ri, c, s) and canBeMoved(c, dest.typ):
         # Rule 3: `=sink`(x, z); wasMoved(z)
         let snk = c.genSink(s, dest, ri, flags)
         result = newTree(nkStmtList, snk, c.genWasMoved(ri))

compiler/lambdalifting.nim

@@ -12,7 +12,7 @@
 import
   options, ast, astalgo, msgs,
   idents, renderer, types, magicsys, lowerings, modulegraphs, lineinfos,
-  transf, liftdestructors, typeallowed
+  transf, liftdestructors, typeallowed, dfa, aliasanalysis
 
 import std/[strutils, tables, intsets]
 
@@ -553,7 +553,10 @@ proc accessViaEnvParam(g: ModuleGraph; n: PNode; owner: PSym): PNode =
       assert obj.kind == tyObject
       let field = getFieldFromObj(obj, s)
       if field != nil:
-        return rawIndirectAccess(access, field, n.info)
+        result = rawIndirectAccess(access, field, n.info)
+        # carry the pre-lifting move-analysis verdict onto the env access:
+        if nfLastUse in n.flags: result.flags.incl nfLastUse
+        return result
       let upField = lookupInRecord(obj.n, getIdent(g.cache, upName))
       if upField == nil: break
       access = rawIndirectAccess(access, upField, n.info)
@@ -718,6 +721,8 @@ proc accessViaEnvVar(n: PNode; owner: PSym; d: var DetectionPass;
   let field = getFieldFromObj(obj, n.sym)
   if field != nil:
     result = rawIndirectAccess(access, field, n.info)
+    # carry the pre-lifting move-analysis verdict onto the env access:
+    if nfLastUse in n.flags: result.flags.incl nfLastUse
   else:
     localError(d.graph.config, n.info, "internal error: not part of closure object type")
     result = n
@@ -874,6 +879,64 @@ proc semCaptureSym*(s, owner: PSym) =
     # since the analysis is not entirely correct, we don't set 'tfCapturesEnv'
     # here
 
+proc collectRoots(n: PNode; fn: PSym; roots: var seq[PSym]; seen: var IntSet) =
+  case n.kind
+  of nkSym:
+    let s = n.sym
+    # captured == defined in an enclosing routine; `fn`'s own locals/params
+    # (owner == fn) are not lifted into a shared env this way.
+    if s.owner != fn and interestingVar(s) and hasDestructor(s.typ) and
+        not seen.containsOrIncl(s.id):
+      roots.add s
+  of nkEmpty..pred(nkSym), succ(nkSym)..nkNilLit, nkTypeSection,
+      nkProcDef, nkFuncDef, nkMethodDef, nkConverterDef, nkMacroDef,
+      nkIteratorDef, nkTemplateDef, nkLambdaKinds:
+    # do not descend into nested routine definitions: their captured
+    # variables get their own marking pass when they are transformed.
+    discard
+  else:
+    for child in n: collectRoots(child, fn, roots, seen)
+
+proc markLastUses*(g: ModuleGraph; fn: PSym; body: PNode) =
+  ## Move analysis *before* lambda lifting. A variable defined in an enclosing
+  ## routine and captured by `fn` is, at this point, still an ordinary
+  ## non-escaping local that the move analyser can reason about. We compute its
+  ## last reads inside `fn`'s body and stamp them with `nfLastUse`. After
+  ## lifting these reads become `env[].field` accesses through a shared `ref`;
+  ## the move analyser in `injectdestructors` can no longer prove they are last
+  ## reads (the env may be aliased), so it trusts the `nfLastUse` flag instead
+  ## (bug #25333). The flag is carried onto the rewritten access by
+  ## `accessViaEnvParam`/`accessViaEnvVar`.
+  ##
+  ## Must run before `transformClosureIterator` turns the body into a state
+  ## machine (whose back-edges would defeat the analysis) and before the
+  ## enclosing lifting pass rewrites the captured reads.
+  # Only *closure iterators* are safe: their environment is created per
+  # instantiation and the body runs to completion exactly once, so a statically
+  # last read is also dynamically the last read. A regular closure's env
+  # persists across invocations, so the body may run again and re-read the
+  # variable; moving it out on a "last read" would corrupt the next call. Hence
+  # we must not mark captured reads in regular closures (bug #25333).
+  if not fn.isIterator: return
+  if optSeqDestructors notin g.config.globalOptions: return
+
+  var roots: seq[PSym] = @[]
+  var seen = initIntSet()
+  collectRoots(body, fn, roots, seen)
+  if roots.len == 0: return
+
+  for root in roots:
+    let cfg = constructCfg(fn, body, root)
+    var otherUsage = unknownLineInfo
+    for j in 0 ..< cfg.len:
+      if cfg[j].kind == use:
+        let m = skipConvDfa(cfg[j].n)
+        # only whole-variable reads can be safely propagated through lifting;
+        # partial accesses (`v.field`, `v[i]`) keep the old behaviour.
+        if m.kind == nkSym and m.sym == root and
+            isLastReadOfRoot(cfg, cfg[j].n, j+1, otherUsage):
+          m.flags.incl nfLastUse
+
 proc liftIterToProc*(g: ModuleGraph; fn: PSym; body: PNode; ptrType: PType;
                      idgen: IdGenerator): PNode =
   var d = initDetectionPass(g, fn, idgen)

compiler/transf.nim

@@ -1392,6 +1392,10 @@ proc transformBody*(g: ModuleGraph; idgen: IdGenerator; prc: PSym; flags: Transf
     liftDefer(c, result)
     result = liftLocalsIfRequested(prc, result, g.cache, g.config, c.idgen)
 
+    # mark last reads of captured variables before the closure-iterator state
+    # machine and the enclosing lifting pass rewrite them (bug #25333):
+    markLastUses(g, prc, result)
+
     if prc.isIterator:
       result = g.transformClosureIterator(c.idgen, prc, result)