Skip to content

Value updated without being assigned, results in bad output #10549

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
EarthenSky opened this issue Jan 9, 2022 · 4 comments
Closed

Value updated without being assigned, results in bad output #10549

EarthenSky opened this issue Jan 9, 2022 · 4 comments
Labels
bug Observed behavior contradicts documented or intended behavior

Comments

@EarthenSky
Copy link

EarthenSky commented Jan 9, 2022

Zig Version

0.9.0

Steps to Reproduce

On windows 11, using the chocolatey zig build of 0.9.0.

The following is the stripped down version of a larger project.

test.zig

const std = @import("std");

pub fn main() !void {
    var list = std.ArrayList(Sphere).init(std.testing.allocator);
    defer list.deinit();

    try list.append(Sphere{.mat = Material.lambertian});
    try list.append(Sphere{.mat = Material.metal});

    // NOTE: manually unrolling the loop fixes

    var out_hit: ?*const Material = null;
    for (list.items) |obj| {
        var possible_hit = obj.hit();
        if (possible_hit.* == Material.lambertian) {
            std.debug.print("I am {}\n", .{possible_hit});
            out_hit = possible_hit; // line 17
        }
    }

    std.debug.print("haha {}\n", .{out_hit}); // line 21
}

pub const Material = enum{
    lambertian,
    metal,
};

pub const Sphere = struct{
    mat: Material,

    pub fn hit(self: *const Sphere) *const Material {
        return &self.mat;
    }
};

do zig run test.zig

Expected Behavior

I expect that since line 17 is only executed once, line 21 should print the same Material as line 16. However, this is not the case.

stdout:

I am Material.lambertian
haha Material.lambertian

Changing the return type of Sphere.hit to Material (non-pointer) fixes this problem. Same with leaving the pointers and manually unrolling.

If it turns out I'm doing something that invokes undefined behaviour, a compiler error or warning would be needed in this situation.

Actual Behavior

stdout:

I am Material.lambertian
haha Material.metal

This is one of those situations where math breaks.

I suspect that the pointer possible_hit is somehow being associated with out_hit, so the second iteration of the for is causing the initialization of possible_hit to assign to out_hit.

@EarthenSky EarthenSky added the bug Observed behavior contradicts documented or intended behavior label Jan 9, 2022
@EarthenSky
Copy link
Author

EarthenSky commented Jan 9, 2022

For additional reference, replacing the for loop with the following will produce valid output in the provided test, but not my larger project.

var i: u32 = 0; 
while (i < list.items.len) : (i += 1) {
    var possible_hit: *const Material = list.items[i].hit();
    if (possible_hit.* == Material.lambertian) {
        std.debug.print("I am {}\n", .{possible_hit});
        out_hit = possible_hit;
    }
}

@EarthenSky EarthenSky changed the title Pointer updated without being assigned, results in bad output Value updated without being assigned, results in bad output Jan 9, 2022
@EarthenSky
Copy link
Author

In addition, the following fails the same as before, but only when the Sphere object is in a tagged union and the material is grabbed using a switch statement. I was hoping the while loop would serve as a workaround, but alas no.

test.zig

const std = @import("std");

pub fn main() !void {
    var objectList = std.ArrayList(VisibleGeometry).init(std.testing.allocator);
    defer objectList.deinit();

    try objectList.append(Sphere.init(Material.lambertian));
    try objectList.append(Sphere.init(Material.metal));

    // NOTE: manually unrolling still works surprisingly

    var out_mat: ?*const Material = null;
    var i: u32 = 0; 
    while (i < objectList.items.len) : (i += 1) {
        var material: *const Material = switch (objectList.items[i]) {
            VisibleGeometryTag.sphere => |sphere| sphere.hit(),
        };

        if (material.* == Material.lambertian) {
            std.debug.print("I am {}\n", .{material});
            out_mat = material;
        }
    }

    std.debug.print("haha {}\n", .{out_mat});
}

pub const VisibleGeometryTag = enum{
    sphere,
};
pub const VisibleGeometry = union(VisibleGeometryTag){
    sphere: Sphere,
};

pub const Material = enum{
    lambertian,
    metal,
};

pub const Sphere = struct{
    mat: Material,

    pub fn init(mat: Material) VisibleGeometry {
        return VisibleGeometry{
            .sphere = Sphere{.mat = mat},
        };
    }

    pub fn hit(self: *const Sphere) *const Material {
        return &self.mat;
    }
};

@Vexu
Copy link
Member

Vexu commented Jan 9, 2022

By default captures |obj| copy the value into a new local constant and out_mat is referencing this new constant which gets replaced each loop and is invalid by the time you access it, capturing by reference |*obj| fixes both of your examples. See #2915 for why this is the default.

@EarthenSky
Copy link
Author

EarthenSky commented Jan 10, 2022

Awesome, thanks for that! I must've missed it when reading through the docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Observed behavior contradicts documented or intended behavior
Projects
None yet
Development

No branches or pull requests

2 participants