Decouple kMaxUserCallDepth's guard from callStack_.size()

include/openscad_cpp_evaluator/evaluator.hpp

@@ -1218,6 +1218,12 @@ class Evaluator {
         // and callStack_.back() is already gone by the time it runs. See
         // activeDeclRefcount_'s own doc comment.
         const oscad::ASTNode* declNode = nullptr;
+        // True only when THIS call incremented nativeUserCallDepth_ (i.e.
+        // skipDepthGuard was false) -- exitUserCall* needs it to decrement
+        // symmetrically, for the same "callStack_.back() is already gone"
+        // reason as `kind`/`declNode` above. See nativeUserCallDepth_'s
+        // own doc comment for why this can't just be "always decrement".
+        bool countedTowardNativeDepth = false;
     };
     // `skipDepthGuard`: kMaxUserCallDepth=50 exists purely to keep the
     // NATIVE C++ stack from overflowing (see its own doc comment) -- true
@@ -1427,6 +1433,37 @@ class Evaluator {
     // eval_error.hpp).
     std::vector<CallStackFrame> callStack_;
 
+    // Count of callStack_ entries that ACTUALLY cost native C++ stack --
+    // i.e. pushed with skipDepthGuard=false (enterUserCall's own
+    // interpreted-call sites: evalUserFunctionCore, evalUserModule).
+    // Deliberately NOT the same number as callStack_.size(): a compiled-
+    // to-compiled push (pushBracketedCallFrame/pushBracketedModuleFrame,
+    // bytecode_vm.cpp, skipDepthGuard=true) still grows callStack_ (for
+    // TRACE/closure-detection/$parent_modules bookkeeping) but costs ZERO
+    // native stack, serviced by driveVm's own heap-based loop instead.
+    //
+    // Checking callStack_.size() itself against kMaxUserCallDepth (the
+    // ORIGINAL design) conflates these two: a real BOSL2 script's own
+    // AMBIENT callStack_ depth, inflated by many cheap skip=true compiled
+    // module-call pushes sitting on callStack_ already, could push a
+    // LATER genuinely-native-recursive call (e.g. an interpreted-path
+    // function call like BOSL2's own `ident()`) over the threshold even
+    // though the REAL native C++ nesting at that point was nowhere near
+    // it -- caught for real: Anklet.scad's own ambient callStack_ depth
+    // (mostly cheap Op::CallModule pushes) reached the mid-30s, tripping
+    // kMaxUserCallDepth=30 for an `ident()` call that itself was nested
+    // only a couple of GENUINE native frames deep. Exactly the same class
+    // of "fixed-count guard conflates logical depth with native-stack
+    // cost" bug driveVmNativeDepth_ already exists to avoid for the
+    // OTHER guard site (bytecode_vm.cpp) -- this is that same fix,
+    // applied here. Incremented/decremented symmetrically by
+    // enterUserCall/exitUserCallSuccess/exitUserCallException, gated on
+    // UserCallHandle::countedTowardNativeDepth (skipDepthGuard is only
+    // available at enter time; the handle carries the decision forward
+    // to exit time, since callStack_.back() -- and thus skipDepthGuard's
+    // own original context -- is already gone by then).
+    size_t nativeUserCallDepth_ = 0;
+
     // Running count of Module-kind frames currently on callStack_ --
     // maintained incrementally by enterUserCall/exitUserCall* rather than
     // rescanned from callStack_ on every module call (buildModuleChildCtx's

pyproject.toml

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

src/user_calls.cpp

@@ -682,13 +682,22 @@ Evaluator::UserCallHandle Evaluator::enterUserCall(const std::string& name, cons
     // entirely; for what's left, the margin check is the proven-UNSAFE
     // mechanism on Windows (confirmed via real CI), this fixed count the
     // proven-safe one.
-    if (!skipDepthGuard && callStack_.size() >= kMaxUserCallDepth) {
+    //
+    // Checked against nativeUserCallDepth_, NOT callStack_.size() -- see
+    // that field's own doc comment (evaluator.hpp) for why: callStack_
+    // also grows from cheap, zero-native-cost skipDepthGuard=true pushes,
+    // which must not count toward this native-stack-safety ceiling.
+    if (!skipDepthGuard && nativeUserCallDepth_ >= kMaxUserCallDepth) {
         error("Recursion too deep while calling " + std::string(isModule ? "module" : "function") + " '" + name + "'",
               declNode);
     }
     UserCallHandle h;
     h.kind = kind;
     h.declNode = &declNode;
+    if (!skipDepthGuard) {
+        ++nativeUserCallDepth_;
+        h.countedTowardNativeDepth = true;
+    }
     h.prof = profileEnter(isModule ? "module" : "function", name, callPos, &declNode.position());
     callStack_.push_back(CallStackFrame{kind, name, callPos, &declNode.position(), &declNode, nullptr, upvalueParent});
     callStack_.back().bodyCtx = &childCtx; // per-frame locals for the debugger
@@ -710,13 +719,15 @@ void Evaluator::exitUserCallSuccess(const std::string& name, const UserCallHandl
     if (fireReturnHook && debugHooks_.returnHook) debugHooks_.returnHook(name, result, static_cast<int>(callStack_.size()));
     callStack_.pop_back();
     if (handle.kind == CallStackFrame::Kind::Module) --moduleCallDepth_;
+    if (handle.countedTowardNativeDepth) --nativeUserCallDepth_;
     noteActiveDeclExit(handle.declNode);
     if (handle.prof) profileExit(*handle.prof);
 }
 
 void Evaluator::exitUserCallException(const UserCallHandle& handle) {
     callStack_.pop_back();
     if (handle.kind == CallStackFrame::Kind::Module) --moduleCallDepth_;
+    if (handle.countedTowardNativeDepth) --nativeUserCallDepth_;
     noteActiveDeclExit(handle.declNode);
     if (handle.prof) profileExit(*handle.prof);
 }

tests/test_bytecode_compiler.cpp

@@ -1390,6 +1390,45 @@ TEST(ModuleBodyCompiles, UnionWrappedRecursionStillHitsTheNativeReentryGuardCont
     }
 }
 
+// Regression test for a real bug this same investigation caught:
+// enterUserCall's own depth guard used to check callStack_.size()
+// directly -- but callStack_ also grows from cheap, zero-native-cost
+// COMPILED module-call pushes (skipDepthGuard=true), which must not
+// count toward the native-stack-safety ceiling a LATER genuinely
+// interpreted call (skipDepthGuard=false) is guarded by. Caught for
+// real: a live BOSL2 script's own ambient callStack_ depth (mostly
+// compiled Op::CallModule pushes) reached the mid-30s, tripping
+// kMaxUserCallDepth=30 for a plain function call even though the REAL
+// native C++ nesting at that point was nowhere near it. Fixed via
+// nativeUserCallDepth_ (evaluator.hpp), a separate counter incremented
+// only for skipDepthGuard=false pushes.
+//
+// recur() recurses 100 levels deep via bare `recur(n-1);` (pure
+// Op::CallModule, skipDepthGuard=true, zero native cost, well past the
+// old kMaxUserCallDepth=30) before calling leaf() at the base case --
+// forced to run INTERPRETED (skipDepthGuard=false) via a fast-continue
+// breakpoint on its own declaration line (mirrors countDebugHookStops'
+// own "force one specific chunk native" trick, above this file's first
+// TEST). Before the fix, this leaf() call -- genuinely nested only 1
+// real native frame deep -- would have falsely tripped the guard purely
+// from callStack_'s own ambient depth.
+TEST(ModuleBodyCompiles, DeepCompiledModuleRecursionDoesNotFalselyTripTheInterpretedFunctionGuard) {
+    ScopedVm vm(true);
+    DebugHooks hooks;
+    hooks.debugHook = [](int, int, bool, bool, const std::string&, const std::vector<CallStackFrame>&,
+                          const DebugFramesFn&) { return DebugAction{}; };
+    Evaluator ev(EchoFn{}, nullptr, nullptr, hooks);
+    ev.setFastContinueBreakpoints(std::unordered_map<std::string, std::set<int>>{{"<string>", {2}}});
+    auto ast = parseSrc("module recur(n) { if (n > 0) { recur(n - 1); } else { cube(leaf()); } }\n"
+                        "function leaf() = 1;\n"
+                        "recur(100);");
+    auto scope = oscad::buildScopes(ast);
+    EvalContext ctx = EvalContext::makeRoot(scope.get());
+    std::vector<std::unique_ptr<CSGNode>> tree = ev.resolveTree(ast, ctx);
+    ASSERT_EQ(tree.size(), 1u);
+    EXPECT_EQ(tree[0]->kind, "cube");
+}
+
 // The realistic case the NativeStatement-gap fix actually targets: the
 // RECURSIVE call itself is a bare statement (pure Op::CallModule, zero
 // native stack, bounded only by the heap-sized kMaxVmCallStackDepth), and