Skip to content
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

Overhaul error handling control-flow and add error-stack intent #7812

Open
mikdusan opened this issue Jan 18, 2021 · 25 comments
Open

Overhaul error handling control-flow and add error-stack intent #7812

mikdusan opened this issue Jan 18, 2021 · 25 comments
Labels
proposal This issue suggests modifications. If it also has the "accepted" label then it is planned.
Milestone

Comments

@mikdusan
Copy link
Member

mikdusan commented Jan 18, 2021

REVISION 3 - Friday 2021-02-12

note: changelog is at bottom of document
note: co-author @marler8997

The primary goal of this proposal is to lexically distinguish error control-flow from regular flow-control:

  1. return statements do not effect error flow-control or error-stack changes
  2. replace current return error.BadValue; with throw error.BadValue;
  3. require explicit syntax to capture an error-union value

secondary (and orthogonal) to but included in this proposal:

  1. add new builtin @throwCurrentErrorHideCurrentFrame() to throw without pushing error/frame onto error-stack
  2. add new builtin @clearErrorStack() for explicit control of when to clear the error-stack.
  3. expose error-union tags as if union(enum) { payload: T, err: E }

Related proposals:

#1923 (comment) overlaps with @clearErrorStack() and opinions related to #1923 are encouraged.

#2562 is no longer required because return <value> is not ambiguous with error handling.

#2647 is proposing syntax changes to enable tagged-error-unions; see bottom of document for more details.

#5610 is orthogonal to this proposal.


pros

  • no more ambiguity between error and value semantics
  • common case is to avoid error semantics unless explicit lexical syntax is used
  • simplified compiler errors for common cases
  • lexical distinction that an error-union value has been taken
  • lexical distinction when/where an error-stack has been cleared
  • allow inspection and propagation of unchanged error-stack

cons

  • new keyword throw
  • overload catch to work as a prefix keyword catch <error-union>
  • requires rare source code changes for decls of error-union types (eg. 16 instances identified by std.zig test coverage)
  • requires source code changes for error-union callsites to choose between error or value semantics, most will be error semantics

example.zig with this proposal in mind

fn afunc() !(error {Bad}) {
    // UNCHANGED-BEHAVIOUR
    var a = try doit();

    // UNCHANGED-BEHAVIOUR
    var b = doit() catch (err) { ... };

    // UNCHANGED-BEHAVIOUR
    if (try doit()) {}

    // UNCHANGED-BEHAVIOUR
    if (doit()) {}
    else |err| {}

    ////////////////////////////////////////

    // OLD: propagate error
    // NEW: return an error value, does not effect error-stack, no error-flow-control.
    return error.Bad;

    // OLD: n/a
    // NEW: propagate error (equivalent to OLD `return error.Bad`)
    throw error.Bad;

    // OLD: `c` is type error-union
    // NEW: do not let an error-union _easily_ become a value. compile-error: unhandled error
    var c = doit();

    // OLD: n/a
    // NEW: let an error-union become a value with overloaded keyword `catch` in a prefix position, `d` type is error-union
    var d = catch doit();
    // exposed union tags
    _ = d.payload; // safety checked
    _ = d.err;     // safety checked

    // UNCHANGED-BEHAVIOUR
    if (d) {}
    else |err| {} 

    // OLD: compile-error: expected type 'bool', found '@typeInfo(@typeInfo(@TypeOf(foo)).Fn.return_type.?).ErrorUnion.error_set!void'
    // NEW: compile-error: unhandled error
    if (doit()) {}
}

fn f0() !void {
    var a = doit() catch (err) {
        // UNCHANGED-BEHAVIOUR but used to be `return` and now `throw`
        //
        // EXIT-ERROR-STACK: push,propagate
        // [1] `f0()`           error {Overflow,Megaflow}
        // [0] `doit()`         error {Overflow,Megaflow}
        throw err;
    };

    // UNCHANGED-BEHAVIOUR
    // sugar for above
    var a = try doit();
}

