Fix recursion-depth gaps vs. OpenSCAD: tail-call optimization + hard abort on detected recursion (#83)

src/app/HeadlessBuild.cpp

@@ -118,8 +118,8 @@ BuildResult runBuild(const std::filesystem::path& path,
     lang::Interpreter interp;
     interp.setViewport(viewport.vpr[0], viewport.vpr[1], viewport.vpr[2], viewport.vpt[0],
                         viewport.vpt[1], viewport.vpt[2], viewport.vpd, viewport.vpf);
-    interp.loadAssignments(ast);
     interp.loadFunctions(ast);
+    interp.loadAssignments(ast);
     auto scene = csgEval.evaluate(ast, interp);
 
     // Forward echo() output as Info diagnostics

src/csg/CsgEvaluator.cpp

@@ -37,8 +37,8 @@ static std::filesystem::path defaultFontPath() {
 // ---------------------------------------------------------------------------
 CsgScene CsgEvaluator::evaluate(const ParseResult& result) {
     Interpreter defaultInterp;
-    defaultInterp.loadAssignments(result);
     defaultInterp.loadFunctions(result);
+    defaultInterp.loadAssignments(result);
     return evaluate(result, defaultInterp);
 }
 
@@ -102,16 +102,29 @@ CsgScene CsgEvaluator::evaluate(const ParseResult& result, Interpreter& interp)
     return scene;
 }
 
+void CsgEvaluator::checkRecursionAbort() {
+    if (m_aborted || !m_interp || !m_interp->recursionAborted()) return;
+    lang::Diagnostic d;
+    d.level = lang::DiagLevel::Error;
+    d.loc = m_interp->recursionAbortedLoc();
+    d.filePath = resolveFilePath(d.loc.fileId);
+    d.message = "Recursion detected calling function '" + m_interp->recursionAbortedFunctionName() + "'";
+    if (m_scene) m_scene->evalDiags.push_back(std::move(d));
+    m_aborted = true;
+}
+
 // ---------------------------------------------------------------------------
 // Node dispatch
 // ---------------------------------------------------------------------------
 CsgNodePtr CsgEvaluator::evalNode(const AstNode& node, const glm::mat4& xform,
                                   const ColorAttr& color) {
-    // A failed assert() aborts the rest of the script (OpenSCAD semantics):
-    // every remaining statement anywhere in the tree — siblings, later
-    // module-body statements, later for-loop iterations, etc. — funnels
-    // through this function, so bailing out here halts all of them without
-    // needing a check in each individual loop.
+    // A failed assert() (or a hard recursion-detected abort — see
+    // checkRecursionAbort()) aborts the rest of the script (OpenSCAD
+    // semantics): every remaining statement anywhere in the tree —
+    // siblings, later module-body statements, later for-loop iterations,
+    // etc. — funnels through this function, so bailing out here halts all
+    // of them without needing a check in each individual loop.
+    checkRecursionAbort();
     if (m_aborted)
         return nullptr;
 
@@ -167,6 +180,25 @@ CsgNodePtr CsgEvaluator::evalNode(const AstNode& node, const glm::mat4& xform,
         },
         node);
 
+    // A recursion abort discovered anywhere during this node's own
+    // evaluation — not just a nested child statement, but this node's own
+    // parameter/argument evaluation (e.g. evalPrimitive's `cube(crash())`,
+    // evalTransform's matrix params, a module call's own args) — must
+    // discard whatever `result` was just built rather than let it become
+    // part of the scene. Real OpenSCAD's exception-based unwind means the
+    // triggering statement never produces geometry at all, not even a
+    // degenerate leaf built from the resulting undef param; without this,
+    // only echo()/assert() (which have their own inline checks, needed
+    // separately since they push a side effect — an echo message or
+    // diagnostic — *during* evalModuleCall, before this point ever runs)
+    // got that right, and everything else funneling through evalNode
+    // (primitives, transforms, booleans, module calls, ...) would still
+    // contribute one partially-evaluated node before the *next* evalNode()
+    // call caught the abort.
+    checkRecursionAbort();
+    if (m_aborted)
+        return nullptr;
+
     if (!result || mods == ModNone)
         return result;
 
@@ -758,6 +790,13 @@ CsgNodePtr CsgEvaluator::evalModuleCall(const ModuleCallNode& call, const glm::m
             bool first = true;
             for (const auto& arg : call.args) {
                 Value v = m_interp->evaluate(*arg.value);
+                // A recursion-detected abort mid-argument must suppress
+                // this echo's own message entirely (matches real OpenSCAD:
+                // `echo(crash())` never prints anything, since evaluating
+                // the argument never completes) rather than falling through
+                // to print whatever partial/undef value came back.
+                checkRecursionAbort();
+                if (m_aborted) return nullptr;
                 msg += first ? " " : ", ";
                 first = false;
                 if (!arg.name.empty()) msg += arg.name + " = ";
@@ -772,6 +811,12 @@ CsgNodePtr CsgEvaluator::evalModuleCall(const ModuleCallNode& call, const glm::m
     if (call.name == "assert") {
         if (!call.args.empty() && m_scene) {
             Value cond = m_interp->evaluate(*call.args[0].value);
+            // Same reasoning as echo() above: a recursion abort inside the
+            // condition itself should report as a recursion error, not an
+            // assert failure (bool(undef) would otherwise read as false and
+            // push the wrong diagnostic message below).
+            checkRecursionAbort();
+            if (m_aborted) return nullptr;
             if (!bool(cond)) {
                 lang::Diagnostic d;
                 d.level = lang::DiagLevel::Error;

src/csg/CsgEvaluator.h

@@ -105,6 +105,21 @@ class CsgEvaluator {
     // this" semantics for `!`.
     std::vector<CsgNodePtr> m_rootOnlyNodes;
 
+    // Folds a freshly-detected Interpreter::recursionAborted() (see its own
+    // comment — set when a function-call recursion guard trips, matching
+    // real OpenSCAD's fatal "Recursion detected calling function 'X'") into
+    // this evaluator's own m_aborted, the same way a failed top-level
+    // assert() already does: pushes one Error diagnostic and halts the rest
+    // of the script. A no-op once m_aborted is already true (from this or
+    // any other cause), so it's safe to call liberally after any expression
+    // evaluation — called at evalNode()'s top for the general case (every
+    // statement funnels through there) and additionally right after
+    // echo()/assert()'s own argument evaluation below, since those two
+    // produce a visible side effect (an echo message / a diagnostic) in the
+    // same statement that could otherwise fire using the aborted undef
+    // result before the *next* evalNode() call ever gets a chance to check.
+    void checkRecursionAbort();
+
     CsgNodePtr evalNode(const chisel::lang::AstNode& node, const glm::mat4& xform,
                         const ColorAttr& color);
     CsgNodePtr evalPrimitive(const chisel::lang::PrimitiveNode& p, const glm::mat4& xform,

src/lang/Interpreter.cpp

@@ -229,10 +229,66 @@ std::unordered_map<std::string, Value> Interpreter::beginCallScope() {
     return saved;
 }
 
+void Interpreter::beginTailCallScope() {
+    std::unordered_map<std::string, Value> fresh = m_globalEnv;
+    for (const auto& [k, v] : m_env)
+        if (!k.empty() && k[0] == '$') fresh[k] = v;
+    m_env = std::move(fresh);
+}
+
+Value Interpreter::evalFunctionBody(const ExprNode& startBody) {
+    const ExprNode* body = &startBody;
+
+    for (int hop = 0; ; ++hop) {
+        if (auto* t = std::get_if<TernaryExpr>(body)) {
+            body = (bool(evaluate(*t->condition)) ? t->then : t->else_).get();
+            continue;
+        }
+        if (auto* let = std::get_if<LetExpr>(body)) {
+            for (const auto& [name, valExpr] : let->bindings)
+                assignVar(name, *valExpr);
+            body = let->body.get();
+            continue;
+        }
+        if (auto* call = std::get_if<FunctionCall>(body)) {
+            // A variable bound to a function-literal value takes priority
+            // over a same-named `function` def (matches the ordinary
+            // FunctionCall case in evaluate()) and isn't tail-hoppable here
+            // — deferred to the terminal evaluate() call below instead.
+            auto varIt = m_env.find(call->name);
+            bool isClosureVar = varIt != m_env.end() && varIt->second.isFunction();
+            auto fit = isClosureVar ? m_funcDefs.end() : m_funcDefs.find(call->name);
+            if (fit == m_funcDefs.end()) break;
+
+            if (hop >= kMaxTailHops) {
+                m_recursionAborted    = true;
+                m_recursionAbortedFnName = call->name;
+                m_recursionAbortedLoc    = call->loc;
+                return Value::undef();
+            }
+
+            std::vector<std::pair<std::string, Value>> orderedArgs;
+            orderedArgs.reserve(call->args.size());
+            for (const auto& arg : call->args)
+                orderedArgs.push_back({arg.name, evaluate(*arg.value)});
+
+            const FunctionDef& def = *fit->second;
+            beginTailCallScope();
+            bindOrderedArgs(def.params, orderedArgs);
+            body = def.body.get();
+            continue;
+        }
+        break;
+    }
+
+    return evaluate(*body);
+}
+
 // ---------------------------------------------------------------------------
 // evaluate — dispatch on ExprNode variant
 // ---------------------------------------------------------------------------
 Value Interpreter::evaluate(const ExprNode& expr) {
+    if (m_recursionAborted) return Value::undef();
     return std::visit([&](const auto& node) -> Value {
         using T = std::decay_t<decltype(node)>;
 
@@ -537,15 +593,20 @@ Value Interpreter::evaluate(const ExprNode& expr) {
             // Try user-defined function first
             auto fit = m_funcDefs.find(node.name);
             if (fit != m_funcDefs.end()) {
-                if (m_callDepth >= kMaxCallDepth) return Value::undef();
+                if (m_callDepth >= kMaxCallDepth) {
+                    m_recursionAborted       = true;
+                    m_recursionAbortedFnName = node.name;
+                    m_recursionAbortedLoc    = node.loc;
+                    return Value::undef();
+                }
 
                 const FunctionDef& def = *fit->second;
                 auto savedEnv = beginCallScope();
 
                 bindOrderedArgs(def.params, orderedArgs);
 
                 ++m_callDepth;
-                Value result = evaluate(*def.body);
+                Value result = evalFunctionBody(*def.body);
                 --m_callDepth;
                 restoreEnv(std::move(savedEnv));
                 return result;
@@ -703,8 +764,14 @@ void Interpreter::assignVar(const std::string& name, const ExprNode& valueExpr)
 // ---------------------------------------------------------------------------
 Value Interpreter::callClosure(Value fnVal,
                                 const std::vector<std::pair<std::string, Value>>& orderedArgs) {
-    if (!fnVal.closure || !fnVal.closure->def || m_callDepth >= kMaxCallDepth)
+    if (!fnVal.closure || !fnVal.closure->def) return Value::undef();
+    if (m_callDepth >= kMaxCallDepth) {
+        m_recursionAborted       = true;
+        m_recursionAbortedFnName =
+            fnVal.closure->selfName.empty() ? "function" : fnVal.closure->selfName;
+        m_recursionAbortedLoc    = fnVal.closure->def->loc;
         return Value::undef();
+    }
     const FunctionLit& def = *fnVal.closure->def;
 
     auto savedEnv = snapshotEnv();

src/lang/Interpreter.h

@@ -46,6 +46,25 @@ class Interpreter {
     // Evaluate an expression to a Value.
     Value evaluate(const ExprNode& expr);
 
+    // Set once a recursion guard (kMaxCallDepth or evalFunctionBody's
+    // kMaxTailHops) trips — matches real OpenSCAD's fatal "Recursion
+    // detected calling function 'X'" behavior (verified against a live
+    // OpenSCAD 2021.01 binary and openscad/openscad's own
+    // FunctionCall::evaluate), as opposed to every *other* Interpreter
+    // failure mode, which degrades gracefully to undef and keeps going.
+    // Once set, evaluate() itself short-circuits to undef immediately (see
+    // its first line) rather than doing any further work.
+    //
+    // Interpreter has no notion of "abort the whole script" on its own —
+    // that's CsgEvaluator's m_aborted, already used for a failed top-level
+    // assert() — so the caller driving evaluation (CsgEvaluator) is
+    // expected to check this after every expression it evaluates directly
+    // and fold it into that same mechanism. See CsgEvaluator::
+    // checkRecursionAbort().
+    bool recursionAborted() const { return m_recursionAborted; }
+    const std::string& recursionAbortedFunctionName() const { return m_recursionAbortedFnName; }
+    SourceLoc recursionAbortedLoc() const { return m_recursionAbortedLoc; }
+
     // Convenience: evaluate and coerce to double (undef → 0.0).
     double evalNumber(const ExprNode& expr);
 
@@ -120,6 +139,14 @@ class Interpreter {
     // scope, which this doesn't currently reconstruct.
     std::unordered_map<std::string, Value> beginCallScope();
 
+    // Same env swap as beginCallScope(), for the tail-call trampoline in
+    // evalFunctionBody(): each hop discards the previous hop's scope rather
+    // than restoring it (only the *outermost* call's pre-call env, saved
+    // once by beginCallScope() at the trampoline's entry point, ever gets
+    // restored — see evalFunctionBody()), so there's no caller env worth
+    // paying to copy and return here.
+    void beginTailCallScope();
+
     // Module-call-name stack backing parent_module()/$parent_modules. Module
     // calls are evaluated by CsgEvaluator, not here, so CsgEvaluator pushes/
     // pops around each user-module call (mirroring how it already sets
@@ -169,12 +196,55 @@ class Interpreter {
     // carries several locals (snapshotEnv's unordered_map copy, arg vectors,
     // Value copies), and the smallest stack we need to fit under is MSVC's
     // default 1 MiB thread stack (Windows CI build has no custom /STACK).
-    // Empirically, an unguarded 1 MiB-stack GCC Release build overflows
-    // between depth 800-1000; this cap stays well under that with margin to
-    // spare for MSVC's likely-larger per-frame footprint.
-    static constexpr int kMaxCallDepth = 200;
+    //
+    // Re-measured for issue #83 (previous cap: 200) by driving unguarded
+    // recursion (via a scratch copy of this file with kMaxCallDepth raised
+    // to a no-op-large value) inside a pthread with an explicit 1 MiB stack,
+    // GCC 13 -O3 -DNDEBUG, catching the resulting SIGSEGV to binary-search
+    // the deepest surviving call. Two shapes were measured since per-call
+    // frame cost varies a lot by body: a bare numeric recursion
+    // (`f(n) = n<=0 ? 0 : f(n-1)`) survived to ~1180, while the heavier,
+    // more representative "recursive list-building" idiom this issue calls
+    // out (`f(n) = n<=0 ? [] : concat([n], f(n-1))`) only survived to ~680.
+    // 300 keeps a >2x margin under that worse-case measurement — deeper
+    // than the old 200, without eating into the safety margin the original
+    // conservative choice was protecting — while leaving headroom for
+    // MSVC's likely-larger per-frame footprint, which this pass could not
+    // measure directly (no Windows toolchain available).
+    //
+    // This only needs to cover genuinely *non-tail* recursion now — see
+    // evalFunctionBody()/kMaxTailHops below for tail calls, which OpenSCAD's
+    // own tail-recursion-tests.scad (fetched directly from openscad/openscad
+    // to confirm, since it isn't in this repo) needs at depths from 2,000 to
+    // 50,000 that no native-stack cap could satisfy. Checked against that
+    // same file: its one genuinely non-tail-recursive case (`f3a`, `a + f3a
+    // (a - 1)` — the recursive call is an operand of `+`, not the whole tail
+    // expression) only goes to depth 100, well inside this cap either way.
+    static constexpr int kMaxCallDepth = 300;
     int m_callDepth = 0;
 
+    // Bounds evalFunctionBody()'s tail-call trampoline — see its own comment
+    // for why a tail hop needs a completely different (much larger) budget
+    // than kMaxCallDepth above: each hop is a plain loop iteration, not a
+    // new native stack frame, so it costs no stack regardless of how large
+    // this is. Matches real OpenSCAD's own tail-call iteration cap exactly
+    // (`recursion_depth`'s 1,000,000 in FunctionCall::evaluate, upstream
+    // src/core/Expression.cc) rather than picking an arbitrary round number
+    // — confirmed empirically fast enough in practice (a few hundred ms) to
+    // still terminate an unconditionally-recursive tail call like
+    // `function f() = f();` promptly.
+    static constexpr int kMaxTailHops = 1'000'000;
+
+    // Backing state for recursionAborted()/recursionAbortedFunctionName()/
+    // recursionAbortedLoc() above — set at the two recursion-guard trip
+    // sites (kMaxCallDepth in evaluate()'s FunctionCall case and callClosure(),
+    // kMaxTailHops in evalFunctionBody()), never cleared automatically since
+    // an Interpreter is constructed fresh per build/evaluation (see
+    // HeadlessBuild.cpp/CsgEvaluator.cpp) rather than reused across scripts.
+    bool        m_recursionAborted = false;
+    std::string m_recursionAbortedFnName;
+    SourceLoc   m_recursionAbortedLoc;
+
     // Guards against a nested list comprehension's element count multiplying
     // out of control — each individual range is already capped at
     // kMaxRangeCount, but that cap is per-range, so
@@ -205,6 +275,35 @@ class Interpreter {
     Value callClosure(Value fnVal,
                        const std::vector<std::pair<std::string, Value>>& orderedArgs);
 
+    // Evaluates a *named* user function's body, trampolining through any
+    // call in tail position instead of recursing — the fix for issue #83's
+    // real gap: OpenSCAD's own tail-recursion-tests.scad expects tail-
+    // recursive functions to reach depths (2,000-50,000) no reasonable
+    // native-stack cap could ever survive, because real OpenSCAD doesn't
+    // recurse for these either (see FunctionCall::evaluate's trampoline loop
+    // in openscad/openscad's src/core/Expression.cc, confirmed by reading
+    // it directly — this mirrors that design, not a guess).
+    //
+    // "Tail position" here means: the entire value of the function body,
+    // reachable by unwrapping only the chosen branch of a ternary and the
+    // body of a let (ChiselCAD's grammar has no assert()/echo()-as-
+    // expression forms to unwrap, unlike upstream — a tail call wrapped in
+    // one of those, e.g. tail-recursion-tests.scad's ftail_mixed, still
+    // falls back to ordinary recursion, or fails to parse at all if
+    // ChiselCAD doesn't support that form yet). Once unwrapping bottoms out
+    // at a FunctionCall naming another (or the same) m_funcDefs entry, that
+    // call is resolved and looped into directly: no new evaluate() stack
+    // frame, so m_callDepth is never touched by a tail hop — only
+    // kMaxTailHops bounds an unconditionally-recursive tail call (e.g.
+    // `function f() = f();`) from looping forever. Anything else — a
+    // variable bound to a function-literal value (which takes priority per
+    // FunctionCall's own evaluate() case), a builtin, or any non-Ternary/
+    // Let/FunctionCall expression shape — isn't tail-simplifiable, so the
+    // loop stops and defers to one ordinary (real, kMaxCallDepth-guarded)
+    // evaluate() call, matching this function's pre-trampoline behavior
+    // exactly for every case that isn't a bare tail call.
+    Value evalFunctionBody(const ExprNode& body);
+
     // Binds `params` in the current environment by replaying orderedArgs
     // (pairs of arg-name/value in original call-site textual order; an
     // empty name means positional) in that order — a positional arg