From bb5bdeab964bf4c7c009e2f35b5c25a56d05acd6 Mon Sep 17 00:00:00 2001 From: Revar Desmera Date: Sat, 1 Aug 2026 06:35:44 -0700 Subject: [PATCH] Add Op::PushCsgWrap: eliminate union/difference/intersection's native reentry A recursive call wrapped in union()/difference()/intersection() still fell to Op::NativeStatement, costing one real native C++ frame per level and capping recursion at kMaxDriveVmNativeDepth=40 -- the same Windows-crash risk Op::PushBuiltinWrap already closed for translate/rotate/scale/mirror/ multmatrix/resize/color/#/%/! and Op::CallChildren closed for children(). These three were left out because resolveCsg needs per-top-level-child "group_sizes" bookkeeping that a single flat bracket can't represent. Op::PushCsgWrap/CsgGroupStart/CsgGroupEnd/PopCsgWrap replicate resolveCsg's own two-pass shape (every assignment evaluated first, then one group per geometry statement) as inline compiled bytecode, so a wrapped recursive call now compiles straight to Op::CallModule -- zero native stack cost, bounded only by the heap-sized kMaxVmCallStackDepth. --- include/openscad_cpp_evaluator/bytecode.hpp | 137 ++++++++++++++++-- .../openscad_cpp_evaluator/bytecode_vm.hpp | 27 ++++ pyproject.toml | 2 +- src/bytecode_compiler.cpp | 46 ++++++ src/bytecode_vm.cpp | 69 +++++++++ tests/test_bytecode_compiler.cpp | 117 +++++++++++++-- 6 files changed, 374 insertions(+), 24 deletions(-) diff --git a/include/openscad_cpp_evaluator/bytecode.hpp b/include/openscad_cpp_evaluator/bytecode.hpp index a36b293..c8205ba 100644 --- a/include/openscad_cpp_evaluator/bytecode.hpp +++ b/include/openscad_cpp_evaluator/bytecode.hpp @@ -325,6 +325,101 @@ enum class Op { // same seam, now a counter). PopBuiltinWrap, + // -- CSG-wrap compilation (closes the LAST native-reentry source in ---- + // -- the original NativeStatement gap: union()/difference()/ - + // -- intersection()) ---------------------------------------------------- + // union()/difference()/intersection() weren't covered by the original + // Op::PushBuiltinWrap (see that op's own doc comment) because they need + // bespoke bookkeeping PushBuiltinWrap's single all-children bracket + // doesn't do: Evaluator's own resolveCsg (booleans.cpp) evaluates each + // TOP-LEVEL child statement of the block SEPARATELY and records how + // many CSGNodes it individually contributed ("group_sizes") -- e.g. + // `difference(){ A; B; C; }` = A - (B u C), preserving A's own group + // even when A itself expands to more than one body (an attachable()- + // style call returning parent+children as one operand). A flat + // all-children bracket the way Transform/Color/Modifier use would lose + // that grouping entirely. This op pair (PushCsgWrap/PopCsgWrap) plus + // Op::CsgGroupStart/CsgGroupEnd (below) replicate resolveCsg's exact + // two-pass shape (all assignments first, THEN one evalChildren-per- + // geometry-statement) as inline compiled bytecode instead, exactly as + // PushBuiltinWrap already did for translate/rotate/scale/mirror/ + // multmatrix/resize/color/#/%/! -- same rationale (a recursive + // union()/difference()/intersection()-wrapped module chain was still a + // real Windows native-reentry depth risk, just never independently + // fixed when PushBuiltinWrap's own set was). + // + // a = index into CompiledChunk::csgWrapSites. Runtime handler: captures + // ev.randsCallCount() BEFORE argument resolution (same rands-in-args + // taint reasoning as PushBuiltinWrap), resolves the (rare, $-only) + // arguments via resolveCallArgs exactly like resolveCsg itself does + // (discarding the positional/named result -- union/difference/ + // intersection take no real parameters -- keeping only the possibly- + // $-scoped child ctx), pushes that ctx onto f.ctxChain unconditionally + // (mirrors Transform/Color's own unconditional push -- a bare + // `union() {...}` with no `$fn=...` override still pushes a ctx that's + // merely a copy, cheap and uniform rather than a special-cased branch), + // pushes a fresh ev.treeStack_ frame (every child statement's own + // CSGNode(s), across every group, land flat in this ONE frame -- + // mirrors Evaluator::buildTreeNode/evalModularCall's own single + // treeStack_.emplace_back() around the whole call, not one per group), + // and stashes {op, randsBefore, siteIdx, empty groupSizes} onto + // VmFrame::csgWrapStack (a real per-frame LIFO, same reasoning as + // builtinWrapStack: nested/sequenced CSG wraps within one frame's own + // instruction stream, e.g. `union() { difference() {...} }`). The + // compiler always emits a plain Op::CheckDebugStatement immediately + // before this (see emitCsgWrap, bytecode_compiler.cpp), mirroring + // emitBuiltinWrap's own pattern -- this is a genuine statement doing + // real work here, not a call transferring control to a declaration. + PushCsgWrap, + + // Opens one "group" within an already-open Op::PushCsgWrap bracket -- + // emitted immediately before each top-level GEOMETRY child statement's + // own inline-compiled bytecode (compileStatementList of exactly that + // one statement). Records ev.treeStack_.back().size() into + // csgWrapStack.back().groupStartSize -- always operates on the + // TOPMOST (innermost still-open) csgWrapStack entry, matching + // PopBuiltinWrap's own back()-is-always-mine LIFO discipline, so no + // operand is needed. Assignments among the block's children are + // compiled separately, BEFORE any CsgGroupStart/End pair at all (see + // emitCsgWrap) -- mirrors resolveCsg's own two-pass split exactly + // (Evaluator::evalChildren(assignNodes, effCtx) always runs to + // completion before the per-geoNode loop starts). + CsgGroupStart, + + // Closes the matching Op::CsgGroupStart: computes + // ev.treeStack_.back().size() - csgWrapStack.back().groupStartSize + // (how many CSGNodes THIS one top-level statement just contributed, + // however many that turned out to be -- 0 for a statement whose own + // evaluation spliced nothing, e.g. a no-op unknown-module warning; >1 + // for an attachable()-style multi-body operand) and appends it, as a + // Value, onto csgWrapStack.back().groupSizes -- exactly one entry per + // top-level geometry statement, in source order, mirroring resolveCsg's + // own `groupSizes.push_back(Value{...})` loop. + CsgGroupEnd, + + // Closes the matching Op::PushCsgWrap bracket: pops + // VmFrame::csgWrapStack's own top entry FIRST (before anything that can + // itself throw, e.g. setTreeDepthOrThrow below -- same "exception- + // teardown's own pending count must already be right" reasoning as + // PopBuiltinWrap), pops the ctx PushCsgWrap unconditionally pushed, + // pops ev.treeStack_ to retrieve every group's own CSGNode(s) (flat, + // exactly like resolveCsg's own `children` result -- group boundaries + // live only in group_sizes, never in the CSGNode list's own shape), and + // builds the tagged CSGNode exactly like Evaluator::buildTreeNode's own + // post-resolveBody() half does, with params = {"op": site.op, + // "group_sizes": ValueList(pending.groupSizes)} -- byte-for-byte what + // native resolveCsg returns -- pushing the result onto the new top of + // treeStack_. a = index into CompiledChunk::csgWrapSites (same site + // Push used). + // + // teardownVmCallStackDownTo's own exception path (bytecode_vm.cpp) pops + // frame->csgWrapStack.size() additional treeStack_ entries per + // torn-down frame, alongside its existing builtinWrapStack/ + // ownsModuleSplice accounting -- same reasoning as PopBuiltinWrap's own + // doc comment: an exception can leave N of these brackets open in one + // frame. + PopCsgWrap, + // A `children()` / `children(N)` statement -- the runtime-varying // sibling of Op::CallModule/Op::PushBuiltinWrap, closing the LAST // dominant native-reentry source (BOSL2's attachable() calls @@ -367,10 +462,11 @@ enum class Op { // no separate site table needed). CallChildren, - // A single "native passthrough" statement -- intersection_for, - // union/difference/intersection and every other builtin - // module call NOT covered by Op::PushBuiltinWrap (see that op's own - // doc comment for exactly which builtins ARE covered), the `*` + // A single "native passthrough" statement -- intersection_for, every + // OTHER builtin module call not covered by Op::PushBuiltinWrap/ + // Op::PushCsgWrap (see those ops' own doc comments for exactly which + // builtins ARE covered -- cube/sphere/hull/linear_extrude/etc., the + // ones that never wrap a recursive call in idiomatic OpenSCAD), the `*` // modifier's own no-op case, 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 @@ -387,18 +483,21 @@ enum class Op { // crash risk in practice, not just a missed optimization. See // Op::PushBuiltinWrap's own doc comment for the real story and the // fix. children() fell here too, and was the LAST and largest such - // reentry source once PushBuiltinWrap's own set was covered -- it - // now has Op::CallChildren, above.) + // reentry source once PushBuiltinWrap's own set was covered -- it now + // has Op::CallChildren, above. union()/difference()/intersection() + // fell here too, and were the last remaining REAL native-reentry risk + // (bespoke group_sizes bookkeeping meant they couldn't just reuse + // PushBuiltinWrap's own bracket) -- they now have Op::PushCsgWrap, + // above.) // 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, // evalStatement. These are still "leaf-shaped" for what's left here - // after Op::PushBuiltinWrap peeled off the proven-risky subset: real - // recursion safety for a recursive module chain is covered by - // CallModule/PushBuiltinWrap/ForIterNext/the Jump-based if/for control - // flow, not by how many of THESE sit alongside them in the same body - // -- true again now that the one construct that violated it - // (translate()-wrapped recursion) has its own real bytecode instead. + // now that Op::PushBuiltinWrap/Op::PushCsgWrap have peeled off every + // proven-risky construct: real recursion safety for a recursive module + // chain is covered by CallModule/PushBuiltinWrap/PushCsgWrap/ + // ForIterNext/the Jump-based if/for control flow, not by how many of + // THESE sit alongside them in the same body. NativeStatement, // If/if-else's own condition, evaluated NATIVELY (Evaluator:: @@ -734,6 +833,19 @@ struct CompiledChunk { const oscad::ASTNode* node = nullptr; }; + // One Op::PushCsgWrap/PopCsgWrap site pair -- see those ops' own doc + // comments for the full "why union/difference/intersection need + // bespoke group_sizes bookkeeping instead of just reusing + // BuiltinWrapSite" rationale. `op` is always "union"/"difference"/ + // "intersection" (the only 3 names resolveDispatch() maps to + // resolveCsg) -- always a genuine ModularCall (unlike BuiltinWrapSite's + // Modifier kind, this construct has no non-ModularCall variant), so + // `node` is typed precisely rather than a generic ASTNode*. + struct CsgWrapSite { + std::string op; + const oscad::ModularCall* node = nullptr; + }; + // 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 @@ -796,6 +908,7 @@ struct CompiledChunk { // own doc comments, above) -- always empty for a function chunk. std::vector moduleCallSites; std::vector builtinWrapSites; + std::vector csgWrapSites; std::vector assertSites; std::vector nativeExprs; std::vector nativeStatements; diff --git a/include/openscad_cpp_evaluator/bytecode_vm.hpp b/include/openscad_cpp_evaluator/bytecode_vm.hpp index 47942a9..1ac73ae 100644 --- a/include/openscad_cpp_evaluator/bytecode_vm.hpp +++ b/include/openscad_cpp_evaluator/bytecode_vm.hpp @@ -30,6 +30,24 @@ struct PendingBuiltinWrap { int siteIdx = -1; }; +// One still-open Op::PushCsgWrap bracket's own state -- see that op's own +// doc comment (bytecode.hpp) for the full contract. `groupSizes` is built +// up incrementally, one entry per Op::CsgGroupStart/CsgGroupEnd pair (one +// per top-level GEOMETRY child statement); `groupStartSize` is scratch +// space for the CURRENTLY OPEN group only (set by CsgGroupStart, consumed +// by the matching CsgGroupEnd) -- safe as a single scalar, not a stack of +// its own, because groups within one CSG wrap are siblings in sequence, +// never nested (unlike PushCsgWrap brackets themselves, which CAN nest, +// e.g. `union() { difference() {...} }` -- that's what makes +// VmFrame::csgWrapStack itself a real LIFO, below). +struct PendingCsgWrap { + std::string op; + std::uint64_t randsBefore = 0; + int siteIdx = -1; + std::vector groupSizes; + size_t groupStartSize = 0; +}; + // One ListCompFor/statement-for assignment's own materialized iteration // state -- see Op::IterMaterialize/IterReset/IterNext's own doc comments // (bytecode.hpp). Lives in the header (not bytecode_vm.cpp's own anonymous @@ -127,6 +145,15 @@ struct VmFrame { // existing invariant ("whoever pops this frame drains its own open // brackets first") already covers it. std::vector builtinWrapStack; + // Op::PushCsgWrap's own per-frame LIFO -- same role/lifetime/teardown + // discipline as builtinWrapStack, just for union()/difference()/ + // intersection() (see PendingCsgWrap's own doc comment, above, and + // Op::PushCsgWrap's, bytecode.hpp). Not explicitly cleared in + // releaseVmFrame, same as builtinWrapStack/accumStack/ctxChain -- the + // existing invariant ("whoever pops this frame drains its own open + // brackets first", normally via matched Push/Pop, or via + // teardownVmCallStackDownTo on the exception path) already covers it. + std::vector csgWrapStack; // The ORIGINAL callee name at push time, used by // Evaluator::exitUserCallSuccess's own returnHook call when this frame // carries a bracket -- deliberately NOT updated by a later tail hop diff --git a/pyproject.toml b/pyproject.toml index 91aac63..3d5066b 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "scikit_build_core.build" [project] name = "openscad_cpp_evaluator" -version = "0.13.4" +version = "0.13.5" 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 a82a338..9dc4277 100644 --- a/src/bytecode_compiler.cpp +++ b/src/bytecode_compiler.cpp @@ -1202,6 +1202,45 @@ class Compiler { out.push_back({Op::PopBuiltinWrap, idx, 0, &wrapperNode.position()}); } + // union()/difference()/intersection() -- see Op::PushCsgWrap's own doc + // comment (bytecode.hpp) for why these can't just reuse emitBuiltinWrap: + // resolveCsg (booleans.cpp) needs per-top-level-child-statement + // "group_sizes" bookkeeping, replicated here as one Op::CsgGroupStart/ + // CsgGroupEnd pair per GEOMETRY child, with every ASSIGNMENT child + // compiled first, unconditionally, regardless of interleaving in + // source -- mirrors resolveCsg's own two-pass split (`assignNodes` + // fully evaluated, THEN one evalChildren call per `geoNodes` entry) + // exactly, including its ModuleDeclaration/FunctionDeclaration + // exclusion (a nested declaration inside a CSG block contributes to + // neither pass -- already hoisted into scope, nothing to run here). + void emitCsgWrap(const oscad::ModularCall& call, std::vector& out) { + out.push_back({Op::CheckDebugStatement, internNativeStatement(&call), 0, nullptr}); + CompiledChunk::CsgWrapSite site; + site.op = call.name->name; + site.node = &call; + chunk_.csgWrapSites.push_back(std::move(site)); + const int idx = static_cast(chunk_.csgWrapSites.size()) - 1; + out.push_back({Op::PushCsgWrap, idx, 0, &call.position()}); + + std::vector assignNodes; + std::vector geoNodes; + for (const auto& c : call.children) { + if (c->kind() == oscad::NodeKind::Assignment) { + assignNodes.push_back(c.get()); + } else if (c->kind() != oscad::NodeKind::ModuleDeclaration && + c->kind() != oscad::NodeKind::FunctionDeclaration) { + geoNodes.push_back(c.get()); + } + } + compileStatementList(assignNodes, out); + for (const oscad::ASTNode* geoNode : geoNodes) { + out.push_back({Op::CsgGroupStart, 0, 0, nullptr}); + compileStatementList(std::vector{geoNode}, out); + out.push_back({Op::CsgGroupEnd, 0, 0, nullptr}); + } + out.push_back({Op::PopCsgWrap, idx, 0, &call.position()}); + } + void compileOneStatement(const oscad::ASTNode& stmt, std::vector& out) { using oscad::NodeKind; trackSpan(stmt); @@ -1434,6 +1473,13 @@ class Compiler { out.push_back({Op::CallChildren, internNativeStatement(&stmt), 0, &stmt.position()}); return; } + // union()/difference()/intersection() -- see emitCsgWrap's + // own doc comment for why these need their own bespoke + // bracket rather than emitBuiltinWrap's. + if (dispatchIt != dispatch.end() && dispatchIt->second == &resolveCsg) { + emitCsgWrap(call, out); + return; + } // Every other builtin, or a name that didn't resolve to a // user module statically -- native passthrough, exactly // like echo/assert/etc. below. Never the recursion-depth diff --git a/src/bytecode_vm.cpp b/src/bytecode_vm.cpp index 9edeeaf..8567ea8 100644 --- a/src/bytecode_vm.cpp +++ b/src/bytecode_vm.cpp @@ -401,6 +401,11 @@ void teardownVmCallStackDownTo(Evaluator& ev, size_t floor) { // at once; at most one module-call splice ever can). for (size_t i = 0; i < frame->builtinWrapStack.size(); ++i) ev.treeStack_.pop_back(); frame->builtinWrapStack.clear(); + // Same reasoning, same LIFO-order requirement, for any still-open + // Op::PushCsgWrap bracket(s) -- see PendingCsgWrap's/Op::PushCsgWrap's + // own doc comments. + for (size_t i = 0; i < frame->csgWrapStack.size(); ++i) ev.treeStack_.pop_back(); + frame->csgWrapStack.clear(); // A module frame's own CALLER (Op::CallModule, mirroring // evalModularCall/buildTreeNode) always pushes a treeStack_ // accumulator for it before pushing this frame -- normally popped @@ -1108,6 +1113,70 @@ Value driveVm(Evaluator& ev, size_t floor) { ++f.pc; break; } + case Op::PushCsgWrap: { + const CompiledChunk::CsgWrapSite& site = f.chunk->csgWrapSites[static_cast(ins.a)]; + // Captured BEFORE argument resolution -- same + // rands-in-args taint reasoning as Op::PushBuiltinWrap. + const std::uint64_t randsBefore = ev.randsCallCount(); + // union/difference/intersection take no positional + // arguments in real OpenSCAD -- `args` is discarded, + // exactly mirroring resolveCsg's own `(void)args;` + // (booleans.cpp). Only `effCtx` (a possibly-$-scoped + // child ctx, e.g. `difference($fn=8) {...}`) matters. + auto [args, effCtx] = resolveCallArgs(ev, site.node->arguments, ctx); + (void)args; + f.ctxChain.push_back(std::move(effCtx)); + ev.treeStack_.emplace_back(); + f.csgWrapStack.push_back({site.op, randsBefore, ins.a, {}, 0}); + ++f.pc; + break; + } + case Op::CsgGroupStart: { + f.csgWrapStack.back().groupStartSize = ev.treeStack_.back().size(); + ++f.pc; + break; + } + case Op::CsgGroupEnd: { + PendingCsgWrap& pending = f.csgWrapStack.back(); + const size_t after = ev.treeStack_.back().size(); + pending.groupSizes.push_back(Value{static_cast(after - pending.groupStartSize)}); + ++f.pc; + break; + } + case Op::PopCsgWrap: { + // Pop this bracket's own bookkeeping FIRST -- before + // anything below that can itself throw + // (setTreeDepthOrThrow) -- same ordering reasoning as + // Op::PopBuiltinWrap's own doc comment. + PendingCsgWrap pending = std::move(f.csgWrapStack.back()); + f.csgWrapStack.pop_back(); + const CompiledChunk::CsgWrapSite& site = + f.chunk->csgWrapSites[static_cast(pending.siteIdx)]; + f.ctxChain.pop_back(); + std::vector> children = std::move(ev.treeStack_.back()); + ev.treeStack_.pop_back(); + // Mirrors resolveCsg's own params exactly (booleans.cpp). + CSGParams params; + params["op"] = Value{pending.op}; + params["group_sizes"] = + Value{std::make_shared(ValueList{std::move(pending.groupSizes)})}; + // Mirrors Evaluator::buildTreeNode's own post- + // resolveBody() half exactly (csg_resolve.cpp). + const bool uncacheable = + (ev.randsCallCount() != pending.randsBefore) || + std::any_of(children.begin(), children.end(), [](const auto& c) { return c->uncacheable; }); + auto treeNode = std::make_unique(); + treeNode->kind = site.op; + treeNode->node = site.node; + treeNode->isBuiltin = true; + treeNode->children = std::move(children); + treeNode->params = std::move(params); + treeNode->uncacheable = uncacheable; + ev.setTreeDepthOrThrow(*treeNode, *site.node); + ev.treeStack_.back().push_back(std::move(treeNode)); + ++f.pc; + break; + } case Op::NativeStatement: { const oscad::ASTNode* stmt = f.chunk->nativeStatements[static_cast(ins.a)]; EvalContext childCtx = ctx.withScope(stmt->scope() ? stmt->scope() : ctx.scope); diff --git a/tests/test_bytecode_compiler.cpp b/tests/test_bytecode_compiler.cpp index fadc25d..bd9a508 100644 --- a/tests/test_bytecode_compiler.cpp +++ b/tests/test_bytecode_compiler.cpp @@ -1397,16 +1397,35 @@ TEST(ModuleBodyCompiles, RecursiveModifierWrappedCallSucceedsWellPastTheOldNativ ASSERT_EQ(e.bodies.size(), 1u); } -// union()/difference()/intersection() are deliberately NOT covered by -// Op::PushBuiltinWrap (bespoke per-statement grouping/group_sizes logic, -// genuinely different from the transform/color/modifier shape -- see that -// op's own doc comment, bytecode.hpp) -- a recursive call wrapped in one -// still falls to Op::NativeStatement and still costs one real native -// frame per level, exactly like every covered construct used to. Proves -// the guard itself (driveVmNativeDepth_/kMaxDriveVmNativeDepth) is still -// live and still catches this, not accidentally disabled by -// Op::PushBuiltinWrap's own changes to the shared NativeStatement path. -TEST(ModuleBodyCompiles, UnionWrappedRecursionStillHitsTheNativeReentryGuardControlledError) { +// union()/difference()/intersection() now have their own real bytecode +// (Op::PushCsgWrap/CsgGroupStart/CsgGroupEnd/PopCsgWrap) instead of falling +// to Op::NativeStatement -- see that op's own doc comment (bytecode.hpp) +// for why they needed a bespoke bracket rather than just reusing +// Op::PushBuiltinWrap's. A union()/difference()/intersection()-wrapped +// recursive call no longer costs any native reentry at all: `recur2(n);` +// (a resolved user-module call) compiles inline to Op::CallModule, a pure +// in-VM push, so the whole chain never touches driveVmNativeDepth_. +// Depth 1500 (same figure the PushBuiltinWrap/CallChildren tests above use) +// comfortably clears the old 40-level Windows-unsafe native-reentry ceiling +// while staying under kMaxCsgTreeDepth=2000 -- each `recur` level's own +// union() still contributes one real CSGNode to the tree (this fix +// eliminates native-stack cost, not CSG-tree depth, which is an orthogonal, +// unrelated cap on the RESULT shape, not the call chain). +TEST(ModuleBodyCompiles, UnionWrappedRecursionSucceedsWellPastTheOldNativeReentryLimit) { + ScopedVm vm(true); + Evaluated e = evalSrc("module recur(n) { union() { recur2(n); } }\n" + "module recur2(n) { if (n > 0) { recur(n - 1); } else { cube(1); } }\n" + "recur(1500);"); + ASSERT_EQ(e.bodies.size(), 1u); +} + +// The SAME chain pushed deep enough (3000 levels, one union() CSGNode per +// `recur` level) now hits the orthogonal, pre-existing CSG-TREE-depth guard +// (kMaxCsgTreeDepth=2000, csg_resolve.cpp) instead of a native-reentry +// guard -- proves the native-reentry elimination didn't silently trade one +// crash risk for an unguarded one; something still stops an unreasonably +// deep result, just a different (and correctly-named) error now. +TEST(ModuleBodyCompiles, UnionWrappedRecursionPastCsgTreeDepthLimitStillErrorsCleanly) { ScopedVm vm(true); try { evalSrc("module recur(n) { union() { recur2(n); } }\n" @@ -1415,10 +1434,86 @@ TEST(ModuleBodyCompiles, UnionWrappedRecursionStillHitsTheNativeReentryGuardCont FAIL() << "expected EvalError"; } catch (const EvalError& e) { const std::string what = e.what(); - EXPECT_NE(what.find("Recursion too deep"), std::string::npos) << what; + EXPECT_NE(what.find("Recursion too deep while building geometry"), std::string::npos) << what; } } +// difference()/intersection() get the same treatment -- one representative +// test each rather than the full union() coverage above, since all three +// share emitCsgWrap/Op::PushCsgWrap verbatim (only the runtime `op` string +// differs, consumed solely by generateCsg at generate time, never by the +// compiled path itself). +TEST(ModuleBodyCompiles, DifferenceWrappedRecursionSucceedsWellPastTheOldNativeReentryLimit) { + ScopedVm vm(true); + Evaluated e = evalSrc("module recur(n) { difference() { recur2(n); } }\n" + "module recur2(n) { if (n > 0) { recur(n - 1); } else { cube(1); } }\n" + "recur(1500);"); + ASSERT_EQ(e.bodies.size(), 1u); +} + +TEST(ModuleBodyCompiles, IntersectionWrappedRecursionSucceedsWellPastTheOldNativeReentryLimit) { + ScopedVm vm(true); + Evaluated e = evalSrc("module recur(n) { intersection() { recur2(n); } }\n" + "module recur2(n) { if (n > 0) { recur(n - 1); } else { cube(1); } }\n" + "recur(1500);"); + ASSERT_EQ(e.bodies.size(), 1u); +} + +// group_sizes/multi-body-operand grouping correctness under the COMPILED +// path (test_booleans.cpp's own Difference.MultipleBodiesInFirstStatement +// FormOnePositiveOperand/Union.MultipleBodiesInFirstStatementAllSurvive +// already cover this for the default ambient VM state; this pins it +// explicitly under ScopedVm(true) plus a MODULE body, not just a top-level +// script, since that's the shape emitCsgWrap actually compiles). +TEST(ModuleBodyCompiles, CsgWrapGroupingSurvivesInsideACompiledModuleBody) { + ScopedVm vm(true); + // First statement's own union() contributes TWO disjoint bodies (one + // group, size 2); the second statement subtracts a cube overlapping + // only the first of those two by 1. A flat (non-grouped) evaluation + // would instead treat the second cube as its own separate operand. + Evaluated e = evalSrc("module m() {" + " difference() {" + " union() { cube(2); translate([10,0,0]) cube(2); }" + " translate([1,0,0]) cube(2);" + " }" + "}" + "m();"); + ASSERT_EQ(e.bodies.size(), 1u); + // 16 - overlap(1*2*2=4) = 12, same expected volume as the top-level test. + EXPECT_NEAR(e.bodies[0].body->Volume(), 12.0, 1e-6); +} + +// $-named-argument propagation into a compiled CSG wrap's children -- +// union/difference/intersection take no positional args in real OpenSCAD, +// but `difference($fn=8) {...}` is legal and must still reach its children, +// exactly like resolveCsg's own resolveCallArgs call preserves natively +// (booleans.cpp). Verifies Op::PushCsgWrap's ctx push (unconditional, +// unlike PushBuiltinWrap's Transform/Color-only push) actually carries it. +TEST(ModuleBodyCompiles, DollarArgPropagatesIntoCompiledCsgWrapChildren) { + ScopedVm vm(true); + EXPECT_EQ(runCapturingEcho("module m() { union($fn = 9) { echo($fn); cube(1); } }\n" + "m();"), + "ECHO: 9"); +} + +// Assignment-before-geometry ordering: resolveCsg evaluates EVERY +// assignment child first, regardless of source interleaving with geometry +// statements, THEN each geometry statement in its own group -- emitCsgWrap +// mirrors this with a separate compile pass, not source order. A geometry +// statement referencing a LATER-in-source assignment must still see it. +TEST(ModuleBodyCompiles, CompiledCsgWrapEvaluatesAllAssignmentsBeforeAnyGeometryStatement) { + ScopedVm vm(true); + Evaluated e = evalSrc("module m() {" + " union() {" + " translate([sz, 0, 0]) cube(1);" // geometry statement first in SOURCE order + " sz = 5;" // assignment second in source order + " }" + "}" + "m();"); + ASSERT_EQ(e.bodies.size(), 1u); + EXPECT_NEAR(e.bodies[0].body->Volume(), 1.0, 1e-6); +} + // The BOSL2 attachable() shape this whole effort targets: a wrapper module // whose body is just `children();`, applied at every level of a recursive // chain. children() used to fall to Op::NativeStatement -- one genuine