fn f1() !u32 {
    // error-stack: EMPTY

    var a = doit() catch (err) {
        // ENTRY-ERROR-STACK:
        //     [0] `doit()`     error {Overflow,Megaflow}

        if (cond) {
            // UNCHANGED-BEHAVIOUR
            // on-error case: push nested error-stack and throw new error
            //
            // EXIT-ERROR-STACK: push,propagate
            // [2] `f1()`       error {Overflow,Megaflow}
            // [1] `doit()`     error {Overflow,Megaflow}
            // [0] `doit()`     error {Overflow,Megaflow}
            try doit();
        }

        switch (err) {
            // NEW-BEHAVIOUR
            // case: throw a new error of same value
            // it is "new" because error-stack depth has increased
            //
            // EXIT-ERROR-STACK: push,propagate
            // [1] `f1()`       error.Overflow
            // [0] `doit()`     error {Overflow,Megaflow}
            .Overflow => throw err,

            // NEW-BEHAVIOUR
            // case: throw a new error of different value
            // it is "new" because error-stack depth has increased
            //
            // EXIT-ERROR-STACK: push,propagate
            // [1] `f1()`       error.Unknown
            // [0] `doit()`     error {Overflow,Megaflow}
            .Megaflow => throw error.Unknown,
        }

        if (cond) {
            // NEW-BEHAVIOUR
            // case: clear error-stack and throw new error
            //
            // EXIT-ERROR-STACK: reset,push,propagate
            // [0] `f1()`       error.BadArgument
            @clearErrorStack();
            throw error.BadArgument;
        }

        if (cond) {
            // NEW-BEHAVIOUR
            // case: re-throw current-error (top of error-stack) without pushing anything onto error-stack
            // if error-stack is empty then panic `@panic("empty error stack")`
            //
            // EXIT-ERROR-STACK: propagate
            // [0] `doit()`     error {Overflow,Megaflow}
            @throwCurrentErrorHideCurrentFrame();
        }
    };

    // assert( error_stack.len == 0) if (previous catch-block was not entered)
    // assert( error_stack.len >  0) if (previous catch-block was entered)

    // This particular programmer desires to clear error-stack before any value return.
    // Expected to be more common than not clearing, but much less common than try/throw occurrances.
    @clearErrorStack();
    // assert( error_stack.len == 0)

    return 42;
}

// fn return-type: error-union
fn f2() !u32 {
    // OLD-BEHAVIOUR
    // both accepted; both do same thing
    return doit();
    return try doit();

    // NEW-BEHAVIOUR
    // compile-error
    return doit();
    // ok
    return try doit();

    // UNCHANGED-BEHAVIOUR
    return 42;
}

// fn return-type: value-error-union
// no motivating use case: emergent behaviour
fn f3() @typeInfo(@TypeOf(f2)).Fn.return_type.? {
    // NEW-BEHAVIOUR
    // let an error-union become value and return value
    // error-stack is untouched, no error control-flow, just a regular value being returned
    return catch f2();

    // desugared
    // `veu` is type error-union
    const veu = catch f2();
    // return value-error-union
    return veu;
}

fn f4() !void {
    // compile-error: `f3` does not throw
    _ = try f3();

    // ok: it's just a value-error-union
    _ = f3();
}

fn doit() !void {
    if (__random__) {
        throw error.Overflow;
    } else {
        throw error.Megaflow;
    }
}

table of fundamental error-handling effects and corresponding syntax

  • throw → error control-flow and exits function
  • push → pushing one (or more) errors onto error-stack
  • clear → clear error-stack (length becomes 0)
throw push clear syntax example
0 0 0 const x = catch doit();
0 0 1 const x = catch doit(); @clearErrorStack();
0 1 0 invalid - why push onto error-stack if not throwing?
0 1 1 invalid - why push onto error-stack if not throwing?
1 0 0 @throwCurrentErrorHideCurrentFrame();
1 0 1 invalid - why throw empty error-stack?
1 1 0 const result_value = try doit();
throw error.BadValue;
1 1 1 @clearErrorStack(); throw error.BadValue;

Questions and Answers

Q. Why a new keyword throw only to handle an expression of error-type. Could existing keyword try be overloaded?

A. The try keyword means "maybe throw" and overloading it to accept expressions of type error would change that use to mean always throw. The semantics of try would depend on the value of the expression after it and its no longer clear what try x does.


Q. Why isn't @throwCurrentErrorHideCurrentFrame() a keyword and why is it so long?

