diff --git a/src/csg/CsgEvaluator.cpp b/src/csg/CsgEvaluator.cpp index 05e4b8e..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,6 +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() + "'"; + 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& 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..9d640b1 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(); } @@ -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>& 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 = - 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; diff --git a/src/lang/Interpreter.h b/src/lang/Interpreter.h index bc88edc..ba0c185 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 @@ -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>& 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/fixtures/eval_diag/function_recursion.scad b/tests/fixtures/eval_diag/function_recursion.scad new file mode 100644 index 0000000..d01ce36 --- /dev/null +++ b/tests/fixtures/eval_diag/function_recursion.scad @@ -0,0 +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 b6fd084..f70d92f 100644 --- a/tests/test_csg_evaluator.cpp +++ b/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); +}