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"; });