Close the NativeStatement gap and guard its new native-reentry path

include/openscad_cpp_evaluator/bytecode_compiler.hpp

@@ -99,4 +99,28 @@ std::optional<CompiledChunk> tryCompileAssignmentBlock(const std::vector<const o
 // (via tryCompileStatementExpr's own NotCompilable, propagated up).
 std::optional<CompiledChunk> tryCompileModuleBody(const oscad::ModuleDeclaration& decl);
 
+// Same compilation (assignment/if/for/resolved-module-call get real
+// bytecode, everything else a native passthrough), but for an ARBITRARY
+// statement list, not just a ModuleDeclaration's own `children` -- the
+// general form behind Evaluator::tryRunCompiledChildren, which every
+// evalChildren() call now tries first (stmt_eval.cpp), including a
+// builtin module's own children (translate()/union()/etc.'s internal
+// evalChildren call, src/builtins/*.cpp) and a for-loop's/let-block's
+// body. This is what actually reaches the "native passthrough wraps a
+// recursive module call" case Op::NativeStatement alone can't -- see this
+// project's own session notes on the Stage 2 "NativeStatement gap" for
+// why a builtin call itself (translate, not just what's declared inside
+// this project's own module bodies) still can't become a SINGLE flat
+// instruction stream with its caller this way (its own resolve function,
+// e.g. resolveTransform, computes params AND evaluates children as one
+// opaque native call -- decoupling that is a separate, larger project),
+// but this DOES mean the children of any wrapping builtin get their own,
+// independent chance to compile, cutting the number of native stack
+// frames needed per recursive "wrap-then-recurse" level rather than
+// eliminating them entirely. `scope`: the list's own lexical scope for
+// resolving a call site's callee -- callers pass `children.front()->
+// scope()`, mirroring tryRunCompiledAssignmentBlock's own convention.
+std::optional<CompiledChunk> tryCompileChildrenList(const std::vector<const oscad::ASTNode*>& children,
+                                                     const oscad::Scope* scope);
+
 } // namespace oscadeval

include/openscad_cpp_evaluator/evaluator.hpp

