Proposal: remove capturing errdefer from the language

[mlugg]

Background

defer is a core and incredibly useful component of Zig, which helps avoid bugs in resource management. Similarly, errdefer helps avoid bugs in situations where a resource should be cleaned up only on error conditions. However, there is a third, lesser-known, piece of syntax: capturing errdefer. It looks like this:

errdefer |err| {
    // `err` is the error being returned; you can do stuff with it!
    // Like a normal `defer/`errdefer`, you can't `return` or `try` in this block, so you can't change
    // which error is returned; but you can e.g. log it
}

This feature is used incredibly rarely; many experienced Zig users do not even know that it exists. As a data point, in the Zig repository, ignoring test coverage and documentation for it, there are exactly two uses of this language feature:

zig/tools/update_cpu_features.zig

Line 1529 in 8e79fc6

errdefer |err| std.debug.panic("panic: {s}", .{@errorName(err)});

zig/lib/fuzzer.zig

Line 298 in 8e79fc6

errdefer |err| oom(err);

A feature being rarely used is not necessarily in itself a reason to remove that feature. However, errdefer has a bigger design problem.

The Problem

Here's a question: in errdefer |err|, what is the type of err?

The obvious thing would be that it has the type of "every error which can be returned after this point in the function", but this isn't a feasible definition; it brings a good amount of (quite boring) complexity to the language specification in terms of how it interacts with things like inferred error sets, and implementing this would require a type of logic in the compiler which likely has compiler performance implications. So, in reality, there are 3 reasonable choices:

• The type of err is the current function's error set.
• The type of err is anyerror.
• The type of err is the error type which is being returned at a given return site (so the errdefer body is reanalyzed at every possible error return with a different type for err).

Let's go through these three options. Note that right now, the compiler has inconsistent and unhelpful behavior here, so this is an unsolved problem.

The first option is actually fairly good, with two big caveats:

