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/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/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/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(&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/src/bytecode_vm.cpp b/src/bytecode_vm.cpp index 55c282e..761bc43 100644 --- a/src/bytecode_vm.cpp +++ b/src/bytecode_vm.cpp @@ -787,7 +787,7 @@ Value driveVm(Evaluator& ev, size_t floor) { 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); 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); +}