From 3cc34d54c12b3fa5314c2ed7533185865106aaa8 Mon Sep 17 00:00:00 2001 From: agra Date: Wed, 10 Jun 2026 17:43:58 +0300 Subject: [PATCH] fix(0108): break/continue run the loop body's pending defers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit lowerBreak/lowerContinue emitted a bare br, and the enclosing block's emitBlockDefers — seeing the terminator — discarded the pending entries on the assumption a return had already drained them. The breaking iteration's defers were silently skipped, leaking whatever the cleanup released. Lowering.loop_defer_base records the defer-stack height at each loop's body start (while / for / range-for, saved and restored alongside break_target); break/continue drain non-onfail entries down to it in LIFO order via the non-truncating emitLoopExitDefers before branching. Truncation stays with the lexical block exits — the same entries still belong to the fall-through path after the branch containing the break. break/continue outside a loop now diagnose instead of no-op'ing. Regression: examples/0049-basic-defer-break-continue.sx (for and while, break and continue, nested-block LIFO drain). --- examples/0049-basic-defer-break-continue.sx | 46 ++++++++ .../0049-basic-defer-break-continue.exit | 1 + .../0049-basic-defer-break-continue.stderr | 1 + .../0049-basic-defer-break-continue.stdout | 18 +++ .../0108-defer-skipped-on-break-continue.md | 107 ++++++++++++++++++ src/ir/lower.zig | 2 + src/ir/lower/control_flow.zig | 21 +++- src/ir/lower/expr.zig | 4 +- src/ir/lower/stmt.zig | 15 +++ 9 files changed, 211 insertions(+), 4 deletions(-) create mode 100644 examples/0049-basic-defer-break-continue.sx create mode 100644 examples/expected/0049-basic-defer-break-continue.exit create mode 100644 examples/expected/0049-basic-defer-break-continue.stderr create mode 100644 examples/expected/0049-basic-defer-break-continue.stdout create mode 100644 issues/0108-defer-skipped-on-break-continue.md diff --git a/examples/0049-basic-defer-break-continue.sx b/examples/0049-basic-defer-break-continue.sx new file mode 100644 index 0000000..8fc3494 --- /dev/null +++ b/examples/0049-basic-defer-break-continue.sx @@ -0,0 +1,46 @@ +// `defer` runs on EVERY exit from the loop body's scope — fall-through, +// `break`, and `continue` alike (LIFO, including entries from nested blocks +// between the loop and the jump). Covers `for` ranges and `while`. +// Regression (issue 0108): break/continue emitted a bare branch and the +// breaking iteration's defers were silently skipped. + +#import "modules/std.sx"; + +main :: () -> s32 { + for 0..3: (i) { + defer print("cleanup {}\n", i); + if i == 1 { break; } + print("body {}\n", i); + } + print("after break loop\n"); + + for 0..3: (i) { + defer print("c2 {}\n", i); + if i == 1 { continue; } + print("b2 {}\n", i); + } + print("done\n"); + + i := 0; + while i < 3 { + defer print("w{}\n", i); + i += 1; + if i == 2 { continue; } + if i == 3 { break; } + print("wbody{}\n", i); + } + print("while done\n"); + + // A break inside a nested block drains the nested block's defers AND the + // loop body's, in LIFO order. + for 0..2: (j) { + defer print("outer {}\n", j); + { + defer print("inner {}\n", j); + if j == 0 { break; } + } + print("unreached\n"); + } + print("nested done\n"); + 0 +} diff --git a/examples/expected/0049-basic-defer-break-continue.exit b/examples/expected/0049-basic-defer-break-continue.exit new file mode 100644 index 0000000..573541a --- /dev/null +++ b/examples/expected/0049-basic-defer-break-continue.exit @@ -0,0 +1 @@ +0 diff --git a/examples/expected/0049-basic-defer-break-continue.stderr b/examples/expected/0049-basic-defer-break-continue.stderr new file mode 100644 index 0000000..8b13789 --- /dev/null +++ b/examples/expected/0049-basic-defer-break-continue.stderr @@ -0,0 +1 @@ + diff --git a/examples/expected/0049-basic-defer-break-continue.stdout b/examples/expected/0049-basic-defer-break-continue.stdout new file mode 100644 index 0000000..eadd231 --- /dev/null +++ b/examples/expected/0049-basic-defer-break-continue.stdout @@ -0,0 +1,18 @@ +body 0 +cleanup 0 +cleanup 1 +after break loop +b2 0 +c2 0 +c2 1 +b2 2 +c2 2 +done +wbody1 +w1 +w2 +w3 +while done +inner 0 +outer 0 +nested done diff --git a/issues/0108-defer-skipped-on-break-continue.md b/issues/0108-defer-skipped-on-break-continue.md new file mode 100644 index 0000000..c1ff684 --- /dev/null +++ b/issues/0108-defer-skipped-on-break-continue.md @@ -0,0 +1,107 @@ +# RESOLVED — 0108: `defer` silently skipped on `break` / `continue` loop exits + +**Root cause:** `lowerBreak`/`lowerContinue` emitted a bare `br`; the enclosing +block's `emitBlockDefers` then saw the terminator and discarded the pending +entries on the assumption they were already emitted (true only for +return/raise). + +**Fix:** `Lowering.loop_defer_base` records the defer-stack height at each +loop's body start (`lowerWhile` / `lowerFor` / `lowerRuntimeRangeFor`, +saved/restored like `break_target`); `lowerBreak`/`lowerContinue` drain +non-`onfail` entries down to it in LIFO order via the new, non-truncating +`emitLoopExitDefers` (`src/ir/lower/stmt.zig`) before branching — truncation +stays with the lexical block exits, since the same entries still belong to the +fall-through path. `break`/`continue` outside a loop now diagnose +(`` `break` outside a loop ``) instead of silently no-op'ing. + +**Regression test:** `examples/0049-basic-defer-break-continue.sx` (`for` +break + continue, `while` break + continue, nested-block LIFO drain; the +breaking iteration's cleanups were missing pre-fix). + +--- + +# 0108 — `defer` silently skipped on `break` / `continue` loop exits + +**Symptom.** A `defer` registered inside a loop body does not run when the +iteration exits via `break` or `continue`. Observed: the cleanup for the +breaking/continuing iteration never executes. Expected (specs.md §6 Defer: +"`defer expr;` schedules `expr` to execute when the enclosing scope block +exits"): `break`/`continue` exit the loop-body scope, so all pending defers of +that iteration must fire before the jump. The normal fall-through end of an +iteration DOES run them — only the `break`/`continue` paths skip. + +Resource impact: `for ... { f := open(...); defer close(f); if cond { break; } }` +leaks the handle on the break path. Same for `continue` (leaks once per +continued iteration). Affects `for` (collection, range) and `while` equally — +all share `lowerBreak`/`lowerContinue`. + +## Reproduction + +```sx +#import "modules/std.sx"; + +main :: () -> s32 { + for 0..3: (i) { + defer print("cleanup {}\n", i); + if i == 1 { break; } + print("body {}\n", i); + } + print("after break loop\n"); + + for 0..3: (i) { + defer print("c2 {}\n", i); + if i == 1 { continue; } + print("b2 {}\n", i); + } + print("done\n"); + 0 +} +``` + +- **Observed** (current master): + `body 0 / cleanup 0 / after break loop / b2 0 / c2 0 / b2 2 / c2 2 / done` + — `cleanup 1` and `c2 1` are missing. +- **Expected**: + `body 0 / cleanup 0 / cleanup 1 / after break loop / b2 0 / c2 0 / c2 1 / b2 2 / c2 2 / done` + +Repro co-located: `issues/0108-defer-skipped-on-break-continue.sx` (unpinned — +pin as the regression once fixed, with the expected output above). + +## Root cause (suspected area) + +`src/ir/lower/control_flow.zig` — `lowerBreak` / `lowerContinue` (~864-876) +emit a bare `self.builder.br(target)` without draining the defer stack. +Contrast `lowerReturn` (`src/ir/lower/stmt.zig` ~501), which calls +`self.emitBlockDefers(self.func_defer_base)` before `ret`. After the bare +`br`, the enclosing `lowerBlock`'s scope-exit `emitBlockDefers` sees +`currentBlockHasTerminator()` and **discards** the entries under the +assumption "cleanups were already emitted" (`stmt.zig` ~1016) — true for +return/raise, false for break/continue. So the cleanups are dropped, not +deferred-elsewhere. + +## Investigation prompt (paste into a fresh session) + +> Fix issue 0108: `defer` is skipped on `break`/`continue` exits. +> +> 1. Record the loop's defer base: in `lowerFor` / `lowerRuntimeRangeFor` / +> `lowerWhile` (`src/ir/lower/control_flow.zig`), alongside the existing +> save/restore of `break_target`/`continue_target`, save +> `self.defer_stack.items.len` into a new `Lowering` field (e.g. +> `loop_defer_base: usize`), restoring the old value after the body. +> 2. In `lowerBreak`/`lowerContinue`, before the `br`, emit pending non-onfail +> cleanups from `defer_stack.items.len` down to `loop_defer_base` in LIFO +> order **without truncating the stack** (mirror `emitErrorCleanup`'s +> non-truncating walk in `src/ir/lower/stmt.zig`, success-exit filtering +> like `emitBlockDefers`). Truncation must stay with the lexical +> `lowerBlock` scope exits — the same defer entries still belong to the +> fall-through lowering path after the `if { break; }` arm. +> 3. `inline for` (`lowerInlineRangeFor`) bodies lower through `lowerBlock` +> per unrolled iteration; check a `break` inside one targets the enclosing +> runtime loop with the same drain (and that `break` with no enclosing loop +> gets a diagnostic rather than the current silent no-op `Ref.none`). +> +> Verify: run the repro in `issues/0108-defer-skipped-on-break-continue.sx`, +> expect `cleanup 1` after `body 0`/`cleanup 0`, and `c2 1` between `c2 0` and +> `b2 2`. Add a `while`-loop break/continue + defer case. Then promote to +> `examples/00xx-basic-defer-break-continue.sx` per the resolution flow, and +> run `zig build && zig build test && bash tests/run_examples.sh` (all ok). diff --git a/src/ir/lower.zig b/src/ir/lower.zig index ef5a891..a40f7d3 100644 --- a/src/ir/lower.zig +++ b/src/ir/lower.zig @@ -204,6 +204,7 @@ pub const Lowering = struct { scope: ?*Scope = null, break_target: ?BlockId = null, continue_target: ?BlockId = null, + loop_defer_base: usize = 0, // defer-stack height at the innermost loop's body start (break/continue drain to here) block_counter: u32 = 0, comptime_counter: u32 = 0, main_file: ?[]const u8 = null, // path of the main file; imported functions are declared extern @@ -1357,6 +1358,7 @@ pub const Lowering = struct { pub const lowerOnFail = lower_stmt.lowerOnFail; pub const diagOnFailNotFailable = lower_stmt.diagOnFailNotFailable; pub const emitBlockDefers = lower_stmt.emitBlockDefers; + pub const emitLoopExitDefers = lower_stmt.emitLoopExitDefers; pub const lowerCleanupBody = lower_stmt.lowerCleanupBody; pub const emitErrorCleanup = lower_stmt.emitErrorCleanup; diff --git a/src/ir/lower/control_flow.zig b/src/ir/lower/control_flow.zig index efbe6db..c0fc53f 100644 --- a/src/ir/lower/control_flow.zig +++ b/src/ir/lower/control_flow.zig @@ -229,11 +229,14 @@ pub fn lowerWhile(self: *Lowering, we: *const ast.WhileExpr) Ref { // Save and set loop targets const old_break = self.break_target; const old_continue = self.continue_target; + const old_defer_base = self.loop_defer_base; self.break_target = exit_bb; self.continue_target = header_bb; + self.loop_defer_base = self.defer_stack.items.len; defer { self.break_target = old_break; self.continue_target = old_continue; + self.loop_defer_base = old_defer_base; } self.lowerBlock(we.body); @@ -371,13 +374,16 @@ pub fn lowerFor(self: *Lowering, fe: *const ast.ForExpr) Ref { // Save and set loop targets const old_break = self.break_target; const old_continue = self.continue_target; + const old_defer_base = self.loop_defer_base; self.break_target = exit_bb; self.continue_target = inc_bb; // continue → increment, not header + self.loop_defer_base = self.defer_stack.items.len; self.lowerBlock(fe.body); self.break_target = old_break; self.continue_target = old_continue; + self.loop_defer_base = old_defer_base; self.scope = old_scope; body_scope.deinit(); @@ -433,13 +439,16 @@ pub fn lowerRuntimeRangeFor(self: *Lowering, fe: *const ast.ForExpr, end_node: * const old_break = self.break_target; const old_continue = self.continue_target; + const old_defer_base = self.loop_defer_base; self.break_target = exit_bb; self.continue_target = inc_bb; + self.loop_defer_base = self.defer_stack.items.len; self.lowerBlock(fe.body); self.break_target = old_break; self.continue_target = old_continue; + self.loop_defer_base = old_defer_base; self.scope = old_scope; body_scope.deinit(); @@ -876,16 +885,24 @@ pub fn lowerMatch(self: *Lowering, me: *const ast.MatchExpr) Ref { return self.builder.constInt(0, .void); } -pub fn lowerBreak(self: *Lowering) Ref { +pub fn lowerBreak(self: *Lowering, span: ast.Span) Ref { if (self.break_target) |target| { + // Leaving the loop body's scope: run the defers registered since the + // loop began (LIFO) before the jump — same as the fall-through exit. + self.emitLoopExitDefers(); self.builder.br(target, &.{}); + } else if (self.diagnostics) |d| { + d.addFmt(.err, span, "`break` outside a loop", .{}); } return Ref.none; } -pub fn lowerContinue(self: *Lowering) Ref { +pub fn lowerContinue(self: *Lowering, span: ast.Span) Ref { if (self.continue_target) |target| { + self.emitLoopExitDefers(); self.builder.br(target, &.{}); + } else if (self.diagnostics) |d| { + d.addFmt(.err, span, "`continue` outside a loop", .{}); } return Ref.none; } diff --git a/src/ir/lower/expr.zig b/src/ir/lower/expr.zig index 20042cb..e75882c 100644 --- a/src/ir/lower/expr.zig +++ b/src/ir/lower/expr.zig @@ -1793,8 +1793,8 @@ pub fn lowerExpr(self: *Lowering, node: *const Node) Ref { .match_expr => |me| self.lowerMatch(&me), .while_expr => |we| self.lowerWhile(&we), .for_expr => |fe| self.lowerFor(&fe), - .break_expr => self.lowerBreak(), - .continue_expr => self.lowerContinue(), + .break_expr => self.lowerBreak(node.span), + .continue_expr => self.lowerContinue(node.span), .call => |c| self.lowerCall(&c), .ffi_intrinsic_call => |fic| self.lowerFfiIntrinsicCall(&fic), .field_access => |fa| self.lowerFieldAccess(&fa, node.span), diff --git a/src/ir/lower/stmt.zig b/src/ir/lower/stmt.zig index f245570..e0beb78 100644 --- a/src/ir/lower/stmt.zig +++ b/src/ir/lower/stmt.zig @@ -1033,6 +1033,21 @@ pub fn emitBlockDefers(self: *Lowering, saved_len: usize) void { self.defer_stack.shrinkRetainingCapacity(saved_len); } +/// Emit pending `defer` cleanups for a `break`/`continue` exit: everything +/// registered since the innermost loop's body began, in LIFO order. `onfail` +/// entries are skipped (a break is a success exit). The stack is NOT +/// truncated — the same entries still belong to the fall-through lowering +/// path after the branch that contains the break; the enclosing block scopes +/// truncate as usual. +pub fn emitLoopExitDefers(self: *Lowering) void { + const stack = self.defer_stack.items; + var i = stack.len; + while (i > self.loop_defer_base) { + i -= 1; + if (!stack[i].is_onfail) self.lowerCleanupBody(stack[i].body); + } +} + /// Run a `defer`/`onfail` cleanup body for its side effects (void context). /// A braced body lowers as statements (NOT as a value) so a trailing-`;` /// last expression is fine here — cleanup bodies never yield a value.