diff --git a/include/openscad_cpp_evaluator/bytecode.hpp b/include/openscad_cpp_evaluator/bytecode.hpp index fdff542..b8c89d4 100644 --- a/include/openscad_cpp_evaluator/bytecode.hpp +++ b/include/openscad_cpp_evaluator/bytecode.hpp @@ -241,7 +241,11 @@ struct Instruction { struct CompiledChunk { struct Param { std::string name; - int slot = 0; + int slot = 0; // unused when isDyn is true -- $-params are never slot-addressed + // $-prefixed parameter: binds through ctx.dyn (dynamically scoped) + // instead of a slot -- see compileFunctionLike's own doc comment, + // bytecode_compiler.cpp, and bindCompiledArgs', bytecode_vm.cpp. + bool isDyn = false; }; // A call site statically resolved at compile time (see diff --git a/pyproject.toml b/pyproject.toml index 9110e8f..51952e1 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "scikit_build_core.build" [project] name = "openscad_cpp_evaluator" -version = "0.9.2" +version = "0.10.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 7c8025c..9c9788e 100644 --- a/src/bytecode_compiler.cpp +++ b/src/bytecode_compiler.cpp @@ -193,7 +193,7 @@ class Compiler; // (a non-empty `enclosing` chain, letting the literal's own free-variable // references resolve as upvalues into whatever lexically contains it). // Returns false (chunk left partially populated, discarded by the caller) -// for a $-prefixed parameter or any NotCompilable thrown while compiling. +// for any NotCompilable thrown while compiling. bool compileFunctionLike(CompiledChunk& chunk, const oscad::Scope* staticScope, const oscad::ASTNode* selfDecl, std::vector enclosing, const std::vector>& params, @@ -1075,21 +1075,21 @@ bool compileFunctionLike(CompiledChunk& chunk, const oscad::Scope* staticScope, std::vector enclosing, const std::vector>& params, const oscad::Expression& bodyExpr) { - for (const auto& p : params) { - // $-prefixed parameters bind through ctx.dyn (dynamically scoped, - // never slot-addressed) via the existing applyDefaults/bindArgs - // machinery -- out of scope for this design, see this file's - // header comment. - 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(); for (const auto& p : params) { - int slot = compiler.declareLocal(bodyScope, p->name->name); - chunk.params.push_back(CompiledChunk::Param{p->name->name, slot}); + // $-prefixed parameters bind through ctx.dyn (dynamically scoped) + // rather than a slot -- never declared as a local, so a reference + // to one inside the body already compiles to Op::LoadDyn via the + // Identifier case's own $-check regardless of scope visibility. + // Still occupies its declared POSITION among all parameters (see + // bindCompiledArgs, bytecode_vm.cpp) for positional-argument + // matching, exactly like the interpreter's own bindArgs. + const bool isDyn = !p->name->name.empty() && p->name->name[0] == '$'; + const int slot = isDyn ? 0 : compiler.declareLocal(bodyScope, p->name->name); + chunk.params.push_back(CompiledChunk::Param{p->name->name, slot, isDyn}); } chunk.defaultCode.resize(params.size()); diff --git a/src/bytecode_vm.cpp b/src/bytecode_vm.cpp index 6ced1c5..5d43671 100644 --- a/src/bytecode_vm.cpp +++ b/src/bytecode_vm.cpp @@ -604,17 +604,18 @@ Value runChunk(Evaluator& ev, const CompiledChunk& chunk, const std::vectorset) since $-parameters never -// reach compilation (see tryCompileFunction) -- so ANY $-named argument here -// is necessarily an undeclared override, exactly like today's interpreter -// path. An undeclared plain (non-$) name has no slot and nothing in this -// chunk can ever reference it by name, so its already-evaluated value is -// simply discarded -- ponytail: ctx.let_ isn't written for it either -// (unlike the interpreter path), which changes nothing observable, since -// nothing downstream of a compiled call ever reads ctx.let_ for a name a -// compiled body didn't itself resolve to a slot. +// effect rule), but writes a matched plain-parameter value directly into +// `slots` instead of a fresh unordered_map, and routes a matched DECLARED +// $-parameter (chunk.params[i].isDyn) straight to `childCtx.dyn` instead of +// a slot -- see compileFunctionLike's own doc comment, bytecode_compiler.cpp. +// A $-named argument that doesn't match any declared parameter is still an +// undeclared override, exactly like today's interpreter path -- routed to +// `childCtx.dyn` too. An undeclared plain (non-$) name has no slot and +// nothing in this chunk can ever reference it by name, so its already- +// evaluated value is simply discarded -- ponytail: ctx.let_ isn't written +// for it either (unlike the interpreter path), which changes nothing +// observable, since nothing downstream of a compiled call ever reads +// ctx.let_ for a name a compiled body didn't itself resolve to a slot. void bindCompiledArgs(Evaluator& ev, const CompiledChunk& chunk, const std::vector>& arguments, EvalContext& callerCtx, EvalContext& childCtx, std::vector& slots, std::vector& bound) { @@ -628,7 +629,11 @@ void bindCompiledArgs(Evaluator& ev, const CompiledChunk& chunk, bool matched = false; for (size_t i = 0; i < nparams; ++i) { if (chunk.params[i].name == name) { - slots[static_cast(chunk.params[i].slot)] = std::move(v); + if (chunk.params[i].isDyn) { + childCtx.dyn->set(name, std::move(v)); + } else { + slots[static_cast(chunk.params[i].slot)] = std::move(v); + } bound[i] = true; matched = true; break; @@ -641,7 +646,12 @@ void bindCompiledArgs(Evaluator& ev, const CompiledChunk& chunk, auto& a = static_cast(*argPtr); Value v = ev.evalExpr(*a.expr, callerCtx); if (positionalIdx < nparams) { - slots[static_cast(chunk.params[positionalIdx].slot)] = std::move(v); + const CompiledChunk::Param& p = chunk.params[positionalIdx]; + if (p.isDyn) { + childCtx.dyn->set(p.name, std::move(v)); + } else { + slots[static_cast(p.slot)] = std::move(v); + } bound[positionalIdx] = true; } ++positionalIdx; @@ -649,6 +659,28 @@ void bindCompiledArgs(Evaluator& ev, const CompiledChunk& chunk, } } +// Shared by runCompiledFunction/runCompiledFunctionFromBound: applies each +// unbound parameter's default (or, for a $-parameter with none, an explicit +// undef -- mirrors Evaluator::applyDefaults' own "declaring a parameter +// always creates a fresh binding" rule, which for a slot-addressed plain +// parameter is already true for free via frame.slots' own zero-initialized +// Value{} fill, but for ctx.dyn needs the same explicit write applyDefaults +// makes, or a stale ancestor-level $-var would incorrectly show through). +void applyCompiledDefaults(Evaluator& ev, const CompiledChunk& chunk, VmFrame& frame, EvalContext& childCtx) { + for (size_t i = 0; i < chunk.params.size(); ++i) { + if (frame.bound[i]) continue; + const CompiledChunk::Param& p = chunk.params[i]; + if (p.isDyn) { + childCtx.dyn->set(p.name, chunk.defaultCode[i].empty() + ? Value{} + : runChunk(ev, chunk, chunk.defaultCode[i], frame.slots, childCtx, frame.stack)); + } else if (!chunk.defaultCode[i].empty()) { + frame.slots[static_cast(p.slot)] = + runChunk(ev, chunk, chunk.defaultCode[i], frame.slots, childCtx, frame.stack); + } + } +} + } // namespace Value runCompiledFunction(Evaluator& ev, const CompiledChunk& chunk, @@ -667,12 +699,7 @@ Value runCompiledFunction(Evaluator& ev, const CompiledChunk& chunk, // started running, never during argument binding itself. ev.setCurrentCallVmFrame(&frame); bindCompiledArgs(ev, chunk, arguments, callerCtx, childCtx, frame.slots, frame.bound); - for (size_t i = 0; i < chunk.params.size(); ++i) { - if (!frame.bound[i] && !chunk.defaultCode[i].empty()) { - frame.slots[static_cast(chunk.params[i].slot)] = - runChunk(ev, chunk, chunk.defaultCode[i], frame.slots, childCtx, frame.stack); - } - } + applyCompiledDefaults(ev, chunk, frame, childCtx); return runChunk(ev, chunk, chunk.bodyCode, frame.slots, childCtx, frame.stack, tailOut); } @@ -684,8 +711,13 @@ Value runCompiledFunctionFromBound(Evaluator& ev, const CompiledChunk& chunk, co frame.bound.assign(chunk.params.size(), false); ev.setCurrentCallVmFrame(&frame); for (size_t i = 0; i < chunk.params.size(); ++i) { - if (const Value* v = bound.find(chunk.params[i].name)) { - frame.slots[static_cast(chunk.params[i].slot)] = *v; + const CompiledChunk::Param& p = chunk.params[i]; + if (const Value* v = bound.find(p.name)) { + if (p.isDyn) { + childCtx.dyn->set(p.name, *v); + } else { + frame.slots[static_cast(p.slot)] = *v; + } frame.bound[i] = true; } } @@ -699,12 +731,7 @@ Value runCompiledFunctionFromBound(Evaluator& ev, const CompiledChunk& chunk, co } if (!matched && !k.empty() && k[0] == '$') childCtx.dyn->set(k, v); } - for (size_t i = 0; i < chunk.params.size(); ++i) { - if (!frame.bound[i] && !chunk.defaultCode[i].empty()) { - frame.slots[static_cast(chunk.params[i].slot)] = - runChunk(ev, chunk, chunk.defaultCode[i], frame.slots, childCtx, frame.stack); - } - } + applyCompiledDefaults(ev, chunk, frame, childCtx); return runChunk(ev, chunk, chunk.bodyCode, frame.slots, childCtx, frame.stack, tailOut); } diff --git a/tests/test_bytecode_compiler.cpp b/tests/test_bytecode_compiler.cpp index 82b7482..618802b 100644 --- a/tests/test_bytecode_compiler.cpp +++ b/tests/test_bytecode_compiler.cpp @@ -175,10 +175,34 @@ TEST(BytecodeCompiler, NonCompilableFunctionContainingACallStillFallsBackCorrect "ECHO: 8"); } -TEST(BytecodeCompiler, DollarPrefixedParameterBailsCompilationAndStillWorks) { - ScopedVm vm(true); +TEST(BytecodeCompiler, DollarPrefixedParameterCompiles) { + ScopedVm vm(true); + // $-prefixed parameters used to bail compileFunctionLike outright + // (bound through ctx.dyn, never slot-addressed) -- now they compile: + // never declared as a local (skipped in the params/declareLocal loop), + // so every reference inside the body still reaches Op::LoadDyn via the + // Identifier case's own $-check, exactly as an undeclared override + // already did. EXPECT_EQ(runCapturingEcho("function withFn($fn = 8) = $fn;\necho(withFn());"), "ECHO: 8"); EXPECT_EQ(runCapturingEcho("function withFn($fn = 8) = $fn;\necho(withFn($fn = 20));"), "ECHO: 20"); + // Positional binding: a $-param still occupies its declared POSITION + // among all parameters for a caller's positional arguments, exactly + // like the interpreter's own bindArgs. + EXPECT_EQ(runCapturingEcho("function withFn($fn, x) = $fn + x;\necho(withFn(10, 1));"), "ECHO: 11"); + // Declared without a default and not passed: shadows to undef locally + // rather than inheriting an ancestor's dyn binding -- mirrors + // Evaluator::applyDefaults' explicit "declaring a parameter always + // creates a fresh binding" write. + EXPECT_EQ(runCapturingEcho("function withFn($fn) = $fn;\necho(let($fn = 99) withFn());"), "ECHO: undef"); + // The proof this is actually running compiled, not silently falling + // back: same ternary-bodied shape and technique as + // FastContinueWithNoBreakpointInFunctionUsesVm above (3 stops = + // compiled, 5 = interpreted), with $fn substituted in for the plain + // parameter both as the declared name and every body reference. + const int stops = countDebugHookStops("function f($fn) = $fn > 0 ? $fn + 1 : $fn - 1;\n" + "echo(f(5));", + std::unordered_map>{{"", {2}}}); + EXPECT_EQ(stops, 3); } TEST(BytecodeCompiler, UndeclaredDollarNamedArgumentReachesDynInsideCompiledFunction) { @@ -735,27 +759,26 @@ TEST(BytecodeCompiler, TernaryWrappedMutualRecursionResolvesCorrectly) { EXPECT_EQ(runCapturingEcho(script), "ECHO: true\nECHO: false"); } -TEST(BytecodeCompiler, ClosureWithDollarParameterStillBailsContainer) { - ScopedVm vm(true); - // Residual, deliberate limitation: a FunctionLiteral with its OWN - // dollar-prefixed parameter still fails compileFunctionLike outright - // (same $-prefixed-parameter rule as a plain named function, see - // DollarPrefixedParameterBailsCompilationAndStillWorks above) -- there's - // no lighter way left to discover what it captures in that case, so the - // WHOLE containing declaration still bails (throw NotCompilable), same - // as before Op::MakeClosure. Value must still be correct via the - // interpreter fallback. - const int stops = countDebugHookStops( - "function outer(x) = let(g = function(y, $fn) y + x + $fn) g(5, $fn=2);\n" - "echo(outer(10));", - std::nullopt); - // `outer` never compiles at all here (more than 1 stop for its own - // call), unlike MakeClosureLetsContainerCompileDespiteNonEscapingClosure - // above where the analogous ($-free) container compiles to 1. - EXPECT_GT(stops, 2); - EXPECT_EQ(runCapturingEcho("function outer(x) = let(g = function(y, $fn) y + x + $fn) g(5, $fn=2);\n" - "echo(outer(10));"), - "ECHO: 17"); +TEST(BytecodeCompiler, ClosureWithDollarParameterNowCompilesToo) { + ScopedVm vm(true); + // A FunctionLiteral with its OWN dollar-prefixed parameter used to fail + // compileFunctionLike outright, bailing the WHOLE containing + // declaration (see DollarPrefixedParameterCompiles above for the plain- + // function case) -- now compileFunctionLike has no $-specific bail left + // at all, so this compiles too, container and closure both. + const std::string script = "function outer(x) = let(g = function(y, $fn) y + x + $fn) g(5, $fn=2);\n" + "echo(outer(10));"; + EXPECT_EQ(runCapturingEcho(script), "ECHO: 17"); + // Proof via the same fast-continue stop-count technique used + // throughout this file (nullopt is NOT a reliable signal here -- it + // forces every function to interpret regardless of eligibility, see + // DebugAttachedWithoutFastContinueAlwaysInterprets above -- a real + // breakpoint map on a non-matching line is what actually distinguishes + // compiled from interpreted). 3 stops: outer's own body-entry, g's + // call-site, g's own body-entry -- no ternary/sub-expression stops for + // either, since both now run compiled. + const int stops = countDebugHookStops(script, std::unordered_map>{{"", {2}}}); + EXPECT_EQ(stops, 3); } // -- Tail-call optimization, VM path (Phase B) ----------------------------- diff --git a/tests/test_tail_calls.cpp b/tests/test_tail_calls.cpp index cb1c058..a2a7576 100644 --- a/tests/test_tail_calls.cpp +++ b/tests/test_tail_calls.cpp @@ -119,6 +119,21 @@ TEST(TailCalls, DollarVarSetEarlyInATailChainStaysVisibleManyHopsLater) { EXPECT_EQ(runCapturingEcho(script), "ECHO: 77"); } +TEST(TailCalls, DollarParameterThreadsThroughCompiledTailRecursion) { + ScopedVm vm(true); + // $fn declared as its own PARAMETER (not just read as an ambient dyn + // var, unlike the test above) rebinds via childCtx.dyn at every tail + // hop -- compileFunctionLike no longer bails on a $-prefixed parameter + // (see its own doc comment, bytecode_compiler.cpp). No tail-call- + // specific hazard: every hop's bindCompiledArgs call is identical to + // what a genuine non-tail call already does, and dyn already threads + // correctly across hops (proved by the test above). + const std::string script = + "function sum_with_fn($fn, n, acc=0) = n == 0 ? $fn + acc : sum_with_fn($fn, n - 1, acc + n);\n" + "echo(sum_with_fn(1000, 50000));"; + EXPECT_EQ(runCapturingEcho(script), "ECHO: 1.25003e+9"); +} + TEST(TailCalls, NonTailRecursionIsUnaffectedByTheTrampoline) { ScopedVm vm(false); // `n * fact(n-1)` is NOT a tail position -- the multiply happens after @@ -316,6 +331,32 @@ TEST(TailCalls, DeepNonTailRecursionHitsAControlledErrorInsteadOfCrashingCompile EXPECT_THROW(ev.resolveTree(ast, ctx), EvalError); } +TEST(TailCalls, CollatzStepsRecursesFromBothTernaryBranchesInterpreted) { + ScopedVm vm(false); + // Real-world example exercising tail calls from BOTH ternary branches at + // once (the even and odd cases each recurse) -- came up when auditing + // for un-trampolined tail positions (see this file's header). Scans + // every n < 10000 and takes the longest chain; the known record holder + // in that range is n=6171 at 261 steps, calling collatz_steps 10000 + // times over. + const std::string script = + "function collatz_steps(n, steps=0) = n == 1 ? steps : " + "(n % 2 == 0 ? collatz_steps(n / 2, steps + 1) : collatz_steps(3 * n + 1, steps + 1));\n" + "lengths = [for (i = [1:10000]) collatz_steps(i)];\n" + "echo(max(lengths));"; + EXPECT_EQ(runCapturingEcho(script), "ECHO: 261"); +} + +TEST(TailCalls, CollatzStepsRecursesFromBothTernaryBranchesCompiled) { + ScopedVm vm(true); + const std::string script = + "function collatz_steps(n, steps=0) = n == 1 ? steps : " + "(n % 2 == 0 ? collatz_steps(n / 2, steps + 1) : collatz_steps(3 * n + 1, steps + 1));\n" + "lengths = [for (i = [1:10000]) collatz_steps(i)];\n" + "echo(max(lengths));"; + EXPECT_EQ(runCapturingEcho(script), "ECHO: 261"); +} + TEST(TailCalls, ShallowNonTailRecursionStillWorksUnderTheDepthGuard) { ScopedVm vm(true); // The guard must not fire for perfectly ordinary, shallow non-tail