diff --git a/include/openscad_cpp_evaluator/bytecode.hpp b/include/openscad_cpp_evaluator/bytecode.hpp index 0db3f9f..9c59df3 100644 --- a/include/openscad_cpp_evaluator/bytecode.hpp +++ b/include/openscad_cpp_evaluator/bytecode.hpp @@ -104,18 +104,29 @@ enum class Op { // themselves lists). AccumMergeEach, - // ListCompFor's per-assignment iteration -- materialize once (a - // dimension's own RHS expression is evaluated exactly once, matching - // the interpreter's own upfront `pairs.push_back(...)` loop), then - // reset+iterate however many times that dimension is (re-)entered - // (once per outer-dimension iteration for a nested `for`). `a` = an - // iterList id (see CompiledChunk::numIterLists), unique per assignment - // across the whole chunk. + // ListCompFor's per-assignment iteration. `a` = an iterList id (see + // CompiledChunk::numIterLists), unique per assignment across the whole + // chunk. Each dimension's own IterMaterialize is emitted INSIDE the + // enclosing dimension's own loop body (compileListElement's own + // emitDim recursion), so it re-executes once per outer-dimension + // binding -- required for a later dimension's own RHS to see an + // earlier one's current value (e.g. `[for (p=[1:N], pt=f(p)) pt]`), + // matching evalFor's own doc comment (stmt_eval.cpp) and real + // OpenSCAD.app's verified behavior. IterMaterialize, // pops one Value (the assignment's RHS), expandIterable()s it into iterLists[a], resets its index - IterReset, // resets iterLists[a]'s index to 0 WITHOUT re-materializing (a re-entry, not the first entry) + // Resets iterLists[a]'s index to 0 WITHOUT re-materializing. UNREACHABLE + // as of the materialize-inside-the-nesting fix above: re-materializing + // on every dimension re-entry already resets the index as a side + // effect (see IterMaterialize's own doc comment), so there's no longer + // a "reset without re-materializing" case left to reach for either + // ListCompFor (compileListElement) or ModularFor (compileForLoop). + // Left defined rather than removed in the same change -- a distinct, + // lower-risk cleanup (same convention as LoadUpvalue's own doc + // comment, above). + IterReset, IterNext, // a = loop-variable slot, b = iterList id, c = jump target for exhaustion: if iterLists[b] has a // next value (at its current index), write it to slots[a], advance the index, fall through; - // else jump to c (index is left as-is; the next IterReset for this id will restart it) + // else jump to c (index is left as-is; the next IterMaterialize for this id will restart it) // ListCompCFor's runaway-loop guard, mirroring evalListElement's own // 1,000,000-iteration safety limit exactly (see its own doc comment diff --git a/pyproject.toml b/pyproject.toml index af14161..8f6ac70 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.7" +version = "0.13.9" description = "C++ OpenSCAD evaluator with Python bindings" readme = "README.md" requires-python = ">=3.12" diff --git a/src/builtins/control.cpp b/src/builtins/control.cpp index bb7f2cc..2bcdea4 100644 --- a/src/builtins/control.cpp +++ b/src/builtins/control.cpp @@ -97,24 +97,21 @@ ColoredBody combineBodies(const std::vector& bodies) { // children) -- same rationale as booleans.cpp's own group_sizes. Mirrors // _resolve_intersection_for/_generate_intersection_for. CSGParams resolveIntersectionFor(Evaluator& ev, const oscad::ModularIntersectionFor& node, EvalContext& ctx) { - std::vector> varSeqs; - varSeqs.reserve(node.assignments.size()); - for (const auto& assign : node.assignments) { - Value values = ev.evalExpr(*assign->expr, ctx); - const oscad::Position* pos = &assign->position(); - varSeqs.emplace_back(assign->name->name, expandIterable(values, [&](size_t count) { - ev.warn("Bad range parameter in for statement: too many elements (" + std::to_string(count) + ")", pos); - })); - } - std::vector bodyNodes; bodyNodes.reserve(node.body.size()); for (const auto& b : node.body) bodyNodes.push_back(b.get()); std::vector groupSizes; + // Each dimension's own RHS is evaluated against `parentCtx` (not + // upfront against the original `ctx`), re-evaluated on every entry + // into this recursion level -- see evalFor's own doc comment + // (stmt_eval.cpp) for the full "verified against real OpenSCAD.app" + // rationale; this is the identical bug in intersection_for's own + // cartesian loop (e.g. `intersection_for (i=[0:2], j=[0:i]) ...` + // needs `i` visible in `j`'s own range expression). std::function recurse = [&](size_t depth, EvalContext& parentCtx) { - if (depth == varSeqs.size()) { + if (depth == node.assignments.size()) { // One body-entry marker per full cartesian-product iteration // and nothing per individual variable binding (unlike // evalFor) -- mirrors _resolve_intersection_for's single @@ -126,9 +123,15 @@ CSGParams resolveIntersectionFor(Evaluator& ev, const oscad::ModularIntersection groupSizes.push_back(Value{static_cast(after - before)}); return; } - for (const Value& val : varSeqs[depth].second) { + const auto& assign = node.assignments[depth]; + Value values = ev.evalExpr(*assign->expr, parentCtx); + const oscad::Position* pos = &assign->position(); + IterableValues iter = expandIterable(values, [&](size_t count) { + ev.warn("Bad range parameter in for statement: too many elements (" + std::to_string(count) + ")", pos); + }); + for (const Value& val : iter) { EvalContext childCtx = parentCtx.childCtx(nullptr, std::nullopt, ctx.childrenNodes, ctx.childrenCallerCtx); - childCtx.let_->set(varSeqs[depth].first, val); + childCtx.let_->set(assign->name->name, val); recurse(depth + 1, childCtx); } }; diff --git a/src/bytecode_compiler.cpp b/src/bytecode_compiler.cpp index 1942d31..d96f64d 100644 --- a/src/bytecode_compiler.cpp +++ b/src/bytecode_compiler.cpp @@ -884,22 +884,27 @@ class Compiler { switch (elem.kind()) { case NodeKind::ListCompFor: { auto& n = static_cast(elem); - // Each assignment's RHS is evaluated exactly once, against - // the OUTER scope (never seeing an earlier sibling - // assignment's own loop variable) -- mirrors the - // interpreter's own upfront `pairs.push_back(...)` loop, - // and vector_element.hpp's own doc comment on this - // asymmetry with ListCompCFor. - std::vector iterIds; - iterIds.reserve(n.assignments.size()); - for (const auto& assign : n.assignments) { - compileExpr(*assign->expr, out, scope); - int id = nextIterList_++; - iterIds.push_back(id); - out.push_back({Op::IterMaterialize, id, 0, &assign->position()}); - } scope.push(); const bool isNestedLc = (n.body->kind() == NodeKind::ListComprehension); + // Each dimension's own RHS is compiled+materialized INSIDE + // the enclosing dimension's own loop body (nested, via the + // recursive call below), so it naturally re-executes once + // per outer binding, resolved against `scope` with every + // OUTER dimension's own slot already declared -- see + // compileForLoop's own doc comment (this file) / evalFor's + // (stmt_eval.cpp) for the full "verified against real + // OpenSCAD.app" rationale. This used to evaluate every + // dimension's RHS exactly once, upfront, against the outer + // scope -- justified at the time by appeal to + // vector_element.hpp's own buildScope() "asymmetric with + // ListCompCFor" comment, but that comment describes STATIC + // scope-TREE construction for name resolution (module/ + // function lookups), not RUNTIME value-binding order + // (always the dynamic ctx.let_ chain regardless of the + // Scope tree) -- it doesn't justify this, and real + // OpenSCAD.app confirms a later dimension's range CAN + // depend on an earlier one's current value (e.g. `[for + // (p=[1:N], pt=f(p)) pt]`). std::function emitDim = [&](size_t depth) { if (depth == n.assignments.size()) { if (isNestedLc) { @@ -910,17 +915,16 @@ class Compiler { } return; } - // Every re-entry (every outer-dimension iteration) - // restarts this dimension from its already-materialized - // values -- IterReset, not another IterMaterialize (the - // RHS expression itself is never re-evaluated). - if (depth > 0) out.push_back({Op::IterReset, iterIds[depth], 0, nullptr}); - int loopVarSlot = declareLocal(scope, n.assignments[depth]->name->name); + const auto& assign = n.assignments[depth]; + compileExpr(*assign->expr, out, scope); + const int iterId = nextIterList_++; + out.push_back({Op::IterMaterialize, iterId, 0, &assign->position()}); + int loopVarSlot = declareLocal(scope, assign->name->name); size_t loopStart = out.size(); Instruction iterNext; iterNext.op = Op::IterNext; iterNext.a = loopVarSlot; - iterNext.b = iterIds[depth]; + iterNext.b = iterId; size_t iterNextIdx = out.size(); out.push_back(iterNext); emitDim(depth + 1); @@ -1250,23 +1254,27 @@ class Compiler { // a cartesian-product loop over `node.assignments`, and at the // innermost base case, ONE currentTreeFrameSize()-delta group per full // iteration. Reuses compileForLoop's own cartesian-loop scaffold - // verbatim (Op::NativeIterMaterialize/ForIterNext/ForIterEnd/ - // IterReset) -- Op::ForIterNext's own ctx construction, - // `ctx.childCtx(nullptr, std::nullopt, ctx.childrenNodes, - // ctx.childrenCallerCtx)`, is exactly what resolveIntersectionFor's - // own recurse lambda does too (`parentCtx.childCtx(nullptr, - // std::nullopt, ctx.childrenNodes, ctx.childrenCallerCtx)`), since - // childrenNodes/childrenCallerCtx propagate unchanged through every - // nested childCtx either way -- just with Op::CsgGroupStart/ - // CsgGroupEnd wrapped around the body instead of a bare - // compileStatementList. The leading checkDebug is only emitted when - // node.body isn't empty, mirroring resolveIntersectionFor's own `if - // (!bodyNodes.empty())` guard exactly (unlike compileForLoop's own - // ModularFor sibling, which always emits one even for an empty body, - // against the FOR node itself as a fallback marker -- intersection_for - // has no such native fallback, so neither does this) -- but - // CsgGroupStart/CsgGroupEnd themselves are UNCONDITIONAL, since native - // measures a (possibly zero-size) group regardless of body emptiness. + // verbatim (Op::NativeIterMaterialize/ForIterNext/ForIterEnd, each + // dimension nested INSIDE the enclosing one's own loop body -- see + // compileForLoop's own doc comment for why this shape, not a flat + // materialize-everything-upfront pass, is required for a later + // dimension's range to see an earlier one's binding) -- Op:: + // ForIterNext's own ctx construction, `ctx.childCtx(nullptr, + // std::nullopt, ctx.childrenNodes, ctx.childrenCallerCtx)`, is exactly + // what resolveIntersectionFor's own recurse lambda does too + // (`parentCtx.childCtx(nullptr, std::nullopt, ctx.childrenNodes, + // ctx.childrenCallerCtx)`), since childrenNodes/childrenCallerCtx + // propagate unchanged through every nested childCtx either way -- + // just with Op::CsgGroupStart/CsgGroupEnd wrapped around the body + // instead of a bare compileStatementList. The leading checkDebug is + // only emitted when node.body isn't empty, mirroring + // resolveIntersectionFor's own `if (!bodyNodes.empty())` guard + // exactly (unlike compileForLoop's own ModularFor sibling, which + // always emits one even for an empty body, against the FOR node + // itself as a fallback marker -- intersection_for has no such native + // fallback, so neither does this) -- but CsgGroupStart/CsgGroupEnd + // themselves are UNCONDITIONAL, since native measures a (possibly + // zero-size) group regardless of body emptiness. void compileIntersectionForLoop(const oscad::ModularIntersectionFor& n, std::vector& out) { out.push_back({Op::CheckDebugStatement, internNativeStatement(&n), 0, nullptr}); CompiledChunk::CsgWrapSite site; @@ -1279,35 +1287,32 @@ class Compiler { out.push_back({Op::PushCsgWrap, idx, 0, &n.position()}); const size_t numDims = n.assignments.size(); - std::vector iterListIds(numDims); - for (size_t d = 0; d < numDims; ++d) { - iterListIds[d] = nextIterList_++; - out.push_back({Op::NativeIterMaterialize, internNativeExpr(n.assignments[d]->expr.get()), iterListIds[d], + std::function emitDim = [&](size_t d) { + if (d == numDims) { + if (!n.body.empty()) { + out.push_back({Op::NativeCheckDebugExprLevel, internNativeStatement(n.body.front().get()), 0, nullptr}); + } + out.push_back({Op::CsgGroupStart, 0, 0, nullptr}); + compileStatementList(n.body, out); + out.push_back({Op::CsgGroupEnd, 0, 0, nullptr}); + return; + } + const int iterListId = nextIterList_++; + out.push_back({Op::NativeIterMaterialize, internNativeExpr(n.assignments[d]->expr.get()), iterListId, &n.assignments[d]->position()}); - } - std::vector topIdx(numDims); - std::vector forIterNextIdx(numDims); - for (size_t d = 0; d < numDims; ++d) { - if (d > 0) out.push_back({Op::IterReset, iterListIds[d], 0, nullptr}); - topIdx[d] = out.size(); - forIterNextIdx[d] = out.size(); + const size_t topIdx = out.size(); + const size_t forIterNextIdx = out.size(); Instruction ins; ins.op = Op::ForIterNext; ins.a = internName(n.assignments[d]->name->name); - ins.b = iterListIds[d]; + ins.b = iterListId; ins.node = n.assignments[d].get(); out.push_back(ins); - } - if (!n.body.empty()) { - out.push_back({Op::NativeCheckDebugExprLevel, internNativeStatement(n.body.front().get()), 0, nullptr}); - } - out.push_back({Op::CsgGroupStart, 0, 0, nullptr}); - compileStatementList(n.body, out); - out.push_back({Op::CsgGroupEnd, 0, 0, nullptr}); - for (size_t i = numDims; i-- > 0;) { - out.push_back({Op::ForIterEnd, static_cast(topIdx[i]), 0, nullptr}); - out[forIterNextIdx[i]].c = static_cast(out.size()); - } + emitDim(d + 1); + out.push_back({Op::ForIterEnd, static_cast(topIdx), 0, nullptr}); + out[forIterNextIdx].c = static_cast(out.size()); + }; + emitDim(0); out.push_back({Op::PopCsgWrap, idx, 0, &n.position()}); } @@ -1643,54 +1648,57 @@ class Compiler { // based code at COMPILE time (bounded by the source's own for-clause // count, always small -- never runtime-driven) rather than the // interpreter's own runtime recursion (evalFor's `recurse(depth+1, - // ...)`, stmt_eval.cpp). Each dimension: materialize once (native RHS - // eval + expandIterable), then a ForIterNext/ForIterEnd pair wrapping - // everything inner -- see Op::ForIterNext/ForIterEnd's own doc - // comments (bytecode.hpp) for exactly why a fresh per-iteration ctx - // (not an in-place mutation) is required for correctness, not just - // parity. + // ...)`, stmt_eval.cpp) -- but structurally mirroring that SAME + // recursion shape at compile time (emitDim, below), not a flat + // "materialize every dimension, then loop every dimension" two-pass + // shape (what used to be here): dimension d's own Op:: + // NativeIterMaterialize is emitted INSIDE dimension d-1's own loop + // body (nested, via the recursive call), so it naturally re-executes + // once per (d-1)-and-outer binding, evaluated against whatever ctx is + // current at that point in the instruction stream -- i.e. with every + // OUTER dimension's own loop variable already bound. Required for real + // OpenSCAD's own documented/verified behavior: a later `for` clause's + // range CAN depend on an earlier one (`for (i=[0:2], j=[0:i])` is a + // standard triangular-loop idiom) -- the old flat-materialize-upfront + // shape evaluated every dimension's RHS before ANY variable was bound, + // so a later dimension's own reference to an earlier one always failed + // with "unknown variable" (confirmed wrong against real OpenSCAD.app + // directly, not just an internal inconsistency; see evalFor's own doc + // comment, stmt_eval.cpp, for the fuller story -- same bug, same fix + // shape, on the interpreter side). + // + // No Op::IterReset needed any more: since Materialize now runs exactly + // once per entry into this dimension's own block (not once total), + // its own index-reset (see that op's own doc comment) already covers + // "restart this dimension for the next outer binding" -- the OLD + // separate reset-without-rematerializing step existed purely to avoid + // an unnecessary re-evaluation under the old (buggy) "materialize + // once ever" shape, which no longer exists. void compileForLoop(const oscad::ModularFor& n, std::vector& out) { const size_t numDims = n.assignments.size(); - std::vector iterListIds(numDims); - for (size_t d = 0; d < numDims; ++d) { - iterListIds[d] = nextIterList_++; - out.push_back({Op::NativeIterMaterialize, internNativeExpr(n.assignments[d]->expr.get()), - iterListIds[d], &n.assignments[d]->position()}); - } - std::vector topIdx(numDims); - std::vector forIterNextIdx(numDims); - for (size_t d = 0; d < numDims; ++d) { - // Every dimension but the outermost needs its own IterList - // index reset to 0 each time it's (re-)entered from the - // ENCLOSING dimension's own successful bind -- NativeIterMaterialize - // only resets it once, up front, before ANY iteration runs; by - // the second (and every later) outer-dimension value, this - // dimension's own index is still sitting at "exhausted" from - // the PREVIOUS pass, and would immediately look exhausted - // again without this. Placed strictly BEFORE this dimension's - // own ForIterNext/topIdx (not at it) so this dimension's OWN - // "try the next value" jump (its own ForIterEnd, below) lands - // AT ForIterNext directly and skips the reset -- only the - // fall-through from the enclosing dimension's bind passes - // through it. Harmless (a no-op) the very first time, since - // the index is already 0 from NativeIterMaterialize then. - if (d > 0) out.push_back({Op::IterReset, iterListIds[d], 0, nullptr}); - topIdx[d] = out.size(); - forIterNextIdx[d] = out.size(); + std::function emitDim = [&](size_t d) { + if (d == numDims) { + const oscad::ASTNode* marker = n.body.empty() ? static_cast(&n) : n.body.front().get(); + out.push_back({Op::NativeCheckDebugExprLevel, internNativeStatement(marker), 0, nullptr}); + compileStatementList(n.body, out); + return; + } + const int iterListId = nextIterList_++; + out.push_back({Op::NativeIterMaterialize, internNativeExpr(n.assignments[d]->expr.get()), iterListId, + &n.assignments[d]->position()}); + const size_t topIdx = out.size(); + const size_t forIterNextIdx = out.size(); Instruction ins; ins.op = Op::ForIterNext; ins.a = internName(n.assignments[d]->name->name); - ins.b = iterListIds[d]; + ins.b = iterListId; ins.node = n.assignments[d].get(); out.push_back(ins); // ins.c (exhaustion target) patched below - } - const oscad::ASTNode* marker = n.body.empty() ? static_cast(&n) : n.body.front().get(); - out.push_back({Op::NativeCheckDebugExprLevel, internNativeStatement(marker), 0, nullptr}); - compileStatementList(n.body, out); - for (size_t i = numDims; i-- > 0;) { - out.push_back({Op::ForIterEnd, static_cast(topIdx[i]), 0, nullptr}); - out[forIterNextIdx[i]].c = static_cast(out.size()); - } + emitDim(d + 1); + out.push_back({Op::ForIterEnd, static_cast(topIdx), 0, nullptr}); + out[forIterNextIdx].c = static_cast(out.size()); + }; + emitDim(0); } private: diff --git a/src/expr_eval.cpp b/src/expr_eval.cpp index 080250a..095c256 100644 --- a/src/expr_eval.cpp +++ b/src/expr_eval.cpp @@ -144,23 +144,18 @@ void Evaluator::evalListElement(const oscad::ASTNode& elem, EvalContext& ctx, st switch (elem.kind()) { case oscad::NodeKind::ListCompFor: { auto& n = static_cast(elem); - struct Pair { - std::string name; - IterableValues values; - }; - std::vector pairs; - pairs.reserve(n.assignments.size()); - for (const auto& assign : n.assignments) { - Value values = evalExpr(*assign->expr, ctx); - const oscad::Position* pos = &assign->position(); - pairs.push_back(Pair{assign->name->name, expandIterable(values, [&](size_t count) { - warn("Bad range parameter in for statement: too many elements (" + std::to_string(count) + ")", pos); - })}); - } const bool isNestedLc = (n.body->kind() == oscad::NodeKind::ListComprehension); + // Each dimension's own RHS is evaluated against `parentCtx` + // (not upfront against the original `ctx`), re-evaluated on + // every entry into this recursion level -- see evalFor's own + // doc comment (stmt_eval.cpp) for the full "verified against + // real OpenSCAD.app" rationale; this is the identical bug in + // the list-comprehension sibling of that same construct (e.g. + // `[for (p=[1:N], pt=f(p)) pt]` needs `p` visible in `pt`'s own + // range expression). std::function recurse = [&](size_t depth, EvalContext& parentCtx) { - if (depth == pairs.size()) { + if (depth == n.assignments.size()) { if (isNestedLc) { out.push_back(evalListLiteral(static_cast(*n.body), parentCtx)); } else { @@ -168,14 +163,20 @@ void Evaluator::evalListElement(const oscad::ASTNode& elem, EvalContext& ctx, st } return; } - for (const Value& val : pairs[depth].values) { + const auto& assign = n.assignments[depth]; + Value values = evalExpr(*assign->expr, parentCtx); + const oscad::Position* pos = &assign->position(); + IterableValues iter = expandIterable(values, [&](size_t count) { + warn("Bad range parameter in for statement: too many elements (" + std::to_string(count) + ")", pos); + }); + for (const Value& val : iter) { EvalContext childCtx = parentCtx.letChildCtx(); - childCtx.let_->set(pairs[depth].name, val); + childCtx.let_->set(assign->name->name, val); // Per-binding stop, same shape as evalFor's -- but with // NO separate body-entry marker at depth == pairs.size() // (_eval_listcomp_for has none; the body's own element // check below supplies the expr-level stop instead). - checkDebug(*n.assignments[depth], childCtx); + checkDebug(*assign, childCtx); recurse(depth + 1, childCtx); } }; diff --git a/src/stmt_eval.cpp b/src/stmt_eval.cpp index 17d1c34..0f04e0e 100644 --- a/src/stmt_eval.cpp +++ b/src/stmt_eval.cpp @@ -71,20 +71,6 @@ void Evaluator::evalAssertStatement(const oscad::ModularAssert& node, EvalContex } void Evaluator::evalFor(const oscad::ModularFor& node, EvalContext& ctx) { - struct AssignPair { - std::string name; - IterableValues values; - }; - std::vector pairs; - pairs.reserve(node.assignments.size()); - for (const auto& assign : node.assignments) { - Value values = evalExprMaybeCompiled(*assign->expr, ctx); - const oscad::Position* pos = &assign->position(); - pairs.push_back(AssignPair{assign->name->name, expandIterable(values, [&](size_t count) { - warn("Bad range parameter in for statement: too many elements (" + std::to_string(count) + ")", pos); - })}); - } - std::vector bodyNodes; bodyNodes.reserve(node.body.size()); for (const auto& b : node.body) bodyNodes.push_back(b.get()); @@ -96,8 +82,23 @@ void Evaluator::evalFor(const oscad::ModularFor& node, EvalContext& ctx) { // _eval_for's `_nested` closure exactly) and binds just that one // variable, so inner levels/the body see all outer bindings already in // `let_`. + // + // Each dimension's own RHS is evaluated HERE, against `parentCtx` (not + // upfront against the original `ctx`), and re-evaluated on every entry + // into this recursion level -- i.e. once per combination of whatever + // dimensions 0..depth-1 are currently bound to. This is required for + // real OpenSCAD's own documented/verified behavior: a later `for` + // clause's range CAN depend on an earlier one (`for (i=[0:2], j=[0:i])` + // is a standard triangular-loop idiom; `pt = f(p)` inside `for (p=..., + // pt=f(p))` needs the same). Evaluating all dimensions upfront in one + // flat pass (the previous shape here) evaluated every RHS against the + // ORIGINAL ctx before ANY variable was bound, so a later dimension's + // own reference to an earlier one always failed with "unknown + // variable" -- confirmed wrong against real OpenSCAD.app directly + // (verified both `for(i=[0:2],j=[0:i])` and a dependent list-comp + // case), not just an internal inconsistency. std::function recurse = [&](size_t depth, EvalContext& parentCtx) { - if (depth == pairs.size()) { + if (depth == node.assignments.size()) { // Per-full-iteration "entering the body" marker, separate from // (and before) the body's own per-statement checks in // evalChildren -- mirrors _eval_for's @@ -106,14 +107,20 @@ void Evaluator::evalFor(const oscad::ModularFor& node, EvalContext& ctx) { evalChildren(bodyNodes, parentCtx); return; } - for (const Value& val : pairs[depth].values) { + const auto& assign = node.assignments[depth]; + Value values = evalExprMaybeCompiled(*assign->expr, parentCtx); + const oscad::Position* pos = &assign->position(); + IterableValues iter = expandIterable(values, [&](size_t count) { + warn("Bad range parameter in for statement: too many elements (" + std::to_string(count) + ")", pos); + }); + for (const Value& val : iter) { EvalContext childCtx = parentCtx.childCtx(nullptr, std::nullopt, ctx.childrenNodes, ctx.childrenCallerCtx); - childCtx.let_->set(pairs[depth].name, val); + childCtx.let_->set(assign->name->name, val); // One statement-level stop per (bound-so-far) combination, on // the loop-variable assignment itself -- so a breakpoint set // directly on an `i=[0:2],`/`j=[0:1]` line fires. Mirrors // _eval_for's `_check_debug(assign_node, child)`. - checkDebug(*node.assignments[depth], childCtx); + checkDebug(*assign, childCtx); recurse(depth + 1, childCtx); } }; diff --git a/src/user_calls.cpp b/src/user_calls.cpp index 3b8c796..cbc9721 100644 --- a/src/user_calls.cpp +++ b/src/user_calls.cpp @@ -925,8 +925,28 @@ std::optional Evaluator::prepareChildrenForward(cons // trail level, and TrailView::items() is ancestry-visible, so reading // from effCtx forwards both the call's own overrides AND everything // the wrapper module's body set (`$fn = 100; children();`). + // + // EXCLUDING "$children"/"$parent_modules" is required, not incidental: + // both are set on `ctx` by buildModuleChildCtx for the WRAPPER module's + // own invocation (e.g. a `left(x) { ... }` positioning helper's own + // child count/nesting depth), completely unrelated to the forwarded + // TARGET's real values -- which `evalCtx` already correctly inherited + // above, from `callerCtx` (the actual forwarding target), a few lines + // up. Before this exclusion, a bare `children()` forwarded through ANY + // intermediate wrapper module silently replaced the target's own real + // `$children` with the wrapper's own (e.g. `left(x) { if ($children > + // N) children(N); }`, an extremely common BOSL/BOSL2-style guarded- + // forwarding idiom, would read the WRAPPER's own 1-child count instead + // of the real caller's, corrupting every such conditional -- silently + // dropping geometry with no warning of any kind, since the guard just + // evaluates false instead of erroring. Found via a real user script + // (snappy-reprap's x_axis_assembly_1, GDMUtils.scad's left()/right()/ + // up() positioning helpers) that render a large fraction of the model + // through exactly this pattern -- confirmed present since at least + // v0.13.4 (this port's very first PyPI release), unrelated to any + // 2026-08-01 VM work. for (const auto& [k, v] : ctx.dyn->items()) { - if (!k.empty() && k[0] == '$') evalCtx.dyn->set(k, v); + if (!k.empty() && k[0] == '$' && k != "$children" && k != "$parent_modules") evalCtx.dyn->set(k, v); } for (const auto& [k, v] : ctx.let_->items()) { if (!k.empty() && k[0] == '$') evalCtx.let_->set(k, v); diff --git a/tests/test_control_flow.cpp b/tests/test_control_flow.cpp index a340259..67a2185 100644 --- a/tests/test_control_flow.cpp +++ b/tests/test_control_flow.cpp @@ -70,6 +70,31 @@ TEST(ForLoop, MultipleVariablesProduceCartesianProduct) { EXPECT_EQ(e.bodies.size(), 4u); // 2x2 } +// A later `for`-clause dimension's own range CAN depend on an earlier +// dimension's current binding (a standard triangular-loop idiom) -- +// verified directly against real OpenSCAD.app: `for (i=[0:2], j=[0:i])` +// produces (0,0)(1,0)(1,1)(2,0)(2,1)(2,2), NOT a flat 3x3 product and NOT +// an "unknown variable 'i'" warning. Regression test for a real bug: this +// used to evaluate every dimension's own range expression exactly once, +// upfront, against the ORIGINAL (pre-loop) ctx, so `j`'s own `[0:i]` never +// saw `i` at all. Pinned under both VM states explicitly (compileForLoop +// and evalFor had independent copies of the same bug). +TEST(ForLoop, LaterDimensionRangeCanDependOnEarlierBindingCompiled) { + ScopedVm vm(true); + std::vector echoed; + runScript("for (i = [0:2], j = [0:i]) echo(i, j);", [&](const std::string& msg) { echoed.push_back(msg); }); + EXPECT_EQ(echoed, (std::vector{"ECHO: 0, 0", "ECHO: 1, 0", "ECHO: 1, 1", "ECHO: 2, 0", "ECHO: 2, 1", + "ECHO: 2, 2"})); +} + +TEST(ForLoop, LaterDimensionRangeCanDependOnEarlierBindingInterpreted) { + ScopedVm vm(false); + std::vector echoed; + runScript("for (i = [0:2], j = [0:i]) echo(i, j);", [&](const std::string& msg) { echoed.push_back(msg); }); + EXPECT_EQ(echoed, (std::vector{"ECHO: 0, 0", "ECHO: 1, 0", "ECHO: 1, 1", "ECHO: 2, 0", "ECHO: 2, 1", + "ECHO: 2, 2"})); +} + TEST(ForLoop, IteratesOverAPlainList) { Evaluated e = evalSrc("for (r = [1,2,3]) translate([r*10,0,0]) sphere(r=r, $fn=8);"); EXPECT_EQ(e.bodies.size(), 3u); @@ -210,6 +235,33 @@ TEST(ListComprehension, ForClauseExpandsRange) { EXPECT_DOUBLE_EQ(asNum(items[4]), 4.0); } +// Same "later dimension depends on earlier one" fix as ForLoop's own +// (stmt_eval.cpp's evalFor) but for the list-comprehension sibling +// (evalListElement's ListCompFor case / compileListElement's own compiled +// form) -- this exact shape (`p` bound by the first clause, referenced by +// the second clause's own RHS) is a real, common BOSL/BOSL2 idiom (e.g. +// snappy-reprap's wiring.scad `fillet_path`). Verified against real +// OpenSCAD.app: `[for (p=[1:3], pt=p*10) pt]` == `[10,20,30]`. +TEST(ListComprehension, LaterForClauseCanDependOnEarlierBindingCompiled) { + ScopedVm vm(true); + RunResult r = runScript("x = [for (p = [1:3], pt = p*10) pt];"); + auto items = std::get(varValue(r, "x"))->items; + ASSERT_EQ(items.size(), 3u); + EXPECT_DOUBLE_EQ(asNum(items[0]), 10.0); + EXPECT_DOUBLE_EQ(asNum(items[1]), 20.0); + EXPECT_DOUBLE_EQ(asNum(items[2]), 30.0); +} + +TEST(ListComprehension, LaterForClauseCanDependOnEarlierBindingInterpreted) { + ScopedVm vm(false); + RunResult r = runScript("x = [for (p = [1:3], pt = p*10) pt];"); + auto items = std::get(varValue(r, "x"))->items; + ASSERT_EQ(items.size(), 3u); + EXPECT_DOUBLE_EQ(asNum(items[0]), 10.0); + EXPECT_DOUBLE_EQ(asNum(items[1]), 20.0); + EXPECT_DOUBLE_EQ(asNum(items[2]), 30.0); +} + TEST(ListComprehension, ForIfFiltersElements) { RunResult r = runScript("x = [for (i = [0:4]) if (i % 2 == 0) i];"); auto items = std::get(varValue(r, "x"))->items; @@ -669,6 +721,30 @@ TEST(UserModule, DollarChildrenCountsStatementsNotBodies) { EXPECT_EQ(captured, "ECHO: 2"); } +TEST(UserModule, GuardedIndexedChildrenForwardingThroughWrapperPreservesRealChildrenCount) { + // Regression: prepareChildrenForward's dyn-copy loop (meant only to + // forward $fn/$fa/$fs/$t-style overrides) used to also overwrite + // $children/$parent_modules with the WRAPPER module's ("left" here) own + // single-statement bookkeeping instead of preserving the real target's + // ("outer()"'s actual call site) count already inherited via + // callerCtx. A bare children() forwarding chain through ANY + // intermediate wrapper silently corrupted $children, so a guard like + // `if ($children > N) children(N)` (a standard BOSL/BOSL2 idiom, e.g. + // GDMUtils.scad's left()/right()/up()) evaluated false and dropped + // geometry with no warning at all. Found via a real user project. + Evaluated e = evalSrc("module left(x=0) { translate([-x,0,0]) children(); }\n" + "module outer() {\n" + " left(10) { if ($children > 0) children(0); }\n" + " left(20) { if ($children > 1) children(1); }\n" + "}\n" + "outer() { cube(101); cube(102); }"); + ASSERT_EQ(e.bodies.size(), 2u); + manifold::Box bbox0 = e.bodies[0].body->BoundingBox(); + manifold::Box bbox1 = e.bodies[1].body->BoundingBox(); + EXPECT_NEAR(bbox0.max.x - bbox0.min.x, 101.0, 1e-9); + EXPECT_NEAR(bbox1.max.x - bbox1.min.x, 102.0, 1e-9); +} + TEST(UserModule, RecursiveModuleCall) { Evaluated e = evalSrc("module stack(n) {" " if (n > 0) {" @@ -810,6 +886,29 @@ TEST(FunctionBuiltins, ParentModuleAtTopLevelIsUndef) { // -- intersection_for ----------------------------------------------------- +// Same "later dimension depends on earlier one" fix as ForLoop's own, for +// intersection_for's own cartesian loop (resolveIntersectionFor/ +// compileIntersectionForLoop). Verified against real OpenSCAD.app: +// `intersection_for(i=[0:1], j=[0:i]) { echo(i,j); cube(1); }` fires the +// body (and its echo) exactly 3 times, for (0,0),(1,0),(1,1) -- a flat +// 2x2 product (4 firings) or an "unknown variable 'i'" warning would both +// be wrong. +TEST(IntersectionFor, LaterDimensionRangeCanDependOnEarlierBindingCompiled) { + ScopedVm vm(true); + std::vector echoed; + runScript("intersection_for (i = [0:1], j = [0:i]) { echo(i, j); cube(1); }", + [&](const std::string& msg) { echoed.push_back(msg); }); + EXPECT_EQ(echoed, (std::vector{"ECHO: 0, 0", "ECHO: 1, 0", "ECHO: 1, 1"})); +} + +TEST(IntersectionFor, LaterDimensionRangeCanDependOnEarlierBindingInterpreted) { + ScopedVm vm(false); + std::vector echoed; + runScript("intersection_for (i = [0:1], j = [0:i]) { echo(i, j); cube(1); }", + [&](const std::string& msg) { echoed.push_back(msg); }); + EXPECT_EQ(echoed, (std::vector{"ECHO: 0, 0", "ECHO: 1, 0", "ECHO: 1, 1"})); +} + TEST(IntersectionFor, IntersectsAllIterations) { // Each iteration cube(2) is centered at a different offset; the // intersection of all 3 should be a smaller sub-region than any one.