Make std.BitStack.pop return an optional

[adurmk1]

Zig Version

0.14.0-dev.2545+e2e363361

Steps to Reproduce and Observed Behavior

Call peek or push on an empty std.BitStack or call std.BitStack.peekWithState or std.BitStack.popWithState with a bit _len.* of 0.
This will result in an integer overflow panic due to subtracting from zero.

Code to reproduce

const std = @import("std");

pub fn main() !void {
    var bit_stack = std.BitStack.init(std.heap.page_allocator);
    defer bit_stack.deinit();
    _ = bit_stack.pop();
}

Panic output

thread 244661 panic: integer overflow
/usr/lib/zig/lib/std/BitStack.zig:58:33: 0x103c156 in peekWithState (test)
    const byte_index = (bit_len - 1) >> 3;
                                ^
/usr/lib/zig/lib/std/BitStack.zig:65:28: 0x103a64b in popWithState (test)
    const b = peekWithState(buf, bit_len.*);
                           ^
/usr/lib/zig/lib/std/BitStack.zig:42:24: 0x1038133 in pop (test)
    return popWithState(self.bytes.items, &self.bit_len);
                       ^
/home/art/test.zig:6:22: 0x1038095 in main (test)
    _ = bit_stack.pop();
                     ^
/usr/lib/zig/lib/std/start.zig:656:37: 0x1038041 in posixCallMainAndExit (test)
            const result = root.main() catch |err| {
                                    ^
/usr/lib/zig/lib/std/start.zig:271:5: 0x1037c4d in _start (test)
    asm volatile (switch (native_arch) {
    ^
???:?:?: 0x0 in ??? (???)

Expected Behavior

I think there are multiple ways to improve/fix this behaviour of these functions, they are ordered in decending order of practicality in my opinion:

1. The functions should assert that self.bit_len/bit_len.* are greater than zero. This shouldn't affect the stdlib at all.
2. The functions return a nullable type or an error union. This would cause some parts of stdlib like std.json.Scanner to have to be rewritten/fixed.
3. Add a function like isEmpty to std.BitStack to make this behaviour more clear through the interface, this also shouldn't affect the stdlib.
4. Document this behaviour of these functions in the doc comments, this could be done in addition to all the prior solutions.

[mlugg]

The functions should assert that self.bit_len/bit_len.* are greater than zero.

Because integer underflow is Illegal Behavior, using x - 1 is considered a way of asserting that x is greater than 0. As such, this is exactly what status quo does.

The functions return a nullable type or an error union.

This is a reasonable suggestion -- although it should definitely be an optional type rather than an error union. Andrew has previously stated an intention to rename e.g. std.ArrayList(T).popOrNull to std.ArrayList(T).pop, so that you need to write my_list.pop().? to assert that the length is non-zero; this would align with that.

Add a function like isEmpty to std.BitStack

This is unnecessary, and would just bloat the API for no reason. The bit_len field is how users should check this.

Document this behavior of these functions in the doc comments

That's a reasonable point.

---

I think it would make sense to make pop return a ?u1, so I'll mark this issue as a proposal for that.