Skip to content

Clearer documentation for errdefer in blocks #7298

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
alinebee opened this issue Dec 4, 2020 · 3 comments
Closed

Clearer documentation for errdefer in blocks #7298

alinebee opened this issue Dec 4, 2020 · 3 comments
Labels
contributor friendly This issue is limited in scope and/or knowledge of Zig internals. docs
Milestone

Comments

@alinebee
Copy link

alinebee commented Dec 4, 2020

The current documentation example for errdefer only shows it being used at function scope, where its behaviour is pretty intuitive. But the behaviour in if/while/for etc blocks - i.e. that the errdefer goes out of scope at the end of the block - is subtle and could use more documentation.

In particular, blocks silently break the pattern described in the documentation where "The deallocation code is always directly following the allocation code." A couple of footguns:

const std = @import("std");

const Error = error {
    FlagrantViolation,
};

// Example 1
fn errdeferAtFunctionScope(allocator: *std.mem.Allocator) !void {
    const ptr = try allocator.alloc(u8, 1);
    
    // Does not leak: the errdefer is at the same scope as the return.
    errdefer allocator.free(ptr);
    
    return Error.FlagrantViolation;
}

// Example 2
fn errdeferInBlock(allocator: *std.mem.Allocator) !void {
    {
        const ptr = try allocator.alloc(u8, 1);

        // Leaks: by the time the error is returned, errdefer has gone out of scope.
        errdefer allocator.free(ptr);
    }
    
    return Error.FlagrantViolation;
}

// Example 3
fn errdeferInLoop(allocator: *std.mem.Allocator) !void {
    var count: usize = 0;
    
    while (true) {
        const ptr = try allocator.alloc(u8, 1);

        // Leaks on the second iteration of the loop: 
        // the errdefer from the first iteration will have gone out of scope.
        errdefer allocator.free(ptr);
        
        count += 1;
        if (count == 2) {
            return Error.FlagrantViolation;
        }
    }
}

test "errdefer at function scope" {
    // Passes
    std.testing.expectError(Error.FlagrantViolation, errdeferAtFunctionScope(std.testing.allocator));
}

test "errdefer in block" {
    // Fails with a leak
    std.testing.expectError(Error.FlagrantViolation, errdeferInBlock(std.testing.allocator));
}

test "errdefer in loop" {
    // Fails with a leak
    std.testing.expectError(Error.FlagrantViolation, errdeferInLoop(std.testing.allocator));
}

The specific case that bit me was Example 3: repeatedly allocating in a failable loop, and assuming that I could keep an errdefer next to the alloc call and undo all previous allocations that way. It makes sense in retrospect that it doesn't work that way, but some scope examples in the docs could help users arrive at that mental model.

(Ideally, useless errdefer blocks like 2 could be a compiler error; 3 might be harder for the compiler to spot.)

@Vexu Vexu added docs contributor friendly This issue is limited in scope and/or knowledge of Zig internals. labels Dec 4, 2020
@Vexu Vexu added this to the 0.8.0 milestone Dec 4, 2020
@andrewrk andrewrk modified the milestones: 0.8.0, 0.9.0 Jun 4, 2021
@andrewrk andrewrk modified the milestones: 0.9.0, 0.11.0 Nov 24, 2021
@DontBreakAlex
Copy link

Hi, just commenting to say that as a new Zig user, I just shot my foot with example 3.

@mslapek
Copy link

mslapek commented May 29, 2023

Ideally, useless errdefer blocks like 2 could be a compiler error;

That's an excellent idea! 👏 I initially thought that errdefer works with nullable (non-error) result, because this func compiled:

fn foo_nullable() ?i64 {
    errdefer {
        _ = stdout.writeAll("foo_nullable\n") catch undefined;
    }
    return null;
}

@Vexu
Copy link
Member

Vexu commented May 29, 2023

This was addressed by #10607.

Ideally, useless errdefer blocks like 2 could be a compiler error;

That's an excellent idea! clap I initially thought that errdefer works with nullable (non-error) result, because this func compiled:

That is #2654

@Vexu Vexu closed this as completed May 29, 2023
@andrewrk andrewrk modified the milestones: 0.13.0, 0.11.0 Jun 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contributor friendly This issue is limited in scope and/or knowledge of Zig internals. docs
Projects
None yet
Development

No branches or pull requests

5 participants