Match OpenSCAD's recursion-abort diagnostic wording exactly (file/line + TRACE lines) - #103
Open
particlesector wants to merge 4 commits into
Open
Match OpenSCAD's recursion-abort diagnostic wording exactly (file/line + TRACE lines)#103particlesector wants to merge 4 commits into
particlesector wants to merge 4 commits into
Conversation
The hard-abort diagnostic for detected infinite recursion only named
the function, e.g. "Recursion detected calling function 'crash'".
Real OpenSCAD appends the call site's file and line
("... in file recursion-test-function.scad, line 1"), which
checkRecursionAbort() can now append too since Interpreter already
tracks the abort's SourceLoc via recursionAbortedLoc() — it just
wasn't being surfaced in the message text yet.
The TRACE: call-stack lines from OpenSCAD's full diagnostic (a
separate call-stack-with-locations feature the interpreter doesn't
track today) are still not implemented; that's the larger remaining
half of #101.
) Completes the diagnostic-wording parity started in the previous commit: real OpenSCAD's recursion-detected error is followed by one "TRACE: called by 'X' in file F, line L" line per unwound call frame, innermost first. ChiselCAD's abort mechanism short-circuits instead of unwinding via exceptions, so there was no call-stack to walk for this. Adds that call stack in two halves that mirror where each frame is actually entered: - Interpreter::m_callStack, pushed/popped around each named function/ closure call (evaluate()'s FunctionCall case, callClosure()) — covers the function-call chain. - CsgEvaluator::m_ctxStack, pushed/popped around every evalModuleCall() invocation (echo, assert, every other builtin, every user module) — covers the enclosing module/builtin-call chain. checkRecursionAbort() walks both, innermost to outermost, bolting the resulting TRACE lines onto the existing Diagnostic's message (there's no separate DiagLevel for "trace"; every consumer that prints d.message verbatim reproduces OpenSCAD's multi-line block this way). Verified byte-for-byte against real OpenSCAD 2021.01's actual output for its own upstream test files (recursion-test-function.scad, issue3118-recur-limit.scad — fetched from openscad/openscad, now copied into tests/fixtures/eval_diag/ verbatim), plus new tests for nested module calls and ordinary (non-tail) recursion, which have no upstream oracle but follow the same mechanism. Full suite (646 cases) still passes.
particlesector
commented
Aug 5, 2026
particlesector
left a comment
particlesector
left a comment
Owner
Author
There was a problem hiding this comment.
Reviewed the diagnostic-wording change. The mechanism is sound: m_callStack (function/closure calls) and m_ctxStack (module/builtin calls) are each maintained correctly — pushed/popped in matching pairs, with evalModuleCall()'s use of an RAII popper protecting against early returns — and checkRecursionAbort() walks both innermost-to-outermost to build the TRACE lines, matching OpenSCAD's format. The three m_recursionAbortedStack capture sites are consistent with whether a frame has already been pushed at that point (verified against the actual push/pop code), avoiding both duplication and off-by-one drops. Byte-exact verification against real OpenSCAD output on two upstream fixtures gives good confidence. Left one minor test-coverage nit inline; nothing blocking.
Generated by Claude Code
…site Review feedback on PR #103: the existing TRACE tests all exercise evaluate()'s FunctionCall path, never callClosure() — a separate kMaxCallDepth check and m_callStack push site for recursive function literals (as opposed to `function` defs). Adds a test that recurses via a self-referencing closure to cover it too.
particlesector
commented
Aug 5, 2026
…ll site Review feedback on PR #103: callClosure() pushed fnVal.closure->def->loc (the FunctionLit's own location, i.e. where `function(...) ...` was written) for both m_recursionAbortedLoc and its m_callStack frame, instead of the call site — unlike the sibling FunctionDef path in evaluate(), which correctly uses node.loc (the FunctionCall's own location). Every recursive call through a given closure would report the same fixed line regardless of where each call was actually made. Threads a callLoc parameter through callClosure() from both of its call sites (FunctionCall and CallExpr in evaluate(), which already had the right node.loc available but weren't passing it), and uses that for both fields instead. The previous closure test's script had the recursive call and the closure literal on the same line, so it couldn't tell def-site from call-site apart — passed either way. Rewrote it against a fixture where they're on different lines, asserting the TRACE lines land on the call site and never on the definition line.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
checkRecursionAbort()'s diagnostic for detected infinite recursion now appends OpenSCAD'sin file X, line Ylocation suffix (using theSourceLocthe interpreter already tracks viarecursionAbortedLoc()).TRACE: called by 'X' in file F, line Lcall-stack lines that follow OpenSCAD'sERROR:line, one per unwound call frame, innermost first:Interpreter::m_callStack— pushed/popped around each named function/closure call (evaluate()'sFunctionCallcase,callClosure()) — covers the function-call chain.CsgEvaluator::m_ctxStack— pushed/popped around everyevalModuleCall()invocation (echo,assert, every other builtin, every user module) — covers the enclosing module/builtin-call chain.checkRecursionAbort()walks both, innermost to outermost, and bolts the resulting lines onto the existingDiagnostic'smessage(there's no separateDiagLevelfor "trace"; any consumer that printsd.messageverbatim — e.g.tests/tools/scad_dump.cpp— reproduces OpenSCAD's multi-line block as-is this way).Closes #101.
Verification
Verified byte-for-byte against real OpenSCAD 2021.01's actual output for its own upstream test files (
recursion-test-function.scad,issue3118-recur-limit.scad— fetched fromopenscad/openscad, now copied verbatim intotests/fixtures/eval_diag/), plus new tests covering nested module calls and ordinary (non-tail) recursion, which have no upstream oracle but exercise the same mechanism.This sandbox had no vcpkg/binary-cache access, so I built the pinned Manifold 3.4.1 + Clipper2 2.0.1 versions from source (matching this repo's vcpkg manifest) plus apt packages for glm/spdlog/nlohmann-json/Catch2, and ran the real
chiselcad_testsbinary rather than relying on static review.Test plan
ctest --test-dir build --output-on-failure(or./build/chiselcad_tests) — all 646 cases pass.Generated by Claude Code