From 2d868cc81d6d62b2ba7b9566c947e2238a605d92 Mon Sep 17 00:00:00 2001 From: Revar Desmera Date: Sat, 22 Aug 2026 13:33:59 -0700 Subject: [PATCH] Warn about backwards ranges only when the step was not written 0.40.0 added the reference's backwards-range warning, and put it in expandIterable so every iterating construct got it from one place. Both halves of that turn out to be wrong. Wrong place: the reference reports this against the range LITERAL, so `r = [5:0];` warns even though nothing ever iterates r. We reported it at iteration and missed that case entirely. Wrong scope: `[5:1:0]` and `[0:-1:5]` warned too. Writing the step out is a statement of intent -- the warning exists for the author who wrote `[5:0]` meaning `[5:-1:0]` and got a silent empty loop, not for someone who spelled out a step and meant it. Both are fixed by the same move. The check now lives in applyRange, the single point where a range literal becomes a value in either engine, gated on the parser's new RangeLiteral::implicitStep. Construction-time is where the reference has it, and it is the only place the flag exists. That deletes more than it adds: RangeDirectionFn, rangeDirectionWarning() and seven per-call-site lambdas all go, since one construction site covers for, list comprehensions, intersection_for, chr() and children() at once. An implicit step is always exactly 1, so the reference's second wording ("begin is smaller than the end, but step is negative") can no longer arise and is gone. A test pins that it stays unreachable. Verified against the real binary for placement, and end-to-end through the CLI under both engines. Co-Authored-By: Claude Opus 5 (1M context) --- external/openscad_cpp_parser | 2 +- include/openscad_cpp_evaluator/evaluator.hpp | 7 ++- include/openscad_cpp_evaluator/value.hpp | 17 +----- pyproject.toml | 2 +- src/builtins/control.cpp | 2 - src/builtins/function_builtins.cpp | 6 +-- src/bytecode_compiler.cpp | 2 +- src/bytecode_vm.cpp | 6 +-- src/expr_eval.cpp | 26 +++++++-- src/stmt_eval.cpp | 2 - src/user_calls.cpp | 4 +- src/value.cpp | 24 +++------ tests/test_control_flow.cpp | 57 ++++++++++++++------ 13 files changed, 82 insertions(+), 75 deletions(-) diff --git a/external/openscad_cpp_parser b/external/openscad_cpp_parser index 8a651c3..ade1d62 160000 --- a/external/openscad_cpp_parser +++ b/external/openscad_cpp_parser @@ -1 +1 @@ -Subproject commit 8a651c31c5433d96fe98fa0e160754e9687fac2f +Subproject commit ade1d62073d3f47544bc562790a94310b549b7ab diff --git a/include/openscad_cpp_evaluator/evaluator.hpp b/include/openscad_cpp_evaluator/evaluator.hpp index 513a568..19decb4 100644 --- a/include/openscad_cpp_evaluator/evaluator.hpp +++ b/include/openscad_cpp_evaluator/evaluator.hpp @@ -392,7 +392,12 @@ class Evaluator { Value applyUnaryOp(oscad::NodeKind kind, const Value& v, const oscad::Position& pos); Value applyIndexAccess(const Value& obj, const Value& idx); Value applyMemberAccess(const Value& obj, const std::string& member); - Value applyRange(const Value& startV, const Value& endV, const Value& stepV); + // `implicitStep` is the RangeLiteral's own flag: true when the source + // wrote `[a:b]` and the parser supplied the 1.0. It only affects whether + // the backwards-range warning fires; the resulting value is the same + // either way. `pos` locates that warning. + Value applyRange(const Value& startV, const Value& endV, const Value& stepV, bool implicitStep = false, + const oscad::Position* pos = nullptr); // Entry point for a call site resolved INSIDE compiled bytecode (the // CALL_FN opcode, bytecode_vm.cpp): `bound` is already fully evaluated diff --git a/include/openscad_cpp_evaluator/value.hpp b/include/openscad_cpp_evaluator/value.hpp index 464f7e5..c5efcae 100644 --- a/include/openscad_cpp_evaluator/value.hpp +++ b/include/openscad_cpp_evaluator/value.hpp @@ -355,20 +355,6 @@ 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 @@ -394,8 +380,7 @@ std::string rangeDirectionWarning(bool stepPositive); // 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, - const RangeDirectionFn& onWrongDirection = nullptr); +IterableValues expandIterable(const Value& v, const RangeTooManyFn& onTooMany = 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 24eb698..eabf514 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "scikit_build_core.build" [project] name = "openscad_cpp_evaluator" -version = "0.40.0" +version = "0.41.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 44d4e65..2bcdea4 100644 --- a/src/builtins/control.cpp +++ b/src/builtins/control.cpp @@ -128,8 +128,6 @@ 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 20b0299..912a1f4 100644 --- a/src/builtins/function_builtins.cpp +++ b/src/builtins/function_builtins.cpp @@ -952,11 +952,7 @@ Value evalBuiltinFunction(Evaluator& ev, const std::string& name, const CallArgs } if (const OscRange* r = std::get_if(&c)) { std::string out; - // 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()); - }); + const IterableValues seq = expandIterable(Value{*r}); for (const Value& item : seq) out += encode(item); return out; } diff --git a/src/bytecode_compiler.cpp b/src/bytecode_compiler.cpp index 7a444dd..e1aee5d 100644 --- a/src/bytecode_compiler.cpp +++ b/src/bytecode_compiler.cpp @@ -466,7 +466,7 @@ class Compiler { compileExpr(*n.start, out, scope); compileExpr(*n.end, out, scope); compileExpr(*n.step, out, scope); - out.push_back({Op::Range, 0, 0, &n.position()}); + out.push_back({Op::Range, n.implicitStep ? 1 : 0, 0, &n.position()}); return; } case NodeKind::PrimaryIndex: { diff --git a/src/bytecode_vm.cpp b/src/bytecode_vm.cpp index e843ddc..961f2aa 100644 --- a/src/bytecode_vm.cpp +++ b/src/bytecode_vm.cpp @@ -634,7 +634,7 @@ Value driveVm(Evaluator& ev, size_t floor) { f.stack.pop_back(); Value start = std::move(f.stack.back()); f.stack.pop_back(); - f.stack.push_back(ev.applyRange(start, end, step)); + f.stack.push_back(ev.applyRange(start, end, step, ins.a != 0, ins.pos)); ++f.pc; break; } @@ -740,8 +740,6 @@ 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(); @@ -1466,8 +1464,6 @@ 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 f7eda91..e8b7bb5 100644 --- a/src/expr_eval.cpp +++ b/src/expr_eval.cpp @@ -177,8 +177,6 @@ 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(); @@ -294,16 +292,36 @@ Value Evaluator::evalListLiteral(const oscad::ListComprehension& node, EvalConte } Value Evaluator::evalRangeLiteral(const oscad::RangeLiteral& node, EvalContext& ctx) { - return applyRange(evalExpr(*node.start, ctx), evalExpr(*node.end, ctx), evalExpr(*node.step, ctx)); + return applyRange(evalExpr(*node.start, ctx), evalExpr(*node.end, ctx), evalExpr(*node.step, ctx), + node.implicitStep, &node.position()); } // Shared with the bytecode VM's RANGE opcode -- see applyBinaryOp's own // comment on why these are factored out as plain Value x Value x Value -> // Value functions. -Value Evaluator::applyRange(const Value& startV, const Value& endV, const Value& stepV) { +Value Evaluator::applyRange(const Value& startV, const Value& endV, const Value& stepV, bool implicitStep, + const oscad::Position* pos) { double start = std::holds_alternative(startV) ? 0.0 : toDoubleLenient(startV); double end = std::holds_alternative(endV) ? 0.0 : toDoubleLenient(endV); double step = std::holds_alternative(stepV) ? 1.0 : toDoubleLenient(stepV); + // A range whose begin is already past its end iterates zero times, which + // is almost always a typo -- [5:0] where [5:-1:0] was meant. Reported + // here, at construction, because that is where the reference reports it: + // `r = [5:0];` warns even if nothing ever iterates r. + // + // Only an IMPLICIT step is checked. Writing the step out is taken as + // deliberate, so [5:1:0] and [0:-1:5] stay silent -- a deliberate + // divergence from the reference, which warns for those too. An implicit + // step is always exactly 1, so the reference's other wording ("begin is + // smaller than the end, but step is negative") cannot arise here and no + // longer exists in this port. + // + // The epsilon matches the one the iteration path used before this check + // moved here, so a range built from float arithmetic that lands a hair + // past its end is still treated as empty-but-fine rather than a typo. + if (implicitStep && (start - end) > 1e-10) { + warn("begin is greater than the end, but step is positive", pos); + } return Value{OscRange{start, step, end}}; } diff --git a/src/stmt_eval.cpp b/src/stmt_eval.cpp index e4fc4d5..0f04e0e 100644 --- a/src/stmt_eval.cpp +++ b/src/stmt_eval.cpp @@ -112,8 +112,6 @@ 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 136dfc7..261e25b 100644 --- a/src/user_calls.cpp +++ b/src/user_calls.cpp @@ -1006,9 +1006,7 @@ std::optional Evaluator::prepareChildrenForward(cons 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()); - }); + const IterableValues iter = expandIterable(idxArg); for (const Value& v : iter) indexValues.push_back(v); } else { warn("Bad parameter type (" + fmtValue(idxArg) + diff --git a/src/value.cpp b/src/value.cpp index 9db5423..b7d29a4 100644 --- a/src/value.cpp +++ b/src/value.cpp @@ -490,30 +490,18 @@ std::optional rangeElementCount(const OscRange& r) { return static_cast(std::floor(n + 1e-10)) + 1; } -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) { +IterableValues expandIterable(const Value& v, const RangeTooManyFn& onTooMany) { 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); - } + // A step pointing away from the end yields nothing, and that is + // almost always a typo -- but the warning for it belongs to the + // range's CONSTRUCTION, not its iteration: only the range literal + // knows whether the author chose the step or the parser supplied + // it. See Evaluator::applyRange. // 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 5e9e6b4..729eef2 100644 --- a/tests/test_control_flow.cpp +++ b/tests/test_control_flow.cpp @@ -1355,18 +1355,34 @@ 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, ImplicitStepPastTheEndWarns) { + EXPECT_TRUE(hasWarning(warningsFrom("for (i=[1:0]) echo(i);"), kGreater)); + EXPECT_TRUE(hasWarning(warningsFrom("for (i=[3:1]) echo(i);"), kGreater)); } -TEST(RangeDirection, NegativeStepPastTheEndWarns) { - EXPECT_TRUE(hasWarning(warningsFrom("for (i=[0:-1:3]) echo(i);"), kSmaller)); +TEST(RangeDirection, AnExplicitStepIsTakenAsDeliberate) { + // Divergence from the reference, which warns for both of these. Writing + // the step out is a statement of intent; the warning exists for the + // author who wrote [3:1] meaning [3:-1:1] and got an empty loop. + for (const char* src : {"for (i=[3:1:1]) echo(i);", "for (i=[0:-1:3]) echo(i);"}) { + EXPECT_TRUE(warningsFrom(src).empty()) << src; + } +} + +TEST(RangeDirection, TheNegativeStepWordingIsUnreachable) { + // An implicit step is always exactly 1, so no input can produce the + // reference's second message. If this ever fires, the gate has been + // widened back to explicit steps and this port needs that wording back. + for (const char* src : {"for (i=[0:-1:3]) echo(i);", "for (i=[3:1]) echo(i);", + "x = [5:0];", "echo(chr([70:65]));"}) { + EXPECT_FALSE(hasWarning(warningsFrom(src), kSmaller)) << src; + } } 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);"}) { + "for (i=[3:3]) echo(i);", "for (i=[0:0.5:2]) echo(i);", + "for (i=[1:3]) echo(i);"}) { EXPECT_TRUE(warningsFrom(src).empty()) << src; } } @@ -1380,24 +1396,33 @@ TEST(RangeDirection, AZeroStepIsNotReportedAsADirectionProblem) { EXPECT_FALSE(hasWarning(w, kSmaller)); } -TEST(RangeDirection, EveryRangeIteratingConstructWarns) { - // The point of wiring this into expandIterable rather than one caller. +TEST(RangeDirection, WarnsWhereTheRangeIsBuiltNotWhereItIsIterated) { + // The reference reports this against the range literal, so a range that + // is assigned and never iterated still warns -- matched here. + EXPECT_TRUE(hasWarning(warningsFrom("r = [5:0];\ncube(1);"), kGreater)); + // And a range built once, iterated twice, warns once. + const std::vector w = + warningsFrom("r = [5:0];\nfor (i=r) echo(i);\nfor (j=r) echo(j);"); + EXPECT_EQ(w.size(), 1u); +} + +TEST(RangeDirection, EveryRangeBuildingConstructWarns) { + // Construction is one shared site, but each of these reaches it by its + // own route -- the interpreter's literal, the VM's Range op, and the + // argument-evaluation path into a builtin. { - const std::vector w = warningsFrom("x = [for (i=[3:1:1]) i];"); + const std::vector w = warningsFrom("x = [for (i=[3: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)) + EXPECT_TRUE(hasWarning(warningsFrom("intersection_for (i=[3:1]) cube(1);"), kGreater)) << "intersection_for"; - EXPECT_TRUE(hasWarning(warningsFrom("echo(chr([70:1:65]));"), kGreater)) - << "chr"; + EXPECT_TRUE(hasWarning(warningsFrom("echo(chr([70:65]));"), kGreater)) << "chr"; EXPECT_TRUE(hasWarning(warningsFrom( "module p() { children([1:0]); }\np() { cube(1); cube(2); }"), kGreater)) << "children"; + EXPECT_TRUE(hasWarning(warningsFrom("function f() = [5:0];\nx = f();"), kGreater)) + << "returned from a function"; } -TEST(RangeDirection, WarnsOncePerEvaluationNotPerAbsentIteration) { - const std::vector w = warningsFrom("for (i=[3:1:1]) echo(i);"); - EXPECT_EQ(w.size(), 1u); -}