for loops over .passive iterators: three fixes, no new protocol for now

lib/std/system.nim

@@ -278,6 +278,10 @@ type
   CoroutineBase* = object of RootObj
     caller*: Continuation
     callee*: ptr CoroutineBase
+    yielded*: bool
+      ## Set by an iterator's `yield`, cleared by the `for` loop that takes the
+      ## value (`iterYielded`). A step that lands in this frame is not by
+      ## itself a yield: a passive proc the iterator called returns into it too.
 
 method cancel*(coro: ptr CoroutineBase) =
   discard "to override"
@@ -336,6 +340,23 @@ proc stopping*(c: Continuation): bool {.inline.} =
   ## True when a coroutine has no next step: either finished or parked.
   c.fn == nil
 
+proc iterYielded*(c: Continuation; myEnv: ptr CoroutineBase): bool {.inline.} =
+  ## Used by the compiler: did the `for` loop's iterator just yield? Its own
+  ## frame has to be the one that stopped, AND it has to have stopped at a
+  ## `yield` — a passive proc it called returns into the same frame without
+  ## producing a value, and the loop ran its body again for the previous one.
+  result = c.env != nil and c.env == myEnv and c.env.yielded
+  if result: c.env.yielded = false
+
+proc iterStopped*(c: Continuation): bool {.inline.} =
+  ## Used by the compiler: the `for` loop's exit test. A PARKED iterator has
+  ## not ended — but this loop cannot wait for it, because the loop and the
+  ## frame it would cancel are both driven from this stack — so it says so
+  ## instead of ending quietly and cancelling a frame the I/O ring still owns.
+  if parked(c):
+    panic "a `.passive` iterator parked inside a `for` loop\n"
+  result = c.fn == nil
+
 proc finished*(c: Continuation): bool {.inline.} =
   ## True once a coroutine has run to completion. Compatible with
   ## Nim's `finished` builtin: returns `true` when there are no more values

src/hexer/coro_transform.nim

@@ -67,6 +67,7 @@ const
   FnFieldName* = "fn.0"
   EnvFieldName* = "env.0"
   CallerFieldName* = "caller.0"
+  YieldedFieldName* = "yielded.0"
   CalleeFieldName* = "callee.0"
   ResultParamName* = "`result.0"
   ResultFieldName* = "`result.0"
