Add Op::CallChildren: eliminate children()'s native reentry

include/openscad_cpp_evaluator/bytecode.hpp

@@ -325,8 +325,50 @@ enum class Op {
     // same seam, now a counter).
     PopBuiltinWrap,
 
+    // A `children()` / `children(N)` statement -- the runtime-varying
+    // sibling of Op::CallModule/Op::PushBuiltinWrap, closing the LAST
+    // dominant native-reentry source (BOSL2's attachable() calls
+    // children() at nearly every wrapper level; measured 85 of 93 native
+    // reentries in a real script). Unlike PushBuiltinWrap's constructs,
+    // the forwarded children aren't known at compile time (they're the
+    // CALLER's own call-site statements, carried on ctx.childrenNodes/
+    // childrenCallerCtx -- see Evaluator::buildModuleChildCtx), so they
+    // can't compile inline; instead the handler resolves the list at
+    // RUNTIME, looks up/compiles its chunk (the same
+    // childrenListChunkCache_ tryRunCompiledChildren uses, via
+    // lookupOrCompileChildrenListChunk -- gated on useBytecodeVm() &&
+    // inResolvePass_, the cache is pass-scoped), and pushes it onto
+    // vmCallStack_ directly, mirroring Op::CallModule's own zero-native-
+    // call push -- but with a THIRD frame shape: splice-owning like a
+    // module frame (ownsModuleSplice=true, mirroring evalModularCall's
+    // own unconditional splice branch for "children"), yet bracketless
+    // like a bare frame (children() never gets a callStack_/profiling
+    // entry natively either -- only enterUserCall pushes those, and
+    // resolveChildren/builtinChildren never call it). driveVm's existing
+    // completion branch and teardownVmCallStackDownTo both already
+    // handle that combination (their bracket and splice concerns are
+    // independent).
+    //
+    // Handler ordering is load-bearing: checkDebug fires in-handler
+    // against the scope-wrapped ctx (byte-for-byte what Op::
+    // NativeStatement does -- NOT via an emitted Op::CheckDebugStatement,
+    // whose handler passes the un-wrapped ctx); randsBefore is captured
+    // BEFORE argument resolution (rands-in-args taint, same lesson
+    // PushBuiltinWrap already encodes); and treeStack_ is pushed LAST,
+    // immediately before the frame push / native fallback (an early-out
+    // or a throw during arg resolution then has nothing to clean up --
+    // the native path's own catch{pop;throw} has no equivalent here, so
+    // the reorder IS the exception-safety mechanism). The not-eligible
+    // fallback reuses the ALREADY-resolved args (never evalStatement,
+    // which would re-resolve them: double rands(), double side effects)
+    // by inlining evalModularCall's own children branch around a native
+    // evalChildren call. a = index into CompiledChunk::nativeStatements
+    // (the ModularCall node -- arguments, error position, splice node;
+    // no separate site table needed).
+    CallChildren,
+
     // A single "native passthrough" statement -- intersection_for,
-    // children(), union/difference/intersection and every other builtin
+    // union/difference/intersection and every other builtin
     // module call NOT covered by Op::PushBuiltinWrap (see that op's own
     // doc comment for exactly which builtins ARE covered), the `*`
     // modifier's own no-op case, or a user-module call that didn't
@@ -344,7 +386,9 @@ enum class Op {
     // pattern that made this op's own native reentry a genuine Windows
     // crash risk in practice, not just a missed optimization. See
     // Op::PushBuiltinWrap's own doc comment for the real story and the
-    // fix.)
+    // fix. children() fell here too, and was the LAST and largest such
+    // reentry source once PushBuiltinWrap's own set was covered -- it
+    // now has Op::CallChildren, above.)
     // a = index into CompiledChunk::nativeStatements. Runtime just does
     // what Evaluator::evalChildren's own per-statement loop already does
     // for one node: derive childCtx via ctx.withScope(...), checkDebug,

include/openscad_cpp_evaluator/evaluator.hpp

@@ -271,6 +271,25 @@ class Evaluator {
     // _builtin_children/_eval_children_lazy.
     void builtinChildren(const CallArgs& args, EvalContext& ctx);
 
+    // The "which nodes, evaluated against what context" half of
+    // builtinChildren, shared with Op::CallChildren's runtime handler
+    // (bytecode_vm.cpp) so the native and compiled paths cannot drift on
+    // any of the subtle parts: the caller-ctx re-derivation, the
+    // $-forwarding loop (which must read the post-resolveCallArgs effCtx
+    // -- a children($fn=12) named-$ override lives only there), and the
+    // children(N) statement-index filtering/bounds-check. nullopt means
+    // "nothing to evaluate" (no forwarded children in scope, empty list,
+    // or index out of range) -- a silent no-op in every existing caller,
+    // exactly matching builtinChildren's own early returns. `ctx` must be
+    // the effCtx resolveCallArgs returned, same as builtinChildren's own
+    // parameter today. Public for the same free-function reasoning as
+    // builtinChildren itself.
+    struct ChildrenForward {
+        EvalContext evalCtx;
+        std::vector<const oscad::ASTNode*> nodes;
+    };
+    std::optional<ChildrenForward> prepareChildrenForward(const CallArgs& args, EvalContext& ctx);
+
     // "WARNING: {message}{locSuffix(position)}" via echoFn_, no-op if unset.
     // Public: builtins/import.cpp's not-manifold warning is emitted from a
     // free function, same reasoning as tagGenerated()/builtinChildren().
@@ -532,6 +551,17 @@ class Evaluator {
     // into the caller's scope exactly the way the native per-statement
     // loop's own writes already do.
     bool tryRunCompiledChildren(const std::vector<const oscad::ASTNode*>& children, EvalContext& ctx);
+public:
+    // The cache-lookup half of tryRunCompiledChildren, shared with
+    // Op::CallChildren's runtime handler (bytecode_vm.cpp, a free
+    // function -- public for the same no-friend-declaration reasoning as
+    // vmCallStack_/treeStack_). Returns the eligible compiled chunk for
+    // `children` or nullptr. Caller owns the useBytecodeVm()/
+    // inResolvePass_ gate -- the pass gate is load-bearing
+    // (childrenListChunkCache_ is pass-scoped, see its own doc comment),
+    // not defensive.
+    const CompiledChunk* lookupOrCompileChildrenListChunk(const std::vector<const oscad::ASTNode*>& children);
+private:
     void evalModularCall(const oscad::ModularCall& node, EvalContext& ctx);
     void evalFor(const oscad::ModularFor& node, EvalContext& ctx);
     void evalLetBlock(const oscad::ModularLet& node, EvalContext& ctx);
@@ -951,15 +981,38 @@ class Evaluator {
     // body -- so a resolvable module call anywhere in that list gets
     // Op::CallModule bytecode, and any if/for control flow around it gets
     // real Jump-based bytecode too, instead of falling through to the
-    // native per-statement loop. Keyed by the list's own FIRST element,
-    // same convention as assignBlockChunkCache_ (stable and unique per
-    // list: a given evalChildren() call site is always handed the same
-    // leading element across repeated evaluations). Same nullopt-means-
+    // native per-statement loop.
+    //
+    // Keyed by (FIRST element, list SIZE) -- NOT the first element alone,
+    // the original convention borrowed from assignBlockChunkCache_
+    // (where it IS sufficient: a leading-assignment run's first member
+    // uniquely determines the run). Here it wasn't: children()'s
+    // forwarding produces two DIFFERENT lists sharing a first element --
+    // bare `children()` forwards the caller's whole list, `children(0)`
+    // a single-element slice of it -- and keying by front alone made
+    // whichever form ran first poison the cache for the other (caught
+    // for real: `module m() { children(); children(0); }\n
+    // m() { cube(); sphere(); }` made children(0) emit BOTH shapes,
+    // or bare children() emit only one, depending on statement order).
+    // (front, size) fully disambiguates every real producer: fixed AST
+    // lists have unique fronts per call site, and the only same-front
+    // pair (bare vs indexed forwarding) always differs in size except
+    // when the lists are literally identical anyway. Same nullopt-means-
     // tried-and-failed convention, same dangling-pointer hazard, same
     // two-part fix (cleared alongside stmtExprChunkCache_ at the top of
     // every resolveTreeImpl call, only read/written while inResolvePass_)
     // as stmtExprChunkCache_ itself.
-    std::unordered_map<const oscad::ASTNode*, std::optional<CompiledChunk>> childrenListChunkCache_;
+    struct ChildrenListKeyHash {
+        size_t operator()(const std::pair<const oscad::ASTNode*, size_t>& k) const {
+            // Standard hash-combine (boost-style golden-ratio mix) --
+            // either half alone collides by construction here.
+            const size_t h1 = std::hash<const oscad::ASTNode*>{}(k.first);
+            const size_t h2 = std::hash<size_t>{}(k.second);
+            return h1 ^ (h2 + 0x9e3779b97f4a7c15ULL + (h1 << 6) + (h1 >> 2));
+        }
+    };
+    std::unordered_map<std::pair<const oscad::ASTNode*, size_t>, std::optional<CompiledChunk>, ChildrenListKeyHash>
+        childrenListChunkCache_;
 
     // See stmtExprChunkCache_'s own doc comment, immediately above, for why
     // this exists. Not a reentrancy guard (resolveTreeImpl is never called
@@ -1115,6 +1168,17 @@ class Evaluator {
         return bytecodeVmEnabled() && (!debugHooks_.debugHook || fastContinueBreakpoints_.has_value());
     }
 
+    // Read accessor for inResolvePass_ (private, below) -- Op::CallChildren's
+    // runtime handler (bytecode_vm.cpp, a free function) must gate its
+    // childrenListChunkCache_ access on it, exactly like
+    // tryRunCompiledChildren's own self-gate: the cache is pass-scoped
+    // (cleared per resolveTreeImpl, AST-address-reuse hazard -- see
+    // stmtExprChunkCache_'s own doc comment), and driveVm CAN run outside
+    // the resolve pass (a host calling evalChildren directly reaches
+    // module opcodes via lookupOrCompileModuleChunk, which is NOT
+    // pass-scoped).
+    bool inResolvePass() const { return inResolvePass_; }
+
 private:
     // The fine-grained half of the check above: even when useBytecodeVm()
     // says compiling/using bytecode is on the table at all, a SPECIFIC

include/openscad_cpp_evaluator/scope_trail.hpp

@@ -368,12 +368,30 @@ class IndexedScopeTrailStorage {
         dirty_[level].push_back(id);
     }
 
+    // Level-aware, NOT a blind pop_back() -- the exact fix
+    // ScopeTrailStorage::popLevel (above) already carries, for the exact
+    // out-of-order-pop bug class its own doc comment describes; this
+    // indexed twin never got the same fix because nothing violated LIFO
+    // view destruction on the dyn trail until Op::CallChildren's
+    // forwarding frame (bytecode_vm.cpp): its evalCtx (a dyn level opened
+    // AFTER the call's own effCtx level) is moved into a VmFrame and
+    // OUTLIVES effCtx, so effCtx's pop runs while evalCtx's later entry
+    // is still physically on top of the same name's stack -- the blind
+    // pop_back() silently removed the still-live forwarded value instead
+    // of the one actually being popped (caught for real: a
+    // children($fn=9) named-$ override read back as the root default 0
+    // inside the forwarded child).
     void popLevel(int level) {
         auto it = dirty_.find(level);
         if (it != dirty_.end()) {
             for (int id : it->second) {
                 auto& stack = stacks_[static_cast<size_t>(id)];
-                if (!stack.empty()) stack.pop_back();
+                for (auto rit = stack.rbegin(); rit != stack.rend(); ++rit) {
+                    if (rit->level == level) {
+                        stack.erase(std::next(rit).base());
+                        break;
+                    }
+                }
             }
             dirty_.erase(it);
         }

pyproject.toml

@@ -4,7 +4,7 @@ build-backend = "scikit_build_core.build"
 
 [project]
 name = "openscad_cpp_evaluator"
-version = "0.13.3"
+version = "0.13.4"
 description = "C++ OpenSCAD evaluator with Python bindings"
 readme = "README.md"
 requires-python = ">=3.12"

src/bytecode_compiler.cpp

@@ -1420,12 +1420,25 @@ class Compiler {
                     emitBuiltinWrap(*wrapKind, call.name->name, call, children, out);
                     return;
                 }
-                // Every other builtin, children(), or a name that didn't
-                // resolve to a user module statically -- native
-                // passthrough, exactly like echo/assert/etc. below. Never
-                // the recursion-depth risk this compiler targets for
-                // THESE (see NativeStatement's own doc comment,
-                // bytecode.hpp).
+                // children() -- the runtime-varying forwarding builtin,
+                // detected by the same function-pointer-identity pattern
+                // as Transform/Color above. Deliberately NO preceding
+                // Op::CheckDebugStatement (unlike emitBuiltinWrap's own
+                // emission): CallChildren's handler fires checkDebug
+                // itself against the SCOPE-WRAPPED ctx, byte-for-byte
+                // matching Op::NativeStatement's own handler -- the
+                // CheckDebugStatement handler passes the un-wrapped ctx,
+                // which would be a subtle behavior change for this node.
+                // See Op::CallChildren's own doc comment (bytecode.hpp).
+                if (dispatchIt != dispatch.end() && dispatchIt->second == &resolveChildren) {
+                    out.push_back({Op::CallChildren, internNativeStatement(&stmt), 0, &stmt.position()});
+                    return;
+                }
+                // Every other builtin, or a name that didn't resolve to a
+                // user module statically -- native passthrough, exactly
+                // like echo/assert/etc. below. Never the recursion-depth
+                // risk this compiler targets for THESE (see
+                // NativeStatement's own doc comment, bytecode.hpp).
                 out.push_back({Op::NativeStatement, internNativeStatement(&stmt), 0, &stmt.position()});
                 return;
             }