ziglang / zig

General-purpose programming language and toolchain for maintaining robust, optimal, and reusable software.
https://ziglang.org
MIT License
33.67k stars 2.47k forks source link

missing compile error for closing over mutable comptime memory #4721

Open Vexu opened 4 years ago

Vexu commented 4 years ago
const assert = @import("std").debug.assert;

test "comptime var loses value" {
    comptime var i = 0;
    comptime assert(i == 0);
    const S = struct {
        fn foo() void {
            i += 4;
            comptime assert(i == 4);
        }
    };
    _ = S.foo;
    // fails
    comptime assert(i == 4);
}

I expected this to work or at least give an error if this is not supposed to be allowed.

Use case here.

BarabasGitHub commented 4 years ago

Why do you expect it to be 4? You didn't call foo.

Vexu commented 4 years ago

Because I was able to mutate it and this fails:

const expect = @import("std").testing.expect;
test "comptime var loses value" {
    comptime var i = 0;
    comptime expect(i == 0);
    const S = struct {
        fn foo() void {
            i += 4;
            // error: encountered @panic at compile-time
            comptime expect(i != 4);
        }
    };
    _ = S.foo;
}
BarabasGitHub commented 4 years ago

I see.

JesseRMeyer commented 4 years ago

Is the bug that you were able to mutate i even though foo() was never called?

Vexu commented 4 years ago

Whether foo was called or not shouldn't matter as the same thing happens when using a top level comptime block. I was just using a function as the example since that is how I found this.

const expect = @import("std").testing.expect;
test "comptime var loses value" {
    comptime var i = 0;
    comptime expect(i == 0);
    const S = struct {
        comptime {
            i += 4;
            comptime expect(i == 4);
        }
    };
    // fails
    comptime expect(i == 4);
}
markfirmware commented 4 years ago
pub fn main() void {
    comptime var i: u32 = 0;
    while (i < 1) : (i += 1) {
        if (true) {
            @import("std").debug.warn("i {}\n", .{i});
        }
    }
}

This loop doesn’t terminate - same thing?

Vexu commented 4 years ago

@markfirmware that is #411

andrewrk commented 1 year ago

Here is the expected behavior:

const std = @import("std");
const assert = std.debug.assert;

test "comptime var loses value" {
    comptime var i = 0;
    comptime assert(i == 0);
    const S = struct {
        fn foo() bool {
            i += 4;
            comptime assert(i == 4);
            return true;
        }
    };
    try std.testing.expect(S.foo());
    comptime assert(i == 0);
}
test.zig:9:28: error: access of mutable comptime value outside scope
            i += 4;
            ^

And then if you delete these two lines, it should pass:

-            i += 4;
-            comptime assert(i == 4);
mlugg commented 1 year ago

If we applied this diff:

-try std.testing.expect(S.foo());
+try std.testing.expect(comptime S.foo());

What should happen? My expectation is that this is allowed (since analysis of foo is happening at comptime within i's scope), and that the assertion in foo succeeds, but the one at the bottom fails since i == 4.

andrewrk commented 1 year ago

I think it should fail the same way: access of mutable comptime value outside scope. If you want to do that you should have to pass it as a comptime parameter (comptime S.foo(&i)).

@SpexGuy mind if I ping you and confirm that this expected behavior matches your expectations as well?

  1. do you agree with https://github.com/ziglang/zig/issues/4721#issuecomment-1656957477 ?
  2. do you agree with the paragraph above in response to https://github.com/ziglang/zig/issues/4721#issuecomment-1656965895 ?
mlugg commented 1 year ago

Okay, yeah, that's also a reasonable way of doing this. Allowing it wouldn't lead to any issues with analysis order being exposed or anything like that AFAICT, but it would maybe sort of be spooky action at a distance. This is simpler and probably clearer.