diff --git a/openscad_packer/__init__.py b/openscad_packer/__init__.py index 98d1305..63460db 100644 --- a/openscad_packer/__init__.py +++ b/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 . +from . import patches as _patches # noqa: F401 apply upstream workarounds + diff --git a/openscad_packer/patches.py b/openscad_packer/patches.py new file mode 100644 index 0000000..79dc7d7 --- /dev/null +++ b/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 . + +"""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() diff --git a/tests/test_upstream_patches.py b/tests/test_upstream_patches.py new file mode 100644 index 0000000..518b351 --- /dev/null +++ b/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 \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 \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 +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 diff --git a/uv.lock b/uv.lock index d9678cb..aa91719 100644 --- a/uv.lock +++ b/uv.lock @@ -147,7 +147,6 @@ wheels = [ [[package]] name = "openscad-packer" -version = "2026.6.2" source = { editable = "." } dependencies = [ { name = "click" },