feat: add monkey-patches for upstream bugs in openscad_parser

openscad_packer/__init__.py

@@ -13,3 +13,5 @@
 # You should have received a copy of the GNU Affero General Public License
 # along with this program.  If not, see <https://gnu.org>.
 
+from . import patches as _patches  # noqa: F401  apply upstream workarounds
+

openscad_packer/patches.py

@@ -0,0 +1,87 @@
+# Copyright (C) 2026  Torgny Bjers
+#
+# This program is free software: you can redistribute it and/or modify
+# it under the terms of the GNU Affero General Public License as published
+# by the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU Affero General Public License for more details.
+#
+# You should have received a copy of the GNU Affero General Public License
+# along with this program.  If not, see <https://gnu.org>.
+
+"""Workarounds for two upstream openscad-parser bugs.
+
+These patches are applied once at import time by monkey-patching the affected
+classes.  Both bugs have upstream PRs filed.
+
+Bug 1 — Empty-string literals rendered as ``"" | ""``
+------------------------------------------------------
+Root cause: Arpeggio's ``RegExMatch._parse`` uses ``if matched:`` to decide
+whether to return a Terminal node.  An empty-string regex match sets
+``matched = ''``, which is falsy, so the Terminal is **not** returned.  The
+``string_contents`` rule (which wraps the regex) therefore produces no child
+node for empty strings, leaving ``visit_string_literal`` with ``children=[]``.
+The fallback branch calls ``str(node.value)``, and ``NonTerminal.value``
+calls ``NonTerminal.__str__``, which joins its children (the two ``"``
+Terminals) with `` | `` — producing ``'" | "'``.  ``StringLiteral.__str__``
+then wraps that in quotes, giving ``"" | ""``.
+
+Fix: when ``children`` is empty inside ``visit_string_literal``, return
+``StringLiteral(val="")`` directly instead of falling back to
+``str(node.value)``.
+
+Bug 2 — Two-argument ranges rendered with wrong step order
+----------------------------------------------------------
+Root cause: ``RangeLiteral.__str__`` emits ``[start:end:step]``.  For a
+three-argument source range ``[s:p:e]`` (OpenSCAD syntax: start : step : end)
+the builder stores ``end=p`` and ``step=e``, so the output ``[s:p:e]`` is
+accidentally correct.  But for a *two*-argument range ``[s:e]``, the builder
+synthesises a default ``step=NumberLiteral(1.0)`` tagged with the **range
+node's own position**, then stores it in the ``step`` field.  The output
+``[s:e:1]`` is read by OpenSCAD as start=s, step=e, end=1 — wrong semantics.
+
+Fix: inside ``RangeLiteral.__str__``, detect the synthesised default by
+comparing ``self.step.position == self.position`` (both were created from the
+same Arpeggio node position).  When they match, emit ``[start:end]``; when
+they differ (explicit three-argument range), emit ``[start:end:step]`` as
+before (the internal field swap is still present but cancels out).
+"""
+from __future__ import annotations
+
+from openscad_parser.ast.builder import ASTBuilderVisitor
+from openscad_parser.ast.nodes import RangeLiteral, StringLiteral
+
+
+def _patch_empty_string() -> None:
+    _orig = ASTBuilderVisitor.visit_string_literal
+
+    def visit_string_literal(self, node, children):  # type: ignore[override]
+        if not children:
+            # Arpeggio dropped the zero-length string_contents Terminal because
+            # RegExMatch._parse uses ``if matched:`` (falsy for '').  Return an
+            # empty string rather than falling back to str(node.value), which
+            # produces the broken Arpeggio ' | '-joined representation.
+            return StringLiteral(val="", position=self._get_node_position(node))
+        return _orig(self, node, children)
+
+    ASTBuilderVisitor.visit_string_literal = visit_string_literal
+
+
+def _patch_range_str() -> None:
+    def __str__(self: RangeLiteral) -> str:  # type: ignore[override]
+        # A synthesised default step (for 2-arg ranges) is given the range
+        # node's own position by visit_range_expr.  An explicit third argument
+        # has its own distinct source position.  Use this to tell them apart.
+        if self.step.position == self.position:
+            return f"[{self.start}:{self.end}]"
+        return f"[{self.start}:{self.end}:{self.step}]"
+
+    RangeLiteral.__str__ = __str__
+
+
+_patch_empty_string()
+_patch_range_str()

tests/test_upstream_patches.py

