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..05e4b8e 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); } @@ -102,16 +102,29 @@ CsgScene CsgEvaluator::evaluate(const ParseResult& result, Interpreter& interp) return scene; } +void CsgEvaluator::checkRecursionAbort() { + if (m_aborted || !m_interp || !m_interp->recursionAborted()) return; + lang::Diagnostic d; + d.level = lang::DiagLevel::Error; + d.loc = m_interp->recursionAbortedLoc(); + d.filePath = resolveFilePath(d.loc.fileId); + d.message = "Recursion detected calling function '" + m_interp->recursionAbortedFunctionName() + "'"; + if (m_scene) m_scene->evalDiags.push_back(std::move(d)); + m_aborted = true; +} + // --------------------------------------------------------------------------- // Node dispatch // --------------------------------------------------------------------------- CsgNodePtr CsgEvaluator::evalNode(const AstNode& node, const glm::mat4& xform, const ColorAttr& color) { - // A failed assert() aborts the rest of the script (OpenSCAD semantics): - // every remaining statement anywhere in the tree — siblings, later - // module-body statements, later for-loop iterations, etc. — funnels - // through this function, so bailing out here halts all of them without - // needing a check in each individual loop. + // A failed assert() (or a hard recursion-detected abort — see + // checkRecursionAbort()) aborts the rest of the script (OpenSCAD + // semantics): every remaining statement anywhere in the tree — + // siblings, later module-body statements, later for-loop iterations, + // etc. — funnels through this function, so bailing out here halts all + // of them without needing a check in each individual loop. + checkRecursionAbort(); if (m_aborted) return nullptr; @@ -167,6 +180,25 @@ CsgNodePtr CsgEvaluator::evalNode(const AstNode& node, const glm::mat4& xform, }, node); + // A recursion abort discovered anywhere during this node's own + // evaluation — not just a nested child statement, but this node's own + // parameter/argument evaluation (e.g. evalPrimitive's `cube(crash())`, + // evalTransform's matrix params, a module call's own args) — must + // discard whatever `result` was just built rather than let it become + // part of the scene. Real OpenSCAD's exception-based unwind means the + // triggering statement never produces geometry at all, not even a + // degenerate leaf built from the resulting undef param; without this, + // only echo()/assert() (which have their own inline checks, needed + // separately since they push a side effect — an echo message or + // diagnostic — *during* evalModuleCall, before this point ever runs) + // got that right, and everything else funneling through evalNode + // (primitives, transforms, booleans, module calls, ...) would still + // contribute one partially-evaluated node before the *next* evalNode() + // call caught the abort. + checkRecursionAbort(); + if (m_aborted) + return nullptr; + if (!result || mods == ModNone) return result; @@ -758,6 +790,13 @@ CsgNodePtr CsgEvaluator::evalModuleCall(const ModuleCallNode& call, const glm::m bool first = true; for (const auto& arg : call.args) { Value v = m_interp->evaluate(*arg.value); + // A recursion-detected abort mid-argument must suppress + // this echo's own message entirely (matches real OpenSCAD: + // `echo(crash())` never prints anything, since evaluating + // the argument never completes) rather than falling through + // to print whatever partial/undef value came back. + checkRecursionAbort(); + if (m_aborted) return nullptr; msg += first ? " " : ", "; first = false; if (!arg.name.empty()) msg += arg.name + " = "; @@ -772,6 +811,12 @@ CsgNodePtr CsgEvaluator::evalModuleCall(const ModuleCallNode& call, const glm::m if (call.name == "assert") { if (!call.args.empty() && m_scene) { Value cond = m_interp->evaluate(*call.args[0].value); + // Same reasoning as echo() above: a recursion abort inside the + // condition itself should report as a recursion error, not an + // assert failure (bool(undef) would otherwise read as false and + // push the wrong diagnostic message below). + checkRecursionAbort(); + if (m_aborted) return nullptr; if (!bool(cond)) { lang::Diagnostic d; d.level = lang::DiagLevel::Error; 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 e346464..4ee3586 100644 --- a/src/lang/Interpreter.cpp +++ b/src/lang/Interpreter.cpp @@ -229,10 +229,66 @@ 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) { + m_recursionAborted = true; + m_recursionAbortedFnName = call->name; + m_recursionAbortedLoc = call->loc; + 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 // --------------------------------------------------------------------------- Value Interpreter::evaluate(const ExprNode& expr) { + if (m_recursionAborted) return Value::undef(); return std::visit([&](const auto& node) -> Value { using T = std::decay_t; @@ -537,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(); @@ -545,7 +606,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; @@ -703,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 f4c421d..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); @@ -120,6 +139,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 @@ -169,12 +196,55 @@ class Interpreter { // carries several locals (snapshotEnv's unordered_map copy, arg vectors, // Value copies), and the smallest stack we need to fit under is MSVC's // default 1 MiB thread stack (Windows CI build has no custom /STACK). - // Empirically, an unguarded 1 MiB-stack GCC Release build overflows - // between depth 800-1000; this cap stays well under that with margin to - // spare for MSVC's likely-larger per-frame footprint. - static constexpr int kMaxCallDepth = 200; + // + // Re-measured for issue #83 (previous cap: 200) by driving unguarded + // recursion (via a scratch copy of this file with kMaxCallDepth raised + // to a no-op-large value) inside a pthread with an explicit 1 MiB stack, + // GCC 13 -O3 -DNDEBUG, catching the resulting SIGSEGV to binary-search + // the deepest surviving call. Two shapes were measured since per-call + // frame cost varies a lot by body: a bare numeric recursion + // (`f(n) = n<=0 ? 0 : f(n-1)`) survived to ~1180, while the heavier, + // more representative "recursive list-building" idiom this issue calls + // out (`f(n) = n<=0 ? [] : concat([n], f(n-1))`) only survived to ~680. + // 300 keeps a >2x margin under that worse-case measurement — deeper + // than the old 200, without eating into the safety margin the original + // conservative choice was protecting — while leaving headroom for + // MSVC's likely-larger per-frame footprint, which this pass could not + // measure directly (no Windows toolchain available). + // + // This only needs to cover genuinely *non-tail* recursion now — see + // evalFunctionBody()/kMaxTailHops below for tail calls, which OpenSCAD's + // own tail-recursion-tests.scad (fetched directly from openscad/openscad + // to confirm, since it isn't in this repo) needs at depths from 2,000 to + // 50,000 that no native-stack cap could satisfy. Checked against that + // same file: its one genuinely non-tail-recursive case (`f3a`, `a + f3a + // (a - 1)` — the recursive call is an operand of `+`, not the whole tail + // expression) only goes to depth 100, well inside this cap either way. + static constexpr int kMaxCallDepth = 300; int m_callDepth = 0; + // Bounds evalFunctionBody()'s tail-call trampoline — see its own comment + // for why a tail hop needs a completely different (much larger) budget + // than kMaxCallDepth above: each hop is a plain loop iteration, not a + // new native stack frame, so it costs no stack regardless of how large + // this is. Matches real OpenSCAD's own tail-call iteration cap exactly + // (`recursion_depth`'s 1,000,000 in FunctionCall::evaluate, upstream + // src/core/Expression.cc) rather than picking an arbitrary round number + // — confirmed empirically fast enough in practice (a few hundred ms) to + // still terminate an unconditionally-recursive tail call like + // `function f() = f();` promptly. + static constexpr int kMaxTailHops = 1'000'000; + + // Backing state for recursionAborted()/recursionAbortedFunctionName()/ + // recursionAbortedLoc() above — set at the two recursion-guard trip + // sites (kMaxCallDepth in evaluate()'s FunctionCall case and callClosure(), + // kMaxTailHops in evalFunctionBody()), never cleared automatically since + // an Interpreter is constructed fresh per build/evaluation (see + // HeadlessBuild.cpp/CsgEvaluator.cpp) rather than reused across scripts. + bool m_recursionAborted = false; + std::string m_recursionAbortedFnName; + SourceLoc m_recursionAbortedLoc; + // Guards against a nested list comprehension's element count multiplying // out of control — each individual range is already capped at // kMaxRangeCount, but that cap is per-range, so @@ -205,6 +275,35 @@ class Interpreter { Value callClosure(Value fnVal, const std::vector>& 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_csg_evaluator.cpp b/tests/test_csg_evaluator.cpp index b848db3..b6fd084 100644 --- a/tests/test_csg_evaluator.cpp +++ b/tests/test_csg_evaluator.cpp @@ -2240,3 +2240,68 @@ 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); +} + +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); +} diff --git a/tests/test_interpreter.cpp b/tests/test_interpreter.cpp index b23ac12..46f8177 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; } @@ -492,13 +492,120 @@ 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)); +} + +// --------------------------------------------------------------------------- +// 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)); +} + +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- +// 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 // --------------------------------------------------------------------------- @@ -1198,12 +1305,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)