ziglang / zig

General-purpose programming language and toolchain for maintaining robust, optimal, and reusable software.
https://ziglang.org
MIT License
35.26k stars 2.57k forks source link

allow comptime unreachable prong for errors not in error set #22138

Open andrewrk opened 21 hours ago

andrewrk commented 21 hours ago
const Error = error{ A, B };

test "example" {
    foo(1) catch |err| switch (err) {
        error.A => {},
        error.B => {},
        error.C => comptime unreachable,
    };
}

fn foo(x: i32) !void {
    switch (x) {
        0 => return error.A,
        1 => return error.B,
        else => return,
    }
}

Current behavior is a compile error:

test.zig:7:9: error: expected type '@typeInfo(@typeInfo(@TypeOf(test.foo)).@"fn".return_type.?).error_union.error_set', found 'error{C}'
        error.C => comptime unreachable,
        ^~~~~~~
test.zig:7:9: note: 'error.C' not a member of destination error set

I propose instead this construct be allowed, since receiving error.C is indeed comptime unreachable exactly due to not being in the error set, and this provides a way to assert (at compile-time) that no such error is ever added to the set.

This pattern is especially important when using the common pattern of having a particular error in a set indicate that an error message was already created and stored in a separate location.

Currently I am dealing with a large refactor that is being hamstrung by lack of this functionality.

This should certainly be implemented for inferred error sets. For anyerror status quo behavior is already correct. For explicit error sets, I'm less certain, but I am leaning towards the behavior being the same as for inferred error sets.

mlugg commented 21 hours ago

On a related note, we currently allow a few forms of redundant else => prong for switches on errors. However, the way this is currently implemented is incorrect. It's done by this logic in Sema:

https://github.com/ziglang/zig/blob/6188cb8e50882a55c90578114556c6d1b7073e9d/src/Sema.zig#L14066-L14105

Reproduced here since GitHub refuses to embed code references into Sema.zig:

            if (has_else and seen_errors.count() == error_names.len) {
                // In order to enable common patterns for generic code allow simple else bodies
                // else => unreachable,
                // else => return,
                // else => |e| return e,
                // even if all the possible errors were already handled.
                const tags = sema.code.instructions.items(.tag);
                const datas = sema.code.instructions.items(.data);
                for (else_case.body) |else_inst| switch (tags[@intFromEnum(else_inst)]) {
                    .dbg_stmt,
                    .dbg_var_val,
                    .ret_type,
                    .as_node,
                    .ret_node,
                    .@"unreachable",
                    .@"defer",
                    .defer_err_code,
                    .err_union_code,
                    .ret_err_value_code,
                    .save_err_ret_index,
                    .restore_err_ret_index_unconditional,
                    .restore_err_ret_index_fn_entry,
                    .is_non_err,
                    .ret_is_non_err,
                    .condbr,
                    => {},
                    .extended => switch (datas[@intFromEnum(else_inst)].extended.opcode) {
                        .restore_err_ret_index => {},
                        else => break,
                    },
                    else => break,
                } else break :else_validation;

                return sema.fail(
                    block,
                    else_case.src,
                    "unreachable else prong; all cases already handled",
                    .{},
                );
            }

This is not okay: we can't just try to match patterns in ZIR. Doing this will essentially always leak compiler implementation details; for instance, here's a case I found after a quick minute of playing around:

export fn foo() u32 {
    switch (error.Something) {
        error.Something => {},
        else => return 2,   // this triggers a compile error...
        //else => return 0, // ...but this works fine due to ZIR `int(...)` elision for zero
    }
    return 1;
}

So, when we implement this proposal, we should perform the relevant check in AstGen and represent it in the ZIR, and we should rework this existing check to do the same.

nektro commented 20 hours ago

strongly hope this does not make it in because it would be another way for dead code to sneak its way into codebases with no easy way to detect it. using compile error-guided adding and deleting is one of my favorite reasons to use switch. here being the use case of an error is removed from an explicit or inferred error set and zig kindly shows you all the cases to remove.

additionally,

const E = enum { a, b };
var instance: E = .a;
switch (instance) {
  .a => {},
  .b => {},
  // with this proposal it would make sense for this to compile but that feels quite silly
  .c => comptime unreachable,
}

Currently I am dealing with a large refactor that is being hamstrung by lack of this functionality.

very curious if you could elaborate on this as it seems to be the root of the inspiration for this proposal

chadwain commented 9 hours ago

This is similar to #21885

scottredig commented 5 hours ago

Is there a strong reason to do this as a language change, when this is doable in user code? eg:

test "example" {
    foo(1) catch |err| {
        std.debug.forbidErrors(err, error.C);
        switch (err) {
            error.A => {},
            error.B => {},
        }
    };
}

fn foo(x: i32) !void {
    switch (x) {
        0 => return error.A,
        1 => return error.B,
        // 2 => return error.C,
        else => return,
    }
}

const std = struct {
    const debug = struct {
        inline fn forbidErrors(err: anytype, comptime forbidden_errors: anytype) void {
            switch (err) {
                inline else => |have_error| switch (forbidden_errors) {
                    inline else => |forbidden_error| if (have_error == forbidden_error) {
                        @compileError("Error " ++ @errorName(forbidden_error) ++ " must not appear in " ++ @typeName(@TypeOf(err)));
                    },
                },
            }
        }
    };
};

inline fn here stops the compile at the call to forbidErrors, instead of giving the error for a missing switch prong before also giving the @compileError erorr.