fix(cfg): replay errdefers on return err from catch/else captures (closes #17)

src/cache/file_cache.zig

@@ -83,6 +83,15 @@ pub const FileCache = struct {
         return z.fixedArrayLen(node) catch null;
     }
 
+    /// Comptime-known non-negative value of `node`, or null when not
+    /// comptime-resolvable / engine unavailable.  A comptime index can't cause a
+    /// runtime underflow/OOB (it would be a compile error), so a non-null result
+    /// means the access is comptime-verified safe.
+    pub fn comptimeIntValueOf(self: *FileCache, node: std.zig.Ast.Node.Index) ?u64 {
+        const z = self.zls orelse return null;
+        return z.comptimeIntValue(node) catch null;
+    }
+
     /// For a type-name reference node `T`: whether `T` denotes a pointer/
     /// optional type (true), a value type (false), or couldn't resolve (null).
     /// Null when the type engine is unavailable.
@@ -98,6 +107,14 @@ pub const FileCache = struct {
         return z.intInfo(node) catch null;
     }
 
+    /// Whether `node`'s type resolves to a floating-point type.  False when the
+    /// type engine is unavailable or the type isn't a float.  Floats are not
+    /// containers, so `typeNameOfNode` can't report them — use this instead.
+    pub fn isFloatType(self: *FileCache, node: std.zig.Ast.Node.Index) bool {
+        const z = self.zls orelse return false;
+        return z.isFloatType(node) catch false;
+    }
+
     /// For `x.ptr` (x a slice): whether x's element is byte-sized (align 1) —
     /// true=byte slice, false=wider element, null=unresolved/unknown.  Null
     /// when the type engine is unavailable.

src/flow/cfg.zig

@@ -15,9 +15,14 @@
 //!   - `return EXPR;` (function exit)
 //!   - `if/else`, `while`, `for`, `switch` branching (real CFG edges)
 //!   - `defer` / `errdefer` (replayed at function-exit / return sites)
+//!   - `try` / `catch` (error-exit sinks + catch-arm forks; errdefers
+//!     replayed on error returns, including `return error.X` and
+//!     `return <error-capture>` from `catch |err|` / `else |err|`)
 //!
 //! What we DON'T model (yet):
-//!   - `try` / `catch` (error-path forking + error-set tracking)
+//!   - error-UNION returns (`return foo()` yielding `!T`): the success
+//!     vs error path isn't forked, so errdefers are not replayed there
+//!     (sound: a residual false-negative, never a false-positive)
 //!   - generics / comptime
 //!   - for-loop iteration variable origin (treated as .plain)
 //!   - switch-case pattern bindings (treated as .plain)
@@ -328,6 +333,12 @@ pub const LocalInfo = struct {
     /// treated as a borrow source.  Set at registerLocalFull from
     /// the same classifier that produces the .decl's init_kind.
     init_hint: InitHint = .other,
+    /// True iff this local is the error-value capture of a `catch |err|`
+    /// or `else |err|` arm.  Such a value is *definitely* an error, so a
+    /// `return <this>` fires errdefers (Zig only runs errdefers on error
+    /// returns).  Lets lowerReturn replay errdefers at `return err`,
+    /// closing the errdefer-double-free-on-error-return false negative.
+    is_error_capture: bool = false,
     /// If this local was declared from a bare identifier that names
     /// a fn in our annotation DB, the name of that fn.  Lets call
     /// sites `local(args)` resolve through the binding to the

src/flow/cfg_builder.zig

@@ -666,7 +666,14 @@ const Builder = struct {
     /// the body now resolve via name_to_local rather than falling
     /// through to .unknown identifier classification.
     fn registerCaptures(self: *Builder, payload_token: Ast.TokenIndex) !void {
-        _ = try self.registerCapturesWith(payload_token, null);
+        _ = try self.registerCapturesImpl(payload_token, null, false);
+    }
+
+    /// Error-capture variant of `registerCaptures`: each registered
+    /// capture is marked `is_error_capture` so `return <cap>` is treated
+    /// as an error return.  Used for `catch |err|`.
+    fn registerErrorCaptures(self: *Builder, payload_token: Ast.TokenIndex) !void {
+        _ = try self.registerCapturesImpl(payload_token, null, true);
     }
 
     /// Heuristic: when an `if (<Type>.fromJS(...))` scrutinee
@@ -792,6 +799,19 @@ const Builder = struct {
     }
 
     fn registerCapturesWith(self: *Builder, payload_token: Ast.TokenIndex, reset_into: ?BlockId) !void {
+        _ = try self.registerCapturesImpl(payload_token, reset_into, false);
+    }
+
+    /// Error-capture variant of `registerCapturesWith`: marks each
+    /// registered capture `is_error_capture`.  Used for `else |err|`.
+    fn registerErrorCapturesWith(self: *Builder, payload_token: Ast.TokenIndex, reset_into: ?BlockId) !void {
+        _ = try self.registerCapturesImpl(payload_token, reset_into, true);
+    }
+
+    /// Shared body for the capture-registration wrappers.  When
+    /// `mark_error` is set, every registered capture local gets
+    /// `is_error_capture = true` (see LocalInfo).
+    fn registerCapturesImpl(self: *Builder, payload_token: Ast.TokenIndex, reset_into: ?BlockId, mark_error: bool) !void {
         const tree = self.tree;
         const tags = tree.tokens.items(.tag);
         var t: Ast.TokenIndex = payload_token;
@@ -802,6 +822,7 @@ const Builder = struct {
                     const name = tree.tokenSlice(t);
                     if (std.mem.eql(u8, name, "_")) continue;
                     const lid = try self.registerLocal(name, self.posOfToken(t));
+                    if (mark_error) self.locals.items[@intFromEnum(lid)].is_error_capture = true;
                     if (reset_into) |blk| {
                         try self.appendStmt(blk, .{
                             .kind = .{ .reset_capture = .{ .local = lid } },
@@ -1098,7 +1119,7 @@ const Builder = struct {
         // Lower the else branch if present.  `else |err|` payload is
         // in scope for the else-body only.
         if (else_block) |eb| {
-            if (if_data.error_token) |et| try self.registerCapturesWith(et, eb);
+            if (if_data.error_token) |et| try self.registerErrorCapturesWith(et, eb);
             var else_cur = eb;
             try self.lowerStmt(if_data.ast.else_expr.unwrap().?, &else_cur);
             try self.addEdge(else_cur, merge_block);
@@ -2069,7 +2090,7 @@ const Builder = struct {
         // `catch |err| BODY` — payload is `main_token + 2` when the
         // token immediately following `catch` is `|`.  Scope: body only.
         if (self.catchPayloadToken(catch_node)) |pt| {
-            try self.registerCaptures(pt);
+            try self.registerErrorCaptures(pt);
         }
 
         var catch_cur = catch_block;
@@ -2715,20 +2736,24 @@ const Builder = struct {
 
         // Now flush defers — fires after the .use's, before the .ret.
         //
-        // `return error.X` (or `return .{...}` resolving to an error-only
-        // path) returns an error value, so Zig fires errdefers too.
-        // Detect the syntactic `error.X` shape and use the error-path
-        // flush; everything else uses the normal-only flush.  This is
-        // conservative — `return foo()` where `foo()` returns an error
-        // also fires errdefers, but we can't tell that from the AST
-        // without callee resolution.  Catching the literal case is the
-        // common bug shape (`errdefer free(p); return error.X` after an
-        // explicit free of `p` is a textbook double-free).
-        const value_is_literal_error = if (value_opt) |expr|
-            isLiteralErrorReturn(tree, expr)
+        // Zig fires errdefers only on returns that yield an ERROR value.
+        // `returnExprIsError` recognises the statically-decidable error
+        // shapes — a literal `error.X` and a returned error-capture
+        // identifier (`catch |err| { ...; return err; }`) — and uses the
+        // error-path flush for them; everything else uses normal-only.
+        //
+        // This is a sound under-approximation: `return foo()` where
+        // `foo()` returns an error UNION (`!T`) may or may not be an
+        // error at runtime, so it is NOT classified here — flushing
+        // errdefers unconditionally would false-positive on the success
+        // path, and deciding it needs callee return-type resolution.
+        // Both caught shapes are common double-free sources, e.g.
+        // `errdefer free(p); something() catch |err| { free(p); return err; }`.
+        const value_is_error = if (value_opt) |expr|
+            self.returnExprIsError(expr)
         else
             false;
-        if (value_is_literal_error) {
+        if (value_is_error) {
             try self.flushErrAndNormalDefers(cur);
         } else {
             try self.flushDefers(cur);
@@ -4551,6 +4576,26 @@ const Builder = struct {
         }
     }
 
+    /// True iff the returned expression is statically known to be an
+    /// error value, so Zig fires errdefers on this return.  Covers:
+    ///   - literal `error.X` (and `try`-wrapped) — `isLiteralErrorReturn`
+    ///   - a bare identifier naming an in-scope error capture
+    ///     (`catch |err|` / `else |err|`), marked `is_error_capture`.
+    /// Sound under-approximation: returns of error UNIONS (`return foo()`
+    /// yielding `!T`) are deliberately NOT classified here — see the
+    /// rationale at the call site in `lowerReturn`.
+    fn returnExprIsError(self: *const Builder, expr: Ast.Node.Index) bool {
+        const tree = self.tree;
+        if (isLiteralErrorReturn(tree, expr)) return true;
+        if (tree.nodeTag(expr) == .identifier) {
+            const name = tree.tokenSlice(tree.nodeMainToken(expr));
+            if (self.name_to_local.get(name)) |lid| {
+                return self.locals.items[@intFromEnum(lid)].is_error_capture;
+            }
+        }
+        return false;
+    }
+
     fn isConstructorName(name: []const u8) bool {
         const list = [_][]const u8{
             "init", "create", "new", "open",

src/flow/cfg_tests.zig

@@ -281,6 +281,78 @@ test "plain defer DOES fire on return — kill visible before ret stmt" {
     try std.testing.expect(saw_kill);
 }
 
+test "errdefer DOES fire on `return err` from a catch capture" {
+    // `errdefer arena.deinit()` must be replayed when the function
+    // returns an error value — here `return err` re-raises the captured
+    // error from `catch |err|`.  Before the fix, only literal
+    // `return error.X` was recognised, so this `.arena_kill` was
+    // silently dropped (the errdefer-double-free-on-error-return FN).
+    const gpa = std.testing.allocator;
+    var result = try parseAndLower(gpa,
+        \\pub fn foo() !void {
+        \\    var arena = Arena.init(0);
+        \\    errdefer arena.deinit();
+        \\    sideEffect() catch |err| {
+        \\        return err;
+        \\    };
+        \\    return;
+        \\}
+        \\const Arena = struct {
+        \\    pub fn init(_: u32) Arena { return .{}; }
+        \\    pub fn deinit(_: *Arena) void {}
+        \\};
+        \\pub fn sideEffect() !void {}
+        \\
+    );
+    defer result.deinit(gpa);
+    const cfg = result.cfg.?;
+
+    // The errdefer's `arena.deinit()` (→ .arena_kill) must be replayed
+    // on the `return err` path.  The trailing `return;` is a success
+    // return and must NOT replay it — so exactly one kill appears.
+    var kill_count: usize = 0;
+    for (cfg.blocks) |b| {
+        for (b.stmts) |s| {
+            if (s.kind == .arena_kill) kill_count += 1;
+        }
+    }
+    try std.testing.expectEqual(@as(usize, 1), kill_count);
+}
+
+test "errdefer does NOT fire on a non-error `return` from a catch arm" {
+    // Symmetric guard against a false positive: a catch arm that
+    // returns a NON-error value (a literal here) is a success return,
+    // so the errdefer must stay silent — no `.arena_kill` anywhere.
+    const gpa = std.testing.allocator;
+    var result = try parseAndLower(gpa,
+        \\pub fn foo() !u32 {
+        \\    var arena = Arena.init(0);
+        \\    errdefer arena.deinit();
+        \\    const v: u32 = compute() catch |err| {
+        \\        _ = err;
+        \\        return 0;
+        \\    };
+        \\    return v;
+        \\}
+        \\const Arena = struct {
+        \\    pub fn init(_: u32) Arena { return .{}; }
+        \\    pub fn deinit(_: *Arena) void {}
+        \\};
+        \\pub fn compute() !u32 { return 1; }
+        \\
+    );
+    defer result.deinit(gpa);
+    const cfg = result.cfg.?;
+
+    var saw_kill = false;
+    for (cfg.blocks) |b| {
+        for (b.stmts) |s| {
+            if (s.kind == .arena_kill) saw_kill = true;
+        }
+    }
+    try std.testing.expect(!saw_kill);
+}
+
 test "try at statement position creates error-exit sink with defers replayed" {
     // `try call();` — adds an error-exit block reachable from cur.
     // The sink should contain whatever defers were active (here: