From bd8530e91fcd07d43364bb0d3b1cabf3b57f9e77 Mon Sep 17 00:00:00 2001 From: Torgny Bjers Date: Fri, 5 Jun 2026 00:00:16 -0400 Subject: [PATCH] Fix RangeLiteral emitting spurious :1 for two-argument ranges MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The builder synthesises a default step (NumberLiteral 1.0) tagged with the range node's own position when the source form is [start:end]. RangeLiteral.__str__ previously always emitted [start:end:step], turning [0:5] into [0:5:1]. OpenSCAD's three-argument syntax is [start:step:end], so [0:5:1] is read as start=0, step=5, end=1 — semantically opposite. Fix __str__ to compare step.position against the range node's position: same position means synthesised default → emit [start:end]; distinct position means an explicit third argument from source → emit [start:end:step]. Updates test_nodes.py to cover both the 2-arg and 3-arg cases with appropriate positions, and adds round-trip regression tests in test_vectors.py. Corrects two test_pretty_print assertions that were hard-coded to the old broken output (:1]). Co-Authored-By: Claude Sonnet 4.6 --- src/openscad_parser/ast/nodes.py | 5 +++++ tests/test_nodes.py | 11 +++++++++-- tests/test_pretty_print.py | 4 ++-- tests/test_vectors.py | 33 ++++++++++++++++++++++++++++++++ 4 files changed, 49 insertions(+), 4 deletions(-) diff --git a/src/openscad_parser/ast/nodes.py b/src/openscad_parser/ast/nodes.py index 5c63518..d035669 100644 --- a/src/openscad_parser/ast/nodes.py +++ b/src/openscad_parser/ast/nodes.py @@ -337,6 +337,11 @@ class RangeLiteral(Primary): step: Expression def __str__(self): + # A synthesised default step (2-arg source range) is tagged with the + # range node's own position; an explicit third argument has a distinct + # source position. + if self.step.position == self.position: + return f"[{self.start}:{self.end}]" return f"[{self.start}:{self.end}:{self.step}]" def build_scope(self, parent_scope: "Scope") -> None: diff --git a/tests/test_nodes.py b/tests/test_nodes.py index 3afe81b..2ef7229 100644 --- a/tests/test_nodes.py +++ b/tests/test_nodes.py @@ -172,8 +172,15 @@ def test_primary_and_range_str(): member = PrimaryMember(left=_ident("obj"), member=_ident("x"), position=_pos()) assert str(member) == "obj.x" - range_lit = RangeLiteral(start=_num(0.0), end=_num(5.0), step=_num(1.0), position=_pos()) - assert str(range_lit) == "[0:5:1]" + # 2-arg: step shares range position (synthesised default) → omit step + range_2arg = RangeLiteral(start=_num(0.0), end=_num(5.0), step=_num(1.0), position=_pos()) + assert str(range_2arg) == "[0:5]" + + # 3-arg: step has a distinct position (explicit in source) → include step + step_pos = Position(origin="", line=1, column=5) + step_explicit = NumberLiteral(val=2.0, position=step_pos) + range_3arg = RangeLiteral(start=_num(0.0), end=_num(5.0), step=step_explicit, position=_pos()) + assert str(range_3arg) == "[0:5:2]" def test_list_comprehension_str(): diff --git a/tests/test_pretty_print.py b/tests/test_pretty_print.py index 7613118..e9792b7 100644 --- a/tests/test_pretty_print.py +++ b/tests/test_pretty_print.py @@ -350,7 +350,7 @@ def test_assert_no_child(self): class TestListCompForFormatting: def test_short_for_expands(self): out = _fmt("x = [for (i = [0:3]) i];") - assert out == "x = [\n for (i = [0:3:1])\n i\n];" + assert out == "x = [\n for (i = [0:3])\n i\n];" def test_long_for_body_on_new_line(self): out = _fmt("x = [for (long_variable_name = [start_value:step_value:end_value]) long_variable_name * scaling_factor_x];") @@ -360,7 +360,7 @@ def test_long_for_body_on_new_line(self): def test_long_for_assignments_multiline(self): out = _fmt("x = [for (very_long_variable_name_alpha = [start_value:end_value], very_long_variable_name_beta = [0:10]) very_long_variable_name_alpha];") assert "for (\n" in out - assert " very_long_variable_name_alpha = [start_value:end_value:1]," in out + assert " very_long_variable_name_alpha = [start_value:end_value]," in out assert ")\n" in out def test_nested_for(self): diff --git a/tests/test_vectors.py b/tests/test_vectors.py index 1915e2f..9d3321c 100644 --- a/tests/test_vectors.py +++ b/tests/test_vectors.py @@ -68,6 +68,39 @@ def test_range_expressions(self, parser): code = "x = [0:2*5:10];" parse_success(parser, code) + def test_range_two_arg_round_trip(self): + """Two-argument range [start:end] must not gain a spurious :1 after round-trip.""" + import tempfile + import os + from openscad_parser.ast import getASTfromFile + from openscad_parser.ast.pretty_print import to_openscad + code = "x = [for(i = [0:5]) i];" + with tempfile.NamedTemporaryFile(mode='w', suffix='.scad', delete=False) as f: + f.write(code) + fname = f.name + output = to_openscad(getASTfromFile(fname, process_includes=False)) + os.unlink(fname) + assert ":1]" not in output, f"Spurious step in output: {output!r}" + assert "[0:5]" in output + + def test_range_three_arg_round_trip(self): + """Three-argument range [start:step:end] must preserve all three arguments.""" + import tempfile + import os + from openscad_parser.ast import getASTfromFile + from openscad_parser.ast.pretty_print import to_openscad + cases = [ + ("x = [for(i = [10:-1:0]) i];", "[10:-1:0]"), + ("x = [for(i = [0:1:4]) i];", "[0:1:4]"), + ] + for src, expected in cases: + with tempfile.NamedTemporaryFile(mode='w', suffix='.scad', delete=False) as f: + f.write(src) + fname = f.name + output = to_openscad(getASTfromFile(fname, process_includes=False)) + os.unlink(fname) + assert expected in output, f"{src!r} → missing {expected!r} in {output!r}" + class TestListComprehensionFor: """Test list comprehension with for."""