From 7ed4c8f4b6b6e9e3f50492c89fa6fb681667e5ed Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 2 Aug 2026 17:51:22 +0000 Subject: [PATCH 1/4] Raise recursion depth cap from 200 to 300, backed by empirical measurement MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Issue #83: kMaxCallDepth was a conservative guess with only a vague "800-1000" citation behind it. Re-measured by driving unguarded recursion inside a 1 MiB-stack thread (matching the documented MSVC-stack constraint) until it overflows, for both a light numeric-recursion shape (~1180) and the heavier recursive-list-building idiom the issue calls out (~680) — the new cap keeps >2x margin under the worse of the two while allowing meaningfully deeper legitimate recursion than before. --- src/lang/Interpreter.h | 26 ++++++++++++++++++++++---- tests/test_interpreter.cpp | 13 ++++++++++++- 2 files changed, 34 insertions(+), 5 deletions(-) diff --git a/src/lang/Interpreter.h b/src/lang/Interpreter.h index f4c421d..caf5a47 100644 --- a/src/lang/Interpreter.h +++ b/src/lang/Interpreter.h @@ -169,10 +169,28 @@ 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). Not a full fix for + // #83: real OpenSCAD test files this was filed against + // (recursion-test-function/tail-recursion-tests/issue3118-recur-limit) + // aren't in this repo and weren't reachable to check their exact + // required depth, so this is a data-driven improvement, not a + // guaranteed parity fix — worth revisiting with an MSVC benchmark and/or + // those actual corpus files. + static constexpr int kMaxCallDepth = 300; int m_callDepth = 0; // Guards against a nested list comprehension's element count multiplying diff --git a/tests/test_interpreter.cpp b/tests/test_interpreter.cpp index b23ac12..12575ec 100644 --- a/tests/test_interpreter.cpp +++ b/tests/test_interpreter.cpp @@ -492,13 +492,24 @@ TEST_CASE("Interp:unbounded function recursion returns undef instead of crashing } TEST_CASE("Interp:deep-but-bounded recursion still computes the correct result", "[interp][bugfix]") { - // Depth 100 is comfortably under kMaxCallDepth (200); the guard must not + // Depth 100 is comfortably under kMaxCallDepth (300); the guard must not // affect legitimate recursive functions at ordinary depths. auto ctx = loadEnvWithFuncs("function sum(n) = n <= 0 ? 0 : n + sum(n - 1);"); ExprNode call = makeCall("sum", {100.0}); REQUIRE(ctx.interp.evalNumber(call) == Approx(5050.0)); } +TEST_CASE("Interp:recursion depth just under the raised cap still succeeds (issue #83)", + "[interp][bugfix]") { + // kMaxCallDepth was raised from 200 to 300 (issue #83, see the comment + // on its definition in Interpreter.h for the measurement behind the new + // value) specifically so legitimate recursion in this 200-300 band — + // previously silently truncated to undef — now computes a real result. + auto ctx = loadEnvWithFuncs("function sum(n) = n <= 0 ? 0 : n + sum(n - 1);"); + ExprNode call = makeCall("sum", {250.0}); + REQUIRE(ctx.interp.evalNumber(call) == Approx(250.0 * 251.0 / 2.0)); +} + // --------------------------------------------------------------------------- // Tier A: concat // --------------------------------------------------------------------------- From 8cd7b9031966a5cf67f3519fe6c12e6cfaa990d1 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 4 Aug 2026 06:37:15 +0000 Subject: [PATCH 2/4] Implement tail-call optimization for recursive functions (issue #83) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Raising kMaxCallDepth alone couldn't close #83: fetched the actual OpenSCAD test files it was filed against (recursion-test-function, tail-recursion-tests, issue3118-recur-limit — not present in this repo) and ran them against a real OpenSCAD 2021.01 oracle plus ChiselCAD's scad_dump tool. tail-recursion-tests.scad expects tail-recursive functions to reach depths of 2,000-50,000, which no native-stack cap could ever survive — real OpenSCAD passes this via genuine tail-call optimization (confirmed by reading FunctionCall::evaluate's trampoline loop directly in openscad/openscad's src/core/Expression.cc), not a generous recursion limit. Interpreter::evalFunctionBody() mirrors that design: a named function's body is evaluated via a loop that unwraps ternary branches and let bindings in place and, on hitting a call in tail position (self or mutual), rebinds params and loops into the callee's body directly instead of recursing — no new native stack frame per tail hop, bounded only by kMaxTailHops (1,000,000, matching upstream's own cap) rather than kMaxCallDepth. Non-tail recursion (e.g. `a + f(a-1)`) is untouched, still going through ordinary evaluate() and kMaxCallDepth. Verified against the real corpus file: every echo() line in tail-recursion-tests.scad (minus ftail_mixed, which uses assert()/ echo()-as-expression forms ChiselCAD's grammar doesn't parse at all — a separate, unrelated gap) now matches real OpenSCAD's output exactly. Also fixes a distinct bug surfaced while validating this: every real evaluation entry point (CsgEvaluator::evaluate's convenience overload, HeadlessBuild.cpp, and the scad_dump/scad_to_stl test tools) called loadAssignments() before loadFunctions(), so a top-level assignment calling a user-defined function (e.g. `y = f(3);`) always evaluated to undef — a pre-existing quirk a test comment had already flagged but left unfixed. loadFunctions() only registers non-owning pointers with no evaluation, so swapping the order is strictly safe. --- src/app/HeadlessBuild.cpp | 2 +- src/csg/CsgEvaluator.cpp | 2 +- src/lang/Interpreter.cpp | 52 +++++++++++++++++++++++- src/lang/Interpreter.h | 66 ++++++++++++++++++++++++++---- tests/test_interpreter.cpp | 80 +++++++++++++++++++++++++++++++++---- tests/tools/scad_dump.cpp | 2 +- tests/tools/scad_to_stl.cpp | 2 +- 7 files changed, 187 insertions(+), 19 deletions(-) diff --git a/src/app/HeadlessBuild.cpp b/src/app/HeadlessBuild.cpp index 3e27fcd..f963a4e 100644 --- a/src/app/HeadlessBuild.cpp +++ b/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 diff --git a/src/csg/CsgEvaluator.cpp b/src/csg/CsgEvaluator.cpp index e5d18d0..da51afd 100644 --- a/src/csg/CsgEvaluator.cpp +++ b/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); } diff --git a/src/lang/Interpreter.cpp b/src/lang/Interpreter.cpp index e346464..b43d2da 100644 --- a/src/lang/Interpreter.cpp +++ b/src/lang/Interpreter.cpp @@ -229,6 +229,56 @@ std::unordered_map Interpreter::beginCallScope() { return saved; } +void Interpreter::beginTailCallScope() { + std::unordered_map 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(body)) { + body = (bool(evaluate(*t->condition)) ? t->then : t->else_).get(); + continue; + } + if (auto* let = std::get_if(body)) { + for (const auto& [name, valExpr] : let->bindings) + assignVar(name, *valExpr); + body = let->body.get(); + continue; + } + if (auto* call = std::get_if(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) return Value::undef(); + + std::vector> 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 // --------------------------------------------------------------------------- @@ -545,7 +595,7 @@ Value Interpreter::evaluate(const ExprNode& expr) { bindOrderedArgs(def.params, orderedArgs); ++m_callDepth; - Value result = evaluate(*def.body); + Value result = evalFunctionBody(*def.body); --m_callDepth; restoreEnv(std::move(savedEnv)); return result; diff --git a/src/lang/Interpreter.h b/src/lang/Interpreter.h index caf5a47..31024b2 100644 --- a/src/lang/Interpreter.h +++ b/src/lang/Interpreter.h @@ -120,6 +120,14 @@ class Interpreter { // scope, which this doesn't currently reconstruct. std::unordered_map 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 @@ -183,16 +191,31 @@ class Interpreter { // 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). Not a full fix for - // #83: real OpenSCAD test files this was filed against - // (recursion-test-function/tail-recursion-tests/issue3118-recur-limit) - // aren't in this repo and weren't reachable to check their exact - // required depth, so this is a data-driven improvement, not a - // guaranteed parity fix — worth revisiting with an MSVC benchmark and/or - // those actual corpus files. + // 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; + // 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 @@ -223,6 +246,35 @@ class Interpreter { Value callClosure(Value fnVal, const std::vector>& 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 diff --git a/tests/test_interpreter.cpp b/tests/test_interpreter.cpp index 12575ec..02afb91 100644 --- a/tests/test_interpreter.cpp +++ b/tests/test_interpreter.cpp @@ -335,8 +335,8 @@ static InterpCtx loadEnvWithFuncs(std::string_view src) { Parser parser(std::move(tokens)); InterpCtx ctx; ctx.result = parser.parse(); - ctx.interp.loadAssignments(ctx.result); ctx.interp.loadFunctions(ctx.result); + ctx.interp.loadAssignments(ctx.result); return ctx; } @@ -510,6 +510,74 @@ TEST_CASE("Interp:recursion depth just under the raised cap still succeeds (issu REQUIRE(ctx.interp.evalNumber(call) == Approx(250.0 * 251.0 / 2.0)); } +// --------------------------------------------------------------------------- +// Tail-call optimization (issue #83) — matches upstream OpenSCAD's own +// tests/data/scad/misc/tail-recursion-tests.scad, fetched directly from +// openscad/openscad and run through scad_dump against these exact depths to +// confirm parity (2000/50000 depths can only pass via a real trampoline, not +// by raising kMaxCallDepth — see evalFunctionBody()'s comment in +// Interpreter.h). +// --------------------------------------------------------------------------- +TEST_CASE("Interp:tail-recursive function reaches a depth no native stack could survive", + "[interp][bugfix][tco]") { + auto ctx = loadEnvWithFuncs("function f3c(a, ret = 0) = a <= 0 ? ret : f3c(a - 1, ret + a);"); + ExprNode call = makeCall("f3c", {2000.0}); + REQUIRE(ctx.interp.evalNumber(call) == Approx(2001000.0)); +} + +TEST_CASE("Interp:tail-recursive function builds a 50000-character string (upstream f2a)", + "[interp][bugfix][tco]") { + auto ctx = loadEnvWithFuncs( + "function c(a, b) = chr(a % 26 + b);" + "function f2a(x, y = 0, t = \"\") = x <= 0 ? t : f2a(x - 1, y + 2, str(t, c(y, 65)));"); + ExprNode call = makeCall("f2a", {50000.0}); + Value r = ctx.interp.evaluate(call); + REQUIRE(r.isString()); + REQUIRE(r.asString().size() == 50000); +} + +TEST_CASE("Interp:tail call to a different function still trampolines (mutual tail recursion)", + "[interp][bugfix][tco]") { + auto ctx = loadEnvWithFuncs( + "function isEven(n) = n <= 0 ? true : isOdd(n - 1);" + "function isOdd(n) = n <= 0 ? false : isEven(n - 1);"); + ExprNode call = makeCall("isEven", {10000.0}); + REQUIRE(bool(ctx.interp.evaluate(call)) == true); +} + +TEST_CASE("Interp:non-tail recursion (f3a shape) is unaffected by the trampoline", + "[interp][bugfix][tco]") { + // The recursive call here is an operand of `+`, not the whole tail + // expression, so this must still go through ordinary (kMaxCallDepth- + // guarded) recursion rather than the trampoline — regression check that + // evalFunctionBody's terminal case still defers to plain evaluate(). + auto ctx = loadEnvWithFuncs("function f3a(a) = a <= 0 ? 0 : a + f3a(a - 1);"); + ExprNode call = makeCall("f3a", {100.0}); + REQUIRE(ctx.interp.evalNumber(call) == Approx(5050.0)); +} + +// --------------------------------------------------------------------------- +// loadFunctions()/loadAssignments() ordering (found while verifying #83 +// against upstream's tail-recursion-tests.scad, which assigns a tail- +// recursive call's result to a top-level variable — `s1 = f2a(50000);` — +// before echoing it). Previously every real call site (CsgEvaluator:: +// evaluate's convenience overload, HeadlessBuild.cpp) called +// loadAssignments() before loadFunctions(), so a top-level assignment could +// never see any user-defined function and silently evaluated to undef. +// loadFunctions() only registers non-owning pointers (no evaluation), so +// swapping the order is strictly safe. +// --------------------------------------------------------------------------- +TEST_CASE("Interp:a top-level assignment can call a user-defined function", "[interp][bugfix]") { + Lexer lexer("function f(x) = x * 2; y = f(3);"); + auto tokens = lexer.tokenize(); + Parser parser(std::move(tokens)); + auto result = parser.parse(); + Interpreter interp; + interp.loadFunctions(result); + interp.loadAssignments(result); + REQUIRE(interp.getVar("y").asNumber() == Approx(6.0)); +} + // --------------------------------------------------------------------------- // Tier A: concat // --------------------------------------------------------------------------- @@ -1209,12 +1277,10 @@ TEST_CASE("Interp:calling the closure result of a named function call", "[interp // that result. Exercises CallExpr chained directly onto a FunctionCall, // not just onto a parenthesised FunctionLit. Built as a manual AST // (like the neighboring named-function tests above) rather than via a - // top-level source assignment: loadAssignments() runs before - // loadFunctions() in every call site (see loadEnvWithFuncs below), so a - // *source-level* `jj = adder(2)(5);` assignment would evaluate before - // "adder" is registered — an unrelated pre-existing load-order quirk - // that would otherwise make this test collide with, rather than - // exercise, the currying feature itself. + // source-level `jj = adder(2)(5);` assignment purely to stay consistent + // with its sibling tests above/below — loadFunctions() now runs before + // loadAssignments() (see loadEnvWithFuncs below), so a source-level + // assignment calling a function works fine here too. auto ctx = loadEnvWithFuncs("function adder(x) = function(y) x + y;"); FunctionCall innerCall; diff --git a/tests/tools/scad_dump.cpp b/tests/tools/scad_dump.cpp index ffd2cc7..b391c93 100644 --- a/tests/tools/scad_dump.cpp +++ b/tests/tools/scad_dump.cpp @@ -49,8 +49,8 @@ int main(int argc, char** argv) { csgEval.fileTable = &loaded.files; lang::Interpreter interp; - interp.loadAssignments(ast); interp.loadFunctions(ast); + interp.loadAssignments(ast); auto scene = csgEval.evaluate(ast, interp); for (const auto& d : scene.evalDiags) diff --git a/tests/tools/scad_to_stl.cpp b/tests/tools/scad_to_stl.cpp index f67c360..f56f09c 100644 --- a/tests/tools/scad_to_stl.cpp +++ b/tests/tools/scad_to_stl.cpp @@ -62,8 +62,8 @@ int main(int argc, char** argv) { csgEval.fileTable = &loaded.files; lang::Interpreter interp; - interp.loadAssignments(ast); interp.loadFunctions(ast); + interp.loadAssignments(ast); auto scene = csgEval.evaluate(ast, interp); for (const auto& d : scene.evalDiags) From ec696dab1b8f4a94d53261191719de6e6c49babb Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 4 Aug 2026 06:53:10 +0000 Subject: [PATCH 3/4] Hard-abort the script on detected infinite recursion (issue #83) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The two remaining upstream corpus files from #83 (recursion-test- function.scad, issue3118-recur-limit.scad) both expect an unconditionally- recursive function to abort the entire script with an error, not degrade to undef and keep going — confirmed against a live OpenSCAD 2021.01 binary: earlier echo()s still print, but the statement whose argument triggers the recursion prints nothing, and every later statement is skipped. CsgEvaluator already has exactly this "abort the rest of the script" mechanism (m_aborted), used today for a failed top-level assert(). Gave Interpreter its own recursionAborted() flag, set at the two existing recursion guards (kMaxCallDepth and evalFunctionBody's kMaxTailHops) instead of just silently returning undef, and made evaluate() itself short-circuit once set. CsgEvaluator::checkRecursionAbort() folds that into its own m_aborted the same way a failed assert() already does: called at evalNode()'s top (covers every subsequent statement generically) and right after echo()/assert()'s own argument evaluation (needed since those two produce a visible side effect in the same statement that would otherwise fire using the aborted undef result). Verified against the real corpus files: both now match real OpenSCAD's behavior (no output for the triggering statement, error diagnostic, empty geometry, earlier statements unaffected). Diagnostic wording is intentionally not byte-exact (missing OpenSCAD's "in file X, line Y" and TRACE lines) — filed as #101, same category as #85's existing diagnostic- wording-parity scope, not attempted here. --- src/csg/CsgEvaluator.cpp | 36 +++++++++++++++++++++++---- src/csg/CsgEvaluator.h | 15 +++++++++++ src/lang/Interpreter.cpp | 23 ++++++++++++++--- src/lang/Interpreter.h | 29 ++++++++++++++++++++++ tests/test_csg_evaluator.cpp | 48 ++++++++++++++++++++++++++++++++++++ tests/test_interpreter.cpp | 28 +++++++++++++++++++++ 6 files changed, 171 insertions(+), 8 deletions(-) diff --git a/src/csg/CsgEvaluator.cpp b/src/csg/CsgEvaluator.cpp index da51afd..38a175d 100644 --- a/src/csg/CsgEvaluator.cpp +++ b/src/csg/CsgEvaluator.cpp @@ -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; @@ -758,6 +771,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 +792,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; diff --git a/src/csg/CsgEvaluator.h b/src/csg/CsgEvaluator.h index ca2679a..778a73c 100644 --- a/src/csg/CsgEvaluator.h +++ b/src/csg/CsgEvaluator.h @@ -105,6 +105,21 @@ class CsgEvaluator { // this" semantics for `!`. std::vector 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, diff --git a/src/lang/Interpreter.cpp b/src/lang/Interpreter.cpp index b43d2da..4ee3586 100644 --- a/src/lang/Interpreter.cpp +++ b/src/lang/Interpreter.cpp @@ -260,7 +260,12 @@ Value Interpreter::evalFunctionBody(const ExprNode& startBody) { auto fit = isClosureVar ? m_funcDefs.end() : m_funcDefs.find(call->name); if (fit == m_funcDefs.end()) break; - if (hop >= kMaxTailHops) return Value::undef(); + if (hop >= kMaxTailHops) { + m_recursionAborted = true; + m_recursionAbortedFnName = call->name; + m_recursionAbortedLoc = call->loc; + return Value::undef(); + } std::vector> orderedArgs; orderedArgs.reserve(call->args.size()); @@ -283,6 +288,7 @@ Value Interpreter::evalFunctionBody(const ExprNode& startBody) { // 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; @@ -587,7 +593,12 @@ 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(); @@ -753,8 +764,14 @@ 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 || 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(); diff --git a/src/lang/Interpreter.h b/src/lang/Interpreter.h index 31024b2..bc88edc 100644 --- a/src/lang/Interpreter.h +++ b/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); @@ -216,6 +235,16 @@ class Interpreter { // `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 diff --git a/tests/test_csg_evaluator.cpp b/tests/test_csg_evaluator.cpp index b848db3..20ca3b1 100644 --- a/tests/test_csg_evaluator.cpp +++ b/tests/test_csg_evaluator.cpp @@ -2240,3 +2240,51 @@ TEST_CASE("CsgEval:a module call before its final redefinition still uses the la REQUIRE(s.echoMessages.size() == 1); REQUIRE(s.echoMessages[0].find("second") != std::string::npos); } + +// --------------------------------------------------------------------------- +// Recursion-detected hard abort (issue #83 follow-up) — matches real +// OpenSCAD's fatal "Recursion detected calling function 'X'" behavior for +// an unconditionally-recursive function, verified against a live OpenSCAD +// 2021.01 binary and the actual upstream test files (recursion-test- +// function.scad, issue3118-recur-limit.scad, fetched from openscad/openscad +// since neither is in this repo): no echo output at all for the statement +// whose argument triggers it, an Error diagnostic naming the function, and +// every later top-level statement skipped — the same "rest of script halts" +// semantics already used for a failed assert(). +// --------------------------------------------------------------------------- +TEST_CASE("CsgEval:unconditional self-recursion aborts the script like a failed assert", + "[csg][bugfix]") { + auto s = evaluate("echo(\"before\");" + "function crash() = crash();" + "echo(crash());" + "echo(\"after\");"); + REQUIRE(s.echoMessages.size() == 1); + REQUIRE(s.echoMessages[0].find("before") != std::string::npos); + REQUIRE(s.evalDiags.size() == 1); + REQUIRE(s.evalDiags[0].level == DiagLevel::Error); + REQUIRE(s.evalDiags[0].message.find("Recursion detected") != std::string::npos); + REQUIRE(s.evalDiags[0].message.find("crash") != std::string::npos); +} + +TEST_CASE("CsgEval:redefining a builtin to unconditionally call itself also aborts", + "[csg][bugfix]") { + // Matches upstream issue3118-recur-limit.scad exactly: redefining `sin` + // to ignore its argument and call itself. + auto s = evaluate("function sin(x) = sin();" + "echo(sin(30));"); + REQUIRE(s.echoMessages.empty()); + REQUIRE(s.evalDiags.size() == 1); + REQUIRE(s.evalDiags[0].message.find("Recursion detected") != std::string::npos); + REQUIRE(s.evalDiags[0].message.find("sin") != std::string::npos); +} + +TEST_CASE("CsgEval:ordinary (non-runaway) recursion produces no recursion-abort diagnostic", + "[csg][bugfix]") { + // Regression check: the new abort path must not fire for legitimate + // recursion that actually terminates. + auto s = evaluate("function fact(n) = n <= 1 ? 1 : n * fact(n - 1);" + "echo(fact(5));"); + REQUIRE(s.evalDiags.empty()); + REQUIRE(s.echoMessages.size() == 1); + REQUIRE(s.echoMessages[0].find("120") != std::string::npos); +} diff --git a/tests/test_interpreter.cpp b/tests/test_interpreter.cpp index 02afb91..46f8177 100644 --- a/tests/test_interpreter.cpp +++ b/tests/test_interpreter.cpp @@ -556,6 +556,34 @@ TEST_CASE("Interp:non-tail recursion (f3a shape) is unaffected by the trampoline REQUIRE(ctx.interp.evalNumber(call) == Approx(5050.0)); } +TEST_CASE("Interp:recursionAborted() is set once an unconditional tail call exhausts kMaxTailHops", + "[interp][bugfix]") { + // Matches upstream recursion-test-function.scad's `function crash() = + // crash();` — a bare tail self-call with no base case ever hits + // kMaxTailHops, not kMaxCallDepth (it never grows the native stack). + auto ctx = loadEnvWithFuncs("function crash() = crash();"); + ExprNode call = makeCall("crash", {}); + Value r = ctx.interp.evaluate(call); + REQUIRE(r.isUndef()); + REQUIRE(ctx.interp.recursionAborted()); + REQUIRE(ctx.interp.recursionAbortedFunctionName() == "crash"); +} + +TEST_CASE("Interp:recursionAborted() is also set by callClosure()'s kMaxCallDepth guard", + "[interp][bugfix]") { + // A closure bound to a variable name never goes through + // evalFunctionBody()'s trampoline (only named m_funcDefs entries do — + // see its own comment), so an unconditionally-recursive function + // literal exercises callClosure()'s separate kMaxCallDepth guard + // instead of evalFunctionBody()'s kMaxTailHops one. + auto ctx = loadEnvWithFuncs("f = function(x) f(x);"); + ExprNode call = makeCall("f", {0.0}); + Value r = ctx.interp.evaluate(call); + REQUIRE(r.isUndef()); + REQUIRE(ctx.interp.recursionAborted()); + REQUIRE(ctx.interp.recursionAbortedFunctionName() == "f"); +} + // --------------------------------------------------------------------------- // loadFunctions()/loadAssignments() ordering (found while verifying #83 // against upstream's tail-recursion-tests.scad, which assigns a tail- From 6a168c0d9bd43059a3e860a28bd457fa790ede84 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 4 Aug 2026 07:15:24 +0000 Subject: [PATCH 4/4] Discard a node's own geometry on a mid-evaluation recursion abort MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review feedback on PR #102: checkRecursionAbort() was only called at evalNode()'s top and inline in echo()/assert()'s own argument loops, so a recursion abort discovered while evaluating a *primitive's* own parameter (e.g. `cube(crash())`) wasn't caught until the next evalNode() call — the degenerate leaf built from the resulting undef param still made it into the scene. echo()/assert() needed their own inline checks regardless (they push a side effect during evalModuleCall itself, before evalNode's dispatch even returns), but nothing else funneling through evalNode's std::visit dispatch (primitives, transforms, booleans, module calls, ...) had an equivalent guard. Added one more check right after that dispatch returns: if evaluating this node just tripped the interpreter's recursion guard, discard whatever result was built and return nullptr instead of letting it become part of the scene — matches real OpenSCAD's exception-based unwind, which never produces geometry for the statement that itself triggered the exception. --- src/csg/CsgEvaluator.cpp | 19 +++++++++++++++++++ tests/test_csg_evaluator.cpp | 17 +++++++++++++++++ 2 files changed, 36 insertions(+) diff --git a/src/csg/CsgEvaluator.cpp b/src/csg/CsgEvaluator.cpp index 38a175d..05e4b8e 100644 --- a/src/csg/CsgEvaluator.cpp +++ b/src/csg/CsgEvaluator.cpp @@ -180,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; diff --git a/tests/test_csg_evaluator.cpp b/tests/test_csg_evaluator.cpp index 20ca3b1..b6fd084 100644 --- a/tests/test_csg_evaluator.cpp +++ b/tests/test_csg_evaluator.cpp @@ -2288,3 +2288,20 @@ TEST_CASE("CsgEval:ordinary (non-runaway) recursion produces no recursion-abort REQUIRE(s.echoMessages.size() == 1); REQUIRE(s.echoMessages[0].find("120") != std::string::npos); } + +TEST_CASE("CsgEval:recursion aborting inside a primitive's own parameter drops that primitive too", + "[csg][bugfix]") { + // Regression test for a review comment on PR #102: a recursion abort + // discovered while evaluating a *primitive's* own parameter (as opposed + // to echo()/assert(), which had their own inline checks from the start) + // must still discard that primitive's own geometry, not just halt + // whatever comes after it — matches real OpenSCAD's exception-based + // unwind, which never produces geometry for the statement that itself + // triggered the exception. + auto s = evaluate("function crash() = crash();" + "cube(crash());" + "cube(5);"); + REQUIRE(s.roots.empty()); + REQUIRE(s.evalDiags.size() == 1); + REQUIRE(s.evalDiags[0].message.find("Recursion detected") != std::string::npos); +}