From d05ded7afb44cef0242cfb1f8849aaf8b7996e5b Mon Sep 17 00:00:00 2001 From: Revar Desmera Date: Thu, 30 Jul 2026 16:10:41 -0700 Subject: [PATCH] Resolve mutual recursion between sibling let-bound closures Extends the letrec pre-declare (v0.8.1's self-reference fix) to cover two-or-more sibling `let`-bound closures that reference each other by name -- fnliterals.scad-style isEven()/isOdd(), each calling the other. All function-literal-RHS assignments in a LetOp now get their own slot pre-declared up front (not just each one's own, right before compiling it), so a forward reference to a not-yet-compiled sibling also resolves as a genuine upvalue. Unlike self-reference, a forward reference has nothing to read OR self-patch into at Op::MakeClosure time -- the sibling it names doesn't exist AT ALL yet, not just "not yet stored." Such captures are left out of the closure's own capture list entirely at compile time and deferred to a new Op::PatchClosureCapture instruction, emitted right after the referenced sibling's own StoreLocal actually runs -- mutating the earlier closure's already-real capturedTrail in place, the same shared TrailView its own later invocations already read through. Verified: 2-way (isEven/isOdd) and 3-way (f1->f2->f3->f1) cycles both resolve correctly, including a case mixing a genuine outer capture with a forward-sibling reference in the same closure. Both scale linearly (100,000-deep isEven/isOdd chain in ~0.07s) -- no repeat of the capturedLet-chaining regression the self-reference fix required a guard against, since a resolved forward reference is a real capture snapshotted once, not a value re-derived on every call. Co-Authored-By: Claude Sonnet 5 --- include/openscad_cpp_evaluator/bytecode.hpp | 25 +++- pyproject.toml | 2 +- src/bytecode_compiler.cpp | 136 +++++++++++++------- src/bytecode_vm.cpp | 23 ++++ tests/test_bytecode_compiler.cpp | 43 +++++++ 5 files changed, 180 insertions(+), 49 deletions(-) diff --git a/include/openscad_cpp_evaluator/bytecode.hpp b/include/openscad_cpp_evaluator/bytecode.hpp index d3c03a6..fdff542 100644 --- a/include/openscad_cpp_evaluator/bytecode.hpp +++ b/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 diff --git a/pyproject.toml b/pyproject.toml index c17fd01..229f6d8 100644 --- a/pyproject.toml +++ b/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" diff --git a/src/bytecode_compiler.cpp b/src/bytecode_compiler.cpp index 40eaf9d..28c665b 100644 --- a/src/bytecode_compiler.cpp +++ b/src/bytecode_compiler.cpp @@ -10,6 +10,7 @@ #include #include +#include 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& code) { for (const Instruction& ins : code) { @@ -566,35 +567,71 @@ 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 letrecSlot(n.assignments.size(), -1); + std::unordered_set 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>> 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(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] == '$') { @@ -602,6 +639,17 @@ class Compiler { } 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; diff --git a/src/bytecode_vm.cpp b/src/bytecode_vm.cpp index 90b1c59..6ced1c5 100644 --- a/src/bytecode_vm.cpp +++ b/src/bytecode_vm.cpp @@ -203,6 +203,29 @@ Value runChunk(Evaluator& ev, const CompiledChunk& chunk, const std::vector(ins.a)]; + if (const auto* closurePtr = std::get_if(&consumerVal); closurePtr && *closurePtr) { + if (auto trail = capturedLetTrail(**closurePtr)) { + trail->set(chunk.names[static_cast(ins.b)], slots[static_cast(ins.c)]); + } + } + ++pc; + break; + } case Op::Range: { Value step = std::move(stack.back()); stack.pop_back(); diff --git a/tests/test_bytecode_compiler.cpp b/tests/test_bytecode_compiler.cpp index 98bb3fd..18d9cbc 100644 --- a/tests/test_bytecode_compiler.cpp +++ b/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>{}); + 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