Resolve mutual recursion between sibling let-bound closures

include/openscad_cpp_evaluator/bytecode.hpp

@@ -131,16 +131,33 @@ enum class Op {
     // chunk actually running Op::MakeClosure), and constructs
     // Closure{node, capturedTrail} -- reusing Closure::capturedLet (value.hpp)
     // and callCtxFor's existing capturedLet-rooting (user_calls.cpp)
-    // UNCHANGED: invoking this closure later (always via the interpreter,
-    // see ClosureSite's own doc comment for why its body is never itself
-    // compiled) works exactly like any interpreter-created escaping closure
-    // always has, no new invocation-side code needed at all. Replaces
+    // UNCHANGED: invoking this closure later works through the exact same
+    // machinery any interpreter-created escaping closure always has, no new
+    // invocation-side code needed at all (its own body may itself run
+    // compiled too now, see Evaluator::lookupCompiledLiteralChunk). Replaces
     // Op::PushConst's frozen `Closure{&n, nullptr}` specifically for a
     // literal whose (transitively merged) capture set is non-empty; a
     // literal that closes over nothing at all -- even transitively -- still
     // takes the cheaper PushConst path, unchanged.
     MakeClosure,
 
+    // Letrec support for TWO OR MORE sibling `let`-bound closures that
+    // reference each other by name (fnliterals.scad-style mutual
+    // recursion, e.g. isEven/isOdd each calling the other) -- see the
+    // LetOp compile case's own doc comment (bytecode_compiler.cpp) for why
+    // a forward reference (an EARLIER closure referencing a LATER sibling
+    // that doesn't exist yet at the point the earlier one is constructed)
+    // can't be resolved by Op::MakeClosure itself the way a self-reference
+    // is: there's nothing to read OR self-patch into, since the later
+    // sibling hasn't even been created yet. a = consumer's own local slot
+    // (already holds a MakeClosure-created closure, via a preceding
+    // Op::StoreLocal), b = name pool index (the captured name, inside the
+    // consumer's own capturedTrail, that's finally ready), c = the local
+    // slot the just-constructed sibling was JUST stored into (also via a
+    // preceding Op::StoreLocal) -- patches slots[a]'s own capturedTrail
+    // with slots[c], keyed by name b.
+    PatchClosureCapture,
+
     // A call whose callee isn't statically resolvable to a builtin or a
     // named FunctionDeclaration (see CallFn) -- the callee expression is
     // compiled to push its own Value first, THEN each argument (so at

pyproject.toml

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

src/bytecode_compiler.cpp

@@ -10,6 +10,7 @@
 
 #include <functional>
 #include <unordered_map>
+#include <unordered_set>
 
 namespace oscadeval {
 
@@ -68,26 +69,26 @@ void bubbleEscapingCaptures(const CompiledChunk::ClosureSite& site, const oscad:
 // True if any instruction in `chunk`'s own body/defaults is Op::LoadFree --
 // i.e. an identifier that resolved to neither a local, a statically-known
 // enclosing upvalue, nor a dyn ($-prefixed) name at compile time (see
-// compileExpr's own Identifier case). The direct `let(a = function(...)
+// compileExpr's own Identifier case). A DIRECT `let(a = function(...)
 // ... a(...) ...)` self-reference (reduce()/accumulate()/while()'s own
-// idiom) no longer hits this -- see the LetOp case's own letrec pre-declare,
-// below -- but anything the pre-declare doesn't cover still does: `a`
-// wrapped in a ternary/let/anything other than a bare FunctionLiteral RHS
-// (`let(a = cond ? function(...) ...a(...)... : function(...) ...)`),
-// mutual recursion between two sibling let-bound closures (each references
-// the OTHER, which isn't declared yet when compiling the first), or any
-// other free variable this phase simply can't resolve statically. For any
-// of those, Evaluator::evalIdentifier's own ctx.scope->lookupVariable()
-// fallback re-evaluates the let-binding's RHS FRESH on every single call,
-// producing a new Closure whose capturedLet is THIS invocation's own
-// ctx.let_ (see expr_eval.cpp's FunctionLiteral case) rather than a stable
-// snapshot -- registering such a chunk for compiled invocation would still
-// be functionally correct, but each recursive call's own
-// capturedLet->openChild() then nests ONE level deeper than the last
-// (confirmed empirically: an O(n) list reduce() degrading to O(n^2) once
-// compiled, before the letrec pre-declare existed), since nothing about
-// that repeated fresh-derivation ever re-roots the trail. Excluding a
-// LoadFree-containing chunk from registration leaves it running
+// idiom) and mutual recursion between two-or-more sibling DIRECT
+// `let`-bound closures (fnliterals.scad-style isEven/isOdd, each calling
+// the other) no longer hit this -- see the LetOp case's own letrec
+// pre-declare, below -- but anything the pre-declare doesn't cover still
+// does: `a` wrapped in a ternary/let/anything other than a bare
+// FunctionLiteral RHS (`let(a = cond ? function(...) ...a(...)... :
+// function(...) ...)`), or any other free variable this phase simply can't
+// resolve statically. For any of those, Evaluator::evalIdentifier's own
+// ctx.scope->lookupVariable() fallback re-evaluates the let-binding's RHS
+// FRESH on every single call, producing a new Closure whose capturedLet is
+// THIS invocation's own ctx.let_ (see expr_eval.cpp's FunctionLiteral case)
+// rather than a stable snapshot -- registering such a chunk for compiled
+// invocation would still be functionally correct, but each recursive
+// call's own capturedLet->openChild() then nests ONE level deeper than the
+// last (confirmed empirically: an O(n) list reduce() degrading to O(n^2)
+// once compiled, before the letrec pre-declare existed), since nothing
+// about that repeated fresh-derivation ever re-roots the trail. Excluding
+// a LoadFree-containing chunk from registration leaves it running
 // interpreted exactly as before -- correct, if not optimized.
 bool containsLoadFree(const std::vector<Instruction>& code) {
     for (const Instruction& ins : code) {
@@ -566,42 +567,89 @@ class Compiler {
                 size_t placeholderIdx = out.size();
                 out.push_back({Op::OpenLocalScope, 0, 0, nullptr});
                 int slotStart = nextSlot_;
-                for (const auto& assign : n.assignments) {
+                // Letrec pre-pass: every DIRECT `name = function(...) ...`
+                // assignment in this LetOp gets its own slot declared up
+                // front, before ANY of them are compiled -- not just so a
+                // closure can reference itself (see the self-reference
+                // handling below), but so an EARLIER one can also reference
+                // a LATER sibling (mutual recursion, e.g. fnliterals.scad-
+                // style isEven/isOdd calling each other). `letrecSlot[i]`
+                // stays -1 for anything else, which keeps the existing
+                // after-the-fact declareLocal path below completely
+                // unchanged (`let(x = x + 1)` must still see the OUTER x,
+                // not a fresh, not-yet-assigned local shadowing it -- only
+                // a function-literal RHS has "see myself/a sibling" as a
+                // sensible reading at all).
+                std::vector<int> letrecSlot(n.assignments.size(), -1);
+                std::unordered_set<int> pendingLetrecSlots;
+                for (size_t i = 0; i < n.assignments.size(); ++i) {
+                    const auto& assign = n.assignments[i];
                     const std::string& name = assign->name->name;
-                    // A DIRECT `name = function(...) ...` RHS -- the
-                    // reduce()/accumulate()/while() idiom, and count_to()-
-                    // style helpers generally -- gets its own slot declared
-                    // BEFORE compiling that RHS (letrec-style), so a
-                    // self-reference inside the closure's own body resolves
-                    // as a genuine upvalue (Op::LoadUpvalue) instead of
-                    // falling through to Op::LoadFree. Every other RHS shape
-                    // keeps the existing after-the-fact declareLocal below
-                    // unchanged (`let(x = x + 1)` must still see the OUTER
-                    // x, not a fresh, not-yet-assigned local shadowing it --
-                    // only a function-literal RHS has "see myself" as a
-                    // sensible reading at all). See Op::MakeClosure's own
-                    // runtime handler (bytecode_vm.cpp) for how the
-                    // resulting self-capture is actually resolved -- the
-                    // closure being created doesn't exist yet at the moment
-                    // its own captures are normally snapshotted, so this
-                    // one is deferred and patched in after construction
-                    // instead of read eagerly like every other capture.
-                    const bool selfBinding = !name.empty() && name[0] != '$' &&
-                                              assign->expr->kind() == oscad::NodeKind::FunctionLiteral;
-                    int preDeclaredSlot = -1;
-                    if (selfBinding) preDeclaredSlot = declareLocal(scope, name);
+                    if (!name.empty() && name[0] != '$' && assign->expr->kind() == oscad::NodeKind::FunctionLiteral) {
+                        letrecSlot[i] = declareLocal(scope, name);
+                        pendingLetrecSlots.insert(letrecSlot[i]);
+                    }
+                }
+                // consumerSlot/name pairs an EARLIER closure's own
+                // Op::MakeClosure left unresolved because they targeted a
+                // sibling that didn't exist yet at that point -- resolved
+                // (an Op::PatchClosureCapture emitted) the moment that
+                // sibling's own StoreLocal runs, below.
+                std::unordered_map<int, std::vector<std::pair<int, std::string>>> pendingWaiters;
+                for (size_t i = 0; i < n.assignments.size(); ++i) {
+                    const auto& assign = n.assignments[i];
+                    const std::string& name = assign->name->name;
+                    const int preDeclaredSlot = letrecSlot[i];
+                    const bool selfBinding = preDeclaredSlot >= 0;
                     compileExpr(*assign->expr, out, scope); // RHS is never tail
                     if (selfBinding && out.back().op == Op::MakeClosure) {
                         CompiledChunk::ClosureSite& site = chunk_.closureSites[static_cast<size_t>(out.back().a)];
-                        for (auto& cap : site.captures) {
-                            if (cap.targetDecl == selfDecl_ && cap.slot == preDeclaredSlot) cap.isSelfReference = true;
+                        auto& captures = site.captures;
+                        for (auto it = captures.begin(); it != captures.end();) {
+                            if (it->targetDecl != selfDecl_ || !pendingLetrecSlots.count(it->slot)) {
+                                ++it;
+                                continue;
+                            }
+                            if (it->slot == preDeclaredSlot) {
+                                // See Op::MakeClosure's own runtime handler
+                                // (bytecode_vm.cpp): the closure being
+                                // created doesn't exist yet at the moment
+                                // its own captures are normally snapshotted,
+                                // so this one is deferred and patched in
+                                // right after construction instead of read
+                                // eagerly like every other capture.
+                                it->isSelfReference = true;
+                                ++it;
+                            } else {
+                                // Forward reference to a sibling that hasn't
+                                // been constructed AT ALL yet -- unlike the
+                                // self-reference case above, there's nothing
+                                // to read OR self-patch into here. Left out
+                                // of `captures` entirely (nothing for
+                                // Op::MakeClosure to even attempt) and
+                                // deferred to a real Op::PatchClosureCapture
+                                // once that sibling's own StoreLocal runs.
+                                pendingWaiters[it->slot].emplace_back(preDeclaredSlot, it->name);
+                                it = captures.erase(it);
+                            }
                         }
                     }
                     if (!name.empty() && name[0] == '$') {
                         out.push_back({Op::StoreDyn, internName(name), 0, &assign->position()});
                     } else {
                         int slot = selfBinding ? preDeclaredSlot : declareLocal(scope, name);
                         out.push_back({Op::StoreLocal, slot, 0, &assign->position()});
+                        if (selfBinding) {
+                            pendingLetrecSlots.erase(slot);
+                            auto waitersIt = pendingWaiters.find(slot);
+                            if (waitersIt != pendingWaiters.end()) {
+                                for (const auto& [consumerSlot, capName] : waitersIt->second) {
+                                    out.push_back({Op::PatchClosureCapture, consumerSlot, internName(capName),
+                                                    &assign->position(), nullptr, slot});
+                                }
+                                pendingWaiters.erase(waitersIt);
+                            }
+                        }
                     }
                 }
                 out[placeholderIdx].a = slotStart;

src/bytecode_vm.cpp

@@ -203,6 +203,29 @@ Value runChunk(Evaluator& ev, const CompiledChunk& chunk, const std::vector<Inst
                 ++pc;
                 break;
             }
+            case Op::PatchClosureCapture: {
+                // Mutual-recursion support (see the LetOp compile case's
+                // own doc comment, bytecode_compiler.cpp): `slots[ins.a]`
+                // (the consumer, e.g. isEven) already holds a real
+                // MakeClosure-created closure whose own capturedTrail is
+                // missing this ONE entry -- it was left out entirely at
+                // compile time because the sibling it names (e.g. isOdd)
+                // hadn't been constructed yet. `slots[ins.c]` (that
+                // sibling's own slot) does now, since this instruction only
+                // ever runs immediately after that sibling's own
+                // Op::StoreLocal. Mutates the SAME shared TrailView the
+                // consumer's own capturedLet already points at -- visible
+                // to it exactly like every other capture, no different
+                // from Op::MakeClosure's own self-reference patch, above.
+                const Value& consumerVal = slots[static_cast<size_t>(ins.a)];
+                if (const auto* closurePtr = std::get_if<ClosurePtr>(&consumerVal); closurePtr && *closurePtr) {
+                    if (auto trail = capturedLetTrail(**closurePtr)) {
+                        trail->set(chunk.names[static_cast<size_t>(ins.b)], slots[static_cast<size_t>(ins.c)]);
+                    }
+                }
+                ++pc;
+                break;
+            }
             case Op::Range: {
                 Value step = std::move(stack.back());
                 stack.pop_back();

tests/test_bytecode_compiler.cpp

@@ -659,6 +659,49 @@ TEST(BytecodeCompiler, PlainLetSelfReferenceStillSeesTheOuterBindingNotItself) {
     EXPECT_EQ(runCapturingEcho("x = 100;\nfunction f() = let(x = x + 1) x;\necho(f());"), "ECHO: 101");
 }
 
+TEST(BytecodeCompiler, MutualRecursionBetweenSiblingLetBoundClosuresResolvesCorrectly) {
+    ScopedVm vm(true);
+    // fnliterals.scad-style isEven()/isOdd(): TWO sibling closures in the
+    // SAME let(), each calling the OTHER by name. `isEven` (compiled
+    // FIRST) references `isOdd`, which doesn't exist at all yet at that
+    // point -- unlike a self-reference, there's nothing to read OR self-
+    // patch into (see Op::PatchClosureCapture's own doc comment,
+    // bytecode.hpp): resolved instead by a real patch instruction emitted
+    // right after `isOdd`'s own StoreLocal, once it actually exists.
+    // 3 stops, not one per logical call (isEvenTest's own entry, plus one
+    // per level of isEven(6)->isOdd(5)->isEven(4)->...->isEven(0)):
+    // isEven<->isOdd calling each other in tail position is exactly what
+    // the tail-call trampoline collapses into ONE evalUserFunctionCore
+    // invocation for the WHOLE chain (see runCompiledFunctionTrampoline,
+    // bytecode_vm.cpp) -- its own unconditional body-entry checkDebug()
+    // fires once for that entire loop, not once per hop. isEvenTest's own
+    // entry (1) + that one trampoline loop (1) + the top-level echo()
+    // statement's own stop (module code is never compiled) = 3.
+    const int stops = countDebugHookStops(
+        "function isEvenTest(n) = let(isEven = function(k) k==0 ? true : isOdd(k-1), isOdd = function(k) "
+        "k==0 ? false : isEven(k-1)) isEven(n);\n"
+        "echo(isEvenTest(6));",
+        std::unordered_map<std::string, std::set<int>>{});
+    EXPECT_EQ(stops, 3);
+    EXPECT_EQ(runCapturingEcho("function isEvenTest(n) = let(isEven = function(k) k==0 ? true : isOdd(k-1), "
+                                "isOdd = function(k) k==0 ? false : isEven(k-1)) isEven(n);\n"
+                                "echo(isEvenTest(6));\necho(isEvenTest(7));"),
+              "ECHO: true\nECHO: false");
+}
+
+TEST(BytecodeCompiler, ThreeWaySiblingMutualRecursionResolvesCorrectly) {
+    ScopedVm vm(true);
+    // The patch mechanism isn't hardcoded to pairs -- a THIRD sibling
+    // (f3, itself forward-referencing f1, the FIRST one compiled) proves
+    // pendingWaiters/Op::PatchClosureCapture generalizes to an arbitrary
+    // cycle length, not just mutual (2-closure) recursion specifically.
+    EXPECT_EQ(runCapturingEcho("function test(n) = let(f1 = function(k) k==0 ? \"f1\" : f2(k-1), f2 = "
+                                "function(k) k==0 ? \"f2\" : f3(k-1), f3 = function(k) k==0 ? \"f3\" : "
+                                "f1(k-1)) f1(n);\n"
+                                "echo(test(9));\necho(test(10));\necho(test(11));"),
+              "ECHO: \"f1\"\nECHO: \"f2\"\nECHO: \"f3\"");
+}
+
 TEST(BytecodeCompiler, ClosureWithDollarParameterStillBailsContainer) {
     ScopedVm vm(true);
     // Residual, deliberate limitation: a FunctionLiteral with its OWN