From 65c902b825688777ef389e3830e77d7d3e9e5390 Mon Sep 17 00:00:00 2001 From: Revar Desmera Date: Mon, 7 Sep 2026 19:22:15 -0700 Subject: [PATCH] Expose the parser's strict-comma mode: --strict-commas, strict_commas= Evaluator half of BelfrySCAD#362. Bumps the parser submodule to v1.2.0 (openscad_cpp_parser#9), which added StrictCommaScope, and hands it to the three places that can ask for it: openscad-cpp-evaluator --strict-commas Evaluator.evaluate(path, vp, strict_commas=True) (Python) oscad::StrictCommaScope (C++, from the parser) A trailing comma in a CALL ARGUMENT list or a let/for ASSIGNMENT list becomes a syntax error, as OpenSCAD 2021.01 had it. A list literal's and a parameter declaration's are untouched: 2021.01 accepted both, and rejecting them would fail files it loads. See the parser PR for the measured table. The scope is constructed on the thread that parses, not set from Python before the call. It is thread-local in the parser, and a host evaluating on a worker thread -- which BelfrySCAD does for every render -- would otherwise set the mode on the main thread and parse on another, silently getting nothing. Costs nothing here and removes the trap outright. Two binding tests, because the facade and the nanobind binding restate the same parameter list by hand and the LAST argument added this way was silently dropped by the facade (see test_export_model_facade_accepts_split_components, added when that happened): one that the flag reaches the parser at all, one that it does not leak into the next evaluation. 1830 C++ tests and 34 Python binding tests pass. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01AzS79GLeG7LpYB6KjgDbi1 --- bindings/module.cpp | 15 +++++++-- external/openscad_cpp_parser | 2 +- pyproject.toml | 2 +- python/openscad_cpp_evaluator/__init__.py | 12 ++++++-- tests/test_python_bindings.py | 37 +++++++++++++++++++++++ tools/cli/cli_lib.cpp | 10 +++++- 6 files changed, 70 insertions(+), 8 deletions(-) diff --git a/bindings/module.cpp b/bindings/module.cpp index 2e3b47f..3d6b516 100644 --- a/bindings/module.cpp +++ b/bindings/module.cpp @@ -375,7 +375,7 @@ nb::list exportModelPy(const std::string& path, const Geometry& geom, const std: nb::object evaluate(const std::string& path, nb::dict viewportParams, std::shared_ptr manifoldCache, bool profile, - bool generate) { + bool generate, bool strictCommas) { std::unordered_map vp = toViewportParams(viewportParams); std::vector bodies; @@ -389,6 +389,12 @@ nb::object evaluate(const std::string& path, nb::dict viewportParams, nb::gil_scoped_release rel; auto logFn = [&echoes](const std::string& m) { echoes.push_back(m); }; try { + // Constructed on the parsing thread, which is the one whose + // thread-local mode StrictCommaScope sets -- a flag flipped from + // Python before the call would sit on the wrong thread the + // moment a host evaluates off the main one. + std::optional strict; + if (strictCommas) strict.emplace(); oscad::ParsedProgram program = oscad::getProgramFromFile(path); oscadeval::ResolvedUseScopes used = oscadeval::resolveUseScopes(program.nodes, path, logFn); oscadeval::Evaluator ev(logFn, nullptr, manifoldCache, oscadeval::DebugHooks{}, profile); @@ -801,11 +807,14 @@ NB_MODULE(_openscad_cpp_evaluator, m) { "rather than restated here, so the Python side cannot drift from what can actually be written."); m.def("evaluate", &evaluate, nb::arg("path"), nb::arg("viewport_params"), nb::arg("manifold_cache") = nullptr, - nb::arg("profile") = false, nb::arg("generate") = true, + nb::arg("profile") = false, nb::arg("generate") = true, nb::arg("strict_commas") = false, "Evaluate a .scad file; return (bodies, echoes, id_to_node, csg_tree, profile_result, dyn, dyn_explicit).\n" "generate=False stops after the resolve pass: the script runs and reports everything " "it normally would, but no Manifold geometry is built, and neither bodies nor csg_tree " - "are populated."); + "are populated.\n\n" + "strict_commas=True makes a trailing comma in a call argument list or a let/for " + "assignment list a syntax error, as OpenSCAD 2021.01 did. List literals and parameter " + "declarations keep theirs, which 2021.01 accepted too."); m.def("parse_decls", &parseDecls, nb::arg("path"), "Parse a .scad file; return top-level declaration (namespace, name, start, end, line, column, origin) tuples."); m.def("strip_slivers", &stripSliversPy, nb::arg("verts"), nb::arg("tris"), diff --git a/external/openscad_cpp_parser b/external/openscad_cpp_parser index a62a7ea..0a33377 160000 --- a/external/openscad_cpp_parser +++ b/external/openscad_cpp_parser @@ -1 +1 @@ -Subproject commit a62a7ea8cd197949bb539584119ac8375e85c7a9 +Subproject commit 0a33377d93daccb89410bebd6d1a49c2dcae7d7c diff --git a/pyproject.toml b/pyproject.toml index 632b2d3..ae12ffd 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "scikit_build_core.build" [project] name = "openscad_cpp_evaluator" -version = "1.8.0" +version = "1.9.0" description = "C++ OpenSCAD evaluator with Python bindings" readme = "README.md" requires-python = ">=3.12" diff --git a/python/openscad_cpp_evaluator/__init__.py b/python/openscad_cpp_evaluator/__init__.py index 8deeafb..01aff78 100644 --- a/python/openscad_cpp_evaluator/__init__.py +++ b/python/openscad_cpp_evaluator/__init__.py @@ -497,7 +497,7 @@ def __init__(self, echo_fn=None, debug_hook=None, error_break_fn=None, return_ho self.dyn_explicit = set() def evaluate(self, source_path: str, viewport_params: Optional[dict] = None, - generate: bool = True): + generate: bool = True, strict_commas: bool = False): """Run `source_path` and return (bodies, id_to_node). `generate=False` stops after the resolve pass. The script still runs @@ -508,6 +508,13 @@ def evaluate(self, source_path: str, viewport_params: Optional[dict] = None, the geometry without skipping the language. Ignored on the debugger path, which has no geometry handle anyway. + + `strict_commas=True` makes a trailing comma in a call argument list + or a let/for assignment list a syntax error, as OpenSCAD 2021.01 did + -- `cube(1,)` and `let(x = 1,)`, but NOT `[2, 4,]` or + `module m(a, b,)`, which 2021.01 accepted. For checking a script + against that version; off, and no divergence from current OpenSCAD, + otherwise. Also ignored on the debugger path. """ vp = viewport_params or {} try: @@ -528,7 +535,8 @@ def evaluate(self, source_path: str, viewport_params: Optional[dict] = None, else: (body_dicts, echoes, id_spans, csg_tree, profile_result, dyn, dyn_explicit, geometry) = _ext.evaluate( - source_path, vp, self._manifold_cache, self._profile, generate) + source_path, vp, self._manifold_cache, self._profile, generate, + strict_commas) # The evaluated bodies, still on the C++ side. Stashed like # csg_tree/profile_result rather than returned, so # evaluate()'s own 2-tuple result is unchanged -- callers diff --git a/tests/test_python_bindings.py b/tests/test_python_bindings.py index 6417084..b145f3c 100644 --- a/tests/test_python_bindings.py +++ b/tests/test_python_bindings.py @@ -659,3 +659,40 @@ def objects_in(path, **kwargs): # Default matches OpenSCAD: one object however many pieces. assert objects_in(tmp_path / "joined.3mf") == 1 assert objects_in(tmp_path / "split.3mf", split_components=True) == 3 + + +def test_strict_commas_reaches_the_parser(tmp_path): + """Evaluator.evaluate(strict_commas=True) must survive both hand-written + parameter lists -- the facade's and the nanobind binding's. The last + argument added this way was dropped by the facade and no C++ test could + see it (see test_export_model_facade_accepts_split_components).""" + import pytest + from openscad_cpp_evaluator import Evaluator, EvalError + + rejected = tmp_path / "call.scad" + rejected.write_text("cube(1,);\n") + accepted = tmp_path / "list.scad" + accepted.write_text("a = [2, 4,];\ncube(a[0]);\n") + + # Off by default: both parse, as they do in current OpenSCAD. + Evaluator(echo_fn=lambda _m: None).evaluate(str(rejected), {}) + Evaluator(echo_fn=lambda _m: None).evaluate(str(accepted), {}) + + # On: a call's trailing comma is a syntax error, a list literal's is not + # -- 2021.01 accepted the list. + with pytest.raises(EvalError): + Evaluator(echo_fn=lambda _m: None).evaluate(str(rejected), {}, strict_commas=True) + Evaluator(echo_fn=lambda _m: None).evaluate(str(accepted), {}, strict_commas=True) + + +def test_strict_commas_does_not_leak_between_evaluations(tmp_path): + """The parser mode is a scope object; a strict parse must not leave it on + for the next caller.""" + import pytest + from openscad_cpp_evaluator import Evaluator, EvalError + + src = tmp_path / "call.scad" + src.write_text("cube(1,);\n") + with pytest.raises(EvalError): + Evaluator(echo_fn=lambda _m: None).evaluate(str(src), {}, strict_commas=True) + Evaluator(echo_fn=lambda _m: None).evaluate(str(src), {}) # must not raise diff --git a/tools/cli/cli_lib.cpp b/tools/cli/cli_lib.cpp index 8ef4130..3f00da9 100644 --- a/tools/cli/cli_lib.cpp +++ b/tools/cli/cli_lib.cpp @@ -222,6 +222,7 @@ int runCli(const std::vector& args, std::istream& in, std::ostream& std::string profileMinSelfStr; std::string profileMinCallsStr; bool debug = false; + bool strictCommas = false; for (size_t i = 0; i < args.size(); ++i) { const std::string& arg = args[i]; if (arg == "-o" && i + 1 < args.size()) { @@ -242,6 +243,8 @@ int runCli(const std::vector& args, std::istream& in, std::ostream& profileMinCallsStr = args[++i]; } else if (arg == "--debug") { debug = true; + } else if (arg == "--strict-commas") { + strictCommas = true; } else if (inputPath.empty()) { inputPath = arg; } @@ -249,7 +252,7 @@ int runCli(const std::vector& args, std::istream& in, std::ostream& if (inputPath.empty() || outputPath.empty()) { err << "usage: openscad-cpp-evaluator -o [--format stl|obj|off|3mf|ply|wrl|x3d] [--ascii-stl] " "[--profile FILENAME [--profile-format text|csv] [--profile-sort self|cumulative|calls|name] " - "[--profile-min-self SECONDS] [--profile-min-calls N]] [--debug]\n"; + "[--profile-min-self SECONDS] [--profile-min-calls N]] [--debug] [--strict-commas]\n"; return 1; } const std::string fmt = formatForPath(format, outputPath); @@ -293,6 +296,11 @@ int runCli(const std::vector& args, std::istream& in, std::ostream& } try { + // Scoped to the parse: a trailing comma in a call argument list or a + // let/for assignment list is a syntax error while it is in effect, + // as OpenSCAD 2021.01 had it. See oscad::StrictCommaScope. + std::optional strict; + if (strictCommas) strict.emplace(); std::vector> ast = oscad::getASTFromFile(inputPath); ResolvedUseScopes used = resolveUseScopes(ast, inputPath, [&out](const std::string& msg) { out << msg << "\n"; });