diff --git a/include/openscad_cpp_evaluator/value.hpp b/include/openscad_cpp_evaluator/value.hpp index c5efcae..464f7e5 100644 --- a/include/openscad_cpp_evaluator/value.hpp +++ b/include/openscad_cpp_evaluator/value.hpp @@ -355,6 +355,20 @@ class IterableValues { // it, keeping expandIterable() itself free of any Evaluator/echo coupling. using RangeTooManyFn = std::function; +// Called when a range's step points AWAY from its end -- begin > end with a +// positive step, or begin < end with a negative step. Such a range is +// naturally empty, and the reference warns rather than iterating zero times +// in silence. `stepPositive` picks which of its two messages applies; use +// rangeDirectionWarning() to build the text. +// +// Separate from RangeTooManyFn for the same reason that one exists: it +// keeps expandIterable() free of any Evaluator/echo coupling. +using RangeDirectionFn = std::function; + +// The reference's own wording, quoted verbatim, for a range whose step +// points away from its end. Shared so the five call sites cannot drift. +std::string rangeDirectionWarning(bool stepPositive); + // Converts a for()/intersection_for() loop assignment's evaluated RHS into // the sequence of values to iterate: undef -> empty, range -> a LAZY // sequence (matching OscRange's own start/step/end iteration, half-open @@ -380,7 +394,8 @@ using RangeTooManyFn = std::function; // one with a live consumer (BelfrySCAD) that surfaced it. The count is // computed in closed form (not IterableValues::size()'s O(n) walk) so a // legitimate huge-but-under-the-limit range never pays an eager pass. -IterableValues expandIterable(const Value& v, const RangeTooManyFn& onTooMany = nullptr); +IterableValues expandIterable(const Value& v, const RangeTooManyFn& onTooMany = nullptr, + const RangeDirectionFn& onWrongDirection = nullptr); // `each `'s own flatten-one-level rule, shared by the AST interpreter // (evalListLiteral/evalListElement's ListCompEach handling, expr_eval.cpp) diff --git a/pyproject.toml b/pyproject.toml index bede180..24eb698 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "scikit_build_core.build" [project] name = "openscad_cpp_evaluator" -version = "0.39.0" +version = "0.40.0" 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 2bcdea4..44d4e65 100644 --- a/src/builtins/control.cpp +++ b/src/builtins/control.cpp @@ -128,6 +128,8 @@ CSGParams resolveIntersectionFor(Evaluator& ev, const oscad::ModularIntersection 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); + }, [&](bool stepPositive) { + ev.warn(rangeDirectionWarning(stepPositive), pos); }); for (const Value& val : iter) { EvalContext childCtx = parentCtx.childCtx(nullptr, std::nullopt, ctx.childrenNodes, ctx.childrenCallerCtx); diff --git a/src/builtins/function_builtins.cpp b/src/builtins/function_builtins.cpp index dae654b..20b0299 100644 --- a/src/builtins/function_builtins.cpp +++ b/src/builtins/function_builtins.cpp @@ -952,7 +952,12 @@ Value evalBuiltinFunction(Evaluator& ev, const std::string& name, const CallArgs } if (const OscRange* r = std::get_if(&c)) { std::string out; - for (const Value& item : expandIterable(Value{*r})) out += encode(item); + // The reference warns here too -- chr([70:1:65]) is + // as much a typo as for(i=[70:1:65]) would be. + const IterableValues seq = expandIterable(Value{*r}, nullptr, [&](bool stepPositive) { + ev.warn(rangeDirectionWarning(stepPositive), &node.position()); + }); + for (const Value& item : seq) out += encode(item); return out; } return {}; diff --git a/src/bytecode_vm.cpp b/src/bytecode_vm.cpp index 761bc43..e843ddc 100644 --- a/src/bytecode_vm.cpp +++ b/src/bytecode_vm.cpp @@ -740,6 +740,8 @@ Value driveVm(Evaluator& ev, size_t floor) { il.values = expandIterable(v, [&](size_t count) { ev.warn("Bad range parameter in for statement: too many elements (" + std::to_string(count) + ")", ins.pos); + }, [&](bool stepPositive) { + ev.warn(rangeDirectionWarning(stepPositive), ins.pos); }); il.index = 0; il.total = il.values.size(); @@ -1464,6 +1466,8 @@ Value driveVm(Evaluator& ev, size_t floor) { il.values = expandIterable(v, [&](size_t count) { ev.warn("Bad range parameter in for statement: too many elements (" + std::to_string(count) + ")", ins.pos); + }, [&](bool stepPositive) { + ev.warn(rangeDirectionWarning(stepPositive), ins.pos); }); il.index = 0; il.total = il.values.size(); diff --git a/src/expr_eval.cpp b/src/expr_eval.cpp index e191d96..f7eda91 100644 --- a/src/expr_eval.cpp +++ b/src/expr_eval.cpp @@ -177,6 +177,8 @@ void Evaluator::evalListElement(const oscad::ASTNode& elem, EvalContext& ctx, st 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); + }, [&](bool stepPositive) { + warn(rangeDirectionWarning(stepPositive), pos); }); for (const Value& val : iter) { EvalContext childCtx = parentCtx.letChildCtx(); diff --git a/src/stmt_eval.cpp b/src/stmt_eval.cpp index 0f04e0e..e4fc4d5 100644 --- a/src/stmt_eval.cpp +++ b/src/stmt_eval.cpp @@ -112,6 +112,8 @@ void Evaluator::evalFor(const oscad::ModularFor& node, EvalContext& ctx) { 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); + }, [&](bool stepPositive) { + warn(rangeDirectionWarning(stepPositive), pos); }); for (const Value& val : iter) { EvalContext childCtx = parentCtx.childCtx(nullptr, std::nullopt, ctx.childrenNodes, ctx.childrenCallerCtx); diff --git a/src/user_calls.cpp b/src/user_calls.cpp index 0f6aa29..136dfc7 100644 --- a/src/user_calls.cpp +++ b/src/user_calls.cpp @@ -984,7 +984,6 @@ std::optional Evaluator::prepareChildrenForward(cons // filtered statement may produce 0 bodies, which would shift every // subsequent body-index lookup, so the Nth statement is evaluated // directly instead. - const int idx = static_cast(toDoubleLenient(idxArg)); std::vector geoNodes; for (const oscad::ASTNode* c : *ctx.childrenNodes) { if (c->kind() != oscad::NodeKind::Assignment && c->kind() != oscad::NodeKind::ModuleDeclaration && @@ -992,8 +991,59 @@ std::optional Evaluator::prepareChildrenForward(cons geoNodes.push_back(c); } } - if (idx < 0 || static_cast(idx) >= geoNodes.size()) return std::nullopt; - return ChildrenForward{std::move(evalCtx), {geoNodes[static_cast(idx)]}}; + + // The argument is a number, a VECTOR of numbers, or a RANGE -- + // children([3:1:5]) is children(3); children(4); children(5), and + // children([3:-1:1]) is 3, 2, 1 in that order. Only a plain number was + // handled before, and toDoubleLenient collapses a list or a range to 0, + // so every vector/range form silently rendered child 0 instead. + // + // Ranges go through expandIterable, the same path a for-loop uses, so + // step direction, fractional steps and naturally-empty ranges all + // behave identically in both places rather than growing a second + // interpretation here. + std::vector indexValues; + if (std::holds_alternative(idxArg)) { + indexValues.push_back(idxArg); + } else if (std::holds_alternative(idxArg) || std::holds_alternative(idxArg)) { + const IterableValues iter = expandIterable(idxArg, nullptr, [&](bool stepPositive) { + warn(rangeDirectionWarning(stepPositive), currentWarnEntry()); + }); + for (const Value& v : iter) indexValues.push_back(v); + } else { + warn("Bad parameter type (" + fmtValue(idxArg) + + ") for children, only accept: empty, number, vector, range.", + currentWarnEntry()); + return std::nullopt; + } + + // Order is preserved and duplicates are kept: children([2,2,2]) really + // does evaluate child 2 three times, and the order shows through in the + // CSG tree even though a union usually hides it. + std::vector picked; + picked.reserve(indexValues.size()); + for (const Value& v : indexValues) { + if (!std::holds_alternative(v)) { + warn("Bad parameter type (" + fmtValue(v) + + ") for children, only accept: empty, number, vector, range.", + currentWarnEntry()); + return std::nullopt; + } + // Truncates, matching the reference: children([1.7]) is child 1. + const long long idx = static_cast(std::get(v)); + if (idx < 0 || static_cast(idx) >= geoNodes.size()) { + // An out-of-range index is skipped, not fatal -- children([0,99]) + // still draws child 0. Warned about either way; this used to + // return silently. + warn("Children index (" + fmtValue(v) + ") out of bounds (" + + std::to_string(geoNodes.size()) + " children)", + currentWarnEntry()); + continue; + } + picked.push_back(geoNodes[static_cast(idx)]); + } + if (picked.empty()) return std::nullopt; + return ChildrenForward{std::move(evalCtx), std::move(picked)}; } void Evaluator::builtinChildren(const CallArgs& args, EvalContext& ctx) { diff --git a/src/value.cpp b/src/value.cpp index bf1c151..9db5423 100644 --- a/src/value.cpp +++ b/src/value.cpp @@ -490,13 +490,30 @@ std::optional rangeElementCount(const OscRange& r) { return static_cast(std::floor(n + 1e-10)) + 1; } -IterableValues expandIterable(const Value& v, const RangeTooManyFn& onTooMany) { +std::string rangeDirectionWarning(bool stepPositive) { + return stepPositive ? "begin is greater than the end, but step is positive" + : "begin is smaller than the end, but step is negative"; +} + +IterableValues expandIterable(const Value& v, const RangeTooManyFn& onTooMany, + const RangeDirectionFn& onWrongDirection) { if (std::holds_alternative(v)) return IterableValues{}; if (const OscRange* r = std::get_if(&v)) { if (const std::optional count = rangeElementCount(*r); count && *count >= 1'000'000) { if (onTooMany) onTooMany(*count); return IterableValues{}; } + // A step pointing away from the end yields nothing. That is almost + // always a typo -- [1:0] where [1:-1:0] was meant -- so the + // reference says so rather than silently running zero times. + // + // A zero step is deliberately NOT reported here: it is a different + // failure (the reference calls it "too many elements") and shares + // rangeElementCount's nullopt with this case only by coincidence. + if (onWrongDirection && r->step != 0.0) { + const double n = (r->end - r->start) / r->step; + if (n < -1e-10) onWrongDirection(r->step > 0.0); + } // Lazy -- IterableValues itself reproduces this exact // termination condition (a zero step is naturally empty: neither // `x <= end` nor `x >= end` branch ever fires for it) without diff --git a/tests/test_control_flow.cpp b/tests/test_control_flow.cpp index 5a51975..5e9e6b4 100644 --- a/tests/test_control_flow.cpp +++ b/tests/test_control_flow.cpp @@ -1157,3 +1157,247 @@ TEST(DollarVarChildren, ChildrenNIndexesStatementNotOutputBodyWhenAnEarlierState ASSERT_EQ(e.bodies.size(), 1u); EXPECT_NEAR(e.bodies[0].body->Volume(), 8.0, 1e-9); // cube(2) } + +// -- children(index): number, vector or range ----------------------------- +// +// children() takes a number, a VECTOR of numbers, or a RANGE, so +// children([3:1:5]) is children(3); children(4); children(5), and +// children([3:-1:1]) is 3, 2, 1 in that order. +// +// Only a plain number used to be handled. toDoubleLenient collapses a list +// or a range to 0, so every vector/range form silently rendered child 0 -- +// wrong geometry, no warning. Every case below was diffed against OpenSCAD +// 2026.02.01. + +namespace { + +// Which children ran, in order, as "c3,c2,c1". Each child echoes its own +// index, which survives ordering where a union of overlapping bodies would +// not -- and unlike geometry it also shows duplicates. +std::string childOrder(const std::string& picker) { + std::string out; + Evaluator ev([&](const std::string& m) { + const std::string tag = "ECHO: \"c"; + const size_t at = m.find(tag); + if (at == std::string::npos) return; + if (!out.empty()) out += ","; + out += "c" + m.substr(at + tag.size(), m.find('"', at + tag.size()) - at - tag.size()); + }); + const std::string src = + "module pick() { " + picker + " }\n" + "pick() { echo(\"c0\"); echo(\"c1\"); echo(\"c2\"); echo(\"c3\"); echo(\"c4\"); echo(\"c5\"); }\n"; + std::vector> ast = test::parseSrc(src); + auto scope = oscad::buildScopes(ast); + EvalContext ctx = EvalContext::makeRoot(scope.get()); + ev.resolveTree(ast, ctx); + return out; +} + +std::vector childWarnings(const std::string& picker) { + std::vector warnings; + Evaluator ev([&](const std::string& m) { + if (m.rfind("WARNING:", 0) == 0) warnings.push_back(m); + }); + const std::string src = + "module pick() { " + picker + " }\n" + "pick() { echo(\"c0\"); echo(\"c1\"); echo(\"c2\"); }\n"; + std::vector> ast = test::parseSrc(src); + auto scope = oscad::buildScopes(ast); + EvalContext ctx = EvalContext::makeRoot(scope.get()); + ev.resolveTree(ast, ctx); + return warnings; +} + +} // namespace + +TEST(ChildrenIndex, PlainNumberStillWorks) { + EXPECT_EQ(childOrder("children(2);"), "c2"); + EXPECT_EQ(childOrder("children();"), "c0,c1,c2,c3,c4,c5"); +} + +TEST(ChildrenIndex, AscendingRangeSelectsEachInTurn) { + EXPECT_EQ(childOrder("children([3:1:5]);"), "c3,c4,c5"); + EXPECT_EQ(childOrder("children([1:3]);"), "c1,c2,c3"); // implicit step +} + +TEST(ChildrenIndex, DescendingRangeRunsBackwards) { + // The sign of the step decides direction, exactly as in a for-loop. + EXPECT_EQ(childOrder("children([3:-1:1]);"), "c3,c2,c1"); + EXPECT_EQ(childOrder("children([5:-1:3]);"), "c5,c4,c3"); + EXPECT_EQ(childOrder("children([2:-1:0]);"), "c2,c1,c0"); +} + +TEST(ChildrenIndex, RangeStepIsHonoured) { + EXPECT_EQ(childOrder("children([5:-2:0]);"), "c5,c3,c1"); + EXPECT_EQ(childOrder("children([0:2:5]);"), "c0,c2,c4"); +} + +TEST(ChildrenIndex, SingleElementRange) { + EXPECT_EQ(childOrder("children([3:-1:3]);"), "c3"); + EXPECT_EQ(childOrder("children([3:1:3]);"), "c3"); +} + +TEST(ChildrenIndex, VectorSelectsInTheGivenOrder) { + EXPECT_EQ(childOrder("children([3,4,5]);"), "c3,c4,c5"); + EXPECT_EQ(childOrder("children([5,0,2]);"), "c5,c0,c2"); +} + +TEST(ChildrenIndex, DuplicatesAreKept) { + // children([2,2,2]) really does evaluate child 2 three times. + EXPECT_EQ(childOrder("children([2,2,2]);"), "c2,c2,c2"); +} + +TEST(ChildrenIndex, FractionalIndicesTruncate) { + EXPECT_EQ(childOrder("children([1.7]);"), "c1"); + EXPECT_EQ(childOrder("children([0:0.5:2]);"), "c0,c0,c1,c1,c2"); +} + +TEST(ChildrenIndex, EmptySelectionsProduceNothing) { + EXPECT_EQ(childOrder("children([]);"), ""); + // Wrong direction for the step -- naturally empty, same as a for-loop. + EXPECT_EQ(childOrder("children([1:0]);"), ""); + EXPECT_EQ(childOrder("children([3:1:1]);"), ""); +} + +TEST(ChildrenIndex, AnOutOfRangeIndexIsSkippedNotFatal) { + // children([0,99]) still draws child 0. + EXPECT_EQ(childOrder("children([0,99]);"), "c0"); + EXPECT_EQ(childOrder("children([99,1]);"), "c1"); + EXPECT_EQ(childOrder("children([-1,0]);"), "c0"); +} + +TEST(ChildrenIndex, OutOfRangeWarns) { + // This used to return silently, for the plain-number form too. + const std::vector w = childWarnings("children(99);"); + ASSERT_EQ(w.size(), 1u); + EXPECT_NE(w[0].find("Children index (99) out of bounds (3 children)"), std::string::npos) << w[0]; +} + +TEST(ChildrenIndex, NegativeIndexWarns) { + const std::vector w = childWarnings("children(-1);"); + ASSERT_EQ(w.size(), 1u); + EXPECT_NE(w[0].find("out of bounds"), std::string::npos) << w[0]; +} + +TEST(ChildrenIndex, EachBadIndexInAVectorWarnsSeparately) { + const std::vector w = childWarnings("children([99,98]);"); + EXPECT_EQ(w.size(), 2u); +} + +TEST(ChildrenIndex, BadParameterTypeWarns) { + // Quoted verbatim from the reference, trailing period included. + for (const char* src : {"children(\"a\");", "children([\"a\"]);", + "children(true);", "children([true]);"}) { + const std::vector w = childWarnings(src); + ASSERT_FALSE(w.empty()) << src; + EXPECT_NE(w[0].find("for children, only accept: empty, number, vector, range."), + std::string::npos) << src << " -> " << w[0]; + } +} + +TEST(ChildrenIndex, GeometryFollowsTheSelection) { + // The echo-based checks above prove ordering; this one proves the + // geometry really is the selected children and not child 0. + // Written out rather than looped: children are counted as STATEMENTS, + // and a for-loop is one statement however many bodies it emits. + Evaluated e = evalSrc( + "module pick() { children([3:1:5]); }\n" + "pick() {\n" + " translate([0,0,0]) cube(1); translate([2,0,0]) cube(1);\n" + " translate([4,0,0]) cube(1); translate([6,0,0]) cube(1);\n" + " translate([8,0,0]) cube(1); translate([10,0,0]) cube(1);\n" + "}\n"); + double total = 0.0; + manifold::Box all; + bool first = true; + for (const ColoredBody& b : e.bodies) { + if (!b.body || b.body->IsEmpty()) continue; + total += b.body->Volume(); + all = first ? b.body->BoundingBox() : all.Union(b.body->BoundingBox()); + first = false; + } + EXPECT_NEAR(total, 3.0, 1e-9); // three unit cubes + EXPECT_NEAR(all.min.x, 6.0, 1e-9); // children 3, 4, 5 sit at x = 6, 8, 10 + EXPECT_NEAR(all.max.x, 11.0, 1e-9); +} + +// -- range direction warnings --------------------------------------------- +// +// A range whose step points away from its end is naturally empty. The +// reference warns rather than iterating zero times in silence, since it is +// almost always a typo -- [1:0] where [1:-1:0] was meant. We did not warn +// anywhere. Wired into expandIterable, so every construct that iterates a +// range gets it: for, list comprehensions, intersection_for, children(). + +namespace { + +std::vector warningsFrom(const std::string& src) { + std::vector warnings; + Evaluator ev([&](const std::string& m) { + if (m.rfind("WARNING:", 0) == 0) warnings.push_back(m); + }); + std::vector> ast = test::parseSrc(src); + auto scope = oscad::buildScopes(ast); + EvalContext ctx = EvalContext::makeRoot(scope.get()); + ev.resolveTree(ast, ctx); + return warnings; +} + +bool hasWarning(const std::vector& w, const std::string& needle) { + for (const std::string& m : w) { + if (m.find(needle) != std::string::npos) return true; + } + return false; +} + +const char* kGreater = "begin is greater than the end, but step is positive"; +const char* kSmaller = "begin is smaller than the end, but step is negative"; + +} // namespace + +TEST(RangeDirection, PositiveStepPastTheEndWarns) { + EXPECT_TRUE(hasWarning(warningsFrom("for (i=[3:1:1]) echo(i);"), kGreater)); + EXPECT_TRUE(hasWarning(warningsFrom("for (i=[1:0]) echo(i);"), kGreater)); // implicit step +} + +TEST(RangeDirection, NegativeStepPastTheEndWarns) { + EXPECT_TRUE(hasWarning(warningsFrom("for (i=[0:-1:3]) echo(i);"), kSmaller)); +} + +TEST(RangeDirection, AWellFormedRangeIsSilent) { + for (const char* src : {"for (i=[1:1:3]) echo(i);", "for (i=[3:-1:1]) echo(i);", + "for (i=[3:3]) echo(i);", "for (i=[0:0.5:2]) echo(i);"}) { + EXPECT_TRUE(warningsFrom(src).empty()) << src; + } +} + +TEST(RangeDirection, AZeroStepIsNotReportedAsADirectionProblem) { + // Different failure entirely -- the reference calls it "too many + // elements". It shares rangeElementCount's nullopt with the + // wrong-direction case only by coincidence. + const std::vector w = warningsFrom("for (i=[0:0:3]) echo(i);"); + EXPECT_FALSE(hasWarning(w, kGreater)); + EXPECT_FALSE(hasWarning(w, kSmaller)); +} + +TEST(RangeDirection, EveryRangeIteratingConstructWarns) { + // The point of wiring this into expandIterable rather than one caller. + { + const std::vector w = warningsFrom("x = [for (i=[3:1:1]) i];"); + std::string got; + for (const std::string& m : w) got += "\n " + m; + EXPECT_TRUE(hasWarning(w, kGreater)) << "list comprehension, got:" << got; + } + EXPECT_TRUE(hasWarning(warningsFrom("intersection_for (i=[3:1:1]) cube(1);"), kGreater)) + << "intersection_for"; + EXPECT_TRUE(hasWarning(warningsFrom("echo(chr([70:1:65]));"), kGreater)) + << "chr"; + EXPECT_TRUE(hasWarning(warningsFrom( + "module p() { children([1:0]); }\np() { cube(1); cube(2); }"), kGreater)) + << "children"; +} + +TEST(RangeDirection, WarnsOncePerEvaluationNotPerAbsentIteration) { + const std::vector w = warningsFrom("for (i=[3:1:1]) echo(i);"); + EXPECT_EQ(w.size(), 1u); +}