popOrNull and friends: Cannot tell the difference between empty list and null element

[Tetralux]

Consider a std.ArrayList(?u64)...

You want to pop the last element and do something if it's depending on whether the element is null or not.

const THRESHOLD = 14;
var list = std.ArrayList(?u64).init(std.debug.global_allocator);
try list.append(null);
const maybe_value = list.popOrNull() orelse unreachable; // fails
if (maybe_value) |n| {
    return n < THRESHOLD;
}
return false;

The orelse unreachable, and of course .? as well, cannot tell the difference between popOrNull saying "the list is empty" and the element being returned but being null.

This seems like a bit of a footgun.

[MasterQ32]

I think the problem is that .pop() can't fail, although it may be called on an empty set. This is a a predictable and probable error source:

pub fn pop(self: *Self) T {
    self.len -= 1;
    return self.items[self.len];
}

if that would return error{OutOfElements} you could write a … catch … orelse … and solve the problem with that.

[CurtisFenner]

I don't understand this issue. The given code works as I would expect (not panicking) because maybe_value.? is null but maybe_value is not -- your if (maybe_value) is determining whether or not the list is empty, and another if inside could determine whether or not that value was null or not.

It needs to work this way so that totally generic code can be programmed against the ArrayList type. Having a ??u64 in a concrete function could definitely lead to confusion but how do you think it can be improved?

[daurnimator]

The given code works as I would expect

I just tried running the code:

const std = @import("std");

pub fn main() !u8 {
    const THRESHOLD = 14;
    var list = std.ArrayList(?u64).init(std.debug.global_allocator);
    try list.append(null);
    const maybe_value = list.popOrNull() orelse unreachable; // fails
    //if (maybe_value) |n| {
    //    return if(n < THRESHOLD) u8(1) else u8(0);
    //}
    return 0;
}

and got:

Call parameter type does not match function signature!
%"??u64"* %0
 %"?u64"*  call fastcc void @"std.array_list.AlignedArrayList(?u64,null).pop"(%"??u64"* sret %0, %"std.array_list.AlignedArrayList(?u64,null)"* %7), !dbg !14056
LLVM ERROR: Broken module found, compilation aborted!

Which is a completely different issue I think?

[Tetralux]

@CurtisFenner The point is that:

1. You naturally expect that orelse will check if popOrNull returns null, like it does for any other element type - not instead check if the element is null, and
2. It's easy to become confused as to why this doesn't do what you expect. It took me way too long to debug this; this is not the sign of a good interface.

This pattern of returning null to indicate the end can be very handy however, especially for while loops where you'd be forced to provide an else branch if it returns an error instead.
If there's a way to allow orelse to distinguish the difference, then that's would might be preferable to having it return an error instead -- not just in this API, but in the future too.

[daurnimator]

const std = @import("std");

pub fn ArrayList(comptime T: type) type {
    return struct {
        const Self = @This();

        items: []T,
        len: usize,

        pub fn pop(self: *Self) T {
            self.len -= 1;
            return self.items[self.len];
        }

        pub fn popOrNull(self: *Self) ?T {
            if (self.len == 0) return null;
            // manually inlining this makes the issue go away too
            return self.pop();
            // self.len -= 1;
            // return self.items[self.len];
        }
    };
}

const A = ArrayList(?u64);

pub fn main() !void {
    var list = A{
        .items = [_]?u64{null},
        .len = 1,
    };
    const maybe_value = list.popOrNull() orelse return error.Foo; // actually throws (the bug)
    // inlining .popOrNull works:
    // const maybe_value = ((??u64)(if (list.len == 0) null else list.pop())) orelse return error.Foo;
}

[LewisGaul]

The symptom of this now appears to be a compile error.

test "" {
    var array = std.ArrayList(?bool).init(testing.allocator);
    _ = array.popOrNull();
}

gives

when analyzing /home/legaul/lib/zig-linux-x86_64-0.8.0/lib/std/array_list.zig:396:28 in compiler source at /home/andy/dev/bootstrap-zig/zig/src/stage1/codegen.cpp:1841: thread 7527 panic: assertion failed. This is a bug in the Zig compiler.