• In functions with inferred error sets, errdefer |err| switch (err) becomes impossible (it would emit a dependency loop because we don't know what cases need to be in the switch).
• It makes something like errdefer |err| fatal("error: {}", .{err}); impossible, since the function needs to return an error for err to be typed correctly.

The second option means that any switch on the captured err must have an else prong, so if you want to switch on the captured error, this option is strictly worse than using a wrapper function which switches on the returned error (since at this point the error type is known and can be exhaustively switched on). However, people are likely to reach for errdefer anyway out of convenience, and shoot themselves in the foot by losing type safety.

The third option is what we usually do today, but:

• It breaks switch on the captured error, because @TypeOf(err) will usually only contain a subset of all possible errors, so you'll get errors that switch prongs are impossible (because e.g. error.Foo can't be returned at this particular return site, so err is error{Bar} instead).
• It makes it significantly harder for any compiler implementation to deduplicate errdefers across different error return sites, because they are analyzed differently. (This Zig implementation does not do this deduplication anyway, but it's good for the language spec to make it viable!)

One observation here is that all three of these solutions have big problems with switch on the captured error -- and this is a construct we want to encourage, not discourage! The main other use case for capturing errdefer is to log errors. The thing is, this use case is actually not a brilliant use for this construct, because it can lead to code bloat (due to the error logging logic being duplicated at every error return site). Generally, the body of an errdefer should be incredibly simple; it's essentially just intended for resource cleanup. Capturing errdefer encourages using the construct in more complex ways, which is usually not a good thing! Also, if you're logging an error, this generally indicates you aren't going to explicitly handle the different error cases higher on the call stack, so it's probably desirable to "collapse" these errors down into one (e.g. turn your set of 5 errors into a blanket error.FooFailed). This is something errdefer |err| does not support. As such, there are several big advantages to using a "wrapper function" approach instead, like this:

pub fn foo() error{FooFailed}!void {
    fooInner() catch |err| {
        std.log.err("error: {s}", .{@errorName(err)});
        return error.FooFailed;
    };
}
fn fooInner() !void {
    // ...
}

So, capturing errdefer syntax raises a difficult design problem, where all solutions seem unsatisfactory. Furthermore, the common use cases for this feature -- and indeed, the ones it seems to encourage -- tend to emit bloated code and suffer from worse error sets. That leads on to this proposal.

Proposal

Remove errdefer |err| syntax from Zig. Uh... yeah, that's kinda it.

Migration

The two uses in the compiler can be trivially rewritten with these diffs.

lib/fuzzer.zig

     fn traceValue(f: *Fuzzer, x: usize) void {
-        errdefer |err| oom(err);
-        try f.traced_comparisons.put(gpa, x, {});
+        f.traced_comparisons.put(gpa, x, {}) catch |err| oom(err);
     }

tools/update_cpu_features.zig

 fn processOneTarget(job: Job) void {
-    errdefer |err| std.debug.panic("panic: {s}", .{@errorName(err)});
+    processOneTargetInner(job) catch |err| std.debug.panic("panic: {s}", .{@errorName(err)});
+}
+fn processOneTargetInner(job: Job) !void {
     const target = job.target;

These are also basically the two solutions for current uses of this feature: either split your function in two, or perhaps use catch at the error site(s) if there aren't many. This improves clarity (one less language feature to understand in order to understand your code!), type safety (no issues with switch exhaustivity, and you can "collapse" error sets where you want as discussed above), and code bloat (no duplication of the error handling path at every possible error return).

[Snektron]

In functions with inferred error sets, errdefer |err| switch (err) becomes impossible (it would emit a dependency loop because we don't know what cases need to be in the switch).

The errdefer block should not be able to influence the result type of the function, right? So technically there shouldn't be any dependency loop here. In any case, this should be the same case as a regular recursive function.

[haze]

Could there be a better name than processOneTargetInner for handling the error case? Perhaps the solution could be updated to something like

fn unsafeProcessOneTarget() !void {
    ...
}

fn processOneTarget() void {
    ...
}

I chose unsafe because the caller is expected to handle error conditions and signify with it's prototype and name that it's handled. But I also know unsafe is a loaded term, so maybe there is a better one.

It's not directly related to the logic of the proposal, but it might help with selling the idea of creating failable / safe variations of functions

[trgwii]

@haze Golang has the convention "Must" for functions that panic, and I think it's pretty reasonable when there is no "panic in the type system":

fn processOneTarget() !void { ... }
fn mustProcessOneTarget() void { ... }

[Luexa]

In functions with inferred error sets, errdefer |err| switch (err) becomes impossible (it would emit a dependency loop because we don't know what cases need to be in the switch).

The errdefer block should not be able to influence the result type of the function, right? So technically there shouldn't be any dependency loop here. In any case, this should be the same case as a regular recursive function.

Well the errdefer could panic or call a noreturn function in which case that error wouldn't be returned, but I think it quickly becomes weird or impossible to alter the inferred error set in response to that.

[Luexa]

Could there be a better name than processOneTargetInner for handling the error case? Perhaps the solution could be updated to something like

fn unsafeProcessOneTarget() !void {
...
}

fn processOneTarget() void {
...
}

I chose unsafe because the caller is expected to handle error conditions and signify with it's prototype and name that it's handled. But I also know unsafe is a loaded term, so maybe there is a better one.

It's not directly related to the logic of the proposal, but it might help with selling the idea of creating failable / safe variations of functions

It's the opposite of safe if it "handles" errors by panicking! Edge cases matter and not returning the error to the caller is harmful if anything,

[lumi2021]

Well the errdefer could panic or call a noreturn function in which case that error wouldn't be returned, but I think it quickly becomes weird or impossible to alter the inferred error set in response to that.

in this case errdefer is not necessary as the panic or noreturn function can be called directly instead of the error. The usefulness of the errdefer capture is only when the function indeed returns, and definitely it cannot change the return type.

[TheHonestHare]

It may be useful to note that if #2765 gets added then this behaviour could be replicated as so:

errdefer if(@result()) |_| unreachable else |err| {
    // do stuff with err
}

[Luexa]

in this case errdefer is not necessary as the panic or noreturn function can be called directly instead of the error. The usefulness of the errdefer capture is only when the function indeed returns, and definitely it cannot change the return type.

Well yeah the whole idea of this issue is it's not necessary. I'm just saying how the inference could technically be influenced (but it almost certainly shouldn't be influenced like that because that's not what defer/errdefer are for).

[Luexa]

It may be useful to note that if #2765 gets added then this behaviour could be replicated as so:

errdefer if(@Result()) |_| unreachable else |err| {
// do stuff with err
}

I will have to hope @result() is forbidden in errdefer.

[190n]

For another datapoint on large projects, Bun contains 257 errdefers but no capturing ones.

[Jarred-Sumner]

Is this specifically for capturing the err value from the errdefer or deprecating errdefer in general?

[Rexicon226]

For capturing the err value.

[chung-leong]

As I explained in a ziggit discussion , there are situations where the resource clean-up procedure is slightly different after an error has occurred. Here's one example:

fn runQuery(allocator: std.mem.Allocator, sql: []const u8) ![]Row {
    const conn = connection_pool.get();
    defer |err_maybe| {
        // do no reuse connection when a connection has timed out
        connection_pool.release(conn, err_maybe == error.Timeout);
    }
    // ...
}

That we cannot capture error in a defer currently is skewing the perception of errdefer. errdefer clauses need to see the error sometimes because they're a type of deferred statements, not because they're some sort of error handlers. The decision to use defer or errdefer is always related to a function's return value. The above example would employ errdefer instead if an iterator is returned:

fn runQuery(sql: []const u8) !RowIterator {
    const conn = connection_pool.get();
    errdefer |err| {
        // do no reuse connection when a connection has timed out
        connection_pool.release(conn, err == error.Timeout);
    }
    // ...
}

Since we would only ever care about specific errors related to the resource being cleaned up, the problem described here concerning switch is largely irrelevant.

[rpkak]

Edit: The in #5610 proposed syntax would add another option to replace errdefer |err| with.

Original comment

Maybe the capturing errdefer syntax could be "replaced" by allowing try to work with labeled blocks.

This would mean that the expressions

try :blk error_union

and

error_union catch |err| break :blk err

would be equivalent.

Code could look like this:

const std = @import("std");

fn doSomething() !u64 {
    // do something
    return error.Foo;
}

fn foo() void {
    const num = blk: {
        const inner_num = try :blk doSomething();

        comptime std.debug.assert(@TypeOf(inner_num) == u64);

        break :blk doSomething();
    } catch |err| switch (err) {
        error.Foo => std.posix.exit(1),
    };

    comptime std.debug.assert(@TypeOf(num) == u64);
}

[cryptocode]

@rpkak I think that's essentially #5610