From f7088b5a68a873bf0875593c347cd518c35aa3c6 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 5 Aug 2026 00:16:06 +0000 Subject: [PATCH 1/4] Include file/line in recursion-abort diagnostic (issue #101) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The hard-abort diagnostic for detected infinite recursion only named the function, e.g. "Recursion detected calling function 'crash'". Real OpenSCAD appends the call site's file and line ("... in file recursion-test-function.scad, line 1"), which checkRecursionAbort() can now append too since Interpreter already tracks the abort's SourceLoc via recursionAbortedLoc() — it just wasn't being surfaced in the message text yet. The TRACE: call-stack lines from OpenSCAD's full diagnostic (a separate call-stack-with-locations feature the interpreter doesn't track today) are still not implemented; that's the larger remaining half of #101. --- src/csg/CsgEvaluator.cpp | 7 +++++++ tests/fixtures/eval_diag/function_recursion.scad | 2 ++ tests/test_csg_evaluator.cpp | 14 ++++++++++++++ 3 files changed, 23 insertions(+) create mode 100644 tests/fixtures/eval_diag/function_recursion.scad diff --git a/src/csg/CsgEvaluator.cpp b/src/csg/CsgEvaluator.cpp index 05e4b8e..4161091 100644 --- a/src/csg/CsgEvaluator.cpp +++ b/src/csg/CsgEvaluator.cpp @@ -109,6 +109,13 @@ void CsgEvaluator::checkRecursionAbort() { d.loc = m_interp->recursionAbortedLoc(); d.filePath = resolveFilePath(d.loc.fileId); d.message = "Recursion detected calling function '" + m_interp->recursionAbortedFunctionName() + "'"; + // Matches OpenSCAD's exact wording (e.g. "... in file + // recursion-test-function.scad, line 1") — bare filename, not the full + // resolved path, and 1-based like every other user-facing line number + // in this codebase (SourceLoc::line is 0-based internally). + if (!d.filePath.empty()) + d.message += " in file " + std::filesystem::path(d.filePath).filename().string() + + ", line " + std::to_string(d.loc.line + 1); if (m_scene) m_scene->evalDiags.push_back(std::move(d)); m_aborted = true; } diff --git a/tests/fixtures/eval_diag/function_recursion.scad b/tests/fixtures/eval_diag/function_recursion.scad new file mode 100644 index 0000000..2a07b91 --- /dev/null +++ b/tests/fixtures/eval_diag/function_recursion.scad @@ -0,0 +1,2 @@ +function crash() = crash(); +echo(crash()); diff --git a/tests/test_csg_evaluator.cpp b/tests/test_csg_evaluator.cpp index b6fd084..c83e36a 100644 --- a/tests/test_csg_evaluator.cpp +++ b/tests/test_csg_evaluator.cpp @@ -2305,3 +2305,17 @@ 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: when the recursion-abort site's file is known (i.e. evaluated +// via a real loaded file with a fileTable, unlike the inline-source tests +// above), the diagnostic wording should carry OpenSCAD's "in file X, line Y" +// location suffix rather than just the bare function name. +TEST_CASE("CsgEval:recursion-detected abort message carries file and line when known", + "[csg][bugfix]") { + auto s = evaluateFile(fixture("eval_diag/function_recursion.scad")); + REQUIRE(s.evalDiags.size() == 1); + REQUIRE(s.evalDiags[0].message.find("Recursion detected calling function 'crash'") != + std::string::npos); + REQUIRE(s.evalDiags[0].message.find("in file function_recursion.scad, line 1") != + std::string::npos); +} From 4436658b91107c660d28ae70727f40c0fbb197a2 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 5 Aug 2026 01:28:12 +0000 Subject: [PATCH 2/4] Add TRACE call-stack lines to the recursion-abort diagnostic (issue #101) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Completes the diagnostic-wording parity started in the previous commit: real OpenSCAD's recursion-detected error is followed by one "TRACE: called by 'X' in file F, line L" line per unwound call frame, innermost first. ChiselCAD's abort mechanism short-circuits instead of unwinding via exceptions, so there was no call-stack to walk for this. Adds that call stack in two halves that mirror where each frame is actually entered: - Interpreter::m_callStack, pushed/popped around each named function/ closure call (evaluate()'s FunctionCall case, callClosure()) — covers the function-call chain. - CsgEvaluator::m_ctxStack, pushed/popped around every evalModuleCall() invocation (echo, assert, every other builtin, every user module) — covers the enclosing module/builtin-call chain. checkRecursionAbort() walks both, innermost to outermost, bolting the resulting TRACE lines onto the existing Diagnostic's message (there's no separate DiagLevel for "trace"; every consumer that prints d.message verbatim reproduces OpenSCAD's multi-line block this way). Verified byte-for-byte against real OpenSCAD 2021.01's actual output for its own upstream test files (recursion-test-function.scad, issue3118-recur-limit.scad — fetched from openscad/openscad, now copied into tests/fixtures/eval_diag/ verbatim), plus new tests for nested module calls and ordinary (non-tail) recursion, which have no upstream oracle but follow the same mechanism. Full suite (646 cases) still passes. --- src/csg/CsgEvaluator.cpp | 53 ++++++++++-- src/csg/CsgEvaluator.h | 17 ++++ src/lang/Interpreter.cpp | 16 +++- src/lang/Interpreter.h | 30 +++++++ .../eval_diag/function_recursion.scad | 1 + .../eval_diag/issue3118_recur_limit.scad | 4 + tests/test_csg_evaluator.cpp | 85 +++++++++++++++++-- 7 files changed, 188 insertions(+), 18 deletions(-) create mode 100644 tests/fixtures/eval_diag/issue3118_recur_limit.scad diff --git a/src/csg/CsgEvaluator.cpp b/src/csg/CsgEvaluator.cpp index 4161091..8ec79dd 100644 --- a/src/csg/CsgEvaluator.cpp +++ b/src/csg/CsgEvaluator.cpp @@ -98,10 +98,22 @@ 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; @@ -109,13 +121,31 @@ void CsgEvaluator::checkRecursionAbort() { d.loc = m_interp->recursionAbortedLoc(); d.filePath = resolveFilePath(d.loc.fileId); d.message = "Recursion detected calling function '" + m_interp->recursionAbortedFunctionName() + "'"; - // Matches OpenSCAD's exact wording (e.g. "... in file - // recursion-test-function.scad, line 1") — bare filename, not the full - // resolved path, and 1-based like every other user-facing line number - // in this codebase (SourceLoc::line is 0-based internally). - if (!d.filePath.empty()) - d.message += " in file " + std::filesystem::path(d.filePath).filename().string() + - ", line " + std::to_string(d.loc.line + 1); + 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; } @@ -786,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& stack; + ~CtxPopper() { stack.pop_back(); } + } ctxPopper{m_ctxStack}; + // ---- Built-in: children() ---- if (call.name == "children") return evalChildren(call, xform, color); diff --git a/src/csg/CsgEvaluator.h b/src/csg/CsgEvaluator.h index 778a73c..71bfd3f 100644 --- a/src/csg/CsgEvaluator.h +++ b/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 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 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, diff --git a/src/lang/Interpreter.cpp b/src/lang/Interpreter.cpp index 4ee3586..c20e1c2 100644 --- a/src/lang/Interpreter.cpp +++ b/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(); } @@ -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; @@ -765,11 +773,13 @@ void Interpreter::assignVar(const std::string& name, const ExprNode& valueExpr) Value Interpreter::callClosure(Value fnVal, const std::vector>& orderedArgs) { 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_recursionAbortedFnName = selfName; m_recursionAbortedLoc = fnVal.closure->def->loc; + m_recursionAbortedStack.assign(m_callStack.rbegin(), m_callStack.rend()); return Value::undef(); } const FunctionLit& def = *fnVal.closure->def; @@ -785,7 +795,9 @@ Value Interpreter::callClosure(Value fnVal, bindOrderedArgs(def.params, orderedArgs); ++m_callDepth; + m_callStack.push_back({selfName, def.loc}); Value result = evaluate(*def.body); + m_callStack.pop_back(); --m_callDepth; restoreEnv(std::move(savedEnv)); return result; diff --git a/src/lang/Interpreter.h b/src/lang/Interpreter.h index bc88edc..fcf8d00 100644 --- a/src/lang/Interpreter.h +++ b/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& 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 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 m_callStack; // Guards against a nested list comprehension's element count multiplying // out of control — each individual range is already capped at diff --git a/tests/fixtures/eval_diag/function_recursion.scad b/tests/fixtures/eval_diag/function_recursion.scad index 2a07b91..d01ce36 100644 --- a/tests/fixtures/eval_diag/function_recursion.scad +++ b/tests/fixtures/eval_diag/function_recursion.scad @@ -1,2 +1,3 @@ function crash() = crash(); +// Recursion as module parameter echo(crash()); diff --git a/tests/fixtures/eval_diag/issue3118_recur_limit.scad b/tests/fixtures/eval_diag/issue3118_recur_limit.scad new file mode 100644 index 0000000..72c340b --- /dev/null +++ b/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)); \ No newline at end of file diff --git a/tests/test_csg_evaluator.cpp b/tests/test_csg_evaluator.cpp index c83e36a..f3ab789 100644 --- a/tests/test_csg_evaluator.cpp +++ b/tests/test_csg_evaluator.cpp @@ -2306,16 +2306,83 @@ TEST_CASE("CsgEval:recursion aborting inside a primitive's own parameter drops t REQUIRE(s.evalDiags[0].message.find("Recursion detected") != std::string::npos); } -// Issue #101: when the recursion-abort site's file is known (i.e. evaluated -// via a real loaded file with a fileTable, unlike the inline-source tests -// above), the diagnostic wording should carry OpenSCAD's "in file X, line Y" -// location suffix rather than just the bare function name. -TEST_CASE("CsgEval:recursion-detected abort message carries file and line when known", +// 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.find("Recursion detected calling function 'crash'") != - std::string::npos); - REQUIRE(s.evalDiags[0].message.find("in file function_recursion.scad, line 1") != - std::string::npos); + 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); } From 8c2fac03bc5aa117c720148d5cedfead543c301f Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 5 Aug 2026 14:22:20 +0000 Subject: [PATCH 3/4] Add a recursion-abort TRACE test covering callClosure()'s call-frame site MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review feedback on PR #103: the existing TRACE tests all exercise evaluate()'s FunctionCall path, never callClosure() — a separate kMaxCallDepth check and m_callStack push site for recursive function literals (as opposed to `function` defs). Adds a test that recurses via a self-referencing closure to cover it too. --- tests/test_csg_evaluator.cpp | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/tests/test_csg_evaluator.cpp b/tests/test_csg_evaluator.cpp index f3ab789..03b90be 100644 --- a/tests/test_csg_evaluator.cpp +++ b/tests/test_csg_evaluator.cpp @@ -2386,3 +2386,25 @@ TEST_CASE("CsgEval:recursion-detected abort TRACE lines walk ordinary (non-tail) 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). +TEST_CASE("CsgEval:recursion-detected abort TRACE lines cover the closure call-frame site too", + "[csg][bugfix]") { + // Non-tail (the recursive call isn't the whole body) and ignores `n`, + // so this never terminates on its own. + auto s = evaluate("f = function(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; + REQUIRE(traceCount > 10); + REQUIRE(msg.find("TRACE: called by 'echo'") != std::string::npos); +} From 017e7f014bb306706d88401c97a078f5fe70980c Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 5 Aug 2026 15:43:30 +0000 Subject: [PATCH 4/4] Fix callClosure() reporting the closure's definition site, not the call site MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review feedback on PR #103: callClosure() pushed fnVal.closure->def->loc (the FunctionLit's own location, i.e. where `function(...) ...` was written) for both m_recursionAbortedLoc and its m_callStack frame, instead of the call site — unlike the sibling FunctionDef path in evaluate(), which correctly uses node.loc (the FunctionCall's own location). Every recursive call through a given closure would report the same fixed line regardless of where each call was actually made. Threads a callLoc parameter through callClosure() from both of its call sites (FunctionCall and CallExpr in evaluate(), which already had the right node.loc available but weren't passing it), and uses that for both fields instead. The previous closure test's script had the recursive call and the closure literal on the same line, so it couldn't tell def-site from call-site apart — passed either way. Rewrote it against a fixture where they're on different lines, asserting the TRACE lines land on the call site and never on the definition line. --- src/lang/Interpreter.cpp | 11 ++++---- src/lang/Interpreter.h | 13 ++++++++-- .../fixtures/eval_diag/closure_recursion.scad | 3 +++ tests/test_csg_evaluator.cpp | 25 +++++++++++++------ 4 files changed, 37 insertions(+), 15 deletions(-) create mode 100644 tests/fixtures/eval_diag/closure_recursion.scad diff --git a/src/lang/Interpreter.cpp b/src/lang/Interpreter.cpp index c20e1c2..9d640b1 100644 --- a/src/lang/Interpreter.cpp +++ b/src/lang/Interpreter.cpp @@ -591,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); @@ -645,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(); @@ -771,14 +771,15 @@ 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>& orderedArgs) { + const std::vector>& 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 = selfName; - m_recursionAbortedLoc = fnVal.closure->def->loc; + m_recursionAbortedLoc = callLoc; m_recursionAbortedStack.assign(m_callStack.rbegin(), m_callStack.rend()); return Value::undef(); } @@ -795,7 +796,7 @@ Value Interpreter::callClosure(Value fnVal, bindOrderedArgs(def.params, orderedArgs); ++m_callDepth; - m_callStack.push_back({selfName, def.loc}); + m_callStack.push_back({selfName, callLoc}); Value result = evaluate(*def.body); m_callStack.pop_back(); --m_callDepth; diff --git a/src/lang/Interpreter.h b/src/lang/Interpreter.h index fcf8d00..ba0c185 100644 --- a/src/lang/Interpreter.h +++ b/src/lang/Interpreter.h @@ -302,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>& 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>& 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 diff --git a/tests/fixtures/eval_diag/closure_recursion.scad b/tests/fixtures/eval_diag/closure_recursion.scad new file mode 100644 index 0000000..87d5210 --- /dev/null +++ b/tests/fixtures/eval_diag/closure_recursion.scad @@ -0,0 +1,3 @@ +f = function(n) + 1 + f(n); +echo(f(0)); diff --git a/tests/test_csg_evaluator.cpp b/tests/test_csg_evaluator.cpp index 03b90be..f70d92f 100644 --- a/tests/test_csg_evaluator.cpp +++ b/tests/test_csg_evaluator.cpp @@ -2392,19 +2392,28 @@ TEST_CASE("CsgEval:recursion-detected abort TRACE lines walk ordinary (non-tail) // 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]") { - // Non-tail (the recursive call isn't the whole body) and ignores `n`, - // so this never terminates on its own. - auto s = evaluate("f = function(n) 1 + f(n);" - "echo(f(0));"); + 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'") != std::string::npos); + 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("TRACE: called by 'f'", pos)) != std::string::npos; - ++pos) + for (std::size_t pos = 0; (pos = msg.find(traceLine, pos)) != std::string::npos; ++pos) ++traceCount; REQUIRE(traceCount > 10); - REQUIRE(msg.find("TRACE: called by 'echo'") != std::string::npos); + // 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); }