diff --git a/include/openscad_cpp_evaluator/bytecode.hpp b/include/openscad_cpp_evaluator/bytecode.hpp index b6afe05..1e85b13 100644 --- a/include/openscad_cpp_evaluator/bytecode.hpp +++ b/include/openscad_cpp_evaluator/bytecode.hpp @@ -276,24 +276,24 @@ struct CompiledChunk { // literal's own ClosureSite still needed from beyond `node`'s own scope // (i.e. any capture whose resolution target isn't `node` itself). This // "bubbling" is what makes a closure nested inside another compiled- - // creating closure resolve correctly even though the inner one's body - // is never itself compiled (see this struct's own body-compilation - // note below): by the time a real, running CompiledChunk executes - // Op::MakeClosure for `node`, every one of `captures`'s entries is - // guaranteed to resolve against THAT SAME running chunk's own `slots` - // array, regardless of how many literal-within-literal levels `node` - // was originally nested through in the source. + // creating closure resolve correctly: at the OUTERMOST level a given + // capture is actually snapshotted (see Op::MakeClosure's own runtime + // doc comment, bytecode_vm.cpp), `captures`'s entries resolve against + // that running chunk's own `slots` array whenever `targetDecl` matches + // its own declaration -- and, when it doesn't (a capture bubbled up + // from an even-more-deeply-nested literal), against the enclosing + // frame via Evaluator::findUpvalue or, failing that, `ctx.let_` + // (rooted at that OUTER closure's own capturedLet whenever it's itself + // an escaped invocation) -- regardless of how many literal-within- + // literal levels `node` was originally nested through in the source. // - // `node`'s own BODY is deliberately never itself compiled/registered - // once it has any capture need (even an empty one bubbled up from - // something it contains) -- it always runs via the ordinary AST - // interpreter when invoked (Evaluator::lookupCompiledLiteralChunk finds - // no cache entry for it), using Closure::capturedLet exactly like any - // other escaping closure the interpreter creates directly. This keeps - // the whole feature to "make closure CREATION safe to compile"; a - // closure's own body additionally getting compiled too (its captures - // would need to read from this snapshot instead of a live-stack walk) - // is a natural follow-on, not attempted here. + // `node`'s own body IS also registered (Evaluator:: + // lookupCompiledLiteralChunk finds a real cache entry for it, same as + // a zero-capture literal), so a closure invocation can run compiled + // too, not just its creation: Op::LoadUpvalue/Op::MakeClosure inside + // that body fall back to `ctx.let_` exactly as described above + // whenever the live-call-stack walk misses, which is precisely what + // happens once this closure has actually escaped its creator. struct ClosureSite { const oscad::FunctionLiteral* node = nullptr; std::vector captures; @@ -307,6 +307,15 @@ struct CompiledChunk { std::vector> argNames; }; + // The FunctionDeclaration/FunctionLiteral this chunk's own body was + // compiled from (bytecode_compiler.cpp's compileFunctionLike sets this + // to its own `selfDecl` parameter) -- Op::MakeClosure's runtime handler + // (bytecode_vm.cpp) compares a ClosureSite capture's own `targetDecl` + // against this to tell "genuinely local to the running chunk" (read + // `slots` directly) from "bubbled up from a nested literal" (needs the + // enclosing-frame/capturedLet fallback) apart. + const oscad::ASTNode* selfDecl = nullptr; + std::vector params; std::vector> defaultCode; std::vector bodyCode; diff --git a/pyproject.toml b/pyproject.toml index 68f73a5..6451851 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "scikit_build_core.build" [project] name = "openscad_cpp_evaluator" -version = "0.7.0" +version = "0.8.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 453f23b..10d8c49 100644 --- a/src/bytecode_compiler.cpp +++ b/src/bytecode_compiler.cpp @@ -44,11 +44,9 @@ bool isListCompClauseKind(oscad::NodeKind kind) { // `owner` itself (targetDecl != owner) -- see ClosureSite's own doc comment // (bytecode.hpp) for why this "bubbling" is necessary: a literal nested // inside `owner` may need something from further out than `owner`'s own -// scope, and since that inner literal's body is never itself compiled -// (only ever invoked via the interpreter once it has any capture need), -// `owner`'s OWN Op::MakeClosure must snapshot that name too -- otherwise -// the inner literal's interpreter-resolved capturedLet ancestry, rooted at -// whatever `owner` itself captured, would simply never contain it. +// scope, so `owner`'s OWN Op::MakeClosure must snapshot that name too -- +// otherwise the inner literal's own capturedLet, rooted at whatever `owner` +// itself captured, would simply never contain it. // Dedupes by (targetDecl, slot): the same enclosing binding reached through // two different nested literals (or the same literal referencing it twice) // is still one capture to make. @@ -67,6 +65,43 @@ 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, +// 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. +bool containsLoadFree(const std::vector& code) { + for (const Instruction& ins : code) { + if (ins.op == Op::LoadFree) return true; + } + return false; +} + +bool containsLoadFree(const CompiledChunk& chunk) { + if (containsLoadFree(chunk.bodyCode)) return true; + for (const auto& defaultCode : chunk.defaultCode) { + if (containsLoadFree(defaultCode)) return true; + } + return false; +} + // Compile-time name -> slot resolution, mirroring the LetOp/nested-scope // shadowing rules callCtx()/letChildCtx() apply at runtime: one frame per // LetOp (pushed/popped around its own assignments+body), innermost frame @@ -306,32 +341,41 @@ class Compiler { for (const auto& nestedSite : literalChunk.closureSites) { bubbleEscapingCaptures(nestedSite, &n, effectiveCaptures); } - if (effectiveCaptures.empty()) { - // Closes over nothing at all, even transitively -- the - // existing fast path: a single frozen constant works - // for every invocation, and this literal's own compiled - // chunk (possibly containing its OWN MakeClosure sites - // for anything nested inside IT that closes only over - // ITS OWN scope) is genuinely usable, registered as - // before. + // Registered (Evaluator::lookupCompiledLiteralChunk finds + // this by node pointer whenever the closure is later + // invoked, whether that's a plain PushConst-created value + // below or a MakeClosure-snapshotted one) whenever it's + // actually safe to run compiled: always for a zero-capture + // literal (a single frozen constant, unaffected by anything + // 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). + // 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 + // those opcodes' own runtime fallback, bytecode_vm.cpp): + // when the live-call-stack walk misses (the creator's frame + // is gone -- a genuinely escaped closure), they fall back to + // `ctx.let_`, which callCtxFor roots at this exact closure's + // own capturedLet snapshot whenever it's invoked -- the same + // snapshot `effectiveCaptures` describes below. + if (effectiveCaptures.empty() || !containsLoadFree(literalChunk)) { chunk_.nestedLiterals.emplace_back(&n, std::move(literalChunk)); + } + if (effectiveCaptures.empty()) { + // Closes over nothing at all, even transitively -- a + // single frozen constant works for every invocation, no + // per-call snapshot needed. out.push_back( {Op::PushConst, internConst(Value{std::make_shared(Closure{&n, nullptr})}), 0, nullptr}); return; } // Escaping-closure support (Op::MakeClosure, bytecode.hpp). - // `literalChunk`'s own bytecode is discarded here -- its own - // upvalue reads would resolve via the live-call-stack walk - // (Op::LoadUpvalue), which is wrong for a closure that - // outlives its creator -- so this literal always runs - // interpreted when invoked instead (Evaluator:: - // lookupCompiledLiteralChunk finds no cache entry for it), - // using the exact same Closure::capturedLet machinery every - // other escaping closure the interpreter creates directly - // already relies on. Only `effectiveCaptures` (the names/ - // slots to snapshot right now) survives from the discarded - // compile attempt. int siteIdx = static_cast(chunk_.closureSites.size()); chunk_.closureSites.push_back(CompiledChunk::ClosureSite{&n, std::move(effectiveCaptures)}); out.push_back({Op::MakeClosure, siteIdx, 0, nullptr}); @@ -896,6 +940,7 @@ bool compileFunctionLike(CompiledChunk& chunk, const oscad::Scope* staticScope, if (!p->name->name.empty() && p->name->name[0] == '$') return false; } + chunk.selfDecl = selfDecl; Compiler compiler(chunk, staticScope, selfDecl, std::move(enclosing)); CompileScope bodyScope; bodyScope.push(); diff --git a/src/bytecode_vm.cpp b/src/bytecode_vm.cpp index 1f269cd..b866d62 100644 --- a/src/bytecode_vm.cpp +++ b/src/bytecode_vm.cpp @@ -127,8 +127,22 @@ Value runChunk(Evaluator& ev, const CompiledChunk& chunk, const std::vector(ins.a)]; const Value* v = ev.findUpvalue(uv.targetDecl, uv.slot); + if (!v) v = ctx.let_->find(uv.name); stack.push_back(v ? *v : Value{}); ++pc; break; @@ -137,24 +151,36 @@ Value runChunk(Evaluator& ev, const CompiledChunk& chunk, const std::vector(ins.a)]; auto capturedTrail = TrailView::makeRoot(); for (const auto& cap : site.captures) { - capturedTrail->set(cap.name, slots[static_cast(cap.slot)]); + if (cap.targetDecl == chunk.selfDecl) { + capturedTrail->set(cap.name, slots[static_cast(cap.slot)]); + continue; + } + const Value* v = ev.findUpvalue(cap.targetDecl, cap.slot); + if (!v) v = ctx.let_->find(cap.name); + capturedTrail->set(cap.name, v ? *v : Value{}); } stack.push_back(Value{std::make_shared(Closure{site.node, std::move(capturedTrail)})}); ++pc; diff --git a/src/user_calls.cpp b/src/user_calls.cpp index e26e84f..d60380d 100644 --- a/src/user_calls.cpp +++ b/src/user_calls.cpp @@ -152,8 +152,21 @@ EvalContext Evaluator::callCtxFor(const oscad::ASTNode& decl, EvalContext& ctx, std::optional Evaluator::isolatedCallCtxFor(const oscad::ASTNode& declNode, EvalContext& ctx, const std::shared_ptr>& capturedLet) { + // `declNode.scope()`, not `ctx.scope` -- matches every other call- + // resolution function's own `fnScope = decl.scope() ? decl.scope() : + // ctx.scope` pattern (evalUserFunction/evalUserFunctionFromBound/ + // evalFunctionLiteral/evalFunctionLiteralFromBound). Mismatching here + // was invisible until a captures-having FunctionLiteral's own body + // could ever actually be trampolined into (Op::MakeClosure's own + // registration -- bytecode_compiler.cpp's FunctionLiteral case): a + // self-referential closure's free-variable resolution can fall through + // to evalIdentifier's ctx.scope->lookupVariable() fallback (see its own + // doc comment, expr_eval.cpp), which needs the CALLEE's own lexical + // scope to walk outward from -- the caller's scope at the call site + // has no relation to it at all. bool usedChildCtx = false; - EvalContext result = callCtxFor(declNode, ctx, ctx.scope, nullptr, nullptr, &usedChildCtx, capturedLet); + const oscad::Scope* declScope = declNode.scope() ? declNode.scope() : ctx.scope; + EvalContext result = callCtxFor(declNode, ctx, declScope, nullptr, nullptr, &usedChildCtx, capturedLet); if (usedChildCtx) return std::nullopt; return result; } diff --git a/tests/test_bytecode_compiler.cpp b/tests/test_bytecode_compiler.cpp index b3eed1d..11a2360 100644 --- a/tests/test_bytecode_compiler.cpp +++ b/tests/test_bytecode_compiler.cpp @@ -511,10 +511,18 @@ TEST(BytecodeCompiler, MakeClosureLetsContainerCompileDespiteNonEscapingClosure) // above), so fast-continue must be enabled (an empty breakpoint map -- // fast-continue "on", nothing set) to actually observe whether `make` // runs compiled here. + // + // 3 stops, not 4: `make`'s own body-entry (1), `g`'s own body-entry + // (1) -- `g` itself now also runs compiled (its captures-having chunk + // is registered too, not discarded -- see the FunctionLiteral case's + // own doc comment, bytecode_compiler.cpp), so it costs exactly one + // stop instead of the extra sub-expression checkpoint an interpreted + // call used to add on top -- plus the top-level echo() statement's own + // stop (module-level code is never compiled). const int stops = countDebugHookStops("function make(x) = let(g = function(y) y + x) g(5);\n" "echo(make(10));", std::unordered_map>{}); - EXPECT_EQ(stops, 4); + EXPECT_EQ(stops, 3); } TEST(BytecodeCompiler, MakeClosureLetsContainerCompileDespiteEscapingClosure) { @@ -573,6 +581,60 @@ TEST(BytecodeCompiler, MakeClosureBubblesCaptureThroughAnIntermediateClosureLeve EXPECT_EQ(runCapturingEcho(script), "ECHO: 111"); } +TEST(BytecodeCompiler, MultiLevelCurriedClosureInvocationRunsCompiled) { + ScopedVm vm(true); + // fnliterals.scad's f_1arg()-style curry adapter: a closure that + // returns ANOTHER closure nested inside it, both with real captures. + // This is exactly the two-level-nesting case Op::MakeClosure's own + // runtime handler (bytecode_vm.cpp) has to get right when a captures- + // having chunk's OWN body is registered too -- the inner literal's own + // MakeClosure instruction runs as part of the OUTER closure's later, + // separate invocation (not inline within f_1arg's own call, the way a + // single-level closure's capture always does), so its own captures + // (bubbled up from `f_1arg` at compile time) are no longer local to + // whatever frame happens to be running -- they resolve via + // Evaluator::findUpvalue/`ctx.let_`, not `slots` directly. Correctness + // (not a stop count -- several calls chain here, and not every shape + // adds its own extra interpreted-only checkpoint, so the total isn't a + // reliable compiled-vs-interpreted signal for this particular script) + // is what actually matters: a wrong result here is exactly what a + // missed/misdirected capture read would produce. + EXPECT_EQ(runCapturingEcho("function f_1arg(target_func) = function(a) a==undef? function(x) target_func(x) : " + "function() target_func(a);\n" + "function double(x) = x * 2;\n" + "adder = f_1arg(function(x) double(x));\n" + "echo(adder(21)());\n" + "echo(adder(undef)(10));"), + "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. + 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(runCapturingEcho("function reduce(func, list, init=0) = let(l = len(list), a = function (x,i) " + "i