Tested on zig0.8.0 and zig0.9.0-dev.257+fc185a6f7.

[cheese3660]

Can confirm on the compiler error, and built the compiler in debug mode, the full error is as follows

when analyzing C:\Users\arall\Documents\zig\zig-out\lib\zig\std\array_list.zig:400:28 in compiler source at C:\Users\arall\Documents\zig\src\stage1\codegen.cpp:1888: thread 16368 panic: assertion failed. This is a bug in the Zig compiler.
C:\Users\arall\Documents\zig\src\stage1.zig:178:5: 0x7ff74ec206e9 in stage2_panic (zig.obj)
    @panic(ptr[0..len]);
    ^
C:\Users\arall\Documents\zig\src\stage1\analyze.cpp:9893:0: 0x7ff74ea5b7de in src_assert_impl (analyze.obj)
    stage2_panic(msg, strlen(msg));

C:\Users\arall\Documents\zig\src\stage1\codegen.cpp:1888:0: 0x7ff74ead85bf in ir_llvm_value (codegen.obj)
        ir_assert(instruction->value->special != ConstValSpecialRuntime, instruction);

C:\Users\arall\Documents\zig\src\stage1\codegen.cpp:7246:0: 0x7ff74eaed669 in ir_render (codegen.obj)
            instruction->llvm_value = ir_render_instruction(g, executable, instruction);

C:\Users\arall\Documents\zig\src\stage1\codegen.cpp:8683:0: 0x7ff74eadf770 in do_code_gen (codegen.obj)
        ir_render(g, fn_table_entry);

C:\Users\arall\Documents\zig\src\stage1\codegen.cpp:9897:0: 0x7ff74eada65c in codegen_build_object (codegen.obj)
    do_code_gen(g);

C:\Users\arall\Documents\zig\src\stage1\stage1.cpp:132:0: 0x7ff74ebdfc8f in zig_stage1_build_object (stage1.obj)
    codegen_build_object(g);

C:\Users\arall\Documents\zig\src\stage1.zig:149:32: 0x7ff74f025946 in Module::Module.build_object (zig.obj)
        zig_stage1_build_object(mod);
                               ^
C:\Users\arall\Documents\zig\src\Compilation.zig:4744:31: 0x7ff74efe7e63 in Compilation.updateStage1Module (zig.obj)
    stage1_module.build_object();
                              ^
C:\Users\arall\Documents\zig\src\Compilation.zig:2736:36: 0x7ff74edad123 in Compilation.processOneJob (zig.obj)
            comp.updateStage1Module(main_progress_node) catch |err| {
                                   ^
C:\Users\arall\Documents\zig\src\Compilation.zig:2269:30: 0x7ff74eda06cb in Compilation.performAllTheWork (zig.obj)
            try processOneJob(self, work_item, main_progress_node);
                             ^
C:\Users\arall\Documents\zig\src\Compilation.zig:1880:31: 0x7ff74ed9c39f in Compilation.update (zig.obj)
    try self.performAllTheWork();
                              ^
C:\Users\arall\Documents\zig\src\main.zig:2892:20: 0x7ff74ed2647c in main.updateModule (zig.obj)
    try comp.update();
                   ^
C:\Users\arall\Documents\zig\src\main.zig:2575:17: 0x7ff74ec3ce7b in main.buildOutputType (zig.obj)
    updateModule(gpa, comp, hook) catch |err| switch (err) {
                ^
C:\Users\arall\Documents\zig\src\main.zig:210:31: 0x7ff74ec1f0df in main.mainArgs (zig.obj)
        return buildOutputType(gpa, arena, args, .{ .build = .Exe });
                              ^
C:\Users\arall\Documents\zig\src\stage1.zig:48:24: 0x7ff74ec1ed8d in main (zig.obj)
        stage2.mainArgs(gpa, arena, args) catch unreachable;
                       ^
Unable to dump stack trace: InvalidDebugInfo

(Compiler version was the latest master branch as of writing this)