Zig: `errdefer` should be a compile error when used inside a function that doesn't return an error

[dbandstra]

This code is currently allowed, but from reading the docs I don't think it's ever possible for that errdefer to be hit.

pub fn main() void {
    var x = false;

    errdefer x = true;
}

[daurnimator]

I once had the same thought, but it came in useful when coding defensively:

fn myfunc() Foo {
    const foo = Foo.init();
    errdefer foo.deinit()
    foo.bar = Bar.init();
    return foo;
}

And then at some point later, Bar.init is changed to sometimes throw an error: a developer only needs to add ! to the return signature: the errdefer is already there.
-> I see this as helping the "good practice" that every .init() is paired with a .deinit()

[dbandstra]

On the other hand, I ran into this when I was refactoring and ended up with this stale code (the errdefers) that I needed to change to regular defers.

(My refactor was something like taking a large init function, which returned errors, and which was called by my main function - and inlining it into my main function which doesn't return errors. The errdefers were previously in the init function)

[CurtisFenner]

What about functions that do return errors, but don't in the paths following the errdefer, like

fn dothing() !void {
    if (badthing()) {
        return error.AProblemHappened;
    }
    errdefer cleanup();
}

I think if you wanted to complain about errdefer in functions that don't return errors at all you'd also want to complain about the above example.

Though, I do think the point @daurnimator is important. Because Zig doesn't have any "automatic" resource cleanup (such as RAII), it seems like it would be good style to always pair resource acquisition with an errdefer wherever appropriate; however, if no-op errdefer is not allowed, then you would be forced to sometimes omit them:

var r1 = try R1.acquire();
errdefer r1.cleanup();

var r2 = try R2.acquire();
errdefer r2.cleanup();

var r3 = try R3.acquire();
// conspicuously missing errdefer

return consume(r1, r2, r3);

If someone were to modify this code to acquire a fourth resource, they probably wouldn't know whether or not r3 is intentionally missing cleanup (without a comment to the effect of "errdefer is missing because the compiler is annoying"), and in large/complex enough functions, the modifier might not even notice.

[nmichaels]

I see the arguments of the people saying that it can be a clean style to have dead code, but it's still dead code. You may as well have

fn dothing() !void {
    if (badthing()) {
        return error.AProblemHappened;
    }
    errdefer unreachable;
}

This relates to the Zig compiler's overall lazy approach to evaluation, where if it doesn't know that an expression is needed in this build it won't try to compile it. Untested code paths are buggy, and if you've got an errdefer block that can't ever be reached, it's untestable. Personally, I'd rather write the code with unreachable and a comment explaining that cleanup() should be called if this changes.

Anyway, I'll try to sum up the debate with examples of bugs from both sides. Either untested code is silently enabled or cleanup isn't done properly.

In the case where a function with no error in its type errdefers:

fn myFunc() Foo {
    const foo = Foo.init();
    errdefer foo.deinit()
    foo.bar = Bar.init();
    errdefer foo.deinit() // <-- See the bug? Here it is.
    const foo.baz = Baz.init();
    return foo;
}

This is not an unlikely scenario (by my estimation) and when myfunc is changed to return !Foo because the programmer changes Bar.init()'s return type to !Bar, there's one chance to catch it when Bar.init() becomes try Bar.init().

In the proposed case:

fn myFunc() Foo {
    const foo = Foo.init();
    foo.bar = Bar.init();
    return foo;
}

When Bar is changed to return !Bar, myfunc has to be changed to return !Foo and Bar.init() gets its try added. The programmer fails to inspect the rest of myfunc and doesn't add the errdefer Foo.init() and now there's a resource leak.

There's another situation, which is how I ended up here: incorrect use of errdefer.

fn myFunc() ?Foo {
    const foo = Foo.init();
    errdefer foo.deinit();
    // We're going to pretend it makes sense for Bar.init() to return null.
    if (Bar.init()) |bar| {
        foo.bar = bar;
    } else {
        return null;
    }
    return foo;
}

Whoops, leaked some memory because Bar.init() returned null and null is not an error. This is just a bug, and it was found with valgrind. I was surprised, though.

All of the above scenarios are bad, and none are unlikely. If I'm already running valgrind on my tests, it will catch all of these situations, but that's only true if the leaked resource is memory. On principle, I prefer the proposal, since I don't like dead code in my codebase.

I totally forgot @dbandstra's case of refactoring that needs to change a return type from !Foo to Foo. It puts some weight on the pro-proposal side.

[hryx]

See also: #335, #282, #952

[andrewrk]

Thank you @nmichaels for your well-considered and detailed response here. I agree with your reasoning, as well as your suggestion for what idiomatic zig code would look like when "coding defensively":

    errdefer unreachable;

This will be allowed, even when there is no possible error returned from the function, for the same reason that unreachable is allowed in unreachable code paths.

The idea here is when the programmer gets the compile error for returning a compile error through errdefer unreachable (possibly related: #4094), the programmer should understand that resource handling for errors was not taken into account previously, and should inspect the entire function, adding resource management where appropriate.

Code reviewers can look for the simple pattern of:

-    errdefer unreachable;

And understand that the entire function's resource management with respect to errors, needs to be double-checked.

---

I also want to note that much like the other issues @hryx noted, multibuilds (#3028) is required to enable this compile error, because the return type and reachability in general could depend on build options.