Match OpenSCAD's recursion-abort diagnostic wording exactly (file/line + TRACE lines)

src/csg/CsgEvaluator.cpp

@@ -98,17 +98,54 @@ CsgScene CsgEvaluator::evaluate(const ParseResult& result, Interpreter& interp)
     m_scene = nullptr;
     m_moduleDefs.clear();
     m_childrenStack.clear();
+    m_ctxStack.clear();
     m_rootOnlyNodes.clear();
     return scene;
 }
 
+// Appends " in file X, line Y" to msg for loc, if loc's file is known —
+// bare filename (not the full resolved path), 1-based line number (loc.line
+// is 0-based internally) — matching OpenSCAD's exact wording. Shared by the
+// ERROR line and every TRACE line checkRecursionAbort() builds below.
+void CsgEvaluator::appendFileLine(std::string& msg, const lang::SourceLoc& loc) const {
+    std::string fp = resolveFilePath(loc.fileId);
+    if (!fp.empty())
+        msg += " in file " + std::filesystem::path(fp).filename().string() + ", line " +
+               std::to_string(loc.line + 1);
+}
+
 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() + "'";
+    appendFileLine(d.message, d.loc);
+
+    // TRACE lines: real OpenSCAD's own diagnostic is this ERROR line
+    // followed by one "TRACE: called by 'X' in file F, line L" per unwound
+    // call frame, innermost first — reproduced here as more lines baked
+    // into this same Diagnostic's message (there's no separate DiagLevel for
+    // "trace"; every consumer that prints d.message verbatim, e.g.
+    // tests/tools/scad_dump.cpp, reproduces OpenSCAD's multi-line block
+    // as-is this way). The first TRACE line always mirrors the ERROR line
+    // itself (OpenSCAD's exception is caught by the very call frame that
+    // detected the recursion, which reports its own current call again) —
+    // then each enclosing Interpreter-level function/closure call
+    // (Interpreter::recursionAbortedStack()), then each enclosing
+    // CsgEvaluator-level module/builtin call (m_ctxStack — e.g. "echo", or a
+    // user module several calls deep), both innermost to outermost.
+    auto appendTrace = [&](const std::string& name, const lang::SourceLoc& loc) {
+        d.message += "\nTRACE: called by '" + name + "'";
+        appendFileLine(d.message, loc);
+    };
+    appendTrace(m_interp->recursionAbortedFunctionName(), m_interp->recursionAbortedLoc());
+    for (const auto& frame : m_interp->recursionAbortedStack())
+        appendTrace(frame.name, frame.loc);
+    for (auto it = m_ctxStack.rbegin(); it != m_ctxStack.rend(); ++it)
+        appendTrace(it->name, it->loc);
+
     if (m_scene) m_scene->evalDiags.push_back(std::move(d));
     m_aborted = true;
 }
