diff --git a/include/openscad_cpp_evaluator/bytecode.hpp b/include/openscad_cpp_evaluator/bytecode.hpp index 1e85b13..d3c03a6 100644 --- a/include/openscad_cpp_evaluator/bytecode.hpp +++ b/include/openscad_cpp_evaluator/bytecode.hpp @@ -266,6 +266,17 @@ struct CompiledChunk { const oscad::ASTNode* targetDecl = nullptr; int slot = 0; std::string name; + // Set only when this entry is a `let(name = function(...) ...)` + // closure's own reference to ITSELF (bytecode_compiler.cpp's LetOp + // case, the letrec-style pre-declared-slot path) -- Op::MakeClosure + // can't read this one out of `slots` the way it does every other + // capture: at the moment it runs, the closure it's building hasn't + // been constructed yet (StoreLocal for `name` hasn't executed), so + // `slots[slot]` would still hold whatever was there before this + // call. See Op::MakeClosure's own runtime doc comment + // (bytecode_vm.cpp) for how this is actually resolved (deferred, + // then patched in after construction). + bool isSelfReference = false; }; // One FunctionLiteral's own capture list for Op::MakeClosure, computed diff --git a/pyproject.toml b/pyproject.toml index 6451851..c17fd01 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.0" +version = "0.8.1" 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 10d8c49..40eaf9d 100644 --- a/src/bytecode_compiler.cpp +++ b/src/bytecode_compiler.cpp @@ -68,25 +68,27 @@ 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). A self-referential recursive closure -// (`let(a = function(x,i) ... a(...) ...)`, the reduce()/accumulate()/ -// while() idiom) hits exactly this: LetOp's own declareLocal for `a` runs -// only AFTER compiling `a`'s RHS, so `a`'s reference to itself can't -// resolve as an upvalue at compile time and falls through to LoadFree -- -// Evaluator::evalIdentifier's own ctx.scope->lookupVariable() fallback, -// which re-evaluates the let-binding's RHS FRESH on every single call, +// compileExpr's own Identifier case). The 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 (see the -// FunctionLiteral case below) 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), 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 -// (this compile-time gap itself isn't fixed here) -- a real, if imperfect, -// scope for now; closing it -- letting `a` resolve as a genuine upvalue of -// its own declaring LetOp -- is future work, not bundled into this change. +// 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) { if (ins.op == Op::LoadFree) return true; @@ -350,10 +352,14 @@ class Compiler { // below), and for a captures-having one only when its own // body contains no Op::LoadFree -- see containsLoadFree's // own doc comment, above, for why a LoadFree-containing - // captures-having closure (the reduce()/accumulate()/ - // while() self-referential-recursion idiom) must still be - // left interpreted: its own capturedLet-chaining, once - // registered, degrades an O(n) reduction into O(n^2). + // captures-having closure must still be left interpreted: + // its own capturedLet-chaining, once registered, degrades + // an O(n) reduction into O(n^2). (The reduce()/ + // accumulate()/while() idiom itself -- a closure directly + // assigned via `let` that calls itself by name -- no longer + // hits this: see the LetOp case's own letrec pre-declare, + // below, which lets that specific self-reference resolve as + // a real upvalue instead of falling through to LoadFree.) // Everything else (Op::LoadUpvalue/Op::MakeClosure inside a // REGISTERED captures-having chunk) is no longer assumed to // resolve only against a still-live creator frame (see @@ -561,12 +567,40 @@ class Compiler { out.push_back({Op::OpenLocalScope, 0, 0, nullptr}); int slotStart = nextSlot_; for (const auto& assign : n.assignments) { - compileExpr(*assign->expr, out, scope); // RHS is never tail 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); + 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; + } + } if (!name.empty() && name[0] == '$') { out.push_back({Op::StoreDyn, internName(name), 0, &assign->position()}); } else { - int slot = declareLocal(scope, name); + int slot = selfBinding ? preDeclaredSlot : declareLocal(scope, name); out.push_back({Op::StoreLocal, slot, 0, &assign->position()}); } } diff --git a/src/bytecode_vm.cpp b/src/bytecode_vm.cpp index b866d62..90b1c59 100644 --- a/src/bytecode_vm.cpp +++ b/src/bytecode_vm.cpp @@ -173,7 +173,20 @@ Value runChunk(Evaluator& ev, const CompiledChunk& chunk, const std::vector(ins.a)]; auto capturedTrail = TrailView::makeRoot(); + // A letrec-style self-reference (UpvalueRef::isSelfReference, + // see its own doc comment) is skipped here, not read -- the + // closure it names is THIS instruction's own result, which + // doesn't exist until the shared_ptr below is constructed. + // Patched in immediately afterward via the exact same + // capturedTrail (a live, shared TrailView -- mutating it + // after `closure` wraps it is exactly as visible to that + // closure's own later invocations as any other entry). + std::vector selfNames; for (const auto& cap : site.captures) { + if (cap.isSelfReference) { + selfNames.push_back(&cap.name); + continue; + } if (cap.targetDecl == chunk.selfDecl) { capturedTrail->set(cap.name, slots[static_cast(cap.slot)]); continue; @@ -182,7 +195,11 @@ Value runChunk(Evaluator& ev, const CompiledChunk& chunk, const std::vectorfind(cap.name); capturedTrail->set(cap.name, v ? *v : Value{}); } - stack.push_back(Value{std::make_shared(Closure{site.node, std::move(capturedTrail)})}); + auto closure = std::make_shared(Closure{site.node, capturedTrail}); + for (const std::string* selfName : selfNames) { + capturedTrail->set(*selfName, Value{closure}); + } + stack.push_back(Value{std::move(closure)}); ++pc; break; } diff --git a/tests/test_bytecode_compiler.cpp b/tests/test_bytecode_compiler.cpp index 11a2360..98bb3fd 100644 --- a/tests/test_bytecode_compiler.cpp +++ b/tests/test_bytecode_compiler.cpp @@ -608,33 +608,57 @@ TEST(BytecodeCompiler, MultiLevelCurriedClosureInvocationRunsCompiled) { "ECHO: 42\nECHO: 20"); } -TEST(BytecodeCompiler, SelfReferentialRecursiveClosureStillRunsInterpretedAndCorrectly) { - ScopedVm vm(true); - // reduce()'s own idiom: `a` calls itself by name, which LetOp's own - // declareLocal-after-compiling-the-RHS ordering means never resolves as - // a genuine upvalue at compile time (falls through to Op::LoadFree - // instead -- see containsLoadFree's own doc comment, bytecode_compiler.cpp). - // Registering such a chunk for compiled invocation is what previously - // (empirically, against a real list) turned an O(n) reduce() into - // O(n^2): every recursive call re-derives a fresh closure via - // Evaluator::evalIdentifier's ctx.scope->lookupVariable() fallback, - // nesting its own capturedLet one level deeper than the last every - // time. `containsLoadFree` excludes exactly this case from - // registration, so `a` keeps running interpreted -- more than 1 stop - // for its own call proves that, and the actual sum must still come out - // right either way. +TEST(BytecodeCompiler, SelfReferentialRecursiveClosureNowResolvesAsUpvalueAndRunsCompiled) { + ScopedVm vm(true); + // reduce()'s own idiom: `a` calls itself by name. A direct + // `let(a = function(...) ... a(...) ...)` RHS gets `a`'s own slot + // pre-declared BEFORE compiling that RHS (bytecode_compiler.cpp's + // LetOp case), letrec-style -- so this self-reference now resolves as + // a genuine upvalue (Op::LoadUpvalue) instead of falling through to + // Op::LoadFree, and `a`'s own body is eligible for registration + // (containsLoadFree no longer sees any unresolved reference in it). + // Op::MakeClosure's own runtime handler defers this ONE capture + // (the closure being built doesn't exist yet when its own captures + // are normally snapshotted) and patches it in immediately after + // constructing the real closure -- see its own doc comment, + // bytecode_vm.cpp. This is also what fixed a real O(n) -> O(n^2) + // regression found while building this feature: every recursive call + // used to re-derive a FRESH closure via Evaluator::evalIdentifier's + // ctx.scope->lookupVariable() fallback, nesting its own capturedLet + // one level deeper than the last -- confirmed via the CLI against a + // 32,000-element list (7+ seconds -> 0.05s once `a` genuinely + // resolves itself as an upvalue instead). + // + // 11 stops, not 30 (confirmed by diffing against the previous, + // containsLoadFree-excluded behavior with the same script): `reduce`'s + // own body-entry (1) + `a`'s own 5 recursive calls (1 each, compiled) + // + `func`'s own 4 calls (1 each, already compiled even before this -- + // zero captures) + the top-level echo() statement's own stop (module + // code is never compiled). const int stops = countDebugHookStops( "function reduce(func, list, init=0) = let(l = len(list), a = function (x,i) i>{}); - EXPECT_GT(stops, 2); + EXPECT_EQ(stops, 11); EXPECT_EQ(runCapturingEcho("function reduce(func, list, init=0) = let(l = len(list), a = function (x,i) " "i