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); -}