@@ -668,8 +669,8 @@ proc emitWhileBegin*(dest: var TokenBuf; info: NifLineInfo;
   ##   try:
   ##     loop:
   ##       it = advance(it)
-  ##       ite stopping(it): jmp exitLab
-  ##       ite it.env == myEnv:
+  ##       ite iterStopped(it): jmp exitLab
+  ##       ite iterYielded(it, myEnv):
   ##         <body-stmts goes here — emit between begin and end>
   ##       continue
   ##     lab exitLab
@@ -678,7 +679,7 @@ proc emitWhileBegin*(dest: var TokenBuf; info: NifLineInfo;
   ## `exitLab`.
   let envFieldSym = pool.symId(EnvFieldName)
   let advanceSym = pool.symId("advance.0." & SystemModuleSuffix)
-  let stoppingSym = pool.symId("stopping.0." & SystemModuleSuffix)
+  let stoppingSym = pool.symId("iterStopped.0." & SystemModuleSuffix)
 
   dest.copyIntoKind LetS, info:
     dest.addSymDef myEnvSym, info
@@ -706,9 +707,9 @@ proc emitWhileBegin*(dest: var TokenBuf; info: NifLineInfo;
         dest.addSymUse exitLab, info
     dest.addDotToken()
   dest.addParLe IteV, info
-  dest.copyIntoKind EqX, info:
-    dest.addParPair PointerT, info
-    emitItEnv(dest, info, itSym, envFieldSym)
+  dest.copyIntoKind CallS, info:
+    dest.addSymUse pool.symId("iterYielded.0." & SystemModuleSuffix), info
+    dest.addSymUse itSym, info
     dest.addSymUse myEnvSym, info
   dest.addParLe StmtsS, info     # body-stmts open
 
@@ -809,11 +810,14 @@ proc trCoroFor*(c: var Context; dest: var TokenBuf; n: var Cursor) =
       dest.addSymUse pool.symId(ContinuationName), info
       dest.copyIntoKind CallS, info:
         dest.add targetBuf
+        # Through `coroTr`: in a coroutine the `for` loop's variable is a frame
+        # field, and `(haddr x)` has to name it there. Copied verbatim, the
+        # iterator wrote through a pointer to a local that no longer exists.
         var w = argsStart
         for i in 0 ..< realArgCount:
-          dest.takeTree w
+          coroTr(c, dest, w)
         var addrW = lastArgPos
-        dest.takeTree addrW
+        coroTr(c, dest, addrW)
         emitStopContinuation(dest, info)
 
     let myEnvSym = pool.symId("`coroEnv." & $c.currentProc.counter)
@@ -1057,6 +1061,16 @@ proc trYield*(c: var Context; dest: var TokenBuf; n: var Cursor) =
   assert state != -1
   let info = n.info
   returnValue(c, dest, n, info)
+  # `this.yielded = true`: what tells the for-loop trampoline that THIS step
+  # produced a value. The continuation alone cannot — a passive proc the
+  # iterator called returns into this same frame (`iterYielded`).
+  dest.copyIntoKind AsgnS, info:
+    dest.copyIntoKind DotX, info:
+      dest.copyIntoKind DerefX, info:
+        dest.addSymUse pool.symId(EnvParamName), info
+      dest.addSymUse pool.symId(YieldedFieldName), info
+      dest.addIntLit 1, info # field is in superclass
+    dest.addParPair TrueX, info
   stashResumeFn(c, dest, state, info)
   dest.copyIntoKind RetS, info:
     contNextState(c, dest, state, info)
@@ -1262,6 +1276,37 @@ proc containsSuspensionPoint*(c: var Context; n: Cursor): bool =
       break
   c.typeCache.closeScope()
 
+proc suspendsHere(c: var Context; n: Cursor): bool =
+  ## Helper of `forBodySuspends`; the scope is the caller's.
+  if n.stmtKind == YldS or c.hooks.isPassiveCall(c, n) or n.exprKind == SuspendX:
+    return true
+  if n.kind != TagLit: return false
+  let sk = n.stmtKind
+  if sk in {LetS, CursorS, PatternvarS, VarS, TvarS, TletS, GvarS, GletS}:
+    var d = n.childCursor      # register it, see `containsSuspensionPoint`
+    let name = d.symId
+    inc d                      # name
+    skip d, SkipExport         # export marker
+    skip d, SkipPragmas        # pragmas
+    c.typeCache.registerLocal(name, cast[SymKind](sk), d)
+  result = false
+  var ch = sub(n)
+  var first = true
+  while ch.hasMore:
+    # A NESTED `for`'s iterator call is that trampoline's business, exactly as
+    # this one's is ours — it is not a suspension point of the routine.
+    if not (sk == CoroforS and first):
+      if suspendsHere(c, ch): return true
+    skip ch
+    first = false
+
+proc forBodySuspends*(c: var Context; n: Cursor): bool =
+  ## Does a `for` body need a state boundary? The trampoline `trCoroFor` builds
+  ## is a loop inside ONE state proc, so there is nowhere for one to go.
+  c.typeCache.openScope()
+  result = suspendsHere(c, n)
+  c.typeCache.closeScope()
+
 const
   KeptLoop* = -1
     ## `ProcContext.loopHeads` entry for a loop kept as a `(loop ...)`
@@ -1564,8 +1609,8 @@ proc trGoto*(c: var Context; dest: var TokenBuf; n: var Cursor) =
         of CallS, CmdS, ResultS, ProcS, FuncS, IteratorS,
             ConverterS, MethodS, MacroS, TemplateS, TypeS,
             BlockS, EmitS, IfS, WhenS,
-            BreakS, ContinueS, ForS, WhileS, CoroforS,
-            RetS, YldS, PragmasS, PragmaxS, InclS, ExclS,
+            BreakS, ContinueS, ForS, WhileS,
+            RetS, YldS, PragmasS, InclS, ExclS,
             IncludeS, ImportS, ImportasS, FromimportS,
             ImportexceptS, ExportS, ExportexceptS, CommentS,
             DiscardS, UnpackdeclS, AssumeS,
@@ -1577,6 +1622,42 @@ proc trGoto*(c: var Context; dest: var TokenBuf; n: var Cursor) =
             while n.hasMore:
               trGoto c, dest, n
           dest.addParRi()
+        of CoroforS:
+          # A `for` loop over a `.passive` iterator stays a construct: the
+          # trampoline `trCoroFor` expands it into is a loop inside ONE state
+          # proc. The iterator call is therefore NOT a suspension point of this
+          # routine — walking it as one put a state boundary inside the loop
+          # and produced a state proc nested in it.
+          dest.addParLe(n.cursorTagId, n.info)
+          n.into:
+            dest.takeTree n                         # the iterator call
+            if forBodySuspends(c, n):
+              # One state proc holds the whole loop, so there is nowhere for a
+              # suspension inside the body to go.
+              quit infoToStr(n.info) &
+                " Error: a `for` loop over a `.passive` iterator cannot suspend" &
+                " in its body"
+            trGotoScoped c, dest, n                 # the body
+            while n.hasMore: skip n
+          dest.addParRi()
+        of PragmaxS:
+          # `(pragmax (pragmas ...) BODY)`: exactly one body, which `trGoto`
+          # would flatten into several statements. A body that suspends must
+          # be flattened all the same — its state labels belong at the top
+          # level — and only a `cast` block can give up its wrapper for that,
+          # since it means nothing at run time.
+          let pragmas = n.childCursor
+          if containsSuspensionPoint(c, n) and
+              pragmas.childCursor.pragmaKind == CastP:
+            n.into:
+              skip n                                # pragmas
+              while n.hasMore: trGoto c, dest, n
+          else:
+            dest.addParLe(n.cursorTagId, n.info)
+            n.into:
+              dest.takeTree n                       # pragmas
+              trGotoScoped c, dest, n
+            dest.addParRi()
         of CaseS:
           # `finalir` keeps `case` as a construct ("Format as existing"), so
           # unlike `nj.nim` — which lowered every branch to an `ite` chain —

tests/nimony/cps/tcorofor.nim

@@ -0,0 +1,65 @@
+# `for` loops over `.passive` iterators.
+#
+# Three things that used to be wrong:
+#  1. a loop in a `.passive` routine did not compile at all — the iterator call
+#     was taken for a suspension point of the routine, which put a state proc
+#     inside the loop; and the loop variable was not redirected into the frame;
+#  2. a passive call inside the iterator returns into the iterator's frame, and
+#     the trampoline's frame-identity test read that as a yield, running the
+#     body again with the previous value;
+#  3. a `{.cast(...)}:` block anywhere in a coroutine crashed hexer.
+#
+# The loop is a trampoline inside ONE state proc, so its body cannot suspend:
+# no passive call, no `yield`, no park in the iterator. Each of those is
+# refused — the first two at compile time, the park at run time.
+
+import std / syncio
+
+proc step() {.passive.} = discard
+
+iterator count(n: int): int {.passive.} =
+  var i = 1
+  while i <= n:
+    yield i
+    {.cast(noSideEffect).}:
+      step()               # a passive call BETWEEN two yields
+    inc i
+
+proc fromRegular() =
+  for x in count(3):
+    echo "regular ", x
+
+proc fromPassive() {.passive.} =
+  var sum = 0
+  for x in count(3):       # `x` lives in this coroutine's frame
+    sum += x
+    echo "passive ", x
+  echo "passive sum ", sum
+
+proc breaks() {.passive.} =
+  for x in count(10):
+    if x == 3: break
+    echo "break ", x
+
+proc returns(): int {.passive.} =
+  for x in count(10):
+    if x == 4: return x
+  result = -1
+
+proc nested() {.passive.} =
+  for a in count(2):
+    for b in count(2):
+      echo "nested ", a, " ", b
+
+proc pragmaBlock() {.passive.} =
+  echo "before"
+  {.cast(noSideEffect).}:
+    step()
+  echo "after"
+
+fromRegular()
+fromPassive()
+breaks()
+echo "returns ", returns()
+nested()
+pragmaBlock()

tests/nimony/cps/tcorofor.output

@@ -0,0 +1,16 @@
+regular 1
+regular 2
+regular 3
+passive 1
+passive 2
+passive 3
+passive sum 6
+break 1
+break 2
+returns 4
+nested 1 1
+nested 1 2
+nested 2 1
+nested 2 2
+before
+after

tests/nimony/errmsgs/tcoroforsuspend.msgs

@@ -0,0 +1 @@
+tests/nimony/errmsgs/tcoroforsuspend.nim(14, 7) Error: a `for` loop over a `.passive` iterator cannot suspend in its body

tests/nimony/errmsgs/tcoroforsuspend.nim

@@ -0,0 +1,15 @@
+# A `for` loop over a `.passive` iterator is a trampoline inside ONE state
+# proc, so its body cannot suspend: there is nowhere for the state boundary to
+# go. Refused rather than mislowered.
+
+proc step() {.passive.} = discard
+
+iterator count(n: int): int {.passive.} =
+  var i = 1
+  while i <= n:
+    yield i
+    inc i
+
+proc consume() {.passive.} =
+  for x in count(3):
+    step()

tests/nimony/errmsgs/tdistinctinfos.msgs

@@ -24,5 +24,5 @@ WINBOOL(0) == 0
 [1] WINBOOL does not match constraint T (declared in lib/std/system/comparisons.nim(217, 1))
 [1] expected: string but got: WINBOOL (declared in lib/std/system/stringimpl.nim(853, 1))
 [1] expected: openArray[T] but got: WINBOOL (declared in lib/std/system/openarrays.nim(74, 1))
-[1] WINBOOL does not match constraint T (declared in lib/std/system.nim(369, 1))
-[1] expected: seq[T] but got: WINBOOL (declared in lib/std/system.nim(375, 1))
+[1] WINBOOL does not match constraint T (declared in lib/std/system.nim(390, 1))
+[1] expected: seq[T] but got: WINBOOL (declared in lib/std/system.nim(396, 1))

tests/nimony/valgrind/tpassivespawn.valgrind

@@ -7,7 +7,7 @@
 ==1519504== 
 ==1519504== HEAP SUMMARY:
 ==1519504==     in use at exit: 0 bytes in 0 blocks
-==1519504==   total heap usage: 9 allocs, 9 frees, 360 bytes allocated
+==1519504==   total heap usage: 9 allocs, 9 frees, 432 bytes allocated
 ==1519504== 
 ==1519504== All heap blocks were freed -- no leaks are possible
 ==1519504==