One scope table for a whole use-resolution, not one per level

pyproject.toml

@@ -4,7 +4,7 @@ build-backend = "scikit_build_core.build"
 
 [project]
 name = "openscad_cpp_evaluator"
-version = "1.4.0"
+version = "1.4.1"
 description = "C++ OpenSCAD evaluator with Python bindings"
 readme = "README.md"
 requires-python = ">=3.12"

src/eval_use.cpp

@@ -17,8 +17,19 @@ ResolvedUseScopes resolveUseScopes(const std::vector<std::unique_ptr<oscad::ASTN
     return resolveUseScopes(borrowed, currentFile, logFn);
 }
 
-ResolvedUseScopes resolveUseScopes(const std::vector<const oscad::ASTNode*>& ownNodes,
-                                    const std::string& currentFile, const std::function<void(const std::string&)>& logFn) {
+namespace {
+
+// The recursion. `table` is threaded through every level rather than each
+// level building its own: `use <file>` gives each used file its own root
+// Scope, but one evaluation reads all of them back, so every node's scope
+// has to land in ONE table. A table per level meant the outer evaluation
+// could not see anything a nested resolution recorded -- every node of a
+// used file read back as having no scope, and `use` silently resolved to
+// undef. BelfrySCAD's own suite caught that; this repo's did not.
+ResolvedUseScopes resolveUseScopesInto(const std::vector<const oscad::ASTNode*>& ownNodes,
+                                        const std::string& currentFile,
+                                        const std::function<void(const std::string&)>& logFn,
+                                        oscad::ScopeTable& table) {
     ResolvedUseScopes result;
 
     std::vector<const oscad::ASTNode*> injected;
@@ -56,7 +67,10 @@ ResolvedUseScopes resolveUseScopes(const std::vector<const oscad::ASTNode*>& own
         result.usedFileAsts.push_back(std::move(lib.ast));
         const std::vector<std::unique_ptr<oscad::ASTNode>>& libAst = result.usedFileAsts.back();
 
-        ResolvedUseScopes nested = resolveUseScopes(libAst, lib.resolvedPath, logFn);
+        std::vector<const oscad::ASTNode*> libBorrowed;
+        libBorrowed.reserve(libAst.size());
+        for (const auto& n : libAst) libBorrowed.push_back(n.get());
+        ResolvedUseScopes nested = resolveUseScopesInto(libBorrowed, lib.resolvedPath, logFn, table);
 
         std::vector<const oscad::ASTNode*> libInjected;
         for (const oscad::ASTNode* n : nested.ownNodesFiltered) {
@@ -92,12 +106,11 @@ ResolvedUseScopes resolveUseScopes(const std::vector<const oscad::ASTNode*>& own
     std::vector<oscad::ASTNode*> mutableProcessed;
     mutableProcessed.reserve(result.processedNodes.size());
     for (const oscad::ASTNode* n : result.processedNodes) mutableProcessed.push_back(const_cast<oscad::ASTNode*>(n));
-    result.rootScope = oscad::buildScopes(mutableProcessed);
+    result.rootScope = oscad::buildScopesInto(mutableProcessed, table);
 
     {
-        // Re-anchoring writes scopes too, into the same table buildScopes()
-        // just filled -- which the root scope now owns.
-        oscad::ScopeTableScope recording(*const_cast<oscad::ScopeTable*>(result.rootScope->table()));
+        // Re-anchoring writes scopes too, into the same shared table.
+        oscad::ScopeTableScope recording(table);
         for (const auto& [libInjected, libRootScope] : reanchor) {
             for (const oscad::ASTNode* n : libInjected) const_cast<oscad::ASTNode*>(n)->buildScope(*libRootScope);
         }
@@ -106,4 +119,16 @@ ResolvedUseScopes resolveUseScopes(const std::vector<const oscad::ASTNode*>& own
     return result;
 }
 
+} // namespace
+
+ResolvedUseScopes resolveUseScopes(const std::vector<const oscad::ASTNode*>& ownNodes, const std::string& currentFile,
+                                    const std::function<void(const std::string&)>& logFn) {
+    // One table for the whole resolution, handed to the root scope at the
+    // end so it lives exactly as long as the scopes it points into.
+    auto table = std::make_unique<oscad::ScopeTable>();
+    ResolvedUseScopes result = resolveUseScopesInto(ownNodes, currentFile, logFn, *table);
+    if (result.rootScope) result.rootScope->adoptTable(std::move(table));
+    return result;
+}
+
 } // namespace oscadeval

tests/test_include_cache.cpp

@@ -130,3 +130,32 @@ TEST(IncludeCache, ADerivedContextKeepsTheScopeTable) {
     ev.evaluate(used.processedNodes, ctx, {}, /*generate=*/false);
     for (const std::string& m : logs) EXPECT_EQ(m.find("Recursion"), std::string::npos) << m;
 }
+
+
+TEST(IncludeCache, NestedUseKeepsItsScopesInTheSharedTable) {
+    // `use <file>` gives each used file its own root Scope, and a used file
+    // may itself `use` another. All of them are read back by ONE
+    // evaluation, so every node's scope has to land in ONE ScopeTable.
+    //
+    // The version of this that built a table per recursion level threw the
+    // nested ones away: the outer evaluation then saw no scope at all for a
+    // used file's nodes, and combo() below resolved to undef instead of
+    // 107. Every C++ test here passed anyway -- BelfrySCAD's suite is what
+    // caught it -- so this is that case, kept where the code lives.
+    writeFile("inner.scad", "inner_val = 100;\nfunction get_inner() = inner_val;\n");
+    writeFile("lib2.scad",
+              "use <oscad_inccache_inner.scad>\n"
+              "lib2_val = 7;\n"
+              "function combo() = get_inner() + lib2_val;\n");
+    const auto main2 = writeFile("main2.scad",
+                                  "use <oscad_inccache_lib2.scad>\n"
+                                  "echo(combo());\n"
+                                  "echo(is_undef(inner_val));\n");
+
+    const std::vector<std::string> echoes = echoesOf(main2);
+    ASSERT_EQ(echoes.size(), 2u);
+    // combo() reaches through lib2's own nested use into inner.scad.
+    EXPECT_EQ(echoes[0], "ECHO: 107");
+    // ...while inner.scad's own declarations stay invisible to main2.
+    EXPECT_EQ(echoes[1], "ECHO: true");
+}