@@ -520,6 +520,18 @@ class Evaluator {
     // "silently equivalent to interpreting it" contract, just for a whole
     // block instead of one leaf expression.
     bool tryRunCompiledAssignmentBlock(const std::vector<const oscad::ASTNode*>& assignments, EvalContext& ctx);
+    // Same contract as tryRunCompiledAssignmentBlock, above, but for
+    // evalChildren's own FULL `children` list (not just its leading
+    // assignment run) -- see tryCompileChildrenList's own doc comment
+    // (bytecode_compiler.hpp) for why every evalChildren() call site
+    // tries this FIRST now, including a builtin module's own children
+    // (translate()/union()/etc.) and a for-loop's/let-block's body, not
+    // just module/top-level bodies. `ctx` is used directly (not a scoped
+    // child), matching runCompiledModuleBody's own contract -- a
+    // children-list chunk's own writes (assignments, $-vars) must persist
+    // 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);
     void evalModularCall(const oscad::ModularCall& node, EvalContext& ctx);
     void evalFor(const oscad::ModularFor& node, EvalContext& ctx);
     void evalLetBlock(const oscad::ModularLet& node, EvalContext& ctx);
@@ -923,6 +935,25 @@ class Evaluator {
     // comment above for the full reasoning.
     std::unordered_map<const oscad::Assignment*, std::optional<CompiledChunk>> assignBlockChunkCache_;
 
+    // Whole-CHILDREN-LIST chunks (see tryCompileChildrenList's own doc
+    // comment, bytecode_compiler.hpp, and tryRunCompiledChildren's,
+    // below) -- every evalChildren() call now tries the FULL list it was
+    // given as one chunk first (not just its own assignments sub-list),
+    // including a builtin module's own children (translate()/union()/
+    // etc.'s internal evalChildren call) and a for-loop's/let-block's
+    // 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-
+    // 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_;
+
     // See stmtExprChunkCache_'s own doc comment, immediately above, for why
     // this exists. Not a reentrancy guard (resolveTreeImpl is never called
     // while another one is already active), just an on/off switch for
@@ -1231,6 +1262,20 @@ class Evaluator {
     // ordering constraint the inline computation already has.
     int callStackTopIndex() const { return callStack_.empty() ? -1 : static_cast<int>(callStack_.size()) - 1; }
 
+    // The innermost active call's own declaration node, for error()'s TRACE
+    // walk -- needed by driveVm's own driveVmNativeDepth_ guard (see that
+    // field's doc comment), which has no single reliable ASTNode of its own
+    // to report against (the top VmFrame's chunk may be a children-list
+    // chunk with a null selfDecl -- see tryCompileChildrenList's own doc
+    // comment, bytecode_compiler.cpp). callStack_ is guaranteed non-empty
+    // by the time this guard can fire (it only trips past
+    // kMaxDriveVmNativeDepth levels of nesting, and every level pushes a
+    // CallStackFrame first via enterUserCall). Same "free function, no
+    // friend declaration" reasoning as callStackTopIndex, just above.
+    const oscad::ASTNode* currentCallDeclNode() const {
+        return callStack_.empty() ? nullptr : callStack_.back().declNode;
+    }
+
     // The explicit, heap-allocated VM call stack that replaced native C++
     // recursion for a call between two compiled chunks -- see this
     // project's own session notes ("iterate over the code, don't use the
@@ -1269,6 +1314,37 @@ class Evaluator {
     // recalibrating for real.
     static constexpr size_t kMaxVmCallStackDepth = 1'000'000;
 
+    // Counts native C++ nesting of driveVm itself -- NOT logical call
+    // depth (that's vmCallStack_.size()/callStack_.size(), both of which
+    // grow just as much for a pure VM-internal Op::CallModule hop, which
+    // costs zero native stack). A hop entirely serviced by driveVm's own
+    // while loop (Op::CallModule/CallFn/CallFnTail/CallDynamic/
+    // CallDynamicTail) never increments this. It DOES increment when a
+    // compiled body statement isn't one of the specially-compiled forms
+    // (e.g. `translate(v) recur(n-1);` -- a builtin-with-children falls to
+    // Op::NativeStatement) and that native evalStatement call itself
+    // re-enters the VM (evalChildren -> tryRunCompiledChildren/
+    // tryRunCompiledAssignmentBlock/evalExprMaybeCompiled -> a fresh
+    // driveVm call, nested on the native stack, unlike Op::CallModule's own
+    // push). This is a REAL native recursive call each time, same hazard
+    // kMaxUserCallDepth guards against -- caught for real: `module
+    // recur(n) { translate([0,0,n]) recur2(n); } module recur2(n) { if
+    // (n>0) recur(n-1); else cube(1); }` segfaulted (exit 139) around
+    // n=3000 with no guard at all, since kMaxCsgTreeDepth only catches this
+    // AFTER the (already-crashed) native descent would have unwound. Public
+    // for the same reason vmCallStack_ is: driveVm (bytecode_vm.cpp) is a
+    // free function, not a member, so it needs direct access with no
+    // friend declaration in play.
+    size_t driveVmNativeDepth_ = 0;
+    // Same figure as kMaxUserCallDepth -- each native re-entry above costs
+    // a comparable handful of native frames (evalStatement, evalModularCall,
+    // a builtin resolve function, evalChildren, tryRunCompiledChildren,
+    // runCompiledModuleBody, driveVm), so the same Windows-CI-calibrated
+    // ceiling applies. Deliberately its own named constant (not a reuse of
+    // kMaxUserCallDepth in place) so the two can be recalibrated
+    // independently if profiling ever shows they should diverge.
+    static constexpr size_t kMaxDriveVmNativeDepth = 30;
+
     // Bracketed public in place: Op::CallModule's own runtime handler and
     // driveVm's module-frame completion branch (bytecode_vm.cpp, a
     // separate translation unit) push/pop this directly -- exactly the

pyproject.toml

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

src/bytecode_compiler.cpp

@@ -1127,10 +1127,24 @@ class Compiler {
     // every statement -- assignment or not -- collapse to the same simple
     // "compile once, in the right position" treatment.
     void compileStatementList(const std::vector<std::unique_ptr<oscad::ASTNode>>& children, std::vector<Instruction>& out) {
+        std::vector<const oscad::ASTNode*> raw;
+        raw.reserve(children.size());
+        for (const auto& c : children) raw.push_back(c.get());
+        compileStatementList(raw, out);
+    }
+
+    // Same, for a list of raw (non-owning) pointers -- Evaluator::
+    // evalChildren's own primary overload already receives its `children`
+    // this shape (a caller-owned list, e.g. a builtin module's own
+    // node.children, or a for-loop's freshly-built bodyNodes vector, not
+    // necessarily one this Compiler's own declaration owns) -- see
+    // tryCompileChildrenList's own doc comment, below, for the entry point
+    // that needs this shape directly.
+    void compileStatementList(const std::vector<const oscad::ASTNode*>& children, std::vector<Instruction>& out) {
         std::vector<const oscad::ASTNode*> assignments;
         std::vector<const oscad::ASTNode*> others;
-        for (const auto& c : children) {
-            (c->kind() == oscad::NodeKind::Assignment ? assignments : others).push_back(c.get());
+        for (const oscad::ASTNode* c : children) {
+            (c->kind() == oscad::NodeKind::Assignment ? assignments : others).push_back(c);
         }
         for (const oscad::ASTNode* stmt : assignments) compileOneStatement(*stmt, out);
         for (const oscad::ASTNode* stmt : others) compileOneStatement(*stmt, out);
@@ -1228,6 +1242,21 @@ class Compiler {
         std::vector<size_t> topIdx(numDims);
         std::vector<size_t> forIterNextIdx(numDims);
         for (size_t d = 0; d < numDims; ++d) {
+            // Every dimension but the outermost needs its own IterList
+            // index reset to 0 each time it's (re-)entered from the
+            // ENCLOSING dimension's own successful bind -- NativeIterMaterialize
+            // only resets it once, up front, before ANY iteration runs; by
+            // the second (and every later) outer-dimension value, this
+            // dimension's own index is still sitting at "exhausted" from
+            // the PREVIOUS pass, and would immediately look exhausted
+            // again without this. Placed strictly BEFORE this dimension's
+            // own ForIterNext/topIdx (not at it) so this dimension's OWN
+            // "try the next value" jump (its own ForIterEnd, below) lands
+            // AT ForIterNext directly and skips the reset -- only the
+            // fall-through from the enclosing dimension's bind passes
+            // through it. Harmless (a no-op) the very first time, since
+            // the index is already 0 from NativeIterMaterialize then.
+            if (d > 0) out.push_back({Op::IterReset, iterListIds[d], 0, nullptr});
             topIdx[d] = out.size();
             forIterNextIdx[d] = out.size();
             Instruction ins;
@@ -1391,4 +1420,26 @@ std::optional<CompiledChunk> tryCompileModuleBody(const oscad::ModuleDeclaration
     return chunk;
 }
 
+std::optional<CompiledChunk> tryCompileChildrenList(const std::vector<const oscad::ASTNode*>& children,
+                                                     const oscad::Scope* scope) {
+    CompiledChunk chunk;
+    // Same completion semantics as a module chunk (no return value, its
+    // whole effect is the side effect of what lands in treeStack_) --
+    // reuses runCompiledModuleBody's own bare-frame entry point as-is
+    // (bytecode_vm.cpp) to run it, no new runtime machinery needed.
+    // selfDecl stays null -- this list has no single declaration identity
+    // of its own the way a ModuleDeclaration's body does (no upvalues are
+    // ever resolved against it either way; module bodies don't create
+    // escaping closures).
+    chunk.isModule = true;
+    Compiler compiler(chunk, scope, nullptr, {});
+    try {
+        compiler.compileStatementList(children, chunk.bodyCode);
+    } catch (const NotCompilable&) {
+        return std::nullopt;
+    }
+    chunk.numIterLists = compiler.nextIterList();
+    return chunk;
+}
+
 } // namespace oscadeval

src/bytecode_vm.cpp

@@ -340,7 +340,22 @@ void teardownVmCallStackDownTo(Evaluator& ev, size_t floor) {
 // returns; only the floor frame's own result escapes to the caller.
 Value driveVm(Evaluator& ev, size_t floor) {
     Value finalResult;
+    // Native-stack-safety guard, distinct from vmCallStack_'s own
+    // heap-bounded kMaxVmCallStackDepth check -- see driveVmNativeDepth_'s
+    // own doc comment (evaluator.hpp) for exactly what this catches (a
+    // NativeStatement-triggered re-entry into the VM, still a genuine
+    // native C++ call unlike an ordinary Op::CallModule/CallFn hop).
+    struct NativeDepthGuard {
+        Evaluator& ev;
+        explicit NativeDepthGuard(Evaluator& e) : ev(e) { ++ev.driveVmNativeDepth_; }
+        ~NativeDepthGuard() { --ev.driveVmNativeDepth_; }
+    } nativeDepthGuard(ev);
     try {
+        if (ev.driveVmNativeDepth_ > Evaluator::kMaxDriveVmNativeDepth) {
+            const oscad::ASTNode* node = ev.currentCallDeclNode();
+            if (!node) node = ev.vmCallStack_.back()->chunk->selfDecl;
+            ev.error("Recursion too deep (native call stack)", *node);
+        }
         while (ev.vmCallStack_.size() > floor) {
             VmFrame& f = *ev.vmCallStack_.back();
             if (f.pc >= f.code->size()) {

src/csg_resolve.cpp

@@ -208,8 +208,13 @@ std::vector<std::unique_ptr<CSGNode>> Evaluator::resolveTreeImpl(const NodeList&
     // "re-render after an edit" pattern) never risks a stale entry from a
     // PRIOR pass whose own AST nodes may since have been freed. See
     // stmtExprChunkCache_'s own doc comment (evaluator.hpp) for the full
-    // hazard this guards against.
+    // hazard this guards against. assignBlockChunkCache_'s own doc
+    // comment already claimed this -- it just wasn't actually done; a
+    // real, pre-existing gap, fixed here alongside childrenListChunkCache_
+    // getting the same treatment from day one.
     stmtExprChunkCache_.clear();
+    assignBlockChunkCache_.clear();
+    childrenListChunkCache_.clear();
     ResolvePassGuard resolvePassGuard(inResolvePass_);
     evalChildren(nodes, ctx);
     std::vector<std::unique_ptr<CSGNode>> tree = std::move(treeStack_.back());

src/stmt_eval.cpp

@@ -221,6 +221,22 @@ void Evaluator::evalStatement(const oscad::ASTNode& node, EvalContext& ctx) {
 }
 
 void Evaluator::evalChildren(const std::vector<const oscad::ASTNode*>& children, EvalContext& ctx) {
+    // Whole-LIST compile first (see tryRunCompiledChildren's own doc
+    // comment, evaluator.hpp) -- tries the FULL `children` as one chunk
+    // before falling back to assignments-then-others below. This is what
+    // lets a resolvable module call anywhere in ANY children list --
+    // not just a module/top-level body, but a for-loop's/let-block's own
+    // body, or a builtin's own children (translate()/union()/etc., whose
+    // own resolve function calls this SAME evalChildren internally) --
+    // get real Op::CallModule bytecode instead of always falling through
+    // to the native per-statement loop below. Unconditionally correct
+    // either way since nothing about these statements' observable
+    // behavior differs based on which path ran them (same reasoning as
+    // tryRunCompiledAssignmentBlock's own doc comment, just for the WHOLE
+    // list instead of its own leading assignment run).
+    lastCtx_ = &ctx;
+    if (tryRunCompiledChildren(children, ctx)) return;
+
     // OpenSCAD executes all assignments in a scope before anything else,
     // each group preserving source order -- mirrors evaluate()/
     // _eval_children's `assignments + others` partition.

src/user_calls.cpp

@@ -82,6 +82,19 @@ bool Evaluator::tryRunCompiledAssignmentBlock(const std::vector<const oscad::AST
     return true;
 }
 
+bool Evaluator::tryRunCompiledChildren(const std::vector<const oscad::ASTNode*>& children, EvalContext& ctx) {
+    if (children.empty() || !useBytecodeVm() || !inResolvePass_) return false;
+    const oscad::ASTNode* first = children.front();
+    auto it = childrenListChunkCache_.find(first);
+    if (it == childrenListChunkCache_.end()) {
+        it = childrenListChunkCache_.emplace(first, tryCompileChildrenList(children, first->scope())).first;
+        if (it->second) flattenNestedLiterals(*it->second);
+    }
+    if (!it->second || !chunkEligibleNow(*it->second)) return false;
+    runCompiledModuleBody(*this, *it->second, ctx);
+    return true;
+}
+
 const Value* Evaluator::findUpvalue(const oscad::ASTNode* targetDecl, int slot) const {
     // Walks upvalueParent links, NOT a blind scan of callStack_ -- see
     // that field's own doc comment for the real bug this distinction