Let a self-referential let-bound closure resolve itself as an upvalue

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

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"

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<Instruction>& 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<size_t>(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()});
                     }
                 }

src/bytecode_vm.cpp

@@ -173,7 +173,20 @@ Value runChunk(Evaluator& ev, const CompiledChunk& chunk, const std::vector<Inst
                 // created escaping closure already uses, unchanged.
                 const CompiledChunk::ClosureSite& site = chunk.closureSites[static_cast<size_t>(ins.a)];
                 auto capturedTrail = TrailView<Value>::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<const std::string*> 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<size_t>(cap.slot)]);
                         continue;
@@ -182,7 +195,11 @@ Value runChunk(Evaluator& ev, const CompiledChunk& chunk, const std::vector<Inst
                     if (!v) v = ctx.let_->find(cap.name);
                     capturedTrail->set(cap.name, v ? *v : Value{});
                 }
-                stack.push_back(Value{std::make_shared<const Closure>(Closure{site.node, std::move(capturedTrail)})});
+                auto closure = std::make_shared<const Closure>(Closure{site.node, capturedTrail});
+                for (const std::string* selfName : selfNames) {
+                    capturedTrail->set(*selfName, Value{closure});
+                }
+                stack.push_back(Value{std::move(closure)});
                 ++pc;
                 break;
             }

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<l? "
         "a(func(x,list[i]), i+1) : x) a(init,0);\n"
         "echo(reduce(function(p,q) p+q, [1,2,3,4], 0));",
         std::unordered_map<std::string, std::set<int>>{});
-    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<l? a(func(x,list[i]), i+1) : x) a(init,0);\n"
                                 "echo(reduce(function(p,q) p+q, [1,2,3,4], 0));"),
               "ECHO: 10");
 }
 
+TEST(BytecodeCompiler, PlainLetSelfReferenceStillSeesTheOuterBindingNotItself) {
+    ScopedVm vm(true);
+    // The letrec pre-declare above is gated on the RHS being a DIRECT
+    // FunctionLiteral specifically because a non-function `let(x = x + 1)`
+    // must keep seeing the OUTER x (real OpenSCAD/letOp shadowing
+    // semantics -- each assignment's RHS sees only what preceded it in the
+    // same let, never itself), not a fresh, not-yet-assigned local
+    // shadowing it. Confirms that ordinary case is untouched by the
+    // FunctionLiteral-only pre-declare.
+    EXPECT_EQ(runCapturingEcho("x = 100;\nfunction f() = let(x = x + 1) x;\necho(f());"), "ECHO: 101");
+}
+
 TEST(BytecodeCompiler, ClosureWithDollarParameterStillBailsContainer) {
     ScopedVm vm(true);
     // Residual, deliberate limitation: a FunctionLiteral with its OWN