From 3c0cd79ca90514d9f4131ef0b61f000f1ca66eb1 Mon Sep 17 00:00:00 2001 From: Revar Desmera Date: Tue, 15 Sep 2026 19:51:04 -0700 Subject: [PATCH 1/3] Record which of the user's own calls reached each body (idToCallSite) idToNode names the node that PRODUCED a body, which for anything a library builds is a node inside that library -- a plain cube(10) maps into BOSL2's builtins.scad the moment BOSL2 is included, since BOSL2 overrides the primitives with its own modules. A picker cannot point the user at their own source with it, and a consumer that forgets to check origin splices a library's byte offsets into the user's buffer (BelfrySCAD #450). idToCallSite maps each originalID to the call in the USER's own file that reached it, or nullptr for geometry written at top level, where idToNode is already the user's node. Nothing new had to be computed. CSGNode::warnEntry already captures callStack_.front().callPosition at resolve time, precisely so a warning raised during generate -- after the stack has unwound -- can still name the user's line. This records it per ID as well as per node. A cache hit restamps to the site REUSING the geometry rather than the one that first produced it: a module called twice genuinely is two call sites, and that is the line the user would be shown for either copy. It resolves to the call, not the body. `module bracket() { cuboid(10); } bracket();` attributes to `bracket();`, because wrapping the body would move every instance. Exposed as node.call_site on each id_to_node entry. The id-span tuple the binding returns grows from 5 fields to 11; the facade in this package is its only consumer. Also ignores build-tbb/, a build directory the .gitignore did not cover. --- .gitignore | 2 + CLAUDE.md | 26 ++++++++- bindings/module.cpp | 23 +++++++- include/openscad_cpp_evaluator/evaluator.hpp | 16 +++++- pyproject.toml | 2 +- python/openscad_cpp_evaluator/__init__.py | 23 ++++++-- src/csg_generate.cpp | 13 ++++- tests/test_python_bindings.py | 56 ++++++++++++++++++++ 8 files changed, 150 insertions(+), 11 deletions(-) diff --git a/.gitignore b/.gitignore index f24e881..2f5643f 100644 --- a/.gitignore +++ b/.gitignore @@ -4,3 +4,5 @@ dist/ wheelhouse/ *.egg-info/ __pycache__/ +build-tbb/ +.DS_Store diff --git a/CLAUDE.md b/CLAUDE.md index 2cc5dc2..802fded 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -1609,8 +1609,32 @@ implementation of "CSG subtree → `object()`"; both engines call it — the interpreter from `evalRenderExpr`, the VM from `Op::PopBuiltinWrap`'s `Kind::Measure` branch. +**`idToCallSite` is what a picker should actually show the user.** `idToNode` +names the node that *produced* the geometry, and for anything a library builds +that is a node inside the library — a plain `cube(10)` maps into BOSL2's +`builtins.scad` the moment BOSL2 is included, because BOSL2 overrides the +primitives with its own modules. Pointing an editor at that span means pointing +it at another file, or (if the consumer forgets to check `origin`) splicing a +library's byte offsets into the user's buffer, which is how BelfrySCAD #450 +appended a `translate()` to the end of a 59-character script. + +`idToCallSite` maps each `originalID` to the call in the *user's* own file that +reached it, or `nullptr` for geometry written at top level, where `idToNode` +already is the user's node. It is `CSGNode::warnEntry` — the same +`callStack_.front().callPosition` captured at resolve time so a warning raised +during generate can still name the user's line — recorded per ID rather than +only per node. A cache hit restamps it to the site reusing the geometry, not the +site that first produced it: a module called twice genuinely is two call sites. + +It resolves to the **call**, not the body: `module bracket() { cuboid(10); } +bracket();` attributes to `bracket();`, since wrapping the body in a transform +would move every instance. + +Exposed to Python as `node.call_site` on each `id_to_node` entry (a `_Position` +or `None`). + `Evaluator::measuring_` is set for the whole generate. It suppresses the four -writes that exist solely to describe *drawn* geometry — `idToNode`/`idToColor` in +writes that exist solely to describe *drawn* geometry — `idToNode`/`idToColor`/`idToCallSite` in `tagGenerated` and `tagDisplayOnly`, the `restampCachedIds` call on a cache hit, and `cacheProducer_` — because those tables are cleared once per pass, so a leak is permanent and surfaces later as wrong click-to-source. It also suppresses diff --git a/bindings/module.cpp b/bindings/module.cpp index 2674028..a937e4a 100644 --- a/bindings/module.cpp +++ b/bindings/module.cpp @@ -196,13 +196,30 @@ struct IdSpan { uint32_t id; int start, end, line, column; std::string origin; + // The user's own call site that reached this geometry, or `hasCall` + // false when it was written at top level and `start`/`origin` above + // already name the user's source. See Evaluator::idToCallSite. + bool hasCall = false; + int callStart = 0, callEnd = 0, callLine = 0, callColumn = 0; + std::string callOrigin; }; void collectIdSpans(const oscadeval::Evaluator& ev, std::vector& out) { out.reserve(ev.idToNode.size()); for (const auto& [id, node] : ev.idToNode) { const oscad::Position& p = node->position(); - out.push_back({id, p.start_offset, p.end_offset, p.line, p.column, p.origin}); + IdSpan s{id, p.start_offset, p.end_offset, p.line, p.column, p.origin}; + auto call = ev.idToCallSite.find(id); + if (call != ev.idToCallSite.end() && call->second) { + const oscad::Position& c = *call->second; + s.hasCall = true; + s.callStart = c.start_offset; + s.callEnd = c.end_offset; + s.callLine = c.line; + s.callColumn = c.column; + s.callOrigin = c.origin; + } + out.push_back(std::move(s)); } } @@ -220,7 +237,9 @@ nb::list bodiesToList(std::vector& bodies) { nb::dict idSpansToDict(const std::vector& idSpans) { nb::dict d; for (const IdSpan& s : idSpans) - d[nb::cast(s.id)] = nb::make_tuple(s.start, s.end, s.line, s.column, s.origin); + d[nb::cast(s.id)] = nb::make_tuple(s.start, s.end, s.line, s.column, s.origin, + s.hasCall, s.callStart, s.callEnd, s.callLine, + s.callColumn, s.callOrigin); return d; } diff --git a/include/openscad_cpp_evaluator/evaluator.hpp b/include/openscad_cpp_evaluator/evaluator.hpp index 4c29bd8..a41ff9c 100644 --- a/include/openscad_cpp_evaluator/evaluator.hpp +++ b/include/openscad_cpp_evaluator/evaluator.hpp @@ -284,9 +284,23 @@ class Evaluator { // Give cached bodies fresh originalIDs so a second call site reusing // them is not confused with the first. See its definition. void restampCachedIds(std::vector& bodies, const oscad::ASTNode& node, - const oscad::ASTNode* producer); + const oscad::ASTNode* producer, const oscad::Position* callSite); std::unordered_map idToNode; + // originalID -> the call site in the USER's own file that reached this + // geometry, or nullptr for geometry written at top level (where + // idToNode already names the user's own node). + // + // idToNode names the node that PRODUCED the geometry, which for + // anything a library builds is a node inside that library -- including + // a plain `cube(10)` once BOSL2 is included, since BOSL2 overrides the + // primitives with its own modules. A consumer that wants to point the + // user at their own source (selection, a gizmo edit) cannot use it. + // + // This is CSGNode::warnEntry, which already captures exactly that at + // resolve time so a warning raised during generate can name the user's + // line. Recorded per ID here so picking can use it too. + std::unordered_map idToCallSite; // Nodes whose bodies were actually generated this run (not served from // the ManifoldCache). Reset per run; a diagnostic, and the only honest // way for a test to tell a cache hit from a miss now that an inner diff --git a/pyproject.toml b/pyproject.toml index 76fadda..eaba213 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "scikit_build_core.build" [project] name = "openscad_cpp_evaluator" -version = "1.20.5" +version = "1.21.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 6087b28..6e0e439 100644 --- a/python/openscad_cpp_evaluator/__init__.py +++ b/python/openscad_cpp_evaluator/__init__.py @@ -47,11 +47,21 @@ def __init__(self, line, column, origin, start_offset, end_offset): class _DeclNode: - __slots__ = ("name", "position") + __slots__ = ("name", "position", "call_site") - def __init__(self, name, position): + def __init__(self, name, position, call_site=None): self.name = name self.position = position + #: For a geometry node: the call in the USER's own file that reached + #: it, or None when it was written at top level (where `position` + #: already names the user's source). + #: + #: `position` names the node that PRODUCED the geometry, which for + #: anything a library builds is inside that library -- a plain + #: `cube(10)` lands in BOSL2's builtins.scad once BOSL2 is included, + #: since BOSL2 overrides the primitives. A caller that wants to show + #: the user their own source wants this instead. + self.call_site = call_site class OscObject: @@ -635,8 +645,13 @@ def evaluate(self, source_path: str, viewport_params: Optional[dict] = None, # originalID -> a node with `.position` (start/end offsets) for WYSIWYG # picking and gizmo write-back. id_to_node = { - oid: _DeclNode(None, _Position(line, column, origin, start, end)) - for oid, (start, end, line, column, origin) in id_spans.items() + oid: _DeclNode( + None, + _Position(line, column, origin, start, end), + _Position(cline, ccolumn, corigin, cstart, cend) if has_call else None, + ) + for oid, (start, end, line, column, origin, + has_call, cstart, cend, cline, ccolumn, corigin) in id_spans.items() } return bodies, id_to_node diff --git a/src/csg_generate.cpp b/src/csg_generate.cpp index 5c50ac7..98c8449 100644 --- a/src/csg_generate.cpp +++ b/src/csg_generate.cpp @@ -68,7 +68,8 @@ std::vector Evaluator::generateTreeImpl(const std::vector if (node.node && !measuring_) { auto producer = cacheProducer_.find(*key); restampCachedIds(node.bodies, *node.node, - producer == cacheProducer_.end() ? nullptr : producer->second); + producer == cacheProducer_.end() ? nullptr : producer->second, + node.warnEntry); } } else { // Recurse into children first (bottom-up) -- populates each @@ -331,7 +332,7 @@ std::shared_ptr> remapParts(const std::vector& bodies, const oscad::ASTNode& node, - const oscad::ASTNode* producer) { + const oscad::ASTNode* producer, const oscad::Position* callSite) { // A cache hit hands back the geometry AND the originalIDs of whichever // call site first produced it. Those IDs are provenance -- "which node // made this" -- not content, so two identical shapes at two call sites @@ -374,6 +375,12 @@ void Evaluator::restampCachedIds(std::vector& bodies, const oscad:: auto old = idToNode.find(id); idToNode[fresh] = (old != idToNode.end() && old->second != producer) ? old->second : &node; + // Unlike idToNode, this is the call site that reached the + // geometry NOW: whichever node the cached copy is being + // reused at is the line the user would be shown, whatever + // produced the original. A module called twice really is + // two different call sites. + idToCallSite[fresh] = callSite; // The colour has to be re-recorded too, or a later merge // cannot tell this body's runs apart from uncoloured ones // (attachTriColors looks runs up here). Across renders @@ -453,6 +460,7 @@ ColoredBody Evaluator::tagGenerated(manifold::Manifold body, const oscad::ASTNod for (uint32_t originalId : mesh.runOriginalID) { idToNode[originalId] = &node; idToColor[originalId] = color; + idToCallSite[originalId] = generateWarnEntry; } } ColoredBody cb; @@ -474,6 +482,7 @@ ColoredBody Evaluator::tagDisplayOnly(manifold::MeshGL mesh, const oscad::ASTNod if (!measuring_) { idToNode[originalId] = &node; idToColor[originalId] = color; + idToCallSite[originalId] = generateWarnEntry; } ColoredBody cb; diff --git a/tests/test_python_bindings.py b/tests/test_python_bindings.py index 73e940d..14de5a7 100644 --- a/tests/test_python_bindings.py +++ b/tests/test_python_bindings.py @@ -13,6 +13,7 @@ wheels.yml's CIBW_TEST_COMMAND instead, which already builds+installs the package on every release platform. """ +import os import sys import tempfile from pathlib import Path @@ -876,3 +877,58 @@ def test_keep_minuend_color_paints_cut_faces_with_the_minuend(tmp_path): kept, _ = Evaluator(keep_minuend_color=True).evaluate(str(src), {}) assert kept[0].tri_colors is None # one colour: the minuend's assert all(abs(a - b) < 1e-3 for a, b in zip(kept[0].color[:3], (1.0, 0.647, 0.0))) + + +def test_id_to_node_carries_the_users_own_call_site(tmp_path): + """A library-built body must be attributable to the line the user wrote. + + `position` names the node that PRODUCED the geometry, which for anything + BOSL2 builds is inside BOSL2 -- a plain cube(10) lands in builtins.scad, + since BOSL2 overrides the primitives. `call_site` is what a caller shows + the user instead. + """ + import openscad_cpp_evaluator as E + + lib = tmp_path / "MYLIB" + lib.mkdir() + (lib / "std.scad").write_text( + "module boxy(s) { cube(s); }\n" + "module wrapped(s) { boxy(s); }\n" + ) + script = tmp_path / "m.scad" + src = 'include \nwrapped(10);\n' + script.write_text(src) + + old = os.environ.get("OPENSCADPATH") + os.environ["OPENSCADPATH"] = str(tmp_path) + try: + ev = E.Evaluator() + _bodies, id_to_node = ev.evaluate(str(script), {}) + finally: + if old is None: + os.environ.pop("OPENSCADPATH", None) + else: + os.environ["OPENSCADPATH"] = old + + assert id_to_node, "the script builds geometry" + for node in id_to_node.values(): + # Produced inside the library... + assert node.position.origin.endswith("std.scad"), node.position.origin + # ...but attributable to the user's own line. + assert node.call_site is not None + assert os.path.realpath(node.call_site.origin) == os.path.realpath(str(script)) + assert src[node.call_site.start_offset:node.call_site.end_offset].startswith("wrapped(10)") + + +def test_top_level_geometry_has_no_call_site(): + """Nothing to redirect to: `position` is already the user's own node.""" + import openscad_cpp_evaluator as E + p = os.path.join(tempfile.gettempdir(), "toplevel_call_site.scad") + with open(p, "w") as f: + f.write("cube(10);\n") + ev = E.Evaluator() + _bodies, id_to_node = ev.evaluate(p, {}) + assert id_to_node + for node in id_to_node.values(): + assert node.call_site is None + assert os.path.realpath(node.position.origin) == os.path.realpath(p) From fa3bab873c7ba34a3ba332d8ae1ccc38134f9915 Mon Sep 17 00:00:00 2001 From: Revar Desmera Date: Tue, 15 Sep 2026 20:06:16 -0700 Subject: [PATCH 2/3] idToCallSite records the innermost user call, not the top-level statement Review of the first commit: selecting the statement that entered the chain is the wrong end of it. `translate([20,0,0]) cuboid(8, rounding=1);` attributed to the whole statement, so clicking the cuboid highlighted the translate too -- and, because the span then started BEFORE that translate, a gizmo's backwards-looking merge could not see it and added a second wrapper instead of updating the first. currentUserCallEntry() takes the innermost frame still in the user's own file. currentWarnEntry() keeps the outermost, unchanged: a warning wants the top-level statement to look at, a click wants the line that placed that object. CSGNode carries both, captured at the same five resolve sites, and generate republishes both. translate([20,0,0]) cuboid(8, rounding=1); -> 'cuboid(8, rounding=1);' module inner() { boxy(8); } ... outer(); -> 'boxy(8);' The merge regex sees ('20','0','0') again with the span where it now is. No stored script path was needed to tell the user's file from a library: callStack_.front() is by construction the call made from top level, so its origin is the file being run. Known consequence, documented rather than worked around: geometry from a module called twice attributes to the same line in that module's body both times, so an edit there moves every instance. Distinguishing instances is what walking the selection outwards is for (BelfrySCAD #455). --- CLAUDE.md | 19 ++++++++-- include/openscad_cpp_evaluator/csg_node.hpp | 8 +++++ include/openscad_cpp_evaluator/evaluator.hpp | 33 +++++++++++++++++ src/bytecode_vm.cpp | 2 ++ src/csg_generate.cpp | 9 +++-- src/csg_resolve.cpp | 3 ++ tests/test_python_bindings.py | 38 ++++++++++++++++++++ 7 files changed, 106 insertions(+), 6 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 802fded..f9ee521 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -1626,9 +1626,22 @@ during generate can still name the user's line — recorded per ID rather than only per node. A cache hit restamps it to the site reusing the geometry, not the site that first produced it: a module called twice genuinely is two call sites. -It resolves to the **call**, not the body: `module bracket() { cuboid(10); } -bracket();` attributes to `bracket();`, since wrapping the body in a transform -would move every instance. +It is the **innermost** call still in the user's file, not the top-level +statement that entered the chain — `currentUserCallEntry()` beside +`currentWarnEntry()`, which keeps the outermost for warnings. A warning wants +the statement to look at; a click wants the line that placed *that* object, so a +drag edits it rather than something wrapping everything the module makes. It +also keeps the span *inside* any enclosing `translate(...)`, which is what lets +a gizmo find and update an existing wrapper instead of adding another. + +The consequence to know: geometry from a module called twice attributes to the +same line in that module's body both times, so an edit there moves every +instance. Distinguishing instances is what walking the selection outwards is +for; this is the innermost answer, deliberately. + +No stored script path is needed to tell the user's file from a library: +`callStack_.front()` is by construction the call made from top level, so its +origin *is* the file being run. Exposed to Python as `node.call_site` on each `id_to_node` entry (a `_Position` or `None`). diff --git a/include/openscad_cpp_evaluator/csg_node.hpp b/include/openscad_cpp_evaluator/csg_node.hpp index 5044aca..3830a5a 100644 --- a/include/openscad_cpp_evaluator/csg_node.hpp +++ b/include/openscad_cpp_evaluator/csg_node.hpp @@ -51,6 +51,14 @@ struct CSGNode { // call sites still share a cache entry. const oscad::Position* warnEntry = nullptr; + // The innermost call still in the user's own file, captured at RESOLVE + // time for the same reason warnEntry is: a picker runs long after the + // stack has unwound. Distinct from warnEntry, which names the call that + // entered the chain from the top level -- see Evaluator's + // currentUserCallEntry() for why a warning and a click want different + // frames. One more pointer per node, on the same reasoning as above. + const oscad::Position* userEntry = nullptr; + // 1 + the deepest child's own treeDepth (1 for a leaf) -- set once, // at construction, by whichever csg_resolve.cpp site finalizes this // node's own `children` (buildTreeNode, evalModularCall's non-splice diff --git a/include/openscad_cpp_evaluator/evaluator.hpp b/include/openscad_cpp_evaluator/evaluator.hpp index a41ff9c..0f770d6 100644 --- a/include/openscad_cpp_evaluator/evaluator.hpp +++ b/include/openscad_cpp_evaluator/evaluator.hpp @@ -341,6 +341,35 @@ class Evaluator { return callStack_.empty() ? nullptr : callStack_.front().callPosition; } + // The INNERMOST call still in the user's own file -- what a picker + // should select, where currentWarnEntry() is what a warning should + // blame. + // + // They differ as soon as the user has modules of their own: + // `module inner() { cuboid(8); } module outer() { inner(); } outer();` + // gives `outer();` from currentWarnEntry (the call that entered the + // chain) and `cuboid(8);` from this (the deepest line the user actually + // wrote). A warning wants the former, so the reader can see which + // top-level statement to look at; clicking geometry wants the latter, + // so the drag edits the line that placed THAT object rather than one + // wrapping everything the module makes. + // + // No stored script path is needed to tell "the user's file" from a + // library: callStack_.front() is by construction the call made from top + // level, so its origin IS the file being run. A `use`d file's top-level + // geometry makes that origin the library, and a consumer that checks + // the origin (as it must) refuses either way. + const oscad::Position* currentUserCallEntry() const { + if (callStack_.empty()) return nullptr; + const oscad::Position* top = callStack_.front().callPosition; + if (!top) return nullptr; + for (auto it = callStack_.rbegin(); it != callStack_.rend(); ++it) { + if (it->callPosition && it->callPosition->origin == top->origin) + return it->callPosition; + } + return top; + } + // Set by generateTreeImpl() to the CSGNode currently being generated, // so warn() can name the user's own call site during a phase where // callStack_ is necessarily empty. Public for the same reason @@ -349,6 +378,10 @@ class Evaluator { // than anything reentrancy-aware, since generateTreeImpl recurses // depth-first on one thread. const oscad::Position* generateWarnEntry = nullptr; + // The same republish for CSGNode::userEntry, read by tagGenerated to + // fill idToCallSite. Separate from generateWarnEntry because a warning + // and a click want different frames -- currentUserCallEntry(). + const oscad::Position* generateUserEntry = nullptr; // Set while generating anything beneath a hull(). An open mesh there is // not a mistake to report: a convex hull needs only points, so BOSL2's diff --git a/src/bytecode_vm.cpp b/src/bytecode_vm.cpp index 363cdc3..60c550d 100644 --- a/src/bytecode_vm.cpp +++ b/src/bytecode_vm.cpp @@ -1400,6 +1400,7 @@ Value driveVm(Evaluator& ev, size_t floor) { treeNode->node = site.node; treeNode->isBuiltin = true; treeNode->warnEntry = ev.currentWarnEntry(); + treeNode->userEntry = ev.currentUserCallEntry(); treeNode->children = std::move(children); treeNode->params = std::move(pending.params); treeNode->uncacheable = uncacheable; @@ -1503,6 +1504,7 @@ Value driveVm(Evaluator& ev, size_t floor) { treeNode->node = site.node; treeNode->isBuiltin = true; treeNode->warnEntry = ev.currentWarnEntry(); + treeNode->userEntry = ev.currentUserCallEntry(); treeNode->children = std::move(children); treeNode->params = std::move(params); treeNode->uncacheable = uncacheable; diff --git a/src/csg_generate.cpp b/src/csg_generate.cpp index 98c8449..074e078 100644 --- a/src/csg_generate.cpp +++ b/src/csg_generate.cpp @@ -69,7 +69,7 @@ std::vector Evaluator::generateTreeImpl(const std::vector auto producer = cacheProducer_.find(*key); restampCachedIds(node.bodies, *node.node, producer == cacheProducer_.end() ? nullptr : producer->second, - node.warnEntry); + node.userEntry); } } else { // Recurse into children first (bottom-up) -- populates each @@ -96,9 +96,12 @@ std::vector Evaluator::generateTreeImpl(const std::vector // can name that line. Restored after (rather than left set) // so a sibling generated at top level doesn't inherit it. const oscad::Position* savedWarnEntry = generateWarnEntry; + const oscad::Position* savedUserEntry = generateUserEntry; generateWarnEntry = node.warnEntry; + generateUserEntry = node.userEntry; node.bodies = it->second(*this, node.params, node.children, *node.node); generateWarnEntry = savedWarnEntry; + generateUserEntry = savedUserEntry; } else { // No registered GenerateFn for this kind (a builtin with // display-only semantics like render(), or a non-builtin @@ -460,7 +463,7 @@ ColoredBody Evaluator::tagGenerated(manifold::Manifold body, const oscad::ASTNod for (uint32_t originalId : mesh.runOriginalID) { idToNode[originalId] = &node; idToColor[originalId] = color; - idToCallSite[originalId] = generateWarnEntry; + idToCallSite[originalId] = generateUserEntry; } } ColoredBody cb; @@ -482,7 +485,7 @@ ColoredBody Evaluator::tagDisplayOnly(manifold::MeshGL mesh, const oscad::ASTNod if (!measuring_) { idToNode[originalId] = &node; idToColor[originalId] = color; - idToCallSite[originalId] = generateWarnEntry; + idToCallSite[originalId] = generateUserEntry; } ColoredBody cb; diff --git a/src/csg_resolve.cpp b/src/csg_resolve.cpp index 4e4fcdc..52615f2 100644 --- a/src/csg_resolve.cpp +++ b/src/csg_resolve.cpp @@ -68,6 +68,7 @@ void Evaluator::buildTreeNode(const std::string& kind, const oscad::ASTNode& nod treeNode->node = &node; treeNode->isBuiltin = true; treeNode->warnEntry = currentWarnEntry(); + treeNode->userEntry = currentUserCallEntry(); treeNode->children = std::move(children); treeNode->params = std::move(params); treeNode->uncacheable = uncacheable; @@ -139,6 +140,7 @@ void Evaluator::evalModularCall(const oscad::ModularCall& node, EvalContext& ctx treeNode->node = &node; treeNode->isBuiltin = true; treeNode->warnEntry = currentWarnEntry(); + treeNode->userEntry = currentUserCallEntry(); treeNode->uncacheable = uncacheable; treeNode->children = std::move(children); treeNode->params = std::move(params); @@ -164,6 +166,7 @@ void Evaluator::spliceModuleChildren(std::vector> child unionNode->node = &callNode; unionNode->isBuiltin = false; unionNode->warnEntry = currentWarnEntry(); + unionNode->userEntry = currentUserCallEntry(); unionNode->uncacheable = std::any_of(children.begin(), children.end(), [](const auto& c) { return c->uncacheable; }); unionNode->children = std::move(children); setTreeDepthOrThrow(*unionNode, callNode); diff --git a/tests/test_python_bindings.py b/tests/test_python_bindings.py index 14de5a7..edcb901 100644 --- a/tests/test_python_bindings.py +++ b/tests/test_python_bindings.py @@ -920,6 +920,44 @@ def test_id_to_node_carries_the_users_own_call_site(tmp_path): assert src[node.call_site.start_offset:node.call_site.end_offset].startswith("wrapped(10)") +def test_call_site_is_the_innermost_call_the_user_wrote(tmp_path): + """Not the top-level statement that entered the chain. + + A warning wants the statement to look at; a click wants the line that + placed THAT object, so a drag edits it rather than something wrapping + everything the module makes. + """ + import openscad_cpp_evaluator as E + + lib = tmp_path / "MYLIB" + lib.mkdir() + (lib / "std.scad").write_text("module boxy(s) { cube(s); }\n") + script = tmp_path / "n.scad" + src = ("include \n" + "module inner() { boxy(8); }\n" + "module outer() { inner(); }\n" + "outer();\n") + script.write_text(src) + + old = os.environ.get("OPENSCADPATH") + os.environ["OPENSCADPATH"] = str(tmp_path) + try: + ev = E.Evaluator() + _bodies, id_to_node = ev.evaluate(str(script), {}) + finally: + if old is None: + os.environ.pop("OPENSCADPATH", None) + else: + os.environ["OPENSCADPATH"] = old + + assert id_to_node + for node in id_to_node.values(): + cs = node.call_site + assert cs is not None + text = src[cs.start_offset:cs.end_offset] + assert text.startswith("boxy(8)"), f"got {text!r}, wanted the innermost user call" + + def test_top_level_geometry_has_no_call_site(): """Nothing to redirect to: `position` is already the user's own node.""" import openscad_cpp_evaluator as E From f76e66c02ddeaa365c47e439fc5af6f7bd24614a Mon Sep 17 00:00:00 2001 From: Revar Desmera Date: Tue, 15 Sep 2026 20:21:47 -0700 Subject: [PATCH 3/3] Record the whole call chain, not one frame: BOSL2 layers are worth stepping into Filtering to the user's own file was wrong. A single cuboid() call is 24 frames -- cuboid -> attachable -> _attach_transform -> _find_anchor, most of them inside BOSL2 -- and someone writing BOSL2 wants to step into exactly those. Which frames a front end can show depends on what it has open, which the evaluator has no business guessing, so it now records all of them and the consumer picks its level. That also removes the origin comparison currentUserCallEntry() was doing: the evaluator no longer needs any notion of "the user's file". Stored as a cactus stack rather than a list per node. Chains nest, so the distinct chains over a run form a tree: callChains_ holds {site, parent, isModule} and a CSGNode holds one uint32 into it. Memory tracks distinct call PATHS -- tens on a real model, where Dalek's 139 bodies share 13 innermost sites -- not CSG nodes, and nothing allocates per node. That is what makes this affordable where csg_node.hpp had ruled out "the full frame list a TRACE would need". isModule separates a module frame, which has geometry behind it, from a function frame like _find_anchor, which does not: both are worth showing, only the former is worth dragging. Exposed as node.call_sites, innermost-first, with node.call_site kept as the innermost for a caller that does not care about the chain. --- CLAUDE.md | 69 +++++------ bindings/module.cpp | 47 ++++---- include/openscad_cpp_evaluator/csg_node.hpp | 13 +-- include/openscad_cpp_evaluator/evaluator.hpp | 113 ++++++++++++------- python/openscad_cpp_evaluator/__init__.py | 46 ++++++-- src/bytecode_vm.cpp | 4 +- src/csg_generate.cpp | 16 +-- src/csg_resolve.cpp | 6 +- tests/test_python_bindings.py | 70 ++++++++---- 9 files changed, 236 insertions(+), 148 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index f9ee521..a2c8d52 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -1609,45 +1609,46 @@ implementation of "CSG subtree → `object()`"; both engines call it — the interpreter from `evalRenderExpr`, the VM from `Op::PopBuiltinWrap`'s `Kind::Measure` branch. -**`idToCallSite` is what a picker should actually show the user.** `idToNode` -names the node that *produced* the geometry, and for anything a library builds -that is a node inside the library — a plain `cube(10)` maps into BOSL2's -`builtins.scad` the moment BOSL2 is included, because BOSL2 overrides the -primitives with its own modules. Pointing an editor at that span means pointing -it at another file, or (if the consumer forgets to check `origin`) splicing a -library's byte offsets into the user's buffer, which is how BelfrySCAD #450 -appended a `translate()` to the end of a 59-character script. - -`idToCallSite` maps each `originalID` to the call in the *user's* own file that -reached it, or `nullptr` for geometry written at top level, where `idToNode` -already is the user's node. It is `CSGNode::warnEntry` — the same -`callStack_.front().callPosition` captured at resolve time so a warning raised -during generate can still name the user's line — recorded per ID rather than -only per node. A cache hit restamps it to the site reusing the geometry, not the -site that first produced it: a module called twice genuinely is two call sites. - -It is the **innermost** call still in the user's file, not the top-level -statement that entered the chain — `currentUserCallEntry()` beside -`currentWarnEntry()`, which keeps the outermost for warnings. A warning wants -the statement to look at; a click wants the line that placed *that* object, so a -drag edits it rather than something wrapping everything the module makes. It -also keeps the span *inside* any enclosing `translate(...)`, which is what lets -a gizmo find and update an existing wrapper instead of adding another. - -The consequence to know: geometry from a module called twice attributes to the -same line in that module's body both times, so an edit there moves every -instance. Distinguishing instances is what walking the selection outwards is -for; this is the innermost answer, deliberately. +**`idToCallChain` is what a picker should actually walk.** `idToNode` names the +node that *produced* the geometry, and for anything a library builds that is a +node inside the library — a plain `cube(10)` maps into BOSL2's `builtins.scad` +the moment BOSL2 is included, because BOSL2 overrides the primitives with its +own modules. Pointing an editor at that span means pointing it at another file, +or (if the consumer forgets to check `origin`) splicing a library's byte offsets +into the user's buffer, which is how BelfrySCAD #450 appended a `translate()` to +the end of a 59-character script. + +`idToCallChain` maps each `originalID` to its **whole call chain**, innermost +frame first. Deliberately unfiltered: a single `cuboid()` is 24 frames, most of +them BOSL2's own `attachable`/`_attach_transform` layers, and someone debugging +the library wants to step into exactly those. Which frames are reachable depends +on what the front end has open, which the evaluator has no business guessing — +so it records all of them and a consumer picks its level (BelfrySCAD defaults to +the last frame in the running script). + +It is stored as a **cactus stack**, not a list per node. Chains nest, so the +distinct chains over a run form a tree: `callChains_` holds `{site, parent, +isModule}` and a `CSGNode` holds one `uint32` into it. Memory tracks distinct +call *paths* — tens on a real model; Dalek's 139 bodies share 13 innermost sites +— rather than CSG nodes, and nothing allocates per node. That is what makes this +affordable where "the full frame list a TRACE would need" (`csg_node.hpp`) was +not. + +`isModule` separates a module frame, which has geometry behind it, from a +function frame like `_find_anchor`, which does not: both are worth showing, only +the former is worth dragging. + +A cache hit restamps to the chain reaching the geometry *now*, not the one that +first produced it: a module called twice genuinely is two call chains. No stored script path is needed to tell the user's file from a library: -`callStack_.front()` is by construction the call made from top level, so its -origin *is* the file being run. +`callStack_.front()` is by construction the call made from top level. -Exposed to Python as `node.call_site` on each `id_to_node` entry (a `_Position` -or `None`). +Exposed to Python as `node.call_sites` (innermost-first `_CallFrame`s, each a +`_Position` plus `is_module`), with `node.call_site` as the innermost. `Evaluator::measuring_` is set for the whole generate. It suppresses the four -writes that exist solely to describe *drawn* geometry — `idToNode`/`idToColor`/`idToCallSite` in +writes that exist solely to describe *drawn* geometry — `idToNode`/`idToColor`/`idToCallChain` in `tagGenerated` and `tagDisplayOnly`, the `restampCachedIds` call on a cache hit, and `cacheProducer_` — because those tables are cleared once per pass, so a leak is permanent and surfaces later as wrong click-to-source. It also suppresses diff --git a/bindings/module.cpp b/bindings/module.cpp index a937e4a..bba7a31 100644 --- a/bindings/module.cpp +++ b/bindings/module.cpp @@ -196,12 +196,17 @@ struct IdSpan { uint32_t id; int start, end, line, column; std::string origin; - // The user's own call site that reached this geometry, or `hasCall` - // false when it was written at top level and `start`/`origin` above - // already name the user's source. See Evaluator::idToCallSite. - bool hasCall = false; - int callStart = 0, callEnd = 0, callLine = 0, callColumn = 0; - std::string callOrigin; + // The whole call chain behind this body, innermost frame first. Empty + // for geometry written at top level, where the span above is already + // the user's own source. Library frames included on purpose -- a BOSL2 + // author stepping into `attachable` wants them; a front end picks the + // level it can actually show. See Evaluator::idToCallChain. + struct Frame { + int start, end, line, column; + std::string origin; + bool is_module; + }; + std::vector chain; }; void collectIdSpans(const oscadeval::Evaluator& ev, std::vector& out) { @@ -209,15 +214,17 @@ void collectIdSpans(const oscadeval::Evaluator& ev, std::vector& out) { for (const auto& [id, node] : ev.idToNode) { const oscad::Position& p = node->position(); IdSpan s{id, p.start_offset, p.end_offset, p.line, p.column, p.origin}; - auto call = ev.idToCallSite.find(id); - if (call != ev.idToCallSite.end() && call->second) { - const oscad::Position& c = *call->second; - s.hasCall = true; - s.callStart = c.start_offset; - s.callEnd = c.end_offset; - s.callLine = c.line; - s.callColumn = c.column; - s.callOrigin = c.origin; + auto found = ev.idToCallChain.find(id); + uint32_t idx = found == ev.idToCallChain.end() + ? oscadeval::Evaluator::kNoCallChain : found->second; + while (idx != oscadeval::Evaluator::kNoCallChain && idx < ev.callChains_.size()) { + const auto& e = ev.callChains_[idx]; + if (e.site) { + s.chain.push_back({e.site->start_offset, e.site->end_offset, + e.site->line, e.site->column, e.site->origin, + e.isModule}); + } + idx = e.parent; } out.push_back(std::move(s)); } @@ -236,10 +243,12 @@ nb::list bodiesToList(std::vector& bodies) { nb::dict idSpansToDict(const std::vector& idSpans) { nb::dict d; - for (const IdSpan& s : idSpans) - d[nb::cast(s.id)] = nb::make_tuple(s.start, s.end, s.line, s.column, s.origin, - s.hasCall, s.callStart, s.callEnd, s.callLine, - s.callColumn, s.callOrigin); + for (const IdSpan& s : idSpans) { + nb::list chain; + for (const IdSpan::Frame& f : s.chain) + chain.append(nb::make_tuple(f.start, f.end, f.line, f.column, f.origin, f.is_module)); + d[nb::cast(s.id)] = nb::make_tuple(s.start, s.end, s.line, s.column, s.origin, chain); + } return d; } diff --git a/include/openscad_cpp_evaluator/csg_node.hpp b/include/openscad_cpp_evaluator/csg_node.hpp index 3830a5a..c4132a7 100644 --- a/include/openscad_cpp_evaluator/csg_node.hpp +++ b/include/openscad_cpp_evaluator/csg_node.hpp @@ -51,13 +51,12 @@ struct CSGNode { // call sites still share a cache entry. const oscad::Position* warnEntry = nullptr; - // The innermost call still in the user's own file, captured at RESOLVE - // time for the same reason warnEntry is: a picker runs long after the - // stack has unwound. Distinct from warnEntry, which names the call that - // entered the chain from the top level -- see Evaluator's - // currentUserCallEntry() for why a warning and a click want different - // frames. One more pointer per node, on the same reasoning as above. - const oscad::Position* userEntry = nullptr; + // This node's interned call chain (Evaluator::callChains_), captured + // at RESOLVE time for the same reason warnEntry is: a picker runs long + // after the stack has unwound. kNoCallChain at top level. One uint32, + // not a frame list -- see Evaluator's call-chain section for why that + // is affordable where a per-node list was not. + uint32_t callChain = UINT32_MAX; // 1 + the deepest child's own treeDepth (1 for a leaf) -- set once, // at construction, by whichever csg_resolve.cpp site finalizes this diff --git a/include/openscad_cpp_evaluator/evaluator.hpp b/include/openscad_cpp_evaluator/evaluator.hpp index 0f770d6..548fed0 100644 --- a/include/openscad_cpp_evaluator/evaluator.hpp +++ b/include/openscad_cpp_evaluator/evaluator.hpp @@ -284,23 +284,21 @@ class Evaluator { // Give cached bodies fresh originalIDs so a second call site reusing // them is not confused with the first. See its definition. void restampCachedIds(std::vector& bodies, const oscad::ASTNode& node, - const oscad::ASTNode* producer, const oscad::Position* callSite); + const oscad::ASTNode* producer, uint32_t callChain); std::unordered_map idToNode; - // originalID -> the call site in the USER's own file that reached this - // geometry, or nullptr for geometry written at top level (where - // idToNode already names the user's own node). + // originalID -> its interned call chain (innermost frame; walk + // callChains_[i].parent outwards), or kNoCallChain for geometry + // written at top level, where idToNode already is the user's node. // // idToNode names the node that PRODUCED the geometry, which for // anything a library builds is a node inside that library -- including // a plain `cube(10)` once BOSL2 is included, since BOSL2 overrides the - // primitives with its own modules. A consumer that wants to point the - // user at their own source (selection, a gizmo edit) cannot use it. - // - // This is CSGNode::warnEntry, which already captures exactly that at - // resolve time so a warning raised during generate can name the user's - // line. Recorded per ID here so picking can use it too. - std::unordered_map idToCallSite; + // primitives with its own modules. A consumer that wants to point an + // editor at a line the user can actually see walks this instead, and + // picks the level it wants: the last frame in the script for an + // ordinary pick, deeper for someone debugging the library itself. + std::unordered_map idToCallChain; // Nodes whose bodies were actually generated this run (not served from // the ManifoldCache). Reset per run; a diagnostic, and the only honest // way for a test to tell a cache hit from a miss now that an inner @@ -341,35 +339,57 @@ class Evaluator { return callStack_.empty() ? nullptr : callStack_.front().callPosition; } - // The INNERMOST call still in the user's own file -- what a picker - // should select, where currentWarnEntry() is what a warning should - // blame. + // -- Call chains ----------------------------------------------------- // - // They differ as soon as the user has modules of their own: - // `module inner() { cuboid(8); } module outer() { inner(); } outer();` - // gives `outer();` from currentWarnEntry (the call that entered the - // chain) and `cuboid(8);` from this (the deepest line the user actually - // wrote). A warning wants the former, so the reader can see which - // top-level statement to look at; clicking geometry wants the latter, - // so the drag edits the line that placed THAT object rather than one - // wrapping everything the module makes. + // The whole call chain behind a body, so a picker can step through it. + // Deliberately UNFILTERED: which frames are reachable depends on what + // the front end has open, which the evaluator has no business guessing. + // A BOSL2 author stepping into `attachable` -> `_attach_transform` is + // the case that settles it; a single cuboid() call is seven frames, six + // of them inside the library. // - // No stored script path is needed to tell "the user's file" from a - // library: callStack_.front() is by construction the call made from top - // level, so its origin IS the file being run. A `use`d file's top-level - // geometry makes that origin the library, and a consumer that checks - // the origin (as it must) refuses either way. - const oscad::Position* currentUserCallEntry() const { - if (callStack_.empty()) return nullptr; - const oscad::Position* top = callStack_.front().callPosition; - if (!top) return nullptr; - for (auto it = callStack_.rbegin(); it != callStack_.rend(); ++it) { - if (it->callPosition && it->callPosition->origin == top->origin) - return it->callPosition; + // Stored as a cactus stack rather than a list per node. Call chains + // nest, so the distinct chains over a run form a TREE: each entry is + // one frame plus its parent's index, and a CSGNode holds a single + // uint32 into the pool. Memory tracks distinct call PATHS (tens on a + // real model -- Dalek's 139 bodies share 13 innermost sites) rather + // than CSG nodes, and nothing allocates per node. That is what makes + // this affordable where "the full frame list a TRACE would need" + // (csg_node.hpp) was not. + static constexpr uint32_t kNoCallChain = UINT32_MAX; + + struct CallChainEntry { + const oscad::Position* site = nullptr; // non-owning, AST-lifetime-bound + uint32_t parent = kNoCallChain; // next frame OUT, or kNoCallChain + bool isModule = false; // a function frame has no geometry to drag + }; + + // Intern callStack_ as it stands and return its index, or kNoCallChain + // at top level. Outermost-first walk, so a chain sharing a prefix with + // one already interned costs only its own tail. + uint32_t internCurrentCallChain() { + uint32_t parent = kNoCallChain; + for (const CallStackFrame& f : callStack_) { + if (!f.callPosition) continue; + const ChainKey key{parent, f.callPosition}; + auto found = chainIndex_.find(key); + if (found != chainIndex_.end()) { + parent = found->second; + continue; + } + const uint32_t idx = static_cast(callChains_.size()); + callChains_.push_back({f.callPosition, parent, + f.kind == CallStackFrame::Kind::Module}); + chainIndex_.emplace(key, idx); + parent = idx; } - return top; + return parent; // the innermost frame, or kNoCallChain } + // The interned pool. Public for the same reason idToNode is: a binding + // walks it after evaluate() to flatten each id's chain. + std::vector callChains_; + // Set by generateTreeImpl() to the CSGNode currently being generated, // so warn() can name the user's own call site during a phase where // callStack_ is necessarily empty. Public for the same reason @@ -378,10 +398,9 @@ class Evaluator { // than anything reentrancy-aware, since generateTreeImpl recurses // depth-first on one thread. const oscad::Position* generateWarnEntry = nullptr; - // The same republish for CSGNode::userEntry, read by tagGenerated to - // fill idToCallSite. Separate from generateWarnEntry because a warning - // and a click want different frames -- currentUserCallEntry(). - const oscad::Position* generateUserEntry = nullptr; + // The same republish for CSGNode::callChain, read by tagGenerated to + // fill idToCallChain. + uint32_t generateCallChain = kNoCallChain; // Set while generating anything beneath a hull(). An open mesh there is // not a mistake to report: a convex hull needs only points, so BOSL2's @@ -1837,6 +1856,22 @@ class Evaluator { // eval_error.hpp). std::vector callStack_; + // (parent chain index, frame position) -> chain index, the memo that + // makes internCurrentCallChain() share prefixes. Cleared with the pool + // wherever idToNode is. + struct ChainKey { + uint32_t parent; + const oscad::Position* site; + bool operator==(const ChainKey& o) const { return parent == o.parent && site == o.site; } + }; + struct ChainKeyHash { + std::size_t operator()(const ChainKey& k) const { + return std::hash{}(k.parent) ^ + (std::hash{}(k.site) << 1); + } + }; + std::unordered_map chainIndex_; + // Count of callStack_ entries that ACTUALLY cost native C++ stack -- // i.e. pushed with skipDepthGuard=false (enterUserCall's own // interpreted-call sites: evalUserFunctionCore, evalUserModule). diff --git a/python/openscad_cpp_evaluator/__init__.py b/python/openscad_cpp_evaluator/__init__.py index 6e0e439..79fe7cc 100644 --- a/python/openscad_cpp_evaluator/__init__.py +++ b/python/openscad_cpp_evaluator/__init__.py @@ -47,21 +47,43 @@ def __init__(self, line, column, origin, start_offset, end_offset): class _DeclNode: - __slots__ = ("name", "position", "call_site") + __slots__ = ("name", "position", "call_sites") - def __init__(self, name, position, call_site=None): + def __init__(self, name, position, call_sites=()): self.name = name self.position = position - #: For a geometry node: the call in the USER's own file that reached - #: it, or None when it was written at top level (where `position` - #: already names the user's source). + #: For a geometry node: the whole call chain behind it, INNERMOST + #: frame first, each a `_CallFrame`. Empty for geometry written at + #: top level, where `position` is already the user's own source. #: #: `position` names the node that PRODUCED the geometry, which for #: anything a library builds is inside that library -- a plain - #: `cube(10)` lands in BOSL2's builtins.scad once BOSL2 is included, - #: since BOSL2 overrides the primitives. A caller that wants to show - #: the user their own source wants this instead. - self.call_site = call_site + #: `cube(10)` lands in BOSL2's builtins.scad once BOSL2 is included. + #: A caller that wants a line to show the user walks this and picks + #: the level it can display: the last frame in the running script + #: for an ordinary pick, a deeper one for someone reading the + #: library itself. Library frames are included deliberately. + self.call_sites = tuple(call_sites) + + @property + def call_site(self): + """The innermost frame, or None. Convenience for a caller that does + not care about the chain.""" + return self.call_sites[0] if self.call_sites else None + + +class _CallFrame(_Position): + """One call in a chain: a position, plus whether it is a MODULE call. + + A function frame (`_find_anchor`, an `assert`) has no geometry of its + own to drag, so a gizmo should decline it even though it is worth + showing. + """ + __slots__ = ("is_module",) + + def __init__(self, line, column, origin, start_offset, end_offset, is_module): + super().__init__(line, column, origin, start_offset, end_offset) + self.is_module = is_module class OscObject: @@ -648,10 +670,10 @@ def evaluate(self, source_path: str, viewport_params: Optional[dict] = None, oid: _DeclNode( None, _Position(line, column, origin, start, end), - _Position(cline, ccolumn, corigin, cstart, cend) if has_call else None, + [_CallFrame(cl, cc, co, cs, ce, mod) + for (cs, ce, cl, cc, co, mod) in chain], ) - for oid, (start, end, line, column, origin, - has_call, cstart, cend, cline, ccolumn, corigin) in id_spans.items() + for oid, (start, end, line, column, origin, chain) in id_spans.items() } return bodies, id_to_node diff --git a/src/bytecode_vm.cpp b/src/bytecode_vm.cpp index 60c550d..19d216f 100644 --- a/src/bytecode_vm.cpp +++ b/src/bytecode_vm.cpp @@ -1400,7 +1400,7 @@ Value driveVm(Evaluator& ev, size_t floor) { treeNode->node = site.node; treeNode->isBuiltin = true; treeNode->warnEntry = ev.currentWarnEntry(); - treeNode->userEntry = ev.currentUserCallEntry(); + treeNode->callChain = ev.internCurrentCallChain(); treeNode->children = std::move(children); treeNode->params = std::move(pending.params); treeNode->uncacheable = uncacheable; @@ -1504,7 +1504,7 @@ Value driveVm(Evaluator& ev, size_t floor) { treeNode->node = site.node; treeNode->isBuiltin = true; treeNode->warnEntry = ev.currentWarnEntry(); - treeNode->userEntry = ev.currentUserCallEntry(); + treeNode->callChain = ev.internCurrentCallChain(); treeNode->children = std::move(children); treeNode->params = std::move(params); treeNode->uncacheable = uncacheable; diff --git a/src/csg_generate.cpp b/src/csg_generate.cpp index 074e078..a9c4b1a 100644 --- a/src/csg_generate.cpp +++ b/src/csg_generate.cpp @@ -69,7 +69,7 @@ std::vector Evaluator::generateTreeImpl(const std::vector auto producer = cacheProducer_.find(*key); restampCachedIds(node.bodies, *node.node, producer == cacheProducer_.end() ? nullptr : producer->second, - node.userEntry); + node.callChain); } } else { // Recurse into children first (bottom-up) -- populates each @@ -96,12 +96,12 @@ std::vector Evaluator::generateTreeImpl(const std::vector // can name that line. Restored after (rather than left set) // so a sibling generated at top level doesn't inherit it. const oscad::Position* savedWarnEntry = generateWarnEntry; - const oscad::Position* savedUserEntry = generateUserEntry; + const uint32_t savedCallChain = generateCallChain; generateWarnEntry = node.warnEntry; - generateUserEntry = node.userEntry; + generateCallChain = node.callChain; node.bodies = it->second(*this, node.params, node.children, *node.node); generateWarnEntry = savedWarnEntry; - generateUserEntry = savedUserEntry; + generateCallChain = savedCallChain; } else { // No registered GenerateFn for this kind (a builtin with // display-only semantics like render(), or a non-builtin @@ -335,7 +335,7 @@ std::shared_ptr> remapParts(const std::vector& bodies, const oscad::ASTNode& node, - const oscad::ASTNode* producer, const oscad::Position* callSite) { + const oscad::ASTNode* producer, uint32_t callChain) { // A cache hit hands back the geometry AND the originalIDs of whichever // call site first produced it. Those IDs are provenance -- "which node // made this" -- not content, so two identical shapes at two call sites @@ -383,7 +383,7 @@ void Evaluator::restampCachedIds(std::vector& bodies, const oscad:: // reused at is the line the user would be shown, whatever // produced the original. A module called twice really is // two different call sites. - idToCallSite[fresh] = callSite; + idToCallChain[fresh] = callChain; // The colour has to be re-recorded too, or a later merge // cannot tell this body's runs apart from uncoloured ones // (attachTriColors looks runs up here). Across renders @@ -463,7 +463,7 @@ ColoredBody Evaluator::tagGenerated(manifold::Manifold body, const oscad::ASTNod for (uint32_t originalId : mesh.runOriginalID) { idToNode[originalId] = &node; idToColor[originalId] = color; - idToCallSite[originalId] = generateUserEntry; + idToCallChain[originalId] = generateCallChain; } } ColoredBody cb; @@ -485,7 +485,7 @@ ColoredBody Evaluator::tagDisplayOnly(manifold::MeshGL mesh, const oscad::ASTNod if (!measuring_) { idToNode[originalId] = &node; idToColor[originalId] = color; - idToCallSite[originalId] = generateUserEntry; + idToCallChain[originalId] = generateCallChain; } ColoredBody cb; diff --git a/src/csg_resolve.cpp b/src/csg_resolve.cpp index 52615f2..c7c34df 100644 --- a/src/csg_resolve.cpp +++ b/src/csg_resolve.cpp @@ -68,7 +68,7 @@ void Evaluator::buildTreeNode(const std::string& kind, const oscad::ASTNode& nod treeNode->node = &node; treeNode->isBuiltin = true; treeNode->warnEntry = currentWarnEntry(); - treeNode->userEntry = currentUserCallEntry(); + treeNode->callChain = internCurrentCallChain(); treeNode->children = std::move(children); treeNode->params = std::move(params); treeNode->uncacheable = uncacheable; @@ -140,7 +140,7 @@ void Evaluator::evalModularCall(const oscad::ModularCall& node, EvalContext& ctx treeNode->node = &node; treeNode->isBuiltin = true; treeNode->warnEntry = currentWarnEntry(); - treeNode->userEntry = currentUserCallEntry(); + treeNode->callChain = internCurrentCallChain(); treeNode->uncacheable = uncacheable; treeNode->children = std::move(children); treeNode->params = std::move(params); @@ -166,7 +166,7 @@ void Evaluator::spliceModuleChildren(std::vector> child unionNode->node = &callNode; unionNode->isBuiltin = false; unionNode->warnEntry = currentWarnEntry(); - unionNode->userEntry = currentUserCallEntry(); + unionNode->callChain = internCurrentCallChain(); unionNode->uncacheable = std::any_of(children.begin(), children.end(), [](const auto& c) { return c->uncacheable; }); unionNode->children = std::move(children); setTreeDepthOrThrow(*unionNode, callNode); diff --git a/tests/test_python_bindings.py b/tests/test_python_bindings.py index edcb901..fcbd790 100644 --- a/tests/test_python_bindings.py +++ b/tests/test_python_bindings.py @@ -879,13 +879,19 @@ def test_keep_minuend_color_paints_cut_faces_with_the_minuend(tmp_path): assert all(abs(a - b) < 1e-3 for a, b in zip(kept[0].color[:3], (1.0, 0.647, 0.0))) -def test_id_to_node_carries_the_users_own_call_site(tmp_path): - """A library-built body must be attributable to the line the user wrote. +def _innermost_in(chain, path): + """What a front end does: the deepest frame it can actually show.""" + for f in chain: + if os.path.realpath(f.origin) == os.path.realpath(path): + return f + return None - `position` names the node that PRODUCED the geometry, which for anything - BOSL2 builds is inside BOSL2 -- a plain cube(10) lands in builtins.scad, - since BOSL2 overrides the primitives. `call_site` is what a caller shows - the user instead. + +def test_call_sites_is_the_whole_chain_library_frames_included(tmp_path): + """The chain is NOT filtered to the running script. + + Someone debugging the library wants to step into it, so the evaluator + records every frame and leaves the choice of level to the caller. """ import openscad_cpp_evaluator as E @@ -912,21 +918,22 @@ def test_id_to_node_carries_the_users_own_call_site(tmp_path): assert id_to_node, "the script builds geometry" for node in id_to_node.values(): - # Produced inside the library... - assert node.position.origin.endswith("std.scad"), node.position.origin - # ...but attributable to the user's own line. - assert node.call_site is not None - assert os.path.realpath(node.call_site.origin) == os.path.realpath(str(script)) - assert src[node.call_site.start_offset:node.call_site.end_offset].startswith("wrapped(10)") - - -def test_call_site_is_the_innermost_call_the_user_wrote(tmp_path): - """Not the top-level statement that entered the chain. - - A warning wants the statement to look at; a click wants the line that - placed THAT object, so a drag edits it rather than something wrapping - everything the module makes. - """ + chain = node.call_sites + assert chain, "library-built geometry has a chain" + origins = [os.path.realpath(f.origin) for f in chain] + # Both layers of the library are present... + assert os.path.realpath(str(lib / "std.scad")) in origins + # ...and so is the user's own call, further out. + assert os.path.realpath(str(script)) in origins + # innermost-first: the library frame comes before the script's + assert origins.index(os.path.realpath(str(lib / "std.scad"))) < \ + origins.index(os.path.realpath(str(script))) + # node.call_site is just the innermost, for a caller that does not care + assert node.call_site is chain[0] + + +def test_a_front_end_picks_the_last_frame_in_the_running_script(tmp_path): + """The default a picker wants: the deepest frame the user wrote.""" import openscad_cpp_evaluator as E lib = tmp_path / "MYLIB" @@ -952,12 +959,27 @@ def test_call_site_is_the_innermost_call_the_user_wrote(tmp_path): assert id_to_node for node in id_to_node.values(): - cs = node.call_site - assert cs is not None - text = src[cs.start_offset:cs.end_offset] + frame = _innermost_in(node.call_sites, str(script)) + assert frame is not None + text = src[frame.start_offset:frame.end_offset] assert text.startswith("boxy(8)"), f"got {text!r}, wanted the innermost user call" +def test_module_and_function_frames_are_distinguished(tmp_path): + """A function frame has no geometry of its own, so a gizmo declines it.""" + import openscad_cpp_evaluator as E + + script = tmp_path / "f.scad" + script.write_text("function dbl(x) = x * 2;\n" + "module boxy(s) { cube(dbl(s)); }\n" + "boxy(4);\n") + ev = E.Evaluator() + _bodies, id_to_node = ev.evaluate(str(script), {}) + assert id_to_node + for node in id_to_node.values(): + assert all(isinstance(f.is_module, bool) for f in node.call_sites) + assert any(f.is_module for f in node.call_sites), "boxy() is a module frame" + def test_top_level_geometry_has_no_call_site(): """Nothing to redirect to: `position` is already the user's own node.""" import openscad_cpp_evaluator as E