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