Warn/error when returning stack variable references

[kuon]

Zig Version

0.11.0-dev.1022+af197d4954

Steps to Reproduce and Observed Behavior

Consider the following code:

const std = @import("std");
const mem = std.mem;
const expect = std.testing.expect;

pub const MsgType = enum(u8) {
    t0 = 1,
    t1,
};

fn Bytes(comptime len: usize) type {
    return struct {
        const Self = @This();
        buffer: [len]u8,
        pub fn set(self: *Self, value: []const u8) !void {
            if (self.buffer.len != value.len) {
                return error.BadLength;
            }
            mem.copy(u8, &self.buffer, value);
        }
        pub fn get(self: Self, key: []const u8) ![]const u8 {
            _ = key;
            return &self.buffer;
        }
    };
}

pub const Id = Bytes(32);

pub const Msg = union(MsgType) {
    t0: Id,
    t1: Id,

    const Self = @This();

    pub fn set(self: *Self, key: []const u8, value: []const u8) !void {
        switch (self.*) {
            inline else => |*inner| try inner.set(key, value),
        }
    }

    pub fn get(self: Self, key: []const u8) ![]const u8 {
        switch (self) {
            inline else => |inner| return try inner.get(key),
        }
    }
};

const SOME_ID: [32]u8 = .{
    0x11,
    0x22,
    0x33,
    0x44,
    0x55,
    0x66,
    0x77,
    0x88,
    0x88,
    0x00,
    0x11,
    0x22,
    0x33,
    0x44,
    0x55,
    0x66,
    0x77,
    0x88,
    0x88,
    0x00,
    0x11,
    0x22,
    0x33,
    0x44,
    0x55,
    0x66,
    0x77,
    0x88,
    0x88,
    0x00,
    0xaa,
    0xbb,
};

test "bug" {
    const msg = Msg{ .t1 = Id{ .buffer = SOME_ID } };
    const val = try msg.get("id");
    switch (msg) {
        .t1 => |tval| std.debug.print("a0 {*}\n", .{&tval.buffer}),
        else => unreachable,
    }
    std.debug.print("a1 {*}\n", .{val.ptr});
    try expect(mem.eql(u8, val, &SOME_ID));
}

It fails with:

Test [1/1] test.bug... a0 [32]u8@20a240
a1 []const u8@7ffded0e7068
Test [1/1] test.bug... FAIL (TestUnexpectedResult)
/usr/lib/zig/std/testing.zig:509:14: 0x20c5b7 in expect (test)
    if (!ok) return error.TestUnexpectedResult;
             ^
/home/kuon/Playground/test/src/main.zig:91:5: 0x20c668 in test.bug (test)
    try expect(mem.eql(u8, val, &SOME_ID));
    ^
0 passed; 0 skipped; 1 failed.
error: the following test command failed with exit code 1:
/home/kuon/Playground/test/zig-cache/o/ea0577f9d0d0c26f57a4f2d1c170da7b/test

The first weird thing is that the two printed addresses are not the same (a0 and a1) so there must be some array copying involved.

The second problem is the test failing.

If I remove the access like this:

test "bug" {
    const msg = Msg{ .t1 = Id{ .buffer = SOME_ID } };
    const val = try msg.get("id");
    try expect(mem.eql(u8, val, &SOME_ID));
}

the test passes.

Expected Behavior

Test should succeed.

[Vexu]

Bytes.get is returning a pointer to a stack variable, this used to work with stage1 since it properly implemented the pass-by-reference optimization. To fix this make Bytes.get and Msg.get take a *const Self.

[kuon]

Is it expected to work again or should that be considered forbidden in the future?

[Vexu]

I expect it to work once the pass-by-reference optimization is fixed, but since it is just an optimization the compiler is allowed to not do it so you should still take a pointer.

[kuon]

Ok.

And "bonus" thing, I notice that this works:

fn Bytes(comptime len: usize) type {
    return struct {
        const Self = @This();
        buffer: [len]u8,
        pub fn set(self: *Self, value: []const u8) !void {
            if (self.buffer.len != value.len) {
                return error.BadLength;
            }
            mem.copy(u8, &self.buffer, value);
        }
        pub fn get(self: *const Self, key: []const u8) ![]const u8 {
            _ = key;
            return &self.buffer;
        }
    };
}

pub const Id = Bytes(32);

pub const Msg = union(MsgType) {
    t0: Id,
    t1: Id,

    const Self = @This();

    pub fn set(self: *Self, key: []const u8, value: []const u8) !void {
        switch (self.*) {
            inline else => |*inner| try inner.set(key, value),
        }
    }

    pub fn get(self: *const Self, key: []const u8) ![]const u8 {
        switch (self.*) {
            inline else => |*inner| return try inner.get(key),
        }
    }
};

But if I do (inner instead of *inner)

    pub fn get(self: *const Self, key: []const u8) ![]const u8 {
        switch (self.*) {
            inline else => |inner| return try inner.get(key),
        }
    }

It still compiles but I get the bug again. Shouldn't that be a compile error?

Edit: just to clarify, I get why I get the bug (still returning stack values) and why it works (it get coerced), but it feels like the compiler could "do something" to help me catch that.

[Vexu]

It still compiles but I get the bug again. Shouldn't that be a compile error?

Not with the current semantics but if you have some idea on how that could be improved feel free to make a proposal.

[kuon]

Thanks for the taking the time to explain.

I will think about how the developer experience could be improved in this case.

[kuon]

I think there are enough issue open about this problem, for reference: #5725, #2646, #13344, #2301