From e71dc2704187797f17e771c2b3f371866c46a685 Mon Sep 17 00:00:00 2001 From: Revar Desmera Date: Thu, 20 Aug 2026 21:54:01 -0700 Subject: [PATCH 1/2] polyhedron() accepts a VNF 2-list [vertices, faces] is what every BOSL2 function passes around and what obj.vnf holds, so it should go straight in rather than having to be split apart at the call site. All four of these are now the same call: polyhedron(obj); polyhedron(obj.vnf); polyhedron(obj.vertices, obj.faces); polyhedron(spheroid(d=30)); // any BOSL2 VNF Only considered when `faces` was not supplied separately, so the two-argument form always wins and can never be reinterpreted. The discriminator is BOSL2's own is_vnf test: a VNF's second element is a list of FACES, i.e. a list of lists. A points list that happens to have length 2 has a POINT there instead -- bare numbers, not lists -- so it is left alone and still errors as the invalid polyhedron it is. Two points cannot describe a polyhedron anyway, so nothing legitimate is taken over. Verified both directions against real BOSL2: polyhedron(spheroid(...)) matches the volume vnf_polyhedron() produces, and vnf_polyhedron(obj.vnf) accepts ours. Minor, not patch: new accepted argument shape. Co-Authored-By: Claude Opus 5 (1M context) --- CLAUDE.md | 14 ++++++++++++++ pyproject.toml | 2 +- src/builtins/primitives_3d.cpp | 31 ++++++++++++++++++++++++++++++ tests/test_render_expr.cpp | 35 ++++++++++++++++++++++++++++++++++ 4 files changed, 81 insertions(+), 1 deletion(-) diff --git a/CLAUDE.md b/CLAUDE.md index b334a1c..9c48283 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -1128,6 +1128,20 @@ vector and `oscEqual` is order-sensitive): `vnf` is the `[vertices, faces]` 2-list BOSL2 functions actually take. `polyhedron()` and `polygon()` also accept the object directly, and will accept *any* object carrying the right keys, not just one `render()` produced. +`polyhedron()` additionally accepts the bare `[vertices, faces]` 2-list, so a +BOSL2 VNF goes straight in — all four of these are the same call: + +```openscad +polyhedron(obj); // the object +polyhedron(obj.vnf); // the 2-list +polyhedron(obj.vertices, obj.faces); // the halves +polyhedron(spheroid(d=30)); // any BOSL2 VNF +``` + +The 2-list form is only considered when `faces` was not given separately, so the +two-argument call can never be reinterpreted. The discriminator is BOSL2's own +`is_vnf` test — a VNF's second element is a list of *lists*, where a plain points +list has a point (bare numbers) there. ### Things that will bite diff --git a/pyproject.toml b/pyproject.toml index 428f0df..bede180 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "scikit_build_core.build" [project] name = "openscad_cpp_evaluator" -version = "0.38.1" +version = "0.39.0" description = "C++ OpenSCAD evaluator with Python bindings" readme = "README.md" requires-python = ">=3.12" diff --git a/src/builtins/primitives_3d.cpp b/src/builtins/primitives_3d.cpp index 0124308..1c2f544 100644 --- a/src/builtins/primitives_3d.cpp +++ b/src/builtins/primitives_3d.cpp @@ -630,6 +630,37 @@ CSGParams resolvePolyhedron(Evaluator& ev, const oscad::ModularCall& node, EvalC facesArg = std::move(newFaces); } + // polyhedron(vnf) -- BOSL2's [vertices, faces] 2-list, which is what + // every BOSL2 function passes around and what obj.vnf holds. Only + // considered when `faces` was not given separately, so the two-argument + // form always wins and can never be reinterpreted. + // + // The discriminator is BOSL2's own is_vnf test: a VNF's second element + // is a list of FACES, i.e. a list of lists. A plain points list of + // length 2 has a point there instead -- bare numbers, not lists -- and + // is left alone. (Two points cannot describe a polyhedron anyway, so + // nothing legitimate is being taken over.) + if (isUndef(facesArg)) { + if (const ListPtr* outer = std::get_if(&pointsArg); outer && *outer && + (*outer)->items.size() == 2) { + const ListPtr* maybeVerts = std::get_if(&(*outer)->items[0]); + const ListPtr* maybeFaces = std::get_if(&(*outer)->items[1]); + const bool facesAreLists = + maybeFaces && *maybeFaces && !(*maybeFaces)->items.empty() && + std::holds_alternative((*maybeFaces)->items[0]); + const bool vertsArePoints = + maybeVerts && *maybeVerts && !(*maybeVerts)->items.empty() && + std::holds_alternative((*maybeVerts)->items[0]); + if (facesAreLists && vertsArePoints) { + // Copy before assigning: both borrow into pointsArg's list. + Value newPoints = (*outer)->items[0]; + Value newFaces = (*outer)->items[1]; + pointsArg = std::move(newPoints); + facesArg = std::move(newFaces); + } + } + } + const ListPtr* pointsList = std::get_if(&pointsArg); const ListPtr* facesList = std::get_if(&facesArg); if (!pointsList || !*pointsList || !facesList || !*facesList) { diff --git a/tests/test_render_expr.cpp b/tests/test_render_expr.cpp index b71e42c..c61d436 100644 --- a/tests/test_render_expr.cpp +++ b/tests/test_render_expr.cpp @@ -459,3 +459,38 @@ TEST(RenderExpr, XorChainMatchesTheSameGeometryBuiltDirectly) { ASSERT_EQ(r.echoes.size(), 1u); EXPECT_EQ(r.echoes[0], "ECHO: 3804.65"); } + +// -- polyhedron() accepts a VNF 2-list ------------------------------------ +// +// [vertices, faces] is what every BOSL2 function passes around and what +// obj.vnf holds, so it should go straight in without being split apart. + +TEST(RenderExpr, PolyhedronAcceptsAVnfTwoList) { + Measured r = runScript( + "o = render() { difference(){ cube(20,center=true); " + "cylinder(d=8,h=40,center=true,$fn=16); } };\n" + "a = render() { polyhedron(o.vnf); };\n" + "b = render() { polyhedron(o.vertices, o.faces); };\n" + "c = render() { polyhedron(o); };\n" + "echo(a.volume == b.volume, b.volume == c.volume, a.genus == c.genus);"); + ASSERT_EQ(r.echoes.size(), 1u); + EXPECT_EQ(r.echoes[0], "ECHO: true, true, true"); +} + +TEST(RenderExpr, ExplicitFacesAlwaysWinOverVnfInterpretation) { + // The 2-list form is only considered when `faces` was not supplied, so + // the two-argument call can never be reinterpreted. + Measured r = runScript("o = render() { cube(4); };\n" + "x = render() { polyhedron(points=o.vertices, faces=o.faces); };\n" + "echo(x.volume);"); + ASSERT_EQ(r.echoes.size(), 1u); + EXPECT_EQ(r.echoes[0], "ECHO: 64"); +} + +TEST(RenderExpr, TwoPointListIsNotMistakenForAVnf) { + // A points list that happens to have length 2 has a POINT as its second + // element -- bare numbers, not lists -- so it must not be taken over. + // It is still not a valid polyhedron, so it errors rather than silently + // building something. + EXPECT_THROW(evalSrc("polyhedron([[0,0,0],[1,1,1]]);"), std::exception); +} From 1f6c616c735550233b238c5728e5d61abdf37069 Mon Sep 17 00:00:00 2001 From: Revar Desmera Date: Fri, 21 Aug 2026 03:04:13 -0700 Subject: [PATCH 2/2] object(): a single-element entry deletes that key a = object(a=42, b=53, c=8); b = object(a, [["d", 18], ["b"]]); // -> { a = 42; c = 8; d = 18; } An unnamed list argument holds entries, and [key, value] SETS while the single-element [key] DELETES. Only the two-element form was implemented, so any script using a delete got undef for the whole call -- silently, with no diagnostic pointing at why. Checking the rest of the semantics against OpenSCAD 2026.02.01 turned up more than the reported gap: - deleting an absent key must be a silent no-op (was: undef) - every malformed entry must WARN as well as yield undef (was: silent) Deletion removes the key outright rather than blanking it, which is observable because ValueObject is insertion-ordered and oscEqual is order-sensitive: object(a, [["b"], ["b", 99]]) puts b LAST, while object(a, [["b", 99], ["b"]]) has no b at all. Both are pinned. The warning text is quoted verbatim from the reference, including two of its own inconsistencies -- the "not a list" case puts spaces inside its parens where every other case does not, and the "unnamed argument" case ends with a trailing space. Reproducing them is the point: these strings are what a user greps for. All 19 cases (values AND warnings) were diffed character-for-character against the reference binary with --enable=object-function; the diff is empty. mergeObjectArgs gained an Evaluator and a position so it can warn, which both call sites -- the interpreter's builtinObject and the VM's Op::CallFn -- now pass through. 967 tests pass under both engines. Co-Authored-By: Claude Opus 5 (1M context) --- .../function_builtins.hpp | 6 +- src/builtins/function_builtins.cpp | 103 ++++++++++++++--- src/bytecode_vm.cpp | 2 +- src/user_calls.cpp | 2 +- tests/test_expr_eval.cpp | 106 ++++++++++++++++++ 5 files changed, 201 insertions(+), 18 deletions(-) diff --git a/include/openscad_cpp_evaluator/function_builtins.hpp b/include/openscad_cpp_evaluator/function_builtins.hpp index 2bb47ab..b94f6ab 100644 --- a/include/openscad_cpp_evaluator/function_builtins.hpp +++ b/include/openscad_cpp_evaluator/function_builtins.hpp @@ -41,7 +41,8 @@ Value evalBuiltinFunction(Evaluator& ev, const std::string& name, const CallArgs // evaluated exactly once (resolving into a CallArgs first and then // re-evaluating from the raw list here would run every argument expression // twice -- wrong for anything with a side effect, e.g. rands()). -Value builtinObject(Evaluator& ev, const std::vector>& arguments, EvalContext& ctx); +Value builtinObject(Evaluator& ev, const std::vector>& arguments, EvalContext& ctx, + const oscad::ASTNode& node); // The shared merge core builtinObject wraps: given ALREADY-EVALUATED // (name-or-nullopt, Value) pairs in exact call-site order, merges them the @@ -52,6 +53,7 @@ Value builtinObject(Evaluator& ev, const std::vector, Value>>& evaluated); +Value mergeObjectArgs(Evaluator& ev, const std::vector, Value>>& evaluated, + const oscad::Position* pos); } // namespace oscadeval diff --git a/src/builtins/function_builtins.cpp b/src/builtins/function_builtins.cpp index eddb32c..dae654b 100644 --- a/src/builtins/function_builtins.cpp +++ b/src/builtins/function_builtins.cpp @@ -454,7 +454,33 @@ bool isBuiltinFunctionName(const std::string& name) { return names.count(name) > 0; } -Value mergeObjectArgs(const std::vector, Value>>& evaluated) { +// object(...) argument merging, matching the reference's own semantics and +// diagnostics (Builtins.cc's builtin_object). +// +// An unnamed argument is either another object (its keys are merged in) or +// a LIST of entries, where each entry is: +// [key, value] -- set (or overwrite) that key +// [key] -- DELETE that key +// +// The single-element delete form is the part that is easy to miss. Deleting +// removes the key outright rather than blanking it, so a later re-set +// appends at the end: object(a, [["b"], ["b", 99]]) puts b last, while +// object(a, [["b", 99], ["b"]]) has no b at all. That ordering is +// observable -- ValueObject is insertion-ordered and oscEqual is +// order-sensitive. +// +// Deleting a key that is not there is a silent no-op, as it is upstream. +// Every malformed entry warns and abandons the whole call (returning undef), +// stopping at the first one. The warning text is quoted verbatim from the +// reference, including its own inconsistent spacing -- the "not a list" +// case really does put spaces inside the parens where the others do not, +// and the "unnamed argument" case really does end with a trailing space. +Value mergeObjectArgs(Evaluator& ev, const std::vector, Value>>& evaluated, + const oscad::Position* pos) { + static const char* kEntryRules = + " In an unnamed list, entries must be [key,value] to set or [key] to delete." + " The key must be ."; + std::vector> result; const auto setKey = [&](const std::string& k, const Value& v) { for (auto& [ek, ev2] : result) { @@ -465,30 +491,79 @@ Value mergeObjectArgs(const std::vector, Va } result.emplace_back(k, v); }; - for (const auto& [name, v] : evaluated) { + const auto deleteKey = [&](const std::string& k) { + for (auto it = result.begin(); it != result.end(); ++it) { + if (it->first == k) { + result.erase(it); + return; + } + } + // Deleting an absent key is deliberately silent. + }; + + for (size_t argIdx = 0; argIdx < evaluated.size(); ++argIdx) { + const auto& [name, v] = evaluated[argIdx]; if (name) { setKey(*name, v); continue; } + const std::string argPrefix = "object(Argument " + std::to_string(argIdx) + " "; if (const ObjectPtr* o = std::get_if(&v); o && *o) { for (const auto& [k, kv] : (*o)->items) setKey(k, kv); - } else if (const ListPtr* l = std::get_if(&v); l && *l) { - for (const Value& entry : (*l)->items) { - const ListPtr* pair = std::get_if(&entry); - if (pair && *pair && (*pair)->items.size() == 2 && std::holds_alternative((*pair)->items[0])) { - setKey(std::get((*pair)->items[0]), (*pair)->items[1]); - } else { - return Value{}; - } - } - } else if (!std::holds_alternative(v)) { + continue; + } + const ListPtr* l = std::get_if(&v); + if (!l || !*l) { + // undef is accepted and contributes nothing, as upstream. + if (std::holds_alternative(v)) continue; + ev.warn(argPrefix + "<" + oscTypeName(v) + ">) An unnamed argument must be either or" + " , it is <" + oscTypeName(v) + ">. ", + pos); return Value{}; } + for (size_t elemIdx = 0; elemIdx < (*l)->items.size(); ++elemIdx) { + const Value& entry = (*l)->items[elemIdx]; + const std::string where = "[Element " + std::to_string(elemIdx) + " "; + const ListPtr* pair = std::get_if(&entry); + if (!pair || !*pair) { + // Note the spaces inside the parens: upstream's own quirk. + ev.warn("object( Argument " + std::to_string(argIdx) + " " + where + "<" + oscTypeName(entry) + + ">] ) Entry type is not a list, it is <" + oscTypeName(entry) + ">." + kEntryRules, + pos); + return Value{}; + } + const size_t n = (*pair)->items.size(); + if (n == 0) { + ev.warn(argPrefix + where + "[]]) Entry is empty." + kEntryRules, pos); + return Value{}; + } + if (n > 2) { + ev.warn(argPrefix + where + "[...]]) Entry length is " + std::to_string(n) + + ", must be 1 [key] or 2 [key,value]." + kEntryRules, + pos); + return Value{}; + } + const Value& key = (*pair)->items[0]; + if (!std::holds_alternative(key)) { + const std::string shape = n == 2 ? "[<" + oscTypeName(key) + ">,value]" + : "[<" + oscTypeName(key) + ">]"; + ev.warn(argPrefix + where + shape + "]) The key of the entry is not but <" + + oscTypeName(key) + ">." + kEntryRules, + pos); + return Value{}; + } + if (n == 2) { + setKey(std::get(key), (*pair)->items[1]); + } else { + deleteKey(std::get(key)); + } + } } return Value{std::make_shared(ValueObject{std::move(result)})}; } -Value builtinObject(Evaluator& ev, const std::vector>& arguments, EvalContext& ctx) { +Value builtinObject(Evaluator& ev, const std::vector>& arguments, EvalContext& ctx, + const oscad::ASTNode& node) { std::vector, Value>> evaluated; evaluated.reserve(arguments.size()); for (const auto& argPtr : arguments) { @@ -499,7 +574,7 @@ Value builtinObject(Evaluator& ev, const std::vector, Value>> pairs; pairs.reserve(argCount); for (size_t i = 0; i < argCount; ++i) pairs.emplace_back(site.argNames[i], std::move(args[i])); - f.stack.push_back(mergeObjectArgs(pairs)); + f.stack.push_back(mergeObjectArgs(ev, pairs, &site.callNode->position())); ++f.pc; } else if (site.isBuiltin) { CallArgs callArgs = buildCallArgs(site, args, argCount); diff --git a/src/user_calls.cpp b/src/user_calls.cpp index ffd6a47..0f6aa29 100644 --- a/src/user_calls.cpp +++ b/src/user_calls.cpp @@ -886,7 +886,7 @@ Value Evaluator::evalFunctionCall(const oscad::PrimaryCall& node, EvalContext& c // call-site interleaved order (see builtinObject's own // comment) -- called directly, before resolveArgs, so // arguments aren't evaluated twice. - if (leftId->name == "object") return builtinObject(*this, node.arguments, ctx); + if (leftId->name == "object") return builtinObject(*this, node.arguments, ctx, node); CallArgs args = resolveArgs(*this, node.arguments, ctx); return evalBuiltinFunction(*this, leftId->name, args, node); } diff --git a/tests/test_expr_eval.cpp b/tests/test_expr_eval.cpp index 6aa9d50..befcff0 100644 --- a/tests/test_expr_eval.cpp +++ b/tests/test_expr_eval.cpp @@ -511,6 +511,112 @@ TEST(ExprEvalIndexing, ObjectIndexingByKey) { EXPECT_TRUE(std::holds_alternative(evalSrc("object(a=1)[\"nope\"]", ev))); } +// -- object() entry lists ------------------------------------------------- +// +// An unnamed list argument holds entries: [key, value] SETS, and the +// single-element [key] DELETES. Every value and every warning below was +// diffed character-for-character against OpenSCAD 2026.02.01 running with +// --enable=object-function. + +namespace { + +// Renders an object as its key list, "a,c,d" -- enough to pin both which +// keys survived and their ORDER, which is observable (ValueObject is +// insertion-ordered and oscEqual is order-sensitive). +std::string keysOf(const Value& v) { + const ObjectPtr* o = std::get_if(&v); + if (!o || !*o) return ""; + std::string out; + for (const auto& [k, _] : (*o)->items) { + if (!out.empty()) out += ","; + out += k; + } + return out; +} + +} // namespace + +TEST(ExprEvalObject, SingleElementEntryDeletesTheKey) { + Evaluator ev; + EXPECT_EQ(keysOf(evalSrc("object(object(a=42,b=53,c=8), [[\"d\",18],[\"b\"]])", ev)), "a,c,d"); + EXPECT_EQ(keysOf(evalSrc("object(object(a=42,b=53,c=8), [[\"b\"]])", ev)), "a,c"); +} + +TEST(ExprEvalObject, TwoElementEntryStillSets) { + Evaluator ev; + EXPECT_EQ(keysOf(evalSrc("object(object(a=42,b=53,c=8), [[\"d\",18]])", ev)), "a,b,c,d"); + EXPECT_DOUBLE_EQ(asNum(evalSrc("object(object(a=1), [[\"a\",9]]).a", ev)), 9.0); +} + +TEST(ExprEvalObject, DeleteRemovesRatherThanBlanks) { + // Observable through ORDER: a deleted key re-set afterwards lands at the + // end, where an overwrite would have kept its original position. + Evaluator ev; + EXPECT_EQ(keysOf(evalSrc("object(object(a=42,b=53,c=8), [[\"b\"],[\"b\",99]])", ev)), "a,c,b"); + EXPECT_EQ(keysOf(evalSrc("object(object(a=42,b=53,c=8), [[\"b\",99],[\"b\"]])", ev)), "a,c"); + EXPECT_DOUBLE_EQ(asNum(evalSrc("object(object(a=42,b=53,c=8), [[\"b\"],[\"b\",99]]).b", ev)), 99.0); +} + +TEST(ExprEvalObject, DeletingAnAbsentKeyIsASilentNoOp) { + std::vector warnings; + Evaluator ev([&](const std::string& m) { warnings.push_back(m); }); + EXPECT_EQ(keysOf(evalSrc("object(object(a=42,b=53,c=8), [[\"zz\"]])", ev)), "a,b,c"); + EXPECT_TRUE(warnings.empty()) << "unexpected: " << (warnings.empty() ? "" : warnings[0]); +} + +TEST(ExprEvalObject, DeletingTwiceIsHarmless) { + Evaluator ev; + EXPECT_EQ(keysOf(evalSrc("object(object(a=42,b=53,c=8), [[\"b\"],[\"b\"]])", ev)), "a,c"); +} + +TEST(ExprEvalObject, MalformedEntriesWarnAndYieldUndef) { + // Each abandons the whole call at the first bad entry. + const char* cases[] = { + "object(object(a=1), [[]])", // empty entry + "object(object(a=1), [[\"a\",1,2]])", // too long + "object(object(a=1), [[5,1]])", // non-string key, 2 elements + "object(object(a=1), [[5]])", // non-string key, 1 element + "object(object(a=1), [\"b\"])", // entry is not a list + "object(object(a=1), 42)", // argument is neither object nor list + }; + for (const char* src : cases) { + std::vector warnings; + Evaluator ev([&](const std::string& m) { warnings.push_back(m); }); + EXPECT_TRUE(std::holds_alternative(evalSrc(src, ev))) << src; + EXPECT_EQ(warnings.size(), 1u) << src; + } +} + +TEST(ExprEvalObject, WarningTextMatchesTheReference) { + // Quoted verbatim from OpenSCAD 2026.02.01, including its own + // inconsistent spacing: the "not a list" case puts spaces inside the + // parens where the others do not, and the "unnamed argument" case ends + // with a trailing space before the position suffix. + struct Case { const char* src; const char* want; }; + const Case cases[] = { + {"object(object(a=1), [[]])", + "object(Argument 1 [Element 0 []]) Entry is empty."}, + {"object(object(a=1), [[\"a\",1,2]])", + "object(Argument 1 [Element 0 [...]]) Entry length is 3, must be 1 [key] or 2 [key,value]."}, + {"object(object(a=1), [[5,1]])", + "object(Argument 1 [Element 0 [,value]]) The key of the entry is not but ."}, + {"object(object(a=1), [[5]])", + "object(Argument 1 [Element 0 []]) The key of the entry is not but ."}, + {"object(object(a=1), [\"b\"])", + "object( Argument 1 [Element 0 ] ) Entry type is not a list, it is ."}, + {"object(object(a=1), 42)", + "object(Argument 1 ) An unnamed argument must be either or , it is . "}, + }; + for (const auto& c : cases) { + std::vector warnings; + Evaluator ev([&](const std::string& m) { warnings.push_back(m); }); + evalSrc(c.src, ev); + ASSERT_EQ(warnings.size(), 1u) << c.src; + EXPECT_NE(warnings[0].find(c.want), std::string::npos) + << "for: " << c.src << "\n got: " << warnings[0] << "\n want: " << c.want; + } +} + TEST(ExprEvalIndexing, ObjectMemberAccessByName) { Evaluator ev; EXPECT_DOUBLE_EQ(asNum(evalSrc("object(a=1, b=2).b", ev)), 2.0);