Pass-by-value this parameter for call is loaded before calls in the argument list (that modify it) apply side-effects

[kcbanner]

Zig Version

0.15.0-dev.204+d05a3fb1b

Steps to Reproduce and Observed Behavior

This is a reduction from a free list implementation I've been working on.

Run the following via zig test:

const std = @import("std");

const Container = struct {
    elements: std.ArrayListUnmanaged(u8) = .{},

    pub fn init(allocator: std.mem.Allocator) !Container {
        var self: Container = .{};
        try self.elements.ensureTotalCapacity(allocator, 16);
        return self;
    }

    pub fn deinit(self: *Container, allocator: std.mem.Allocator) void {
        self.elements.deinit(allocator);
    }

    pub fn acquire(self: *Container) !usize {
        const next_free = self.elements.items.len;
        if (next_free == self.elements.capacity) return error.Overflow;
        self.elements.addOneAssumeCapacity().* = undefined;
        return next_free;
    }

    pub fn ptr(self: Container, index: usize) *u8 {
        return &self.elements.items[index];
    }
};

test {
    var c = try Container.init(std.testing.allocator);
    defer c.deinit(std.testing.allocator);

    _ = c.ptr(try c.acquire());

    // This works as expected:
    //
    // const i = try c.acquire();
    // _ = c.ptr(i);
}

This panic occurs:

$ zig test value.zig
thread 25088 panic: index out of bounds: index 0, len 0
C:\cygwin64\home\kcbanner\temp\value.zig:24:36: 0x7ff6ac0431c1 in ptr (test.exe.obj)
        return &self.elements.items[index];
                                   ^
C:\cygwin64\home\kcbanner\temp\value.zig:32:14: 0x7ff6ac0414c5 in test_0 (test.exe.obj)
    _ = c.ptr(try c.acquire());
             ^
C:\cygwin64\home\kcbanner\kit\zig\build-stage3-release\lib\zig\compiler\test_runner.zig:214:25: 0x7ff6ac0cf25c in mainTerminal (test.exe.obj)
        if (test_fn.func()) |_| {
                        ^
C:\cygwin64\home\kcbanner\kit\zig\build-stage3-release\lib\zig\compiler\test_runner.zig:62:28: 0x7ff6ac0ca5c8 in main (test.exe.obj)
        return mainTerminal();
                           ^
C:\cygwin64\home\kcbanner\kit\zig\build-stage3-release\lib\zig\std\start.zig:486:53: 0x7ff6ac0ca092 in WinStartup (test.exe.obj)
    std.os.windows.ntdll.RtlExitUserProcess(callMain());
                                                    ^
???:?:?: 0x7ffd354f7373 in ??? (KERNEL32.DLL)
???:?:?: 0x7ffd361fcc90 in ??? (ntdll.dll)
error: the following test command failed with exit code 3:
C:\cygwin64\home\kcbanner\temp\.zig-cache\o\94e320991b15d71965d18146f4352906\test.exe --seed=0xc06fc463

This can be resolved by either applying this diff

-    _ = c.ptr(try c.acquire());
+    const i = try c.acquire();
+    _ = c.ptr(i);

Or this diff:

-    pub fn ptr(self: Container, index: usize) *u8 {
+    pub fn ptr(self: *const Container, index: usize) *u8 {

Expected Behavior

The side effects by the function calls in the argument list should be observed in the outer call to ptr.

[mlugg]

I think this is working as intended. Consider that c.ptr(try c.acquire()) is sugar for Container.ptr(c, try c.acquire()). Zig generally follows a left-to-right evaluation order, so it's pretty clear here that c should be evaluated before try c.acquire(). That's the behavior you're seeing.

Do you have an argument as to why this is a bad behavior?

[rohlem]

While this does look unintuitive/surprising at first glance, I think I agree with mlugg's analysis,
and it's difficult for me to define the flaw here at the syntax/language level.

Do you have an argument as to why this is a bad behavior?

Currently I can't come up with a counter-example, where c.a(c.b())
intuitively reads (to me) like observed behavior (introducing a separate copy of c before the line) is intentional.
Maybe the best argument would be the behavior changing depending on the argument type.
Since c.a(c.b()) is written the same whether it calls fn(Container) or fn(*const Container),
a bug (introducing a copy vs not, whichever was unintended) is non-local, inobservable at the call site.
(If syntax differentiated calls-with-reference, say hypothetical c.a(c->b()) or c.a((&c).b()),
this would be more obvious.)

For combined-assignment operators like +=, the example x += (x += 1); I think is UB in C,
and the Zig language forbids it on a syntax level.
This example is conceptually similar, but the only fix would have to be much more complex.
If we already had some kind of stack/lifetime tracking/analysis,
I think a compile error would be ideal:
The code should be disambiguated by introducing an explicit stack copy,
or splitting the calls into two separate lines.

[kcbanner]

I think this is working as intended. Consider that c.ptr(try c.acquire()) is sugar for Container.ptr(c, try c.acquire()). Zig generally follows a left-to-right evaluation order, so it's pretty clear here that c should be evaluated before try c.acquire(). That's the behavior you're seeing.

I think once you look at the "unsweetened" version, it's much more clear what is going on.

I think the unintuitive part for me, is that my mental model of the capture of the this argument something that was not really part of the argument list evaluation, but happened after that right before the call. Of course, this line of thinking seems silly when you expand the syntax.

It also would seem odd to argue for a special case re-ordering of argument evaluation for the shortened version, as I imagine that would lead to the opposite bug report in the future.