fix(0108): break/continue run the loop body's pending defers
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).
This commit is contained in:
46
examples/0049-basic-defer-break-continue.sx
Normal file
46
examples/0049-basic-defer-break-continue.sx
Normal file
@@ -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
|
||||||
|
}
|
||||||
1
examples/expected/0049-basic-defer-break-continue.exit
Normal file
1
examples/expected/0049-basic-defer-break-continue.exit
Normal file
@@ -0,0 +1 @@
|
|||||||
|
0
|
||||||
1
examples/expected/0049-basic-defer-break-continue.stderr
Normal file
1
examples/expected/0049-basic-defer-break-continue.stderr
Normal file
@@ -0,0 +1 @@
|
|||||||
|
|
||||||
18
examples/expected/0049-basic-defer-break-continue.stdout
Normal file
18
examples/expected/0049-basic-defer-break-continue.stdout
Normal file
@@ -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
|
||||||
107
issues/0108-defer-skipped-on-break-continue.md
Normal file
107
issues/0108-defer-skipped-on-break-continue.md
Normal file
@@ -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).
|
||||||
@@ -204,6 +204,7 @@ pub const Lowering = struct {
|
|||||||
scope: ?*Scope = null,
|
scope: ?*Scope = null,
|
||||||
break_target: ?BlockId = null,
|
break_target: ?BlockId = null,
|
||||||
continue_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,
|
block_counter: u32 = 0,
|
||||||
comptime_counter: u32 = 0,
|
comptime_counter: u32 = 0,
|
||||||
main_file: ?[]const u8 = null, // path of the main file; imported functions are declared extern
|
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 lowerOnFail = lower_stmt.lowerOnFail;
|
||||||
pub const diagOnFailNotFailable = lower_stmt.diagOnFailNotFailable;
|
pub const diagOnFailNotFailable = lower_stmt.diagOnFailNotFailable;
|
||||||
pub const emitBlockDefers = lower_stmt.emitBlockDefers;
|
pub const emitBlockDefers = lower_stmt.emitBlockDefers;
|
||||||
|
pub const emitLoopExitDefers = lower_stmt.emitLoopExitDefers;
|
||||||
pub const lowerCleanupBody = lower_stmt.lowerCleanupBody;
|
pub const lowerCleanupBody = lower_stmt.lowerCleanupBody;
|
||||||
pub const emitErrorCleanup = lower_stmt.emitErrorCleanup;
|
pub const emitErrorCleanup = lower_stmt.emitErrorCleanup;
|
||||||
|
|
||||||
|
|||||||
@@ -229,11 +229,14 @@ pub fn lowerWhile(self: *Lowering, we: *const ast.WhileExpr) Ref {
|
|||||||
// Save and set loop targets
|
// Save and set loop targets
|
||||||
const old_break = self.break_target;
|
const old_break = self.break_target;
|
||||||
const old_continue = self.continue_target;
|
const old_continue = self.continue_target;
|
||||||
|
const old_defer_base = self.loop_defer_base;
|
||||||
self.break_target = exit_bb;
|
self.break_target = exit_bb;
|
||||||
self.continue_target = header_bb;
|
self.continue_target = header_bb;
|
||||||
|
self.loop_defer_base = self.defer_stack.items.len;
|
||||||
defer {
|
defer {
|
||||||
self.break_target = old_break;
|
self.break_target = old_break;
|
||||||
self.continue_target = old_continue;
|
self.continue_target = old_continue;
|
||||||
|
self.loop_defer_base = old_defer_base;
|
||||||
}
|
}
|
||||||
|
|
||||||
self.lowerBlock(we.body);
|
self.lowerBlock(we.body);
|
||||||
@@ -371,13 +374,16 @@ pub fn lowerFor(self: *Lowering, fe: *const ast.ForExpr) Ref {
|
|||||||
// Save and set loop targets
|
// Save and set loop targets
|
||||||
const old_break = self.break_target;
|
const old_break = self.break_target;
|
||||||
const old_continue = self.continue_target;
|
const old_continue = self.continue_target;
|
||||||
|
const old_defer_base = self.loop_defer_base;
|
||||||
self.break_target = exit_bb;
|
self.break_target = exit_bb;
|
||||||
self.continue_target = inc_bb; // continue → increment, not header
|
self.continue_target = inc_bb; // continue → increment, not header
|
||||||
|
self.loop_defer_base = self.defer_stack.items.len;
|
||||||
|
|
||||||
self.lowerBlock(fe.body);
|
self.lowerBlock(fe.body);
|
||||||
|
|
||||||
self.break_target = old_break;
|
self.break_target = old_break;
|
||||||
self.continue_target = old_continue;
|
self.continue_target = old_continue;
|
||||||
|
self.loop_defer_base = old_defer_base;
|
||||||
self.scope = old_scope;
|
self.scope = old_scope;
|
||||||
body_scope.deinit();
|
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_break = self.break_target;
|
||||||
const old_continue = self.continue_target;
|
const old_continue = self.continue_target;
|
||||||
|
const old_defer_base = self.loop_defer_base;
|
||||||
self.break_target = exit_bb;
|
self.break_target = exit_bb;
|
||||||
self.continue_target = inc_bb;
|
self.continue_target = inc_bb;
|
||||||
|
self.loop_defer_base = self.defer_stack.items.len;
|
||||||
|
|
||||||
self.lowerBlock(fe.body);
|
self.lowerBlock(fe.body);
|
||||||
|
|
||||||
self.break_target = old_break;
|
self.break_target = old_break;
|
||||||
self.continue_target = old_continue;
|
self.continue_target = old_continue;
|
||||||
|
self.loop_defer_base = old_defer_base;
|
||||||
self.scope = old_scope;
|
self.scope = old_scope;
|
||||||
body_scope.deinit();
|
body_scope.deinit();
|
||||||
|
|
||||||
@@ -876,16 +885,24 @@ pub fn lowerMatch(self: *Lowering, me: *const ast.MatchExpr) Ref {
|
|||||||
return self.builder.constInt(0, .void);
|
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| {
|
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, &.{});
|
self.builder.br(target, &.{});
|
||||||
|
} else if (self.diagnostics) |d| {
|
||||||
|
d.addFmt(.err, span, "`break` outside a loop", .{});
|
||||||
}
|
}
|
||||||
return Ref.none;
|
return Ref.none;
|
||||||
}
|
}
|
||||||
|
|
||||||
pub fn lowerContinue(self: *Lowering) Ref {
|
pub fn lowerContinue(self: *Lowering, span: ast.Span) Ref {
|
||||||
if (self.continue_target) |target| {
|
if (self.continue_target) |target| {
|
||||||
|
self.emitLoopExitDefers();
|
||||||
self.builder.br(target, &.{});
|
self.builder.br(target, &.{});
|
||||||
|
} else if (self.diagnostics) |d| {
|
||||||
|
d.addFmt(.err, span, "`continue` outside a loop", .{});
|
||||||
}
|
}
|
||||||
return Ref.none;
|
return Ref.none;
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -1793,8 +1793,8 @@ pub fn lowerExpr(self: *Lowering, node: *const Node) Ref {
|
|||||||
.match_expr => |me| self.lowerMatch(&me),
|
.match_expr => |me| self.lowerMatch(&me),
|
||||||
.while_expr => |we| self.lowerWhile(&we),
|
.while_expr => |we| self.lowerWhile(&we),
|
||||||
.for_expr => |fe| self.lowerFor(&fe),
|
.for_expr => |fe| self.lowerFor(&fe),
|
||||||
.break_expr => self.lowerBreak(),
|
.break_expr => self.lowerBreak(node.span),
|
||||||
.continue_expr => self.lowerContinue(),
|
.continue_expr => self.lowerContinue(node.span),
|
||||||
.call => |c| self.lowerCall(&c),
|
.call => |c| self.lowerCall(&c),
|
||||||
.ffi_intrinsic_call => |fic| self.lowerFfiIntrinsicCall(&fic),
|
.ffi_intrinsic_call => |fic| self.lowerFfiIntrinsicCall(&fic),
|
||||||
.field_access => |fa| self.lowerFieldAccess(&fa, node.span),
|
.field_access => |fa| self.lowerFieldAccess(&fa, node.span),
|
||||||
|
|||||||
@@ -1033,6 +1033,21 @@ pub fn emitBlockDefers(self: *Lowering, saved_len: usize) void {
|
|||||||
self.defer_stack.shrinkRetainingCapacity(saved_len);
|
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).
|
/// 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-`;`
|
/// A braced body lowers as statements (NOT as a value) so a trailing-`;`
|
||||||
/// last expression is fine here — cleanup bodies never yield a value.
|
/// last expression is fine here — cleanup bodies never yield a value.
|
||||||
|
|||||||
Reference in New Issue
Block a user