diff --git a/REFERENCE.md b/REFERENCE.md index aad604d..c0ba3b6 100644 --- a/REFERENCE.md +++ b/REFERENCE.md @@ -75,6 +75,41 @@ return .{ .delete_image = .{ .by_id = 1 } }; // Free cached image return .{ .delete_image = .all }; // Free all cached images ``` +#### Batch lifetimes + +`batch` and `sequence` hold a **slice**, which the runtime reads after `update` +has returned. A `&.{ ... }` literal only lives in static memory when every +element is comptime-known — one runtime value in there and the array becomes a +stack temporary that is gone by the time the runtime walks it: + +```zig +// Wrong: `row` is a runtime value, so this slice dangles on return. +return .{ .batch = &.{ + .{ .cache_image = .{ .source = .{ .file = path }, .image_id = 1 } }, + .{ .place_cached_image = .{ .image_id = 1, .row = row } }, +} }; +``` + +Either copy it into the frame allocator: + +```zig +return try zz.Cmd(Msg).batchAlloc(ctx.allocator, &.{ + .{ .cache_image = .{ .source = .{ .file = path }, .image_id = 1 } }, + .{ .place_cached_image = .{ .image_id = 1, .row = row } }, +}); +``` + +or keep the storage on the model, which outlives any single `update`: + +```zig +pending_batch: [2]zz.Cmd(Msg) = undefined, +// ... +self.pending_batch = .{ cache_cmd, place_cmd }; +return .{ .batch = &self.pending_batch }; +``` + +A batch of nothing but comptime-known commands is fine as a literal. + ### Styling Build styles by chaining properties: diff --git a/build.zig b/build.zig index 013164a..c62e353 100644 --- a/build.zig +++ b/build.zig @@ -95,6 +95,7 @@ pub fn build(b: *std.Build) void { "tests/layout_tests.zig", "tests/unicode_tests.zig", "tests/program_tests.zig", + "tests/command_tests.zig", "tests/focus_tests.zig", "tests/modal_tests.zig", "tests/tooltip_tests.zig", diff --git a/examples/hello_world.zig b/examples/hello_world.zig index f7f7c94..91a9cf5 100644 --- a/examples/hello_world.zig +++ b/examples/hello_world.zig @@ -21,6 +21,9 @@ const Model = struct { image_path: []const u8, protocol: zz.ImageProtocol, caps: zz.ImageCapabilities, + /// Backing storage for a `.batch` command. It lives on the model because + /// the runtime reads the slice after `update` has returned. + pending_batch: [2]zz.Cmd(Msg) = undefined, const image_gap_lines: u16 = 1; const cache_id: u32 = 42; @@ -75,7 +78,11 @@ const Model = struct { self.image_visible = true; self.image_cached = true; const layout = self.computeImageLayout(ctx); - return .{ .batch = &.{ + // Held on the model, not built as a `&.{ ... }` + // literal: a batch containing runtime values + // would be a stack temporary that dangles the + // moment this function returns. + self.pending_batch = .{ .{ .cache_image = .{ .source = .{ .file = self.image_path }, .image_id = cache_id, @@ -89,7 +96,8 @@ const Model = struct { .col = layout.col, .move_cursor = false, } }, - } }; + }; + return .{ .batch = &self.pending_batch }; } }, // Toggle z-index (behind text) diff --git a/src/core/command.zig b/src/core/command.zig index bb504aa..4463ec3 100644 --- a/src/core/command.zig +++ b/src/core/command.zig @@ -154,10 +154,17 @@ pub fn Cmd(comptime Msg: type) type { /// Request repeating tick at interval (nanoseconds) every: u64, - /// Execute a batch of commands + /// Execute a batch of commands. + /// + /// The slice has to outlive the command. `&.{ ... }` is only safe when + /// every element is comptime-known; one runtime value in there makes + /// the array a stack temporary that dangles as soon as `update` + /// returns. Use `batchAlloc` with the frame allocator, or point at an + /// array declared outside the function. batch: []const Cmd(Msg), - /// Execute commands in sequence (wait for each to complete) + /// Execute commands in sequence (wait for each to complete). + /// Same lifetime rule as `batch`. sequence: []const Cmd(Msg), /// Send a message to the update function @@ -231,16 +238,39 @@ pub fn Cmd(comptime Msg: type) type { return .{ .every = sec * std.time.ns_per_s }; } - /// Create a batch of commands + /// Create a batch of commands from a slice that already outlives the + /// command. See the lifetime note on `batch`. pub fn batchOf(cmds: []const Self) Self { return .{ .batch = cmds }; } - /// Create a sequence of commands + /// Create a sequence of commands from a slice that already outlives + /// the command. See the lifetime note on `batch`. pub fn sequenceOf(cmds: []const Self) Self { return .{ .sequence = cmds }; } + /// Create a batch by copying `cmds` into `allocator`. + /// + /// This is the safe way to build a batch out of runtime values: + /// + /// return try zz.Cmd(Msg).batchAlloc(ctx.allocator, &.{ + /// .{ .set_title = title }, + /// .{ .image_file = .{ .path = path, .row = row } }, + /// }); + /// + /// The frame allocator is the right choice — commands are processed + /// during the tick that produced them, and it is reset on the next one. + pub fn batchAlloc(allocator: std.mem.Allocator, cmds: []const Self) !Self { + return .{ .batch = try allocator.dupe(Self, cmds) }; + } + + /// Create a sequence by copying `cmds` into `allocator`. + /// See `batchAlloc`. + pub fn sequenceAlloc(allocator: std.mem.Allocator, cmds: []const Self) !Self { + return .{ .sequence = try allocator.dupe(Self, cmds) }; + } + /// Send a message pub fn send(message: Msg) Self { return .{ .msg = message }; diff --git a/tests/command_tests.zig b/tests/command_tests.zig new file mode 100644 index 0000000..36a1f9e --- /dev/null +++ b/tests/command_tests.zig @@ -0,0 +1,128 @@ +//! Command tests, focused on the lifetime of `batch`/`sequence` slices. +//! +//! `.{ .batch = &.{ ... } }` puts the array in static memory only when every +//! element is comptime-known. One runtime value and it becomes a stack +//! temporary, which dangles as soon as `update` returns — the runtime then +//! walks freed stack memory. These tests pin down the two ways to build a +//! batch that survives. + +const std = @import("std"); +const testing = std.testing; +const zz = @import("zigzag"); + +const Msg = union(enum) { key: zz.KeyEvent }; +const Cmd = zz.Cmd(Msg); + +/// Overwrites the stack region a returned-by-value temporary would occupy, so +/// a dangling slice shows up as garbage rather than reading intact. +fn clobberStack(seed: u64) u64 { + var scratch: [1024]u64 = undefined; + for (&scratch, 0..) |*slot, i| slot.* = seed +% i; + return scratch[seed % scratch.len]; +} + +fn buildWithAlloc(allocator: std.mem.Allocator, interval: u64) !Cmd { + return Cmd.batchAlloc(allocator, &.{ + .{ .set_title = "built at runtime" }, + .{ .every = interval }, + .{ .tick = interval * 2 }, + }); +} + +test "batchAlloc survives the frame that built it" { + var arena = std.heap.ArenaAllocator.init(testing.allocator); + defer arena.deinit(); + + // A runtime value, so the array cannot be folded into static memory. + var interval: u64 = 100; + std.mem.doNotOptimizeAway(&interval); + interval += 1; + + const cmd = try buildWithAlloc(arena.allocator(), interval); + _ = clobberStack(3); + + try testing.expect(cmd == .batch); + try testing.expectEqual(@as(usize, 3), cmd.batch.len); + try testing.expect(cmd.batch[0] == .set_title); + try testing.expectEqualStrings("built at runtime", cmd.batch[0].set_title); + try testing.expectEqual(interval, cmd.batch[1].every); + try testing.expectEqual(interval * 2, cmd.batch[2].tick); +} + +test "sequenceAlloc survives the frame that built it" { + var arena = std.heap.ArenaAllocator.init(testing.allocator); + defer arena.deinit(); + + var ns: u64 = 5; + ns += 1; + + const cmd = try Cmd.sequenceAlloc(arena.allocator(), &.{ + .{ .tick = ns }, + .quit, + }); + _ = clobberStack(11); + + try testing.expect(cmd == .sequence); + try testing.expectEqual(@as(usize, 2), cmd.sequence.len); + try testing.expectEqual(ns, cmd.sequence[0].tick); + try testing.expect(cmd.sequence[1] == .quit); +} + +/// The allocation-free alternative: storage that belongs to the model, which +/// outlives any single `update` call. +const Holder = struct { + slots: [2]Cmd = undefined, + + fn build(self: *Holder, interval: u64) Cmd { + self.slots = .{ + .{ .every = interval }, + .{ .set_title = "from model storage" }, + }; + return .{ .batch = &self.slots }; + } +}; + +test "model-owned storage survives the frame that built it" { + var holder = Holder{}; + + var interval: u64 = 42; + interval += 1; + + const cmd = holder.build(interval); + _ = clobberStack(23); + + try testing.expectEqual(@as(usize, 2), cmd.batch.len); + try testing.expectEqual(interval, cmd.batch[0].every); + try testing.expectEqualStrings("from model storage", cmd.batch[1].set_title); +} + +test "a comptime-known batch literal lives in static memory" { + const S = struct { + fn build() Cmd { + return .{ .batch = &.{ .{ .set_title = "static" }, .quit } }; + } + }; + + const cmd = S.build(); + _ = clobberStack(31); + + try testing.expectEqual(@as(usize, 2), cmd.batch.len); + try testing.expectEqualStrings("static", cmd.batch[0].set_title); + try testing.expect(cmd.batch[1] == .quit); +} + +test "batch helpers keep their contents in order" { + var arena = std.heap.ArenaAllocator.init(testing.allocator); + defer arena.deinit(); + + const nested = try Cmd.batchAlloc(arena.allocator(), &.{ + .{ .tick = 1 }, + try Cmd.batchAlloc(arena.allocator(), &.{ .{ .tick = 2 }, .{ .tick = 3 } }), + .{ .tick = 4 }, + }); + + try testing.expectEqual(@as(u64, 1), nested.batch[0].tick); + try testing.expectEqual(@as(usize, 2), nested.batch[1].batch.len); + try testing.expectEqual(@as(u64, 3), nested.batch[1].batch[1].tick); + try testing.expectEqual(@as(u64, 4), nested.batch[2].tick); +}