@@ -779,6 +816,15 @@ std::string CsgEvaluator::formatValue(const Value& v) {
 // ---------------------------------------------------------------------------
 CsgNodePtr CsgEvaluator::evalModuleCall(const ModuleCallNode& call, const glm::mat4& xform,
                                         const ColorAttr& color) {
+    // Pushed for every call this function handles (children(), echo, every
+    // other builtin, every user module) and popped on any return path via
+    // RAII — see m_ctxStack's own comment.
+    m_ctxStack.push_back({call.name, call.loc});
+    struct CtxPopper {
+        std::vector<CallCtxFrame>& stack;
+        ~CtxPopper() { stack.pop_back(); }
+    } ctxPopper{m_ctxStack};
+
     // ---- Built-in: children() ----
     if (call.name == "children")
         return evalChildren(call, xform, color);

src/csg/CsgEvaluator.h

@@ -73,6 +73,19 @@ class CsgEvaluator {
     // Each user module call pushes its children; evalChildren pops/re-pushes for nesting.
     std::vector<ChildrenFrame> m_childrenStack;
 
+    // One entry of the module/builtin-call chain currently active — pushed
+    // at the top of evalModuleCall() for *every* call (children(), echo,
+    // assert, every other builtin, and every user module), popped when it
+    // returns. Lets checkRecursionAbort() name the enclosing construct(s)
+    // (e.g. "echo", or a user module several calls deep) in a recursion
+    // abort's TRACE lines the same way Interpreter::m_callStack names the
+    // enclosing function call(s) — see that member's own comment.
+    struct CallCtxFrame {
+        std::string name;
+        chisel::lang::SourceLoc loc;
+    };
+    std::vector<CallCtxFrame> m_ctxStack;
+
     // Non-owning pointer to the scene being built — valid during evaluate().
     CsgScene* m_scene = nullptr;
 
@@ -120,6 +133,10 @@ class CsgEvaluator {
     // result before the *next* evalNode() call ever gets a chance to check.
     void checkRecursionAbort();
 
+    // Appends " in file X, line Y" to msg for loc, if loc's file is known
+    // — see definition.
+    void appendFileLine(std::string& msg, const chisel::lang::SourceLoc& loc) const;
+
     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

@@ -264,6 +264,9 @@ Value Interpreter::evalFunctionBody(const ExprNode& startBody) {
                 m_recursionAborted    = true;
                 m_recursionAbortedFnName = call->name;
                 m_recursionAbortedLoc    = call->loc;
+                // m_callStack.back() is this same tail-hopping call, already
+                // captured above — see m_callStack's own comment.
+                m_recursionAbortedStack.assign(m_callStack.rbegin() + 1, m_callStack.rend());
                 return Value::undef();
             }
 
@@ -588,7 +591,7 @@ Value Interpreter::evaluate(const ExprNode& expr) {
             // exists under that name.
             auto varIt = m_env.find(node.name);
             if (varIt != m_env.end() && varIt->second.isFunction())
-                return callClosure(varIt->second, orderedArgs);
+                return callClosure(varIt->second, orderedArgs, node.loc);
 
             // Try user-defined function first
             auto fit = m_funcDefs.find(node.name);
@@ -597,6 +600,9 @@ Value Interpreter::evaluate(const ExprNode& expr) {
                     m_recursionAborted       = true;
                     m_recursionAbortedFnName = node.name;
                     m_recursionAbortedLoc    = node.loc;
+                    // Nothing's been pushed for this (refused) call yet, so
+                    // the whole current stack is "outer" frames.
+                    m_recursionAbortedStack.assign(m_callStack.rbegin(), m_callStack.rend());
                     return Value::undef();
                 }
 
@@ -606,7 +612,9 @@ Value Interpreter::evaluate(const ExprNode& expr) {
                 bindOrderedArgs(def.params, orderedArgs);
 
                 ++m_callDepth;
+                m_callStack.push_back({node.name, node.loc});
                 Value result = evalFunctionBody(*def.body);
+                m_callStack.pop_back();
                 --m_callDepth;
                 restoreEnv(std::move(savedEnv));
                 return result;
@@ -637,7 +645,7 @@ Value Interpreter::evaluate(const ExprNode& expr) {
                 orderedArgs.push_back({arg.name, evaluate(*arg.value)});
 
             if (!callee.isFunction()) return Value::undef();
-            return callClosure(std::move(callee), orderedArgs);
+            return callClosure(std::move(callee), orderedArgs, node.loc);
         }
 
         return Value::undef();
@@ -763,13 +771,16 @@ void Interpreter::assignVar(const std::string& name, const ExprNode& valueExpr)
 // callClosure — invoke a Value::Tag::Function closure
 // ---------------------------------------------------------------------------
 Value Interpreter::callClosure(Value fnVal,
-                                const std::vector<std::pair<std::string, Value>>& orderedArgs) {
+                                const std::vector<std::pair<std::string, Value>>& orderedArgs,
+                                const SourceLoc& callLoc) {
     if (!fnVal.closure || !fnVal.closure->def) return Value::undef();
+    std::string selfName =
+        fnVal.closure->selfName.empty() ? "function" : fnVal.closure->selfName;
     if (m_callDepth >= kMaxCallDepth) {
         m_recursionAborted       = true;
-        m_recursionAbortedFnName =
-            fnVal.closure->selfName.empty() ? "function" : fnVal.closure->selfName;
-        m_recursionAbortedLoc    = fnVal.closure->def->loc;
+        m_recursionAbortedFnName = selfName;
+        m_recursionAbortedLoc    = callLoc;
+        m_recursionAbortedStack.assign(m_callStack.rbegin(), m_callStack.rend());
         return Value::undef();
     }
     const FunctionLit& def = *fnVal.closure->def;
@@ -785,7 +796,9 @@ Value Interpreter::callClosure(Value fnVal,
     bindOrderedArgs(def.params, orderedArgs);
 
     ++m_callDepth;
+    m_callStack.push_back({selfName, callLoc});
     Value result = evaluate(*def.body);
+    m_callStack.pop_back();
     --m_callDepth;
     restoreEnv(std::move(savedEnv));
     return result;

src/lang/Interpreter.h

@@ -65,6 +65,21 @@ class Interpreter {
     const std::string& recursionAbortedFunctionName() const { return m_recursionAbortedFnName; }
     SourceLoc recursionAbortedLoc() const { return m_recursionAbortedLoc; }
 
+    // One entry of the call chain leading to a recursion-detected abort —
+    // see m_callStack and recursionAbortedStack() below.
+    struct CallFrame {
+        std::string name;
+        SourceLoc   loc;
+    };
+
+    // The call chain *above* the abort site, innermost first, captured at
+    // the moment recursionAborted() was set — used to build OpenSCAD's
+    // "TRACE: called by 'X' in file F, line L" lines (one per frame; the
+    // abort site itself, matching recursionAbortedFunctionName()/
+    // recursionAbortedLoc(), is the implicit first TRACE line and isn't
+    // repeated in this list — see CsgEvaluator::checkRecursionAbort()).
+    const std::vector<CallFrame>& recursionAbortedStack() const { return m_recursionAbortedStack; }
+
     // Convenience: evaluate and coerce to double (undef → 0.0).
     double evalNumber(const ExprNode& expr);
 
@@ -244,6 +259,21 @@ class Interpreter {
     bool        m_recursionAborted = false;
     std::string m_recursionAbortedFnName;
     SourceLoc   m_recursionAbortedLoc;
+    std::vector<CallFrame> m_recursionAbortedStack;
+
+    // Active named-call frames, outermost first (m_callStack.back() is the
+    // innermost/most-recently-entered call) — pushed just before a named
+    // function/closure call's body starts evaluating and popped right after
+    // it returns, in evaluate()'s FunctionCall case and in callClosure().
+    // Copied into m_recursionAbortedStack (innermost first, i.e. reversed)
+    // at each of the three recursion-abort trip sites: as a whole, for the
+    // ordinary evaluate()/callClosure() kMaxCallDepth checks (nothing's been
+    // pushed yet for the call that's about to be refused); with its own
+    // back() dropped, for evalFunctionBody()'s kMaxTailHops check (that
+    // back() *is* the frame currently tail-hopping — already represented by
+    // recursionAbortedFnName()/recursionAbortedLoc() — so including it too
+    // would duplicate the trace's first line).
+    std::vector<CallFrame> m_callStack;
 
     // Guards against a nested list comprehension's element count multiplying
     // out of control — each individual range is already capped at
@@ -272,8 +302,17 @@ class Interpreter {
     // m_env wholesale as its first step (to switch to the closure's own
     // captured scope) — a reference into the old map would dangle the
     // moment that happens.
-    Value callClosure(Value fnVal,
-                       const std::vector<std::pair<std::string, Value>>& orderedArgs);
+    //
+    // callLoc is the *call* expression's own location (FunctionCall::loc or
+    // CallExpr::loc at each of this function's two call sites) — deliberately
+    // not fnVal.closure->def->loc (the closure literal's definition site):
+    // a recursion abort's TRACE line for this frame must name where this
+    // particular call was textually made, the same as the sibling
+    // FunctionDef call path a few lines up in evaluate() (which pushes its
+    // own node.loc, not the callee's def->loc either), not the one fixed
+    // location every call of the same closure would otherwise share.
+    Value callClosure(Value fnVal, const std::vector<std::pair<std::string, Value>>& orderedArgs,
+                       const SourceLoc& callLoc);
 
     // Evaluates a *named* user function's body, trampolining through any
     // call in tail position instead of recursing — the fix for issue #83's

tests/fixtures/eval_diag/closure_recursion.scad

@@ -0,0 +1,3 @@
+f = function(n)
+    1 + f(n);
+echo(f(0));

tests/fixtures/eval_diag/function_recursion.scad

@@ -0,0 +1,3 @@
+function crash() = crash();
+// Recursion as module parameter
+echo(crash());

tests/fixtures/eval_diag/issue3118_recur_limit.scad

@@ -0,0 +1,4 @@
+// Issue #3118
+// Verify recursion limit is reached when no arguments provided in call.
+function sin(x) = sin(); 
+echo(sin(30));

tests/test_csg_evaluator.cpp

@@ -2305,3 +2305,115 @@ TEST_CASE("CsgEval:recursion aborting inside a primitive's own parameter drops t
     REQUIRE(s.evalDiags.size() == 1);
     REQUIRE(s.evalDiags[0].message.find("Recursion detected") != std::string::npos);
 }
+
+// Issue #101: full diagnostic-wording parity, including the "TRACE: called
+// by ..." call-stack lines, verified byte-for-byte (modulo the fixture's own
+// filename) against real OpenSCAD 2021.01's actual output for its own
+// upstream test files — both fixtures below are untouched copies of
+// recursion-test-function.scad/issue3118-recur-limit.scad (fetched from
+// openscad/openscad, neither is otherwise in this repo).
+TEST_CASE("CsgEval:recursion-detected abort message matches OpenSCAD's file/line/TRACE wording",
+          "[csg][bugfix]") {
+    auto s = evaluateFile(fixture("eval_diag/function_recursion.scad"));
+    REQUIRE(s.evalDiags.size() == 1);
+    REQUIRE(s.evalDiags[0].message ==
+            "Recursion detected calling function 'crash' in file function_recursion.scad, line 1\n"
+            "TRACE: called by 'crash' in file function_recursion.scad, line 1\n"
+            "TRACE: called by 'echo' in file function_recursion.scad, line 3");
+}
+
+TEST_CASE("CsgEval:recursion-detected abort matches OpenSCAD wording for a redefined builtin too",
+          "[csg][bugfix]") {
+    // Matches upstream issue3118-recur-limit.scad exactly: redefining `sin`
+    // to ignore its argument and call itself.
+    auto s = evaluateFile(fixture("eval_diag/issue3118_recur_limit.scad"));
+    REQUIRE(s.evalDiags.size() == 1);
+    REQUIRE(s.evalDiags[0].message ==
+            "Recursion detected calling function 'sin' in file issue3118_recur_limit.scad, line 3\n"
+            "TRACE: called by 'sin' in file issue3118_recur_limit.scad, line 3\n"
+            "TRACE: called by 'echo' in file issue3118_recur_limit.scad, line 4");
+}
+
+// General case (no upstream oracle for this exact shape, unlike the two
+// byte-exact tests above — but a straightforward extension of the same
+// mechanism): nested user-module calls should each contribute their own
+// TRACE line, innermost to outermost, ending at the echo() that triggered
+// evaluation.
+TEST_CASE("CsgEval:recursion-detected abort TRACE lines walk nested module calls", "[csg][bugfix]") {
+    auto s = evaluate("function crash() = crash();"
+                      "module b() { echo(crash()); }"
+                      "module a() { b(); }"
+                      "a();");
+    REQUIRE(s.evalDiags.size() == 1);
+    const std::string& msg = s.evalDiags[0].message;
+    auto posCrash = msg.find("Recursion detected calling function 'crash'");
+    auto posTraceCrash = msg.find("TRACE: called by 'crash'");
+    auto posEcho = msg.find("TRACE: called by 'echo'");
+    auto posB = msg.find("TRACE: called by 'b'");
+    auto posA = msg.find("TRACE: called by 'a'");
+    REQUIRE(posCrash != std::string::npos);
+    REQUIRE(posTraceCrash != std::string::npos);
+    REQUIRE(posEcho != std::string::npos);
+    REQUIRE(posB != std::string::npos);
+    REQUIRE(posA != std::string::npos);
+    // Innermost (crash's own self-call) to outermost (a() at the top level).
+    REQUIRE(posCrash < posTraceCrash);
+    REQUIRE(posTraceCrash < posEcho);
+    REQUIRE(posEcho < posB);
+    REQUIRE(posB < posA);
+}
+
+// General case, ordinary (non-tail) recursion via kMaxCallDepth rather than
+// evalFunctionBody()'s tail-hop trampoline: each real nested call is its own
+// Interpreter::m_callStack frame, so the abort's TRACE should walk all of
+// them (unlike the collapsed single self-line a pure tail call produces
+// above).
+TEST_CASE("CsgEval:recursion-detected abort TRACE lines walk ordinary (non-tail) call frames",
+          "[csg][bugfix]") {
+    // `1 + f(n)` is not a tail call (the recursive call isn't the whole
+    // body), so this recurses for real rather than trampolining.
+    auto s = evaluate("function f(n) = 1 + f(n);"
+                      "echo(f(0));");
+    REQUIRE(s.evalDiags.size() == 1);
+    const std::string& msg = s.evalDiags[0].message;
+    REQUIRE(msg.find("Recursion detected calling function 'f'") != std::string::npos);
+    std::size_t traceCount = 0;
+    for (std::size_t pos = 0; (pos = msg.find("TRACE: called by 'f'", pos)) != std::string::npos;
+         ++pos)
+        ++traceCount;
+    // One TRACE line per real recursive call frame beneath the abort site —
+    // comfortably more than the single collapsed line a tail call produces.
+    REQUIRE(traceCount > 10);
+    REQUIRE(msg.find("TRACE: called by 'echo'") != std::string::npos);
+}
+
+// Same general case as above, but for a recursive *closure* (a function-
+// literal value bound to a variable) rather than a `function` def — self-
+// calls on a closure route through Interpreter::callClosure(), a separate
+// kMaxCallDepth check/m_callStack push site from evaluate()'s FunctionCall
+// case the tests above exercise (see PR #103 review).
+//
+// The fixture deliberately puts the recursive call `f(n)` on a *different*
+// line from the `function(n)` literal itself: callClosure() must report
+// each frame's location as the call site (this test's line 2), not the
+// closure's definition site (line 1) — pushing def->loc instead would still
+// pass a same-line version of this test undetected (see PR #103 review,
+// which caught exactly that on an earlier version of this test).
+TEST_CASE("CsgEval:recursion-detected abort TRACE lines cover the closure call-frame site too",
+          "[csg][bugfix]") {
+    auto s = evaluateFile(fixture("eval_diag/closure_recursion.scad"));
+    REQUIRE(s.evalDiags.size() == 1);
+    const std::string& msg = s.evalDiags[0].message;
+    REQUIRE(msg.find("Recursion detected calling function 'f' in file closure_recursion.scad, "
+                      "line 2") != std::string::npos);
+    const std::string traceLine = "TRACE: called by 'f' in file closure_recursion.scad, line 2";
+    std::size_t traceCount = 0;
+    for (std::size_t pos = 0; (pos = msg.find(traceLine, pos)) != std::string::npos; ++pos)
+        ++traceCount;
+    REQUIRE(traceCount > 10);
+    // The definition site (line 1) must never appear — that would mean a
+    // frame fell back to def->loc instead of the call site.
+    REQUIRE(msg.find("line 1") == std::string::npos);
+    REQUIRE(msg.find("TRACE: called by 'echo' in file closure_recursion.scad, line 3") !=
+            std::string::npos);
+}