From 9d1b1801373eb6c6de42ebff1992573be164bf94 Mon Sep 17 00:00:00 2001 From: Revar Desmera Date: Sat, 22 Aug 2026 13:22:29 -0700 Subject: [PATCH] RangeLiteral records whether the step was written `[a:b]` and `[a:1:b]` produced identical ASTs, because makeRangeLiteral synthesizes the 1.0 for the two-argument form. That is fine for evaluating the range -- the value is the same either way -- but it throws away the one thing the evaluator needs to tell a typo from a choice: `[5:0]` is almost always `[5:-1:0]` written wrong, while `[5:1:0]` is someone saying what they meant. So keep a flag. The synthesized step node stays exactly where it was, so every consumer that just wants the value is untouched. toString() now prints back the form that was written. Emitting the synthesized step for a two-argument range produced a program that behaves identically but reads as an explicit step -- so reformatting a file would have quietly suppressed the evaluator's warning for it. Co-Authored-By: Claude Opus 5 (1M context) --- include/openscad_cpp_parser/ast/expression.hpp | 12 ++++++++++-- src/ast/expression.cpp | 4 ++++ src/grammar/driver.cpp | 6 ++++-- src/serialization/json_io.cpp | 4 +++- tests/test_pretty_print.cpp | 13 +++++++++++++ 5 files changed, 34 insertions(+), 5 deletions(-) diff --git a/include/openscad_cpp_parser/ast/expression.hpp b/include/openscad_cpp_parser/ast/expression.hpp index cf84932..456d499 100644 --- a/include/openscad_cpp_parser/ast/expression.hpp +++ b/include/openscad_cpp_parser/ast/expression.hpp @@ -80,14 +80,22 @@ class UndefinedLiteral : public Primary { class RangeLiteral : public Primary { public: RangeLiteral(Position position, std::unique_ptr start, std::unique_ptr end, - std::unique_ptr step) + std::unique_ptr step, bool implicitStep = false) : Primary(NodeKind::RangeLiteral, std::move(position)), start(std::move(start)), end(std::move(end)), - step(std::move(step)) {} + step(std::move(step)), implicitStep(implicitStep) {} std::unique_ptr start; std::unique_ptr end; std::unique_ptr step; + // True when the source wrote the two-argument form `[a:b]` and the step + // node below is the literal 1.0 this parser synthesized for it. Consumers + // that only need the value can ignore this and read `step` as always + // present; it exists because "the author did not choose a step" is not + // recoverable from the synthesized node, and the evaluator's backwards-range + // warning fires only for that case (an explicit step is taken as deliberate). + bool implicitStep = false; + std::string toString() const override; void buildScope(Scope& parentScope) override; }; diff --git a/src/ast/expression.cpp b/src/ast/expression.cpp index 180aaf6..84875af 100644 --- a/src/ast/expression.cpp +++ b/src/ast/expression.cpp @@ -22,6 +22,10 @@ std::string NumberLiteral::toString() const { } std::string RangeLiteral::toString() const { + // Print back the form that was written. Emitting the synthesized step for + // a two-argument range would round-trip `[5:0]` into `[5 : 1 : 0]`, which + // reads identically but suppresses the evaluator's backwards-range warning. + if (implicitStep) return "[" + start->toString() + " : " + end->toString() + "]"; return "[" + start->toString() + " : " + step->toString() + " : " + end->toString() + "]"; } diff --git a/src/grammar/driver.cpp b/src/grammar/driver.cpp index 36f7092..fb6a26d 100644 --- a/src/grammar/driver.cpp +++ b/src/grammar/driver.cpp @@ -46,11 +46,13 @@ NodePtr makeUndefinedLiteral(ParserDriver& driver, const OscadLocation& loc) { } NodePtr makeRangeLiteral(ParserDriver& driver, const OscadLocation& loc, NodePtr start, NodePtr end, NodePtr step) { - if (!step) { + const bool implicitStep = !step; + if (implicitStep) { step = std::make_unique(driver.toPosition(loc), 1.0); } return std::make_unique(driver.toPosition(loc), nodeCast(std::move(start)), - nodeCast(std::move(end)), nodeCast(std::move(step))); + nodeCast(std::move(end)), nodeCast(std::move(step)), + implicitStep); } NodePtr makePositionalArgument(ParserDriver& driver, const OscadLocation& loc, NodePtr expr) { diff --git a/src/serialization/json_io.cpp b/src/serialization/json_io.cpp index bef1843..c5faf7d 100644 --- a/src/serialization/json_io.cpp +++ b/src/serialization/json_io.cpp @@ -112,6 +112,7 @@ json toJsonImpl(const ASTNode& node, bool includePos) { j["start"] = valueToJson(n.start.get(), includePos); j["end"] = valueToJson(n.end.get(), includePos); j["step"] = valueToJson(n.step.get(), includePos); + if (n.implicitStep) j["implicitStep"] = true; break; } case NodeKind::ParameterDeclaration: { @@ -410,7 +411,8 @@ const std::unordered_map& registry() { {"RangeLiteral", [](const json& j, Position pos) -> std::unique_ptr { return std::make_unique(std::move(pos), childFromJson(j, "start"), - childFromJson(j, "end"), childFromJson(j, "step")); + childFromJson(j, "end"), childFromJson(j, "step"), + j.value("implicitStep", false)); }}, {"ParameterDeclaration", [](const json& j, Position pos) -> std::unique_ptr { diff --git a/tests/test_pretty_print.cpp b/tests/test_pretty_print.cpp index 7caa093..720211f 100644 --- a/tests/test_pretty_print.cpp +++ b/tests/test_pretty_print.cpp @@ -515,3 +515,16 @@ TEST(PrettyPrint, AssignmentRhsWithLeadingCommentAndTrailingLineComment) { // no other translation unit -- including this test file -- can name them. // Left as a known, permanent coverage ceiling rather than weakening the // encapsulation just to reach 100%. + +// The two-argument range keeps its shape through a print/reparse cycle. +// It would be easy to always print the synthesized step, and the result +// would still be a correct program -- but `[5:0]` and `[5:1:0]` mean +// different things to the evaluator's backwards-range warning, so +// reformatting a file must not quietly convert one into the other. +TEST(PrettyPrint, ImplicitRangeStepSurvivesRoundTrip) { + auto ast = parseAst("a = [5:0];\nb = [5:1:0];\n"); + const std::string printed = toOpenscad(ast); + EXPECT_NE(printed.find("[5 : 0]"), std::string::npos) << printed; + EXPECT_NE(printed.find("[5 : 1 : 0]"), std::string::npos) << printed; + expectStablePrint("a = [5:0];\nb = [5:1:0];\n"); +}