From ac70b907572188fe929d3a7f74bf669d30777dca Mon Sep 17 00:00:00 2001 From: Revar Desmera Date: Sat, 1 Aug 2026 14:00:04 -0700 Subject: [PATCH 1/2] Fix dependent for-loop/list-comp ranges (later dim sees earlier binding) A later for-clause/list-comprehension-for dimension's own range/RHS expression could never see an earlier dimension's current binding -- `for (i=[0:2], j=[0:i])` (a standard triangular-loop idiom) or `[for (p=[1:N], pt=f(p)) pt]` (a common BOSL/BOSL2 pattern, e.g. snappy-reprap's wiring.scad fillet_path) both warned "Ignoring unknown variable" and silently produced undef/wrong results, cascading into large amounts of spurious downstream "undefined operation" warnings and partial/wrong geometry in real scripts. Root cause: every dimension's own RHS expression was evaluated in one flat pass, upfront, against the ORIGINAL (pre-loop) ctx/scope, before ANY variable was bound -- present in six independent copies of the same shape: evalFor/evalListElement's ListCompFor case/resolveIntersectionFor (interpreter) and compileForLoop/compileIntersectionForLoop/ compileListElement's ListCompFor case (VM compiler, including the intersection_for compiler added earlier this week, which inherited the bug from compileForLoop's own scaffold it reused). Verified wrong against real OpenSCAD.app directly for all three constructs, not just an internal inconsistency -- confirmed by a real-world repro (snappy-reprap's full_assembly.scad): 6024 spurious warnings and a visibly incomplete model before this fix, 2 (a real, unrelated, pre-existing warning) after. Fix: each dimension's own RHS now evaluates INSIDE the recursion/nested- loop-emission (interpreter) or is emitted structurally INSIDE the enclosing dimension's own loop body (compiler), so it re-evaluates once per outer-dimension binding with every earlier dimension already bound -- exactly mirroring ordinary nested-loop semantics, matching real OpenSCAD. Op::IterReset becomes unreachable as a result (materializing now naturally resets the iterator's own index as a side effect on every re-entry) -- left defined rather than removed in this same change. Co-Authored-By: Claude Sonnet 5 --- include/openscad_cpp_evaluator/bytecode.hpp | 29 ++- pyproject.toml | 2 +- src/builtins/control.cpp | 29 +-- src/bytecode_compiler.cpp | 212 ++++++++++---------- src/expr_eval.cpp | 35 ++-- src/stmt_eval.cpp | 43 ++-- tests/test_control_flow.cpp | 75 +++++++ 7 files changed, 265 insertions(+), 160 deletions(-) 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..17a7f48 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.8" 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/tests/test_control_flow.cpp b/tests/test_control_flow.cpp index a340259..3b92c91 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; @@ -810,6 +862,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. From 48488272d49a16f0b872f3f753791740ad012526 Mon Sep 17 00:00:00 2001 From: Revar Desmera Date: Sat, 1 Aug 2026 14:51:44 -0700 Subject: [PATCH 2/2] Fix $children/$parent_modules corruption during children() forwarding prepareChildrenForward's dyn-variable copy loop, meant only to forward $fn/$fa/$fs/$t-style overrides from the intermediate wrapper's scope, also swept up $children/$parent_modules and overwrote the correctly inherited target values with the wrapper module's own bookkeeping. This silently broke the common BOSL/BOSL2 guarded-forwarding idiom `module left(x) { if ($children > N) children(N); }`: the guard read the wrapper's own 1-child count instead of the real caller's, so it evaluated false and dropped geometry with no warning. Found via a real user project (snappy-reprap) where this pattern accounts for a large fraction of the rendered model. Bump to 0.13.9. --- pyproject.toml | 2 +- src/user_calls.cpp | 22 +++++++++++++++++++++- tests/test_control_flow.cpp | 24 ++++++++++++++++++++++++ 3 files changed, 46 insertions(+), 2 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index 17a7f48..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.8" +version = "0.13.9" description = "C++ OpenSCAD evaluator with Python bindings" readme = "README.md" requires-python = ">=3.12" 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 3b92c91..67a2185 100644 --- a/tests/test_control_flow.cpp +++ b/tests/test_control_flow.cpp @@ -721,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) {"