A. The motivating case for this feature is rare and did not warrant promotion to a keyword. Also we wanted the rare use to be self-explanatory, conveying "a throw of current-error without pushing error/frame onto error-stack".

// this could be made more generic
fn trace_errors(f: fn() !void) !void {
    f() catch |err| {
        std.debug.warn("error traced: {}\n", .{err});
        @throwCurrentErrorHideCurrentFrame();
    }
}

Q. Why isn't @clearErrorStack() a keyword or some other logic engaged to automatically clear?

A. We found multiple cases where a programmer may want to clear the error stack at different places or even not at all. By providing this builtin, the programmer has full control over when to clear the error stack, if at all. Note that this builtin does not prevent other features from also being implemented to clear the error stack such as #1923 (comment) . Also this is relatively rare comparing to common usage of error flow-control and we again decided against promotion to a keyword.


What #2647 syntax would look like under this proposal:

// syntax from #2647
{
    // from comment: https://github.com/ziglang/zig/issues/2647#issuecomment-500694683
    return error{ .InvalidChar = index };

    // from comment: from https://github.com/ziglang/zig/issues/2647#issuecomment-501545471
    return ParseError{ .InvalidChar = index };
}

// syntax under this proposal
{
    // error flow-control
    throw error{ .InvalidChar = index };
    throw ParseError{ .InvalidChar = index };
    throw .{ .InvalidChar = index };

    // return union-init is regular flow-control
    return .{ .age, value };
}

REVISION 3 changes

Sorry for the fast update. Rev2 really helped put a lot of things in perspective and with @marler8997's help we brainstormed for a bit and here is the product.

  • introduce builtins @clearErrorStack() and @throwCurrentErrorHideCurrentFrame()
  • and consequently drop new keyword failagain
  • and consequently discontinue block-logic or impacting keywords to clear error-stack
  • and consequently use keyword throw instead of overloading try
  • bikeshed: rename fail to throw
  • update code samples

REVISION 2 changes

@mikdusan mikdusan added the proposal This issue suggests modifications. If it also has the "accepted" label then it is planned. label Jan 18, 2021
@daurnimator
Copy link
Collaborator

Do not let an error-union become a value.

Error unions as values are one of the things that makes error handling in zig feel "simple", I would be loathe to give it up. Furthermore, return error.Foo makes it clear that errors are just a value, and not C++ errors or SEH.


Aside, I'm not a fan of the proposed notry keyword, it feels like it shouldn't be needed: "I didn't tell you to try, why do I need to tell you to not try?"

@mikdusan
Copy link
Member Author

Error unions as values are one of the things that makes error handling in zig feel "simple"

Can you show me idiomatic usage of this? Where are error-unions used as values? As far as I know, error-union tags are not easily accessed leaving only prescribed control-flow mechanics. Can use if. Cannot use switch. So what can and what is done with it?

Furthermore, return error.Foo makes it clear that errors are just a value, and not C++ errors or SEH.

return error.Foo is unclear: returning a value or propagating an error? return should not be linked with error semantics. You are still free to return error unions. This would probably be more for comptime meta use than anything. And in that use, errors are not desired.

In all of std.zig unit testing there are only 16 cases of error-unions landing as a DeclVar. sample:

zig/lib/std/fs.zig

Lines 1249 to 1265 in 8118336

/// `flags` must contain `os.O_DIRECTORY`.
fn openDirFlagsZ(self: Dir, sub_path_c: [*:0]const u8, flags: u32) OpenError!Dir {
const result = if (need_async_thread)
std.event.Loop.instance.?.openatZ(self.fd, sub_path_c, flags, 0)
else
os.openatZ(self.fd, sub_path_c, flags, 0);
const fd = result catch |err| switch (err) {
error.FileTooBig => unreachable, // can't happen for directories
error.IsDir => unreachable, // we're providing O_DIRECTORY
error.NoSpaceLeft => unreachable, // not providing O_CREAT
error.PathAlreadyExists => unreachable, // not providing O_CREAT
error.FileLocksNotSupported => unreachable, // locking folders is not supported
error.WouldBlock => unreachable, // can't happen for directories
else => |e| return e,
};
return Dir{ .fd = fd };
}

