diff --git a/include/openscad_cpp_evaluator/bytecode.hpp b/include/openscad_cpp_evaluator/bytecode.hpp index d58ffbe..94f1d41 100644 --- a/include/openscad_cpp_evaluator/bytecode.hpp +++ b/include/openscad_cpp_evaluator/bytecode.hpp @@ -14,6 +14,7 @@ class FunctionDeclaration; class FunctionLiteral; class ModuleDeclaration; class ModularCall; +class ModularAssert; class Expression; } // namespace oscad @@ -259,23 +260,26 @@ enum class Op { // which splice is unconditional). CallModule, - // A single "native passthrough" statement -- assignment, echo, assert, - // let-block, modifier wrapper, intersection_for, children(), a builtin - // module call, or a user-module call that didn't resolve at compile - // time (shadowed, forward-declared, or otherwise not staticaly known) - // -- anything compileStatementList doesn't give its own real bytecode. + // A single "native passthrough" statement -- a modifier wrapper, + // intersection_for, children(), a builtin module call, or a user- + // module call that didn't resolve at compile time (shadowed, forward- + // declared, or otherwise not statically known) -- anything + // compileStatementList doesn't give its own real bytecode. (Assignment/ + // ModularEcho/ModularAssert/ModularLet used to fall here too; they now + // have their own real bytecode -- Op::StoreModuleVar/Op::Echo/ + // Op::AssertStatement/Op::OpenLetScope+StoreLetVar -- purely a + // throughput change, since none of these were ever the recursion-depth + // risk this compiler targets.) // a = index into CompiledChunk::nativeStatements. Runtime just does // what Evaluator::evalChildren's own per-statement loop already does - // for one node: derive childCtx via ctx.withScope(...), checkDebug - // (skipped for ModularLet, exactly like evalChildren's own condition - // -- ModularLet fires its own per-assignment checkDebug internally), + // for one node: derive childCtx via ctx.withScope(...), checkDebug, // evalStatement. These are "leaf-shaped" in the sense the Stage 2 plan // means it: whatever native recursion they make (a builtin's own - // resolve function walking ITS children, evalLetBlock, etc.) is - // bounded by ordinary script structure, not by how deep a recursive - // module chain goes -- that risk is fully covered by CallModule/ - // ForIterNext/the Jump-based if/for control flow instead, regardless - // of how many of THESE sit alongside them in the same body. + // resolve function walking ITS children, etc.) is bounded by ordinary + // script structure, not by how deep a recursive module chain goes -- + // that risk is fully covered by CallModule/ForIterNext/the Jump-based + // if/for control flow instead, regardless of how many of THESE sit + // alongside them in the same body. NativeStatement, // If/if-else's own condition, evaluated NATIVELY (Evaluator:: @@ -334,6 +338,108 @@ enum class Op { // ForIterNext instruction to attempt the next one. a = jump target // (the matching ForIterNext's own pc). ForIterEnd, + + // -- Module-body leaf-statement compilation (throughput, not a -------- + // -- recursion-safety concern -- see NativeStatement's own doc comment + // for why THAT stayed native for these same node kinds in Stage 2; + // this closes the "every sub-expression is a separate nested driveVm + // call via evalExprMaybeCompiled" gap instead by inlining them into + // the enclosing chunk's own instruction stream) -------------------- + // + // A module-body statement's own top-level checkDebug -- the compiled + // analog of what evalChildren's native per-statement loop / Op:: + // NativeStatement's own handler already does for the SAME node + // (`ev.checkDebug(*stmt, ctx)`, exprLevel=false, forced=false) -- kept + // as a real op (not simply omitted) so a compiled statement's debug + // behavior is identical to its native form, not just "usually doesn't + // matter because fast-continue only compiles breakpoint-free chunks." + // a = index into CompiledChunk::nativeStatements (reusing that table). + CheckDebugStatement, + + // Assignment's own compiled form -- pops one Value (already evaluated + // via an ordinary inline compileExpr, NOT native evalExprMaybeCompiled) + // and replicates Evaluator::evalAssignment exactly: a `$`-prefixed name + // goes to ctx.dyn (+ dynExplicit), everything else checks + // ctx.dynPositions for the "assigned but overwritten" warning, then + // writes ctx.let_ and records this position. Never slot-addressed -- + // module-level/top-level assignments are always ctx-visible, dynamic- + // scope writes, exactly like today. a = name pool index, pos = the + // Assignment node's own position (for both the overwritten warning and + // the position recorded into dynPositions). + StoreModuleVar, + + // ModularEcho/ModularAssert's own statement form share Op::Echo/a + // dedicated AssertStatement op (below) for the "pop N already-compiled + // argument values" mechanics -- see those ops' own doc comments + // (Op::Echo, above; Op::AssertStatement, below) for why the + // EXPRESSION-form opcodes are directly reusable (statement-shaped + // already: no value pushed, straight fall-through) while assert's + // needs its own op (named-argument support + eager-not-lazy message + // evaluation, both genuinely different from AssertOp's own contract). + + // Isolates ONE inline-compiled sub-expression's own `$`-writes from the + // rest of this chunk's shared, long-lived ctx -- pushes ctx. + // letChildCtx() (reads still see through to the parent level; only + // WRITES are isolated) onto f.ctxChain, mirroring exactly what + // runCompiledExprChunk's own wrapper already does for a native call + // into a compiled EXPRESSION chunk (evalExprMaybeCompiled). Needed + // because a module-body statement's own inline-compiled sub- + // expression (Assignment's RHS, one echo()/assert() argument) has NO + // such wrapper of its own the way a genuinely separate chunk call + // does -- without this, `v1 = let($fn=55) f(); v2 = f();` leaks $fn=55 + // into v2 too (caught for real by UserFunction. + // DollarVarLetAsAssignmentRhsDoesNotLeak once Assignment got its own + // inline compiled form). Always emitted, unconditionally, around + // EVERY such sub-expression -- not just ones statically known to + // contain a `let($...)` -- for the same reason runCompiledExprChunk's + // own wrapper is unconditional: a nested call/closure could still + // reach one indirectly, and the cost when nothing $-scoped is present + // is just one cheap trail-level open, not a real expression re-walk. + OpenExprScope, + // Pops the ctx OpenExprScope just pushed (f.ctxChain.pop_back()) -- + // always paired, immediately after the sub-expression's own compiled + // code finishes (its RESULT, if any, is already on f.stack by then; + // this only tears down the ctx, never touches the value stack). Also + // reused to close Op::OpenLetScope's own child ctx (below) -- both are + // just "pop the current ctx level", identical either way. + CloseExprScope, + + // ModularLet's own (statement-form) child scope -- mirrors + // Evaluator::evalLetBlock's own `ctx.childCtx(nullptr, std::nullopt, + // ctx.childrenNodes, ctx.childrenCallerCtx)` call exactly, pushed onto + // f.ctxChain (Op::CloseExprScope, above, pops it back off once the + // let-block's own body finishes compiling/running). Every assignment's + // own RHS is compiled+evaluated BEFORE this runs (while the PARENT ctx + // is still current -- see ModularLet's own compileOneStatement case, + // bytecode_compiler.cpp, for why: the statement form's RHS must never + // see an EARLIER sibling's own write in the SAME let-block, unlike the + // let-EXPRESSION form's sequential visibility), each left on f.stack; + // only the WRITES (Op::StoreLetVar, below) happen after this runs. + OpenLetScope, + + // Op::StoreLetVar -- pops one value and writes it into the CURRENT ctx + // (by then, the child Op::OpenLetScope just pushed) via the same + // `$`-prefix branch Op::StoreModuleVar uses, but WITHOUT its + // dynPositions "assigned but overwritten" bookkeeping -- evalLetBlock's + // own writes never do that check either (a let-block's own child scope + // is always fresh, so "reassigning the same name in the same scope" + // can never happen the way a module-body Assignment's own can). a = + // name pool index, pos = the Assignment node's own position (unused by + // the handler itself, kept for consistency/future debugging only). + StoreLetVar, + + // a = index into CompiledChunk::assertSites. Pops site.argCount values + // (all of this statement's own arguments, already compiled+pushed in + // source order via compileExpr -- EVERY one, not just condition/ + // message, mirrors evalAssertStatement's own eager resolveArgs() over + // AssertOp's lazy-message contract) and replicates + // Evaluator::evalAssertStatement exactly: named "condition"/"message" + // win over positional 0/1 (resolved at COMPILE time into + // AssertSite::conditionArgIndex/messageArgIndex -- the argument SHAPE + // is static, only values vary per call), throws via Evaluator::error() + // on failure, otherwise runs any chained node.children natively (rare; + // not worth its own compiled path -- see AssertSite's own doc comment). + AssertStatement, }; struct Instruction { @@ -477,6 +583,32 @@ struct CompiledChunk { std::string calleeName; }; + // One Op::AssertStatement site -- see that op's own doc comment for + // the full contract. `conditionArgIndex`/`messageArgIndex` are indices + // into the site's own argCount-sized popped-argument array (source + // order), resolved at COMPILE time via the same "named wins over + // positional 0/1" priority Evaluator::getArg applies at runtime -- + // nullopt means "no argument supplies this logical parameter" (mirrors + // getArg's own defaultValue fallback: absent condition reads as + // `Value{true}`, absent message means no ": ..." suffix). + // `condTextConstIdx`: constants[] index of the condition argument's own + // source text (or "false" when no condition argument exists, matching + // evalAssertStatement's `node.arguments.empty() ? "false" : ...`), + // interned once at compile time. `node`: for both Evaluator::error()'s + // TRACE walk and the (rare) chained-children fallback below. + // + // ponytail: a script with TWO named args of the SAME name (e.g. + // `assert(condition=true, condition=false)`) would resolve to the + // LAST one here, not getArg's own first-match; not worth the extra + // bookkeeping to fix for input that's already nonsensical. + struct AssertSite { + int argCount = 0; + std::optional conditionArgIndex; + std::optional messageArgIndex; + int condTextConstIdx = -1; + const oscad::ModularAssert* node = nullptr; + }; + // Every distinct chunk this SPECIFIC CompiledChunk owns is either a // function body (bodyCode is one expression's own instruction stream) // or a module body (bodyCode is a compiled STATEMENT list -- see @@ -512,6 +644,7 @@ struct CompiledChunk { // NativeCondJumpIfFalse/NativeCheckDebugExprLevel/NativeIterMaterialize's // own doc comments, above) -- always empty for a function chunk. std::vector moduleCallSites; + std::vector assertSites; std::vector nativeExprs; std::vector nativeStatements; int numSlots = 0; diff --git a/include/openscad_cpp_evaluator/evaluator.hpp b/include/openscad_cpp_evaluator/evaluator.hpp index 6521ec3..e630b68 100644 --- a/include/openscad_cpp_evaluator/evaluator.hpp +++ b/include/openscad_cpp_evaluator/evaluator.hpp @@ -1336,14 +1336,37 @@ class Evaluator { // free function, not a member, so it needs direct access with no // friend declaration in play. size_t driveVmNativeDepth_ = 0; - // Same figure as kMaxUserCallDepth -- each native re-entry above costs - // a comparable handful of native frames (evalStatement, evalModularCall, - // a builtin resolve function, evalChildren, tryRunCompiledChildren, - // runCompiledModuleBody, driveVm), so the same Windows-CI-calibrated - // ceiling applies. Deliberately its own named constant (not a reuse of - // kMaxUserCallDepth in place) so the two can be recalibrated - // independently if profiling ever shows they should diverge. - static constexpr size_t kMaxDriveVmNativeDepth = 30; + // Originally set to kMaxUserCallDepth's own figure (30) on the + // assumption the two native-reentry chains cost a comparable handful + // of frames each -- WRONG in practice: a real differential sweep + // against BOSL2's own test corpus found 3 genuine scripts + // (test_ball_bearing, test_teardrop_corner_mask, test_rounding_hole_ + // mask -- all built on deeply layered _translate()/_show_highlight()/ + // attachment-wrapper composition, not runaway recursion) that used to + // render successfully but started failing with "Recursion too deep + // (native call stack)" once this guard shipped at 30. + // + // Locally binary-searched (macOS) to a first candidate of 50, the + // smallest value tried that let all 3 succeed again, then FIRST tried + // doubled to 100 for headroom -- that immediately segfaulted on + // Windows CI (ModuleBodyCompiles. + // NativeReentrantRecursionHitsAControlledErrorInsteadOfCrashing, its + // own guard-safety regression test). Backed off to 50 -- STILL + // segfaulted there. Confirms this chain's real per-level native-frame + // cost is heavier than kMaxUserCallDepth's own (which needed the SAME + // kind of comedown, an initial 50 down to 30, for a shallower chain), + // and that Windows' real safe ceiling for THIS chain sits somewhere + // below 50, above the already-Windows-verified-safe 30 (confirmed + // safe by PR #60's own successful merge, before this value ever + // changed). Currently at 40 -- ALSO still functionally sufficient + // (all 3 real scripts still render locally at this value) -- bisecting + // downward toward Windows' actual ceiling via successive CI runs, one + // step at a time; if 40 itself is still unsafe there, keep coming + // down. Do not raise this value again based on local (macOS/Linux) + // testing alone -- only a real Windows CI pass is evidence of safety + // here. Deliberately its own named constant (not a reuse of + // kMaxUserCallDepth) so the two can be recalibrated independently. + static constexpr size_t kMaxDriveVmNativeDepth = 40; // Bracketed public in place: Op::CallModule's own runtime handler and // driveVm's module-frame completion branch (bytecode_vm.cpp, a diff --git a/pyproject.toml b/pyproject.toml index cd58756..7436864 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "scikit_build_core.build" [project] name = "openscad_cpp_evaluator" -version = "0.12.0" +version = "0.13.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 caad082..7fbb790 100644 --- a/src/bytecode_compiler.cpp +++ b/src/bytecode_compiler.cpp @@ -1150,9 +1150,46 @@ class Compiler { for (const oscad::ASTNode* stmt : others) compileOneStatement(*stmt, out); } + // Compiles one inline sub-expression wrapped in Op::OpenExprScope/ + // CloseExprScope -- see those ops' own doc comment (bytecode.hpp) for + // why every module-body statement's own inline-compiled RHS/argument + // needs this (a `$`-write leak this session's own regression test, + // UserFunction.DollarVarLetAsAssignmentRhsDoesNotLeak, caught for + // real). Shared by Assignment/ModularEcho/ModularAssert's own + // compileOneStatement cases, below. + void compileIsolatedExpr(const oscad::Expression& expr, std::vector& out, CompileScope& scope) { + out.push_back({Op::OpenExprScope, 0, 0, nullptr}); + compileExpr(expr, out, scope); + out.push_back({Op::CloseExprScope, 0, 0, nullptr}); + } + void compileOneStatement(const oscad::ASTNode& stmt, std::vector& out) { using oscad::NodeKind; trackSpan(stmt); + // A statement's own .scope() can differ from this whole chunk's + // fixed scope_ member (e.g. a `use` statement earlier in the SAME + // script/module body changes what later statements can see) -- + // compileExpr's own PrimaryCall case resolves a callee via scope_ + // DIRECTLY, not per-node, unlike the ModularCall case just below + // (which already does its own stmt.scope()-first lookup). Any + // inline sub-expression compiled from Assignment/ModularEcho/ + // ModularAssert, below, needs scope_ swapped to match THIS + // statement for its own duration, or a nested function call inside + // it resolves against the wrong (chunk-wide) scope instead of its + // own. Caught for real: UseStatement.NestedUseNotReExported + // started spuriously resolving a NOT-re-exported nested `use`'s + // function once echo() got its own inline-compiled argument. + // RAII (not "restore before every return") since NotCompilable can + // unwind through here -- harmless either way (the whole Compiler + // is discarded on that path), but cheap to get right regardless. + struct ScopeGuard { + const oscad::Scope*& scope; + const oscad::Scope* saved; + ScopeGuard(const oscad::Scope*& s, const oscad::ASTNode& node) : scope(s), saved(s) { + if (const oscad::Scope* stmtScope = node.scope()) scope = stmtScope; + } + ~ScopeGuard() { scope = saved; } + } scopeGuard(scope_, stmt); switch (stmt.kind()) { // Pure declarations, no-ops at statement-eval time (already // hoisted into scope by buildScopes()) -- matches evalStatement's @@ -1160,6 +1197,145 @@ class Compiler { case NodeKind::ModuleDeclaration: case NodeKind::FunctionDeclaration: return; + // Assignment/ModularEcho/ModularAssert: real bytecode instead + // of Op::NativeStatement -- a throughput improvement (see + // CheckDebugStatement/StoreModuleVar/AssertStatement's own doc + // comments, bytecode.hpp), NOT a recursion-safety one (these + // were never the risk Stage 2 targeted -- see NativeStatement's + // own doc comment). Assignment specifically closes a real + // regression: it used to get a genuinely optimized path + // (Evaluator::tryRunCompiledAssignmentBlock) via evalChildren's + // own native fallback loop, but tryRunCompiledChildren's + // broader whole-list compile (the NativeStatement-gap fix) + // ALWAYS succeeds first now, silently shadowing it -- every + // assignment fell back to generic Op::NativeStatement, one + // nested driveVm call per sub-expression via + // evalExprMaybeCompiled, instead of this inline form. + case NodeKind::Assignment: { + auto& n = static_cast(stmt); + out.push_back({Op::CheckDebugStatement, internNativeStatement(&stmt), 0, nullptr}); + CompileScope exprScope; // module-level vars are never slot-addressed -- + // see StoreModuleVar's own doc comment. + compileIsolatedExpr(*n.expr, out, exprScope); + out.push_back({Op::StoreModuleVar, internName(n.name->name), 0, &n.position()}); + return; + } + case NodeKind::ModularEcho: { + // Op::Echo itself is reused verbatim from the EXPRESSION + // form's own compile case (AssertOp's sibling, above in + // compileExpr) -- already statement-shaped (no value + // pushed, straight fall-through), so nothing new is needed + // there. `.children` is deliberately never read here, + // matching evalStatement's own ModularEcho case (doEcho + // only ever takes `.arguments` -- an echo statement's + // trailing children, if the grammar even produces any, are + // not evaluated today; this mirrors that exactly rather + // than silently changing behavior). + auto& n = static_cast(stmt); + out.push_back({Op::CheckDebugStatement, internNativeStatement(&stmt), 0, nullptr}); + CompiledChunk::EchoSite site; + CompileScope exprScope; + for (const auto& argPtr : n.arguments) { + if (argPtr->kind() == NodeKind::NamedArgument) { + auto& a = static_cast(*argPtr); + compileIsolatedExpr(*a.expr, out, exprScope); + site.argNames.push_back(a.name->name); + } else { + auto& a = static_cast(*argPtr); + compileIsolatedExpr(*a.expr, out, exprScope); + site.argNames.push_back(std::nullopt); + } + } + int siteIdx = static_cast(chunk_.echoSites.size()); + chunk_.echoSites.push_back(std::move(site)); + out.push_back({Op::Echo, siteIdx, static_cast(n.arguments.size()), &n.position()}); + return; + } + case NodeKind::ModularAssert: { + // Genuinely different contract from AssertOp's own compiled + // form (Op::AssertFail): the statement form supports named + // arguments AND evaluates every argument EAGERLY (mirrors + // Evaluator::evalAssertStatement's own resolveArgs() call + // exactly -- unlike AssertOp's lazily-compiled message, + // guarded by a runtime JumpIfTrue), so every argument is + // compiled+pushed here unconditionally, in source order. + auto& n = static_cast(stmt); + out.push_back({Op::CheckDebugStatement, internNativeStatement(&stmt), 0, nullptr}); + CompiledChunk::AssertSite site; + site.node = &n; + site.argCount = static_cast(n.arguments.size()); + CompileScope exprScope; + for (size_t i = 0; i < n.arguments.size(); ++i) { + compileIsolatedExpr(*argExpr(*n.arguments[i]), out, exprScope); + if (n.arguments[i]->kind() == NodeKind::NamedArgument) { + auto& na = static_cast(*n.arguments[i]); + if (na.name->name == "condition") site.conditionArgIndex = static_cast(i); + else if (na.name->name == "message") site.messageArgIndex = static_cast(i); + } + } + // Second pass: positional 0/1 fill whichever logical + // parameter a named arg didn't already claim -- run AFTER + // the named pass above (not interleaved) so a named arg + // wins regardless of its position relative to its + // positional counterpart in source, matching + // Evaluator::getArg's own "named first" priority exactly. + int posCounter = 0; + for (size_t i = 0; i < n.arguments.size(); ++i) { + if (n.arguments[i]->kind() == NodeKind::NamedArgument) continue; + if (posCounter == 0 && !site.conditionArgIndex) site.conditionArgIndex = static_cast(i); + else if (posCounter == 1 && !site.messageArgIndex) site.messageArgIndex = static_cast(i); + ++posCounter; + } + // Precomputed once, like AssertFail's own condText -- + // mirrors evalAssertStatement's `node.arguments.empty() ? + // "false" : argExpr(*node.arguments[0])->toString()` + // exactly (a MISSING condition arg -- e.g. a bare + // `assert(message="x");` -- reads the same as an EMPTY + // arg list here: no arg supplies "condition" either way). + site.condTextConstIdx = internConst(Value{ + site.conditionArgIndex + ? argExpr(*n.arguments[static_cast(*site.conditionArgIndex)])->toString() + : std::string("false")}); + int siteIdx = static_cast(chunk_.assertSites.size()); + chunk_.assertSites.push_back(std::move(site)); + out.push_back({Op::AssertStatement, siteIdx, 0, &n.position()}); + return; + } + case NodeKind::ModularLet: { + // Mirrors Evaluator::evalLetBlock exactly (stmt_eval.cpp): + // every assignment's RHS is evaluated against the + // ORIGINAL (parent) ctx, never an earlier sibling's own + // write in THIS SAME let-block (a documented, deliberate + // divergence from the let-EXPRESSION form's sequential + // visibility) -- so every RHS is compiled+evaluated FIRST, + // all left on f.stack, and only written into the freshly- + // opened child scope (Op::OpenLetScope) AFTER every one of + // them has already run against the still-current PARENT + // ctx. No outer statement-level check for the let-block + // node itself (matches evalChildren's own ModularLet + // exclusion) -- each assignment gets its own instead, + // exactly like evalLetBlock's own per-assignment + // checkDebug. The body (n.children) compiles inline, + // recursively, against the now-current child ctx -- same + // Compiler instance, so nested statements get every bit of + // this same real-bytecode treatment too. + auto& n = static_cast(stmt); + CompileScope exprScope; + for (const auto& assign : n.assignments) { + out.push_back({Op::CheckDebugStatement, internNativeStatement(assign.get()), 0, nullptr}); + compileIsolatedExpr(*assign->expr, out, exprScope); + } + out.push_back({Op::OpenLetScope, 0, 0, nullptr}); + // Values pop in REVERSE push order (LIFO) -- iterate the + // assignment list backwards so each Op::StoreLetVar is + // paired with the value that was actually pushed for IT. + for (auto it = n.assignments.rbegin(); it != n.assignments.rend(); ++it) { + out.push_back({Op::StoreLetVar, internName((*it)->name->name), 0, &(*it)->position()}); + } + compileStatementList(n.children, out); + out.push_back({Op::CloseExprScope, 0, 0, nullptr}); + return; + } case NodeKind::ModularCall: { auto& call = static_cast(stmt); const oscad::Scope* lookupScope = stmt.scope() ? stmt.scope() : scope_; @@ -1213,9 +1389,10 @@ class Compiler { return; } default: - // echo/assert/let-block/modifiers/intersection_for -- - // deliberately native, see this section's own header - // comment above. + // modifiers/intersection_for -- deliberately native (see + // this section's own header comment above); echo/assert/ + // assignment/let-block now have their own real bytecode + // cases, above. out.push_back({Op::NativeStatement, internNativeStatement(&stmt), 0, &stmt.position()}); return; } @@ -1405,11 +1582,16 @@ std::optional tryCompileModuleBody(const oscad::ModuleDeclaration CompiledChunk chunk; chunk.isModule = true; chunk.selfDecl = &decl; - // No params/defaultCode/numSlots here -- a module's own parameters are - // bound natively (Evaluator::buildModuleChildCtx, called by the - // CALLER before this chunk's own body ever runs), never slot- - // addressed the way a function's are. See this file's own module- - // chunk doc comment (bytecode.hpp) for the full reasoning. + // No params/defaultCode here -- a module's own parameters are bound + // natively (Evaluator::buildModuleChildCtx, called by the CALLER + // before this chunk's own body ever runs), never slot-addressed the + // way a function's are. numSlots CAN still be nonzero though: a + // nested let-EXPRESSION inside a compiled echo/assert/assignment + // argument (Assignment/ModularEcho/ModularAssert/ModularLet's own + // compileOneStatement cases) reuses LetOp's existing slot-allocating + // compile path, just scoped to that one statement's own sub- + // expression -- see this file's own module-chunk doc comment + // (bytecode.hpp) for the full reasoning. Compiler compiler(chunk, decl.scope(), nullptr, {}); try { compiler.compileStatementList(decl.children, chunk.bodyCode); @@ -1417,6 +1599,7 @@ std::optional tryCompileModuleBody(const oscad::ModuleDeclaration return std::nullopt; } chunk.numIterLists = compiler.nextIterList(); + chunk.numSlots = compiler.nextSlot(); return chunk; } @@ -1439,6 +1622,7 @@ std::optional tryCompileChildrenList(const std::vectorchunk = &chunk; frame->code = &chunk.bodyCode; frame->pc = 0; - frame->slots.clear(); + // See runCompiledModuleBody's own doc comment (below) for why this is + // .assign(numSlots) now, not .clear() -- a nested let-expression + // inside a compiled echo/assert/assignment argument can need local + // slots even though module parameters themselves never do. + frame->slots.assign(static_cast(chunk.numSlots), Value{}); frame->stack.clear(); frame->bound.clear(); frame->accumStack.clear(); @@ -878,14 +882,101 @@ Value driveVm(Evaluator& ev, size_t floor) { case Op::NativeStatement: { const oscad::ASTNode* stmt = f.chunk->nativeStatements[static_cast(ins.a)]; EvalContext childCtx = ctx.withScope(stmt->scope() ? stmt->scope() : ctx.scope); - // Mirrors evalChildren's own per-statement loop exactly, - // including its ModularLet exclusion (evalLetBlock fires - // its own per-assignment checkDebug internally). - if (stmt->kind() != oscad::NodeKind::ModularLet) ev.checkDebug(*stmt, childCtx); + // Mirrors evalChildren's own per-statement loop exactly + // -- no ModularLet exclusion needed here (unlike that + // loop's own): ModularLet has its own compiled form now + // (Op::OpenLetScope/StoreLetVar) and never reaches + // Op::NativeStatement at all. + ev.checkDebug(*stmt, childCtx); ev.evalStatement(*stmt, childCtx); ++f.pc; break; } + case Op::OpenExprScope: { + f.ctxChain.push_back(ctx.letChildCtx()); + ++f.pc; + break; + } + case Op::CloseExprScope: { + f.ctxChain.pop_back(); + ++f.pc; + break; + } + case Op::OpenLetScope: { + f.ctxChain.push_back(ctx.childCtx(nullptr, std::nullopt, ctx.childrenNodes, ctx.childrenCallerCtx)); + ++f.pc; + break; + } + case Op::StoreLetVar: { + Value v = std::move(f.stack.back()); + f.stack.pop_back(); + const std::string& name = f.chunk->names[static_cast(ins.a)]; + if (!name.empty() && name[0] == '$') { + ctx.dyn->set(name, std::move(v)); + ctx.dynExplicit->set(name, true); + } else { + ctx.let_->set(name, std::move(v)); + } + ++f.pc; + break; + } + case Op::CheckDebugStatement: { + const oscad::ASTNode* stmt = f.chunk->nativeStatements[static_cast(ins.a)]; + ev.checkDebug(*stmt, ctx); + ++f.pc; + break; + } + case Op::StoreModuleVar: { + Value v = std::move(f.stack.back()); + f.stack.pop_back(); + const std::string& name = f.chunk->names[static_cast(ins.a)]; + // Mirrors Evaluator::evalAssignment exactly (stmt_eval.cpp) -- + // see StoreModuleVar's own doc comment (bytecode.hpp). + if (!name.empty() && name[0] == '$') { + ctx.dyn->set(name, std::move(v)); + ctx.dynExplicit->set(name, true); + } else { + if (const oscad::Position* const* firstPosEntry = ctx.dynPositions->find(name)) { + const oscad::Position* firstPos = *firstPosEntry; + const int firstLine = firstPos ? firstPos->line : 0; + ev.warn(name + " was assigned on line " + std::to_string(firstLine) + " but was overwritten", + ins.pos); + } + ctx.let_->set(name, std::move(v)); + ctx.dynPositions->set(name, ins.pos); + } + ++f.pc; + break; + } + case Op::AssertStatement: { + const CompiledChunk::AssertSite& site = f.chunk->assertSites[static_cast(ins.a)]; + std::vector args(static_cast(site.argCount)); + for (int i = site.argCount - 1; i >= 0; --i) { + args[static_cast(i)] = std::move(f.stack.back()); + f.stack.pop_back(); + } + const Value condVal = site.conditionArgIndex ? std::move(args[static_cast(*site.conditionArgIndex)]) + : Value{true}; + if (!truthy(condVal)) { + std::string err = "Assertion '" + + std::get(f.chunk->constants[static_cast(site.condTextConstIdx)]) + + "' failed"; + if (site.messageArgIndex) { + const Value& msgArg = args[static_cast(*site.messageArgIndex)]; + if (!std::holds_alternative(msgArg)) { + const std::string* s = std::get_if(&msgArg); + err += ": \"" + (s ? *s : fmtValue(msgArg)) + "\""; + } + } + ev.error(err, *site.node, "assert"); + } + // Rare (assert(...) translate(...) children();-shaped) -- + // not worth its own compiled path; evalChildren already + // tries ITS OWN compiled fast path internally regardless. + if (!site.node->children.empty()) ev.evalChildren(site.node->children, ctx); + ++f.pc; + break; + } case Op::NativeCondJumpIfFalse: { const oscad::Expression* cond = f.chunk->nativeExprs[static_cast(ins.a)]; const bool truthyVal = truthy(ev.evalExprMaybeCompiled(*cond, ctx)); @@ -1038,7 +1129,15 @@ void runCompiledModuleBody(Evaluator& ev, const CompiledChunk& chunk, EvalContex frame->chunk = &chunk; frame->code = &chunk.bodyCode; frame->pc = 0; - frame->slots.clear(); + // .assign, not .clear() -- a module chunk's own bodyCode has no + // PARAMETER slots (module params are always bound natively, see this + // function's own doc comment above), but CAN still declare LOCAL slots + // via a nested let-EXPRESSION inside a compiled echo/assert/assignment + // argument (Op::LetOp's own compile case, reused as-is for these -- + // see compileOneStatement's Assignment/ModularEcho/ModularAssert/ + // ModularLet cases). chunk.numSlots is 0 for the (still-common) case + // where nothing inside this body ever does that. + frame->slots.assign(static_cast(chunk.numSlots), Value{}); frame->stack.clear(); frame->bound.clear(); frame->accumStack.clear(); diff --git a/src/csg_resolve.cpp b/src/csg_resolve.cpp index b462625..035c301 100644 --- a/src/csg_resolve.cpp +++ b/src/csg_resolve.cpp @@ -175,17 +175,21 @@ void Evaluator::evalModifier(const oscad::ModuleInstantiation& child, const osca // always empty (role tagging happens entirely in the matching // generate function -- generateHighlight/Background/ShowOnly). buildTreeNode(kind, wrapperNode, [&]() { - // The wrapped child gets its own statement-level stop, in addition - // to the modifier node's (which evalChildren already fired before - // dispatching here) -- so `#cube(1);` pauses twice, like the - // reference. There, _resolve_modifier_child has no _check_debug of - // its own: it calls _eval_statement(node.child, ctx), and _that_ - // dispatcher independently checks every _TREE_NODE_TYPES node it - // is handed. This port's evalStatement has no such blanket check - // (evalChildren owns it), so the equivalent stop is made explicit - // here. - checkDebug(child, ctx); - evalStatement(child, ctx); + // A single-element evalChildren() call, not a hand-rolled + // checkDebug+evalStatement pair -- gives the wrapped child its own + // statement-level stop exactly like the old explicit checkDebug() + // did (evalChildren's own per-statement loop fires the identical + // check, at the identical point, for a lone non-declaration + // non-ModularLet node), while ALSO trying the compiled + // Op::CallModule path first (tryRunCompiledChildren, the + // NativeStatement-gap fix) -- so `#recur(n-1);` gets the same + // depth-safe treatment translate()/union()/let()/intersection_for + // already do, instead of always falling through evalStatement -> + // evalModularCall -> evalUserModule capped at kMaxUserCallDepth=30. + // No double stop: whichever path runs (compiled Op::CallModule, + // compiled Op::NativeStatement, or the native loop) fires the + // check exactly once, same as evalChildren's own existing callers. + evalChildren(std::vector{&child}, ctx); return CSGParams{}; }); } diff --git a/src/user_calls.cpp b/src/user_calls.cpp index 5a67b05..d6f4dbf 100644 --- a/src/user_calls.cpp +++ b/src/user_calls.cpp @@ -143,6 +143,28 @@ void Evaluator::releaseVmFrame(std::unique_ptr frame) { frame->stack.clear(); frame->slots.clear(); frame->bound.clear(); + // A REUSED (pooled) frame does NOT get VmFrame's own default member + // initializers re-applied -- those only fire for a genuinely fresh + // std::make_unique() (acquireVmFrame's own empty-pool + // fallback). Only pushBracketedModuleFrame ever sets ownsModuleSplice + // true (and moduleRandsBefore/moduleSpliceCallNode alongside it); no + // OTHER push site (pushBareFrame, pushBracketedCallFrame, + // runCompiledFunction(FromBound), a parameter default's own frame) + // ever resets it back to false. Without this, a frame released here + // after owning a module's own splice, then reacquired later for an + // ordinary FUNCTION call, keeps ownsModuleSplice=true -- if THAT + // function call is later torn down by an exception (teardownVmCall + // StackDownTo), its own `if (frame->ownsModuleSplice) treeStack_. + // pop_back();` fires for a frame that never pushed a treeStack_ entry + // of its own, silently popping one too many and corrupting treeStack_ + // for everything after. Caught for real via a heap-buffer-overflow + // (ASan) inside teardownVmCallStackDownTo's own treeStack_.pop_back(), + // triggered by BOSL2's poly_roots() -- a non-tail self-recursive + // function, reusing a frame previously released by an earlier, + // unrelated MODULE call -- throwing partway through. + frame->ownsModuleSplice = false; + frame->moduleRandsBefore = 0; + frame->moduleSpliceCallNode = nullptr; vmFramePool_.push_back(std::move(frame)); } diff --git a/tests/test_bytecode_compiler.cpp b/tests/test_bytecode_compiler.cpp index af19d25..d2bcb2f 100644 --- a/tests/test_bytecode_compiler.cpp +++ b/tests/test_bytecode_compiler.cpp @@ -1351,6 +1351,109 @@ TEST(ModuleBodyCompiles, DeepPureVmRecursionWithNonRecursingBuiltinWrappedLeafSu ASSERT_EQ(e.bodies.size(), 101u); } +// evalModifier used to dispatch its single wrapped child via a hand-rolled +// checkDebug()+evalStatement() pair, bypassing evalChildren entirely -- so +// a `#`/`!`-wrapped recursive module call never got the NativeStatement-gap +// fix's Op::CallModule treatment, unlike an otherwise-identical `translate`- +// wrapped call. Same shape as +// DeepPureVmRecursionWithNonRecursingBuiltinWrappedLeafSucceeds, just with +// `#` instead of `translate` wrapping the non-recursing leaf, proving the +// fix now applies here too. +TEST(ModuleBodyCompiles, DeepPureVmRecursionWithNonRecursingModifierWrappedLeafSucceeds) { + ScopedVm vm(true); + Evaluated e = evalSrc("module recur(n) {" + " if (n > 0) {" + " #translate([0,0,n]) cube(1);" + " recur(n - 1);" + " } else {" + " cube(1);" + " }" + "}" + "recur(100);"); + ASSERT_EQ(e.bodies.size(), 101u); +} + +// -- Leaf-statement compilation (Assignment/ModularEcho/ModularAssert/ ----- +// -- ModularLet get real bytecode instead of Op::NativeStatement) --------- + +TEST(ModuleBodyCompiles, AssignmentStatementCompilesWithDollarVarAndOverwrittenWarning) { + ScopedVm vm(true); + std::string captured = runCapturingEcho("$fn = 6;\n" + "function f() = $fn;\n" + "echo(f());\n" + "x = 1;\n" + "x = 2;\n" // overwritten warning + "echo(x);\n"); + EXPECT_NE(captured.find("ECHO: 6"), std::string::npos); + EXPECT_NE(captured.find("was assigned on line"), std::string::npos); + EXPECT_NE(captured.find("but was overwritten"), std::string::npos); + EXPECT_NE(captured.find("ECHO: 2"), std::string::npos); +} + +TEST(ModuleBodyCompiles, EchoStatementCompilesWithNamedAndPositionalArgs) { + ScopedVm vm(true); + EXPECT_EQ(runCapturingEcho("echo(1, b=2, 3);"), "ECHO: 1, b = 2, 3"); +} + +TEST(ModuleBodyCompiles, AssertStatementCompilesWithNamedArgsMessageAndEagerSideEffects) { + ScopedVm vm(true); + // Named args in reverse order (message before condition), PLUS a 3rd, + // logically-unused positional argument -- must still be evaluated + // eagerly for its own side effect (mirrors evalAssertStatement's own + // resolveArgs() call, unlike AssertOp's lazy message). + std::string captured = + runCapturingEcho("assert(message=\"unused, condition true\", condition=true, echo(\"side effect\"));\n" + "echo(\"reached\");\n"); + EXPECT_NE(captured.find("ECHO: \"side effect\""), std::string::npos); + EXPECT_NE(captured.find("ECHO: \"reached\""), std::string::npos); +} + +TEST(ModuleBodyCompiles, AssertStatementCompiledFormFailsWithNamedMessage) { + ScopedVm vm(true); + try { + evalSrc("assert(condition=false, message=\"custom\");"); + FAIL() << "expected EvalError"; + } catch (const EvalError& e) { + const std::string what = e.what(); + EXPECT_NE(what.find("Assertion 'false' failed"), std::string::npos) << what; + EXPECT_NE(what.find("\"custom\""), std::string::npos) << what; + } +} + +TEST(ModuleBodyCompiles, LetBlockStatementDoesNotSeeItsOwnEarlierSiblingAssignment) { + ScopedVm vm(true); + // The statement form's own documented divergence from let-EXPRESSION + // sequential visibility: b's RHS must see the OUTER a (1), not this + // same let-block's own a=2 -- b should be 2, not 3. + EXPECT_EQ(runCapturingEcho("a = 1;\n" + "let (a = 2, b = a + 1) { echo(a, b); }\n" + "echo(a);\n"), + "ECHO: 2, 2\nECHO: 1"); +} + +TEST(ModuleBodyCompiles, LetBlockStatementIsolatesDollarVarOverrideFromLaterStatements) { + ScopedVm vm(true); + EXPECT_EQ(runCapturingEcho("function f() = $fn;\n" + "$fn = 10;\n" + "let ($fn = 99) { echo(f()); }\n" + "echo(f());\n"), + "ECHO: 99\nECHO: 10"); +} + +TEST(ModuleBodyCompiles, NestedLetExpressionInsideCompiledStatementArgumentAllocatesSlotsCorrectly) { + // Exercises the numSlots/frame->slots fix -- a compiled module-body + // statement's own inline argument can itself contain a LetOp + // EXPRESSION (as opposed to the ModularLet STATEMENT form, above), + // which DOES use real local slots (unlike anything else in a module + // chunk) -- without frame->slots sized to chunk.numSlots, this + // indexes out of bounds. + ScopedVm vm(true); + EXPECT_EQ(runCapturingEcho("echo(let(x = 5, y = x + 1) x + y);\n" + "v = let(q = 3) q * q;\n" + "echo(v);\n"), + "ECHO: 11\nECHO: 9"); +} + TEST(ModuleBodyCompiles, RecursiveModuleThrowingPartwayUnwindsCleanlyAndVmFrameRunsAgain) { ScopedVm vm(true); Evaluator ev;