diff --git a/include/openscad_cpp_evaluator/evaluator.hpp b/include/openscad_cpp_evaluator/evaluator.hpp index 8ce609e..272ac31 100644 --- a/include/openscad_cpp_evaluator/evaluator.hpp +++ b/include/openscad_cpp_evaluator/evaluator.hpp @@ -425,6 +425,13 @@ class Evaluator { fastContinueHookSkippable_ = hookSkippable; } + // The statement checkpoint currently in progress at each call depth, + // for the call-site collapse described on checkDebug. Per depth, not a + // single slot: `a = [f(1), f(2), f(3)];` runs each callee's own body + // checkpoints in between, and those must not be mistaken for the + // caller having moved on to a new statement. + std::vector> lastStmtByDepth_; + // The other half of hook-skippable mode's safety net. checkDebug()'s // whole premise (see its own doc comment, debug_profile.cpp) is that it // can skip calling into Python for a line with no breakpoint -- but the @@ -475,7 +482,18 @@ class Evaluator { // (function body entry), and builtins/control.cpp's // resolveBreakpoint/resolveIntersectionFor (free functions). Mirrors // Evaluator._check_debug, including its parameter defaults. - void checkDebug(const oscad::ASTNode& node, EvalContext& ctx, bool forced = false, bool exprLevel = false); + // `callSite` marks the stop fired just before descending into a user + // function or function literal. It is a real, steppable stop -- a + // debugger's Step Over should pause on the call line -- but it is NOT + // a second execution of the line it sits on. When it lands on the same + // line and depth as the statement checkpoint immediately before it (as + // in `x = f(y);`, where the assignment and the call are one line), it + // is dropped, so a breakpoint there fires once per execution instead + // of twice. A call on its own line, or on a different line from the + // enclosing statement, still stops -- which is what keeps stepping + // through a list comprehension alternating for-line/call-line. + void checkDebug(const oscad::ASTNode& node, EvalContext& ctx, bool forced = false, bool exprLevel = false, + bool callSite = false); // (origin, line) for each top-level, non-declaration child of the // node checkDebug() was just called with -- i.e., if that node is a diff --git a/pyproject.toml b/pyproject.toml index 4c63aca..7a4f8f1 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "scikit_build_core.build" [project] name = "openscad_cpp_evaluator" -version = "0.25.0" +version = "0.26.0" description = "C++ OpenSCAD evaluator with Python bindings" readme = "README.md" requires-python = ">=3.12" diff --git a/src/debug_profile.cpp b/src/debug_profile.cpp index c6796f9..11b2278 100644 --- a/src/debug_profile.cpp +++ b/src/debug_profile.cpp @@ -74,9 +74,21 @@ std::vector Evaluator::buildDebugFrames(const EvalContext* ctx) cons return frames; } -void Evaluator::checkDebug(const oscad::ASTNode& node, EvalContext& ctx, bool forced, bool exprLevel) { +void Evaluator::checkDebug(const oscad::ASTNode& node, EvalContext& ctx, bool forced, bool exprLevel, + bool callSite) { if (!debugHooks_.debugHook) return; const oscad::Position& pos = node.position(); + const size_t stmtDepth = callStack_.size(); + if (lastStmtByDepth_.size() <= stmtDepth) lastStmtByDepth_.resize(stmtDepth + 1, {-1, std::string{}}); + if (callSite && !forced && lastStmtByDepth_[stmtDepth] == std::make_pair(pos.line, pos.origin)) { + // The statement this call belongs to already stopped on this exact + // line, at this depth. See checkDebug's declaration for why this + // one is dropped. + return; + } + if (!exprLevel && !callSite) { + lastStmtByDepth_[stmtDepth] = {pos.line, pos.origin}; + } // Fast-continue's hook-skippable mode (setFastContinueBreakpoints' own // doc comment): a plain "Continue" with no step pending needs the debug // hook called ONLY for a line that actually has a breakpoint -- every diff --git a/src/user_calls.cpp b/src/user_calls.cpp index 945ce02..12eda53 100644 --- a/src/user_calls.cpp +++ b/src/user_calls.cpp @@ -547,7 +547,11 @@ std::variant Evaluator::simp const oscad::ASTNode* declNode = ctx.scope->lookupFunction(leftId->name); if (declNode && declNode->kind() == oscad::NodeKind::FunctionDeclaration) { const auto& decl = static_cast(*declNode); - checkDebug(n, ctx); // call-site stop, per hop (see evalFunctionCall's) + // Expression-level: a call sitting inside a larger + // statement is not its own statement, and pausing here + // as well as at the statement makes a breakpoint on + // that line fire twice per execution. + checkDebug(n, ctx, /*forced=*/false, /*exprLevel=*/false, /*callSite=*/true); // call-site stop, per hop const bool hasChunk = useBytecodeVm() && lookupOrCompileChunk(decl) != nullptr; std::optional step = tryTailStepFor(leftId->name, decl, decl.parameters, *decl.expr, hasChunk, n.arguments, ctx, n.position()); @@ -570,7 +574,7 @@ std::variant Evaluator::simp if (const auto* closurePtr = std::get_if(&funcVal); closurePtr && *closurePtr) { const Closure& closure = **closurePtr; const oscad::FunctionLiteral& funcNode = *closure.node; - checkDebug(n, ctx); // call-site stop, function-literal callee + checkDebug(n, ctx, /*forced=*/false, /*exprLevel=*/false, /*callSite=*/true); // call-site stop, function-literal callee const bool hasChunk = useBytecodeVm() && lookupCompiledLiteralChunk(funcNode) != nullptr; std::optional step = tryTailStepFor("", funcNode, funcNode.parameters, *funcNode.body, hasChunk, @@ -886,7 +890,7 @@ Value Evaluator::evalFunctionCall(const oscad::PrimaryCall& node, EvalContext& c // evalUserFunctionCore). Builtins deliberately get none -- // mirrors _eval_function_call, where only the user-function and // function-literal branches call _check_debug. - checkDebug(node, ctx); + checkDebug(node, ctx, /*forced=*/false, /*exprLevel=*/false, /*callSite=*/true); return evalUserFunction(leftId->name, static_cast(*decl), node.arguments, ctx, &node); } @@ -899,7 +903,7 @@ Value Evaluator::evalFunctionCall(const oscad::PrimaryCall& node, EvalContext& c // unknown callee gets exactly one warning below, not two. Value funcVal = leftId ? evalIdentifier(leftId->name, &leftId->position(), ctx, false) : evalExpr(*node.left, ctx); if (const auto* closurePtr = std::get_if(&funcVal); closurePtr && *closurePtr) { - checkDebug(node, ctx); // same call-site stop, function-literal callee + checkDebug(node, ctx, /*forced=*/false, /*exprLevel=*/false, /*callSite=*/true); // same call-site stop, function-literal callee return evalFunctionLiteral(**closurePtr, node.arguments, ctx, &node); } diff --git a/tests/test_bytecode_compiler.cpp b/tests/test_bytecode_compiler.cpp index 2175936..421d0b1 100644 --- a/tests/test_bytecode_compiler.cpp +++ b/tests/test_bytecode_compiler.cpp @@ -197,13 +197,13 @@ TEST(BytecodeCompiler, DollarPrefixedParameterCompiles) { EXPECT_EQ(runCapturingEcho("function withFn($fn) = $fn;\necho(let($fn = 99) withFn());"), "ECHO: undef"); // The proof this is actually running compiled, not silently falling // back: same ternary-bodied shape and technique as - // FastContinueWithNoBreakpointInFunctionUsesVm above (3 stops = - // compiled, 5 = interpreted), with $fn substituted in for the plain + // FastContinueWithNoBreakpointInFunctionUsesVm above (2 stops = + // compiled, 4 = interpreted), with $fn substituted in for the plain // parameter both as the declared name and every body reference. const int stops = countDebugHookStops("function f($fn) = $fn > 0 ? $fn + 1 : $fn - 1;\n" "echo(f(5));", std::unordered_map>{{"", {2}}}); - EXPECT_EQ(stops, 3); + EXPECT_EQ(stops, 2); } TEST(BytecodeCompiler, UndeclaredDollarNamedArgumentReachesDynInsideCompiledFunction) { @@ -828,7 +828,7 @@ TEST(BytecodeCompiler, ClosureWithDollarParameterNowCompilesToo) { // silent even for a closure capturing an enclosing binding with its // own $-parameter. const int stops = countDebugHookStops(script, std::unordered_map>{{"", {2}}}); - EXPECT_EQ(stops, 3); + EXPECT_EQ(stops, 2); } // -- Tail-call optimization, VM path (Phase B) ----------------------------- @@ -1156,7 +1156,7 @@ TEST(BytecodeCompiler, DebugAttachedWithoutFastContinueAlwaysInterprets) { const int stops = countDebugHookStops("function f(x) = x > 0 ? x + 1 : x - 1;\n" "echo(f(5));", std::nullopt); - EXPECT_EQ(stops, 5); + EXPECT_EQ(stops, 4); } TEST(BytecodeCompiler, FastContinueWithNoBreakpointInFunctionUsesVm) { @@ -1167,7 +1167,7 @@ TEST(BytecodeCompiler, FastContinueWithNoBreakpointInFunctionUsesVm) { const int stops = countDebugHookStops("function f(x) = x > 0 ? x + 1 : x - 1;\n" "echo(f(5));", std::unordered_map>{{"", {2}}}); - EXPECT_EQ(stops, 3); + EXPECT_EQ(stops, 2); } TEST(BytecodeCompiler, FastContinueWithBreakpointInsideFunctionStillInterprets) { diff --git a/tests/test_debug_hooks.cpp b/tests/test_debug_hooks.cpp index 305376b..88a121c 100644 --- a/tests/test_debug_hooks.cpp +++ b/tests/test_debug_hooks.cpp @@ -595,17 +595,50 @@ TEST(DebugHooksParity, ListCompCForFiresEveryInitAndIncrSeparately) { EXPECT_EQ(stops.size(), 14u); // + 3 condition checks + 2 body expressions } -// (13) User function / function-literal call sites: a statement-level stop -// in the CALLER's context, before the callee's own body-entry stop. +// (13) User function / function-literal call sites: a stop in the CALLER's +// context, before the callee's own body-entry stop. +// +// DELIBERATE DIVERGENCE from the reference, and only for the case that +// duplicates: when the call sits on the same line and depth as the +// statement that just stopped, its call-site stop is dropped. Otherwise +// every consumer treating a statement-level stop as a breakpoint hit +// pauses twice for one execution of `a = double(5);` -- BelfrySCAD's +// debugger stuttered on Continue and reported the same loop iteration +// twice. Consumers cannot collapse it themselves: fast-continue skips the +// checkpoints in between, so a duplicate is indistinguishable from a +// genuine second visit to the line. +// +// A call on a different line from its enclosing statement still stops -- +// see CallSiteOnItsOwnLineStillStops, which is what keeps Step Over +// through a list comprehension alternating for-line/call-line. TEST(DebugHooksParity, UserFunctionCallSiteStopsBeforeDescendingIntoTheCallee) { - // line 2 twice (assignment, then call site), then line 1 (body entry). + // line 2 (assignment; the call site on the same line is collapsed), + // then line 1 (body entry). EXPECT_EQ(recordStops("function double(x) = x * 2;\na = double(5);\n"), - (std::vector{{2, false, false}, {2, false, false}, {1, false, false}})); + (std::vector{{2, false, false}, {1, false, false}})); } TEST(DebugHooksParity, FunctionLiteralCallSiteAlsoStops) { EXPECT_EQ(recordStops("f = function(x) x * 2;\na = f(5);\n"), - (std::vector{{1, false, false}, {2, false, false}, {2, false, false}, {1, false, false}})); + (std::vector{{1, false, false}, {2, false, false}, {1, false, false}})); +} + +// The other half of the rule above: a call that is NOT on its enclosing +// statement's line keeps its own stop, so stepping still pauses on the +// call before descending. This is the list-comprehension shape. +TEST(DebugHooksParity, CallSiteOnItsOwnLineStillStops) { + const std::vector stops = recordStops("function fx(x) = x*2;\n" + "a = [\n" + " for (i = [0:1])\n" + " fx(i)\n" + "];\n"); + // Line 4 holds only the call. Two iterations, so two call-site stops, + // both statement-level and none collapsed -- the statement that + // stopped before each was the `for` on line 3. + int line4 = 0; + for (const Stop& s : stmtStops(stops)) + if (s.line == 4) ++line4; + EXPECT_EQ(line4, 2); } TEST(DebugHooksParity, BuiltinFunctionCallGetsNoCallSiteStop) { diff --git a/tests/test_tail_calls.cpp b/tests/test_tail_calls.cpp index 3c398f9..93ac0b9 100644 --- a/tests/test_tail_calls.cpp +++ b/tests/test_tail_calls.cpp @@ -283,9 +283,12 @@ TEST(TailCalls, DebugHookFiresPerHopInsideATailChain) { // abandoning TCO whenever a debugger is attached. EXPECT_GT(calls, 1000); // All statement-level stops on line 1 (the whole function declaration): - // 501 ternary stops (n = 500..0), 500 recursive call-site stops, and the - // single body-entry stop. - EXPECT_EQ(bodyEntry, 501 + 500 + 1); + // 501 ternary stops (n = 500..0) and the single body-entry stop. The + // 500 recursive call-site stops still fire -- they are counted in + // `calls` above -- but as expression-level, since a call inside a + // larger expression is not a statement of its own (see + // DebugHooksParity.UserFunctionCallSiteStopsBeforeDescendingIntoTheCallee). + EXPECT_EQ(bodyEntry, 501 + 1); } TEST(TailCalls, DeepNonTailRecursionHitsAControlledErrorInsteadOfCrashingInterpreted) {