@@ -0,0 +1,236 @@
+"""Regression tests for workarounds to upstream openscad-parser bugs.
+
+These tests verify that openscad_packer.patches correctly fixes two
+round-trip bugs present in the openscad-parser package.  See the docstring
+in openscad_packer/patches.py for full root-cause analysis.
+
+Coverage areas
+--------------
+* Bug 1 (empty-string literal) — empty "" round-trips cleanly, not as "" | ""
+* Bug 2 (two-argument range)   — [s:e] round-trips as [s:e], not [s:e:1]
+* Integration                  — BOSL-style patterns that triggered both bugs
+"""
+from __future__ import annotations
+
+from pathlib import Path
+
+from openscad_parser.ast import getASTfromFile
+from openscad_parser.ast.pretty_print import to_openscad
+
+from openscad_packer.packer import Packer
+
+
+# ---------------------------------------------------------------------------
+# Helpers
+# ---------------------------------------------------------------------------
+
+def parse_and_print(path: Path, src: str) -> str:
+    """Write *src* to *path*, parse it, and pretty-print back to string."""
+    path.write_text(src, encoding="utf-8")
+    nodes = getASTfromFile(str(path), process_includes=False) or []
+    return to_openscad(nodes)
+
+
+def pack(entry: Path, library_paths: list[str] | None = None) -> str:
+    return Packer(str(entry), library_paths or []).pack()
+
+
+def write(directory: Path, name: str, content: str) -> Path:
+    f = directory / name
+    f.write_text(content, encoding="utf-8")
+    return f
+
+
+# ---------------------------------------------------------------------------
+# Bug 1: empty-string literals
+# ---------------------------------------------------------------------------
+
+class TestEmptyStringLiteral:
+    """Empty string "" must round-trip as "" not "" | ""."""
+
+    def test_empty_string_assignment(self, tmp_path):
+        out = parse_and_print(tmp_path / "t.scad", 'x = "";')
+        assert '""' in out
+        assert '"" | ""' not in out
+
+    def test_empty_string_equality(self, tmp_path):
+        src = 'function is_empty(v) = v == "";'
+        out = parse_and_print(tmp_path / "t.scad", src)
+        assert '"" | ""' not in out
+        assert 'v ==' in out
+        assert '""' in out
+
+    def test_empty_string_in_logical_or(self, tmp_path):
+        # Mimics BOSL compat.scad is_str pattern:
+        # function is_str(v) = v=="" || v[0]!=undef;
+        src = 'function is_str(v) = v=="" || v[0]!=undef;'
+        out = parse_and_print(tmp_path / "t.scad", src)
+        assert '"" | ""' not in out
+        assert '""' in out
+        assert '||' in out
+
+    def test_empty_string_in_ternary(self, tmp_path):
+        src = 'x = (v=="") ? "yes" : "no";'
+        out = parse_and_print(tmp_path / "t.scad", src)
+        assert '"" | ""' not in out
+
+    def test_multiple_empty_strings(self, tmp_path):
+        src = 'x = (a=="" && b=="") ? 1 : 0;'
+        out = parse_and_print(tmp_path / "t.scad", src)
+        assert '"" | ""' not in out
+        # Both empty strings should appear
+        assert out.count('""') >= 2
+
+    def test_non_empty_string_unaffected(self, tmp_path):
+        src = 'x = "hello";'
+        out = parse_and_print(tmp_path / "t.scad", src)
+        assert '"hello"' in out
+
+    def test_empty_string_round_trip_via_packer(self, tmp_path):
+        write(tmp_path, "lib.scad", 'function isempty(v) = v == "";')
+        entry = write(tmp_path, "entry.scad", 'use <lib.scad>\ny = isempty("test");')
+        result = pack(entry)
+        assert '"" | ""' not in result
+        assert '""' in result
+        assert 'isempty' in result
+
+
+# ---------------------------------------------------------------------------
+# Bug 2: two-argument range literals
+# ---------------------------------------------------------------------------
+
+class TestRangeLiteral:
+    """Two-arg ranges [s:e] must round-trip as [s:e], not [s:e:1]."""
+
+    def test_two_arg_range_integer(self, tmp_path):
+        src = "x = [for (i=[0:2]) i];"
+        out = parse_and_print(tmp_path / "t.scad", src)
+        assert "[0:2]" in out
+        assert "[0:2:1]" not in out
+
+    def test_two_arg_range_variable(self, tmp_path):
+        src = "x = [for (i=[0:len(v)-1]) v[i]];"
+        out = parse_and_print(tmp_path / "t.scad", src)
+        # The key symptom was [0:len(v1) - 1:1] — the :1 should not appear
+        assert ":1]" not in out
+
+    def test_two_arg_range_negative(self, tmp_path):
+        src = "x = [for (i=[-3:3]) i];"
+        out = parse_and_print(tmp_path / "t.scad", src)
+        assert "[-3:3]" in out
+        assert "[-3:3:1]" not in out
+
+    def test_three_arg_range_explicit_step_preserved(self, tmp_path):
+        # [0:1:2] in OpenSCAD means start=0, step=1, end=2
+        src = "x = [for (i=[0:1:2]) i];"
+        out = parse_and_print(tmp_path / "t.scad", src)
+        assert "[0:1:2]" in out
+
+    def test_three_arg_range_large_step_preserved(self, tmp_path):
+        # [0:5:1] means start=0, step=5, end=1 — semantically different from [0:5]
+        src = "x = [for (i=[0:5:1]) i];"
+        out = parse_and_print(tmp_path / "t.scad", src)
+        assert "[0:5:1]" in out
+        assert "[0:5]" not in out
+
+    def test_three_arg_range_negative_step_preserved(self, tmp_path):
+        # [10:0:-1] means start=10, step=0, end=-1 in internal storage
+        src = "x = [for (i=[10:-1:0]) i];"
+        out = parse_and_print(tmp_path / "t.scad", src)
+        assert "[10:-1:0]" in out
+
+    def test_three_arg_range_float_step_preserved(self, tmp_path):
+        src = "x = [for (i=[0:0.5:2]) i];"
+        out = parse_and_print(tmp_path / "t.scad", src)
+        assert "[0:0.5:2]" in out
+
+    def test_two_arg_range_in_for_loop_module(self, tmp_path):
+        src = "module m() { for (i=[0:3]) { cube(i); } }"
+        out = parse_and_print(tmp_path / "t.scad", src)
+        assert "[0:3]" in out
+        assert "[0:3:1]" not in out
+
+    def test_two_arg_range_round_trip_via_packer(self, tmp_path):
+        write(tmp_path, "lib.scad",
+              "function vsum(v) = [for (i=[0:len(v)-1]) v[i]];")
+        entry = write(tmp_path, "entry.scad",
+                      "use <lib.scad>\nx = vsum([1,2,3]);")
+        result = pack(entry)
+        # Should not have the broken :1] suffix on the 2-arg range
+        assert ":1]" not in result
+        assert "vsum" in result
+
+
+# ---------------------------------------------------------------------------
+# Integration: BOSL-style patterns
+# ---------------------------------------------------------------------------
+
+class TestBOSLPatterns:
+    """Patterns drawn from revarbat/BOSL that triggered both bugs."""
+
+    def test_is_str_pattern(self, tmp_path):
+        # Mimics BOSL compat.scad is_str — uses v=="" and || together
+        src = (
+            'function is_str(v) = '
+            '(version_num() > 20190100) ? is_string(v) : '
+            '(v=="" || (v[0]!=undef));'
+        )
+        out = parse_and_print(tmp_path / "t.scad", src)
+        assert '"" | ""' not in out
+        assert '""' in out
+        assert '||' in out
+
+    def test_vmul_pattern(self, tmp_path):
+        # Mimics BOSL math.scad vmul — uses 2-arg range [0:len(v1)-1]
+        src = "function vmul(v1, v2) = [for (i=[0:len(v1)-1]) v1[i]*v2[i]];"
+        out = parse_and_print(tmp_path / "t.scad", src)
+        assert "[0:len(v1) - 1]" in out
+        assert ":1]" not in out
+
+    def test_point2d_pattern(self, tmp_path):
+        # Mimics BOSL math.scad point2d — uses 2-arg range [0:1]
+        src = "function point2d(p) = [for (i=[0:1]) p[i]==undef ? 0 : p[i]];"
+        out = parse_and_print(tmp_path / "t.scad", src)
+        assert "[0:1]" in out
+        assert "[0:1:1]" not in out
+
+    def test_point3d_pattern(self, tmp_path):
+        # Mimics BOSL math.scad point3d — uses 2-arg range [0:2]
+        src = "function point3d(p) = [for (i=[0:2]) p[i]==undef ? 0 : p[i]];"
+        out = parse_and_print(tmp_path / "t.scad", src)
+        assert "[0:2]" in out
+        assert "[0:2:1]" not in out
+
+    def test_scalar_vec3_pattern(self, tmp_path):
+        # Mimics BOSL compat.scad scalar_vec3 — both bugs potentially triggered
+        src = (
+            "function default(v, dflt) = v==undef ? dflt : v;\n"
+            "function scalar_vec3(v, dflt) =\n"
+            "  !is_def(v) ? undef :\n"
+            "  is_array(v) ? [for (i=[0:2]) default(v[i], default(dflt, 0))] :\n"
+            "  is_def(dflt) ? [v,dflt,dflt] : [v,v,v];\n"
+        )
+        out = parse_and_print(tmp_path / "t.scad", src)
+        assert "[0:2]" in out
+        assert "[0:2:1]" not in out
+
+    def test_bosl_compat_mini_via_packer(self, tmp_path):
+        """Packs a mini BOSL compat subset and verifies no corruption."""
+        write(tmp_path, "compat.scad", """\
+function is_def(v) = v != undef;
+function is_str(v) = v=="" || (is_def(v) && is_def(v[0]));
+function vmul(v1, v2) = [for (i=[0:len(v1)-1]) v1[i]*v2[i]];
+function point3d(p) = [for (i=[0:2]) p[i]==undef ? 0 : p[i]];
+""")
+        entry = write(tmp_path, "entry.scad", """\
+use <compat.scad>
+a = vmul([1,2,3],[4,5,6]);
+b = point3d([1,2]);
+c = is_str("hello");
+""")
+        result = pack(entry)
+        assert '"" | ""' not in result
+        assert ":1]" not in result
+        assert "vmul" in result
+        assert "point3d" in result
+        assert "is_str" in result