It is such a rarity. Why subject folks to wall of error messages when a simple try has been omitted? I'm being generous by showing this short wall; the sheer amount of error messages thrown out in comptime coding and build.zig coding is daunting. Depending on how comptime is used, it can be very tricky to even find oft-buried "consider using try..." helper:

./z0.zig:2:8: error: error is ignored. consider using `try`, `catch`, or `if`
    bar();
       ^
./z0.zig:1:24: note: referenced here
export fn entry() void {
                       ^
./z0.zig:7:12: error: expected type 'error:5:11!void', found '@typeInfo(@typeInfo(@TypeOf(foo)).Fn.return_type.?).ErrorUnion.error_set!void'
    return x;
           ^
./z0.zig:7:12: note: error set '@typeInfo(@typeInfo(@TypeOf(foo)).Fn.return_type.?).ErrorUnion.error_set' cannot cast into error set 'error:5:11'
    return x;
           ^
./z0.zig:11:17: note: 'error.Yes' not a member of destination error set
    return error.Yes;
                ^

@pfgithub
Copy link
Contributor

Depending on how comptime is used, it can be very tricky to even find oft-buried "consider using try..." helper:

The "consider using try…" message will always be at the top of the stack of errors. There is another issue open for improving how error messages are printed so it is easier to find the top of the error list because right now every error is buried at the top of a long list. This proposal has no effect on how long the list of error messages is when ignoring an error (for example the error for var a = doit(); would still be buried at the top of the entire call stack).

@mikdusan
Copy link
Member Author

mikdusan commented Jan 19, 2021

message will always be at the top of the stack of errors.

untrue, @compileLog() can emit before try helper messages. But that's a compiler detail.

Zig has no hidden flow control. But zig does hide error handling return err;. Unclear if this is an error or a value. If it is an error, unclear if it resets or grows the error stack. And once we have no hidden error handling, deciphering a wall of errors becomes easier, not more difficult.

This proposal has no effect on how long the list of error messages is when ignoring an error (for example the error for var a = doit(); would still be buried at the top of the entire call stack).

yes, one error is swapped for another.

edit: but in all cases where we have excessive stack depth caused by lack-of-reset this proposal will yield shorter error lists.

@matu3ba
Copy link
Contributor

matu3ba commented Jan 20, 2021

@mikdusan
zig uses a simpler syntax as to not manage error on each type (with all combinations and necessary syntax complexity) and your suggestion is only a syntax-heavy compromise between those two approaches.

Sure, you can name your error

const AllFine = error {
    AccessDenied,
    OutOfMemory,
    FileNotFound,
};

or

const allfine = foo(AllocationError.OutOfMemory);
std.testing.expect(allfine == FileOpenError.OutOfMemory);

Thats the tradeoff zig made in simplicity. The alternative is to handle things like Rust with Result and Option, which are required to be explicitly defined.

@Vexu Vexu added this to the 0.8.0 milestone Jan 26, 2021
@mikdusan
Copy link
Member Author

mikdusan commented Feb 2, 2021

@matu3ba, re:

const allfine = foo(AllocationError.OutOfMemory);
std.testing.expect(allfine == FileOpenError.OutOfMemory);

I am not understanding your point. That code should work unchanged with this proposal because it is only dealing with error-set. This proposal is at the error-union level of things.

@npaskov
Copy link

npaskov commented Feb 9, 2021

i like zig error handling (except the use of error sets - there was a reason C++ moved away from it).

i also like the idea here that error returns be treated as exceptional cases, default being to expect results, not errors.

the notry keyword is inadequate, I'd suggest reusing catch instead, meaning here "I want the whole error union"

example:

extern fn doit() !i32;

const a = try doit(); // getting the i32 value, propagating the error
const b = doit() catch |err| @errorToInt(err); // getting the i32, transforming the error
const c = catch doit();    // catch prefix: explicitly wanting the whole error union
const d = doit(); // compile error - error-handling intent unclear

@mikdusan
Copy link
Member Author

mikdusan commented Feb 9, 2021

const c = catch doit();    // catch prefix: explicitly wanting the whole error union

The problem with this is catch implies error but not error-union, which is what I'd like to make clear. And if we care, it's also the same with many (most?) langs that also have keyword catch it only grabs the actual error thing or exception.

@npaskov
Copy link

npaskov commented Feb 11, 2021 via email

@marler8997
Copy link
Contributor

marler8997 commented Feb 11, 2021

Just want to say I really like the concepts being proposed here. This is making Zig easier to use correctly and harder to use incorrectly. Zig's error handling semantics are already very slick, but they do have a few minor idiosyncrasies that I think this proposal nicely addresses.

Could the proposed raise operator also be used to solve #2647 ? For example, raise ParseError { .index = index };.

Obligatory Bikeshed: Not sure I'm stoked on the keyword notry, maybe something like keeperror or supresserror or catcherror? Or what if we just used catch, like var x = catch foo();? Or if that's ambiguous, maybe var x = foo() catch;.

@marler8997
Copy link
Contributor

For those reading the comments, all comments up to here were for "Revision 1". At this point in time, the proposal is now at Revision 3.

@notYuriy
Copy link

notYuriy commented Feb 19, 2021

Can you show me idiomatic usage of this? Where are error-unions used as values?

I use that to write wrappers on the functions that can error. Example from my code:

    fn read(self: *@This(), index: usize, old_perms: Permissions, new_perms: Permissions) !ObjectRef {
        /*...*/
    }
    pub fn read_from_consumer(self: *@This(), index: usize) !ObjectRef {
        return self.read(index, .ToBeReadByConsumer, .OwnedByConsumer);
    }

    pub fn read_from_producer(self: *@This(), index: usize) !ObjectRef {
        return self.read(index, .GrantedReadRights, .OwnedByConsumer);
    }

@mikdusan
Copy link
Member Author

Can you show me idiomatic usage of this? Where are error-unions used as values?

@notYuriy by that I meant "put into a variable" and something more than just returning it. ie, the whole context was what is the motivation for putting it into a variable, and is that rare?

@matu3ba
Copy link
Contributor

matu3ba commented Feb 24, 2021

@mikdusan Looks like a good proposal now. Some suggestions to improve readability and understandability from my side:

  1. fn f2() !u32 looks like it was not updated or is missing NEW BEHAVIOR/OLD BEHAVIOR distinguishment.
  2. Can you think of a more distinct error than MegaError ?
  3. Can you align the the errors to the language style guideline?
throw error{ .InvalidChar = index };
throw ParseError{ .InvalidChar = index };`

looks rather ugly.
4. You should point out that you dont need any overloading than the orthogonal solution #5610 .
5. I dont understand the table of fundamental error-handling effects and corresponding syntax: The push is abit misleading as catch applies a pop operation (which is not the same as 0) on the error stack. So pop intuitively decreases the size of the stack by 1, which is not reflected in the table.

EDIT: 5 is not quite true, as it depends on the entry and exit paths of catch and throw and how they change the available error set for the followup operations in the control-flow. However, I hope you get abit the idea what I mean with that.

@Luukdegram
Copy link
Sponsor Member

One thing I can't really tell from this proposal is who is responsible for the memory of the propagated value.
What if your library had a way of handling errors by returning some string value with a message in it. You hit an error.OutOfMemory and need to allocate more memory for the string message, what would happen in such case? We could make the values be comptime known, but that would limit the benefits of this proposal I believe.

@mikdusan
Copy link
Member Author

@Luukdegram this proposal doesn't seek to change that an error value is a primitive -- large enough to give a unique value to all possible error "enums".

@Luukdegram
Copy link
Sponsor Member

Luukdegram commented Mar 12, 2021

@mikdusan I see, thanks for clarifying that.

@SpexGuy
Copy link
Contributor

SpexGuy commented Mar 12, 2021

I really like this proposal in some ways, but I'm unclear on what exactly the rules are for prefix catch. Is it required in these situations?

// required for function call when assigning
const a = catch returnsErrorUnion(); // required
// what about if an explicit type is given?
const b: Error!usize = returnsErrorUnion();
// required when forwarding a temporary?
const c = eatError(returnsErrorUnion());
// needed for a copy?
const d = a;
const e = Type.errorUnionDecl;
const f = inst.errorUnionField;
const g = optionalErrorUnion.?;
const h = ptrToErrorUnion.*;
// needed for complex expression?
const i = for (list) |item| {
  if (isAcceptable(item)) break item;
} else error.NotFound;
// what about generics?
const T = Error!usize;
const j = undefinedInstance(T);
// or when storing into a field?
inst.errorUnionField = returnsErrorUnion();
errorUnionArray[idx] = returnsErrorUnion();
// and in tuples?
print("{}\n", .{ returnsErrorUnion() });

@mikdusan
Copy link
Member Author

mikdusan commented Mar 12, 2021

@SpexGuy, added inline comments beginning with ALLCAPS word:

// required for function call when assigning
const a = catch returnsErrorUnion(); // required

// what about if an explicit type is given?
// ERROR: unhandled error
const b: Error!usize = returnsErrorUnion();
// OK
const b: Error!usize = catch returnsErrorUnion();

// required when forwarding a temporary?
// ERROR: unhandled error
const c = eatError(returnsErrorUnion());
// OK
const c = eatError(catch returnsErrorUnion());

// needed for a copy?
// OK: not a function call
const d = a;

// OK: @typeOf(Type.errorUnionDecl) != error-union and not a function call
const e = Type.errorUnionDecl;

// OK: not a function call
const f = inst.errorUnionField;
// OK: not a function call
const g = optionalErrorUnion.?;
// OK: not a function call
const h = ptrToErrorUnion.*;

// needed for complex expression?
// OK: not a function call
const i = for (list) |item| {
  if (isAcceptable(item)) break item;
} else error.NotFound;

// what about generics?
// OK: @typeOf(Error!usize) != error-union and not a function call
const T = Error!usize;
// ERROR: unhandled error; if we want to shuttle around return values like that, it needs to be boxed
const j = undefinedInstance(T);
// or when storing into a field?
// ERROR: unhandled error
inst.errorUnionField = returnsErrorUnion();
// ERROR: unhandled error
errorUnionArray[idx] = returnsErrorUnion();
// and in tuples?
// ERROR: unhandled error
print("{}\n", .{ returnsErrorUnion() });

I think the right thing to do here to add to this proposal something that says "returning an error-union directly shall result in a runtime check if the union tag is in error clear error-stack, add current frame, otherwise it's just returning a value."

see my next comment

@mikdusan
Copy link
Member Author

Here I think are 2 rules that work:

  1. to pull and error-union value out of error-control-flow, use prefix catch
var eu = catch doit();
  1. to push an error-union value in to error-control-flow, use try
var eu = catch doit();
return try eu;
  • equivalent to:
return try doit();

@batiati
Copy link
Sponsor

batiati commented Mar 19, 2021

Instead of @throwCurrentErrorHideCurrentFrame(), we could use just the throw keyword, without arguments.

fn trace_errors(f: fn() !void) !void {
    f() catch |err| {
        std.debug.warn("error traced: {}\n", .{err});
        throw;
    }
}

C# has similar concept

@mikdusan
Copy link
Member Author

mikdusan commented Mar 19, 2021

C++ and Ruby also have similar keyword-without-arg to re-throw/re-raise. Definitely worth considering imo.

@andrewrk andrewrk modified the milestones: 0.8.0, 0.9.0 May 19, 2021
@andrewrk andrewrk modified the milestones: 0.9.0, 0.10.0 Nov 23, 2021
@ayende
Copy link
Contributor

ayende commented Jan 7, 2022

Some counter points to catch doIt(), it is fairly common in my code to do more complex things with errors.

For example, you may have:

var rc : anyerror!usize = writeToDisk();
if (rc) |size| {
   return size;
}
else |err| {
   rc = mitigateError(err); // here I'm handling the error and retry
   if (rc) |size| {
      return size;
   }
   return err; // return the _original_ error. 
}

Error as values is a really nice concept and making it harder to work with errors directly make it more painful.

@marler8997
Copy link
Contributor

Just FYI, I believe your example is equivalent to this one line:

    return writeToDisk() catch |err| (mitigateError(err) orelse err);

@ayende
Copy link
Contributor

ayende commented Jan 7, 2022

When it is simple, sure, but it is usually not just a single call, at which point I can either use {} for multiple expressions, or more explicit control flow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
proposal This issue suggests modifications. If it also has the "accepted" label then it is planned.
Projects
None yet
Development

No branches or pull requests