Skip to content
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

Design flaw: Swapping struct fields yields unexpected value #12064

Open
bcrist opened this issue Jul 10, 2022 · 7 comments
Open

Design flaw: Swapping struct fields yields unexpected value #12064

bcrist opened this issue Jul 10, 2022 · 7 comments
Labels
breaking Implementing this issue could cause existing code to no longer compile or have different behavior. use case Describes a real use case that is difficult or impossible, but does not propose a solution.
Milestone

Comments

@bcrist
Copy link
Contributor

bcrist commented Jul 10, 2022

Zig Version

0.10.0-dev.2880+6f0807f50

Steps to Reproduce

const std = @import("std");

const X = struct {
    a: u16,
    b: u16,
};


test {
    var x = X {
        .a = 47,
        .b = 234,
    };

    // swap x.a and x.b
    x = .{
        .a = x.b,
        .b = x.a,
    };

    try std.testing.expectEqual(@as(u16, 234), x.a);
    try std.testing.expectEqual(@as(u16, 47), x.b);
}

Expected Behavior

Conceptually, I expect everything on the right side of an assignment to be evaluated before the left side is modified.

So x.a should be 234 and x.b should be 47, and the test should pass.

Actual Behavior

Both x.a and x.b ends up being equal to 234, and the test fails:

Test [1/1] test ""... expected 47, found 234
Test [1/1] test ""... FAIL (TestExpectedEqual)
C:\...\zig\lib\std\testing.zig:79:17: 0x7ff607721fc8 in td.testing.expectEqual (test.obj)
                return error.TestExpectedEqual;
                ^
C:\...\swap.zig:21:5: 0x7ff60772176a in est "" (test.obj)
    try std.testing.expectEqual(@as(u16, 47), x.b);
    ^
0 passed; 0 skipped; 1 failed.

It would appear that the compiler has transformed the swap into the equivalent of:

x.a = x.b;
x.b = x.a;

The documentation doesn't say much about order of evaluation guarantees as far as I can tell, so maybe this is just undefined behavior in zig, but if that's the case, it seems like a pretty big footgun.

@bcrist bcrist added the bug Observed behavior contradicts documented or intended behavior label Jul 10, 2022
@bcrist
Copy link
Contributor Author

bcrist commented Jul 10, 2022

Note this still happens even if the swapping is moved to a function:

const std = @import("std");

const X = struct {
    a: u16,
    b: u16,
};

fn swap(x: X) X {
    return X {
        .a = x.b,
        .b = x.a,
    };
}

test {
    var x = X {
        .a = 47,
        .b = 234,
    };

    x = swap(x);

    try std.testing.expectEqual(@as(u16, 234), x.a);
    try std.testing.expectEqual(@as(u16, 47), x.b);
}

I suspect the function is getting inlined though; changing the swap function to store it in a temporary variable before returning fixes it:

fn swap(x: X) X {
    var y = X {
        .a = x.b,
        .b = x.a,
    };
    return y;
}

Which makes sense since even if it's inlined, the assignment now happens in a separate statement.

@rohlem
Copy link
Contributor

rohlem commented Jul 10, 2022

I believe your explanation is on point, this is yet another manifestation of #3696 (which is a bit hard to find for how commonly it crops up imo).

The current semantics are to split struct assignments up into individual field writes(, always ordered as written in source code I believe?).
Also, the compiler currently assumes that a function result does not alias any function argument.

IMO this is a big footgun and language semantics should be changed, however, this is not currently being worked on.

@bcrist
Copy link
Contributor Author

bcrist commented Jul 10, 2022

Thanks for the reference. In my opinion optimizations which rely on (non)aliasing assumptions shouldn't be enabled by default, except maybe for release-fast. Based on the existence of the noalias keyword, I had assumed that Zig only used such assumptions when that was present. This kind of undefined behavior is some of the most pernicious in C++, because it's so easy to accidentally fall into, but at least in C++ you have the option of using -fno-strict-aliasing.

@nektro
Copy link
Contributor

nektro commented Jul 10, 2022

this is intentional behavior due to a quirk in Zig's Result Location Semantics. the rules behind it allow for many great things, but this one pattern of in-line reassignment now becomes problematic. its a topic of open research to fix before 1.0 but thats how it works at the moment.

It would appear that the compiler has transformed the swap into the equivalent of:

x.a = x.b;
x.b = x.a;

this is correct, so in your original example both x.a and x.b will be equal to 234.

@bcrist
Copy link
Contributor Author

bcrist commented Jul 10, 2022

Thanks, "Result Location Semantics" was the search term that I didn't know to search for (since #2809 hasn't gotten any love). Since it's functioning as intended (for now at least) I'll close this ticket.

@bcrist bcrist closed this as completed Jul 10, 2022
@andrewrk
Copy link
Member

Hope you don't mind I'm going to reopen this issue because it concisely demonstrates a design flaw with result location semantics and I'm not ready to accept status quo as permanent due to aliasing issues like this.

@andrewrk andrewrk reopened this Jul 10, 2022
@andrewrk andrewrk added this to the 0.11.0 milestone Jul 10, 2022
@andrewrk andrewrk changed the title Swapping struct fields yields unexpected value Design flaw: Swapping struct fields yields unexpected value Jul 10, 2022
@andrewrk andrewrk added breaking Implementing this issue could cause existing code to no longer compile or have different behavior. use case Describes a real use case that is difficult or impossible, but does not propose a solution. and removed bug Observed behavior contradicts documented or intended behavior labels Jul 10, 2022
@SpexGuy
Copy link
Contributor

SpexGuy commented Jul 25, 2022

For tracking, this issue is also demonstrated in #4021, #3234, #3696, and #5973 (which is why it breaks when wrapped in a function).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Implementing this issue could cause existing code to no longer compile or have different behavior. use case Describes a real use case that is difficult or impossible, but does not propose a solution.
Projects
None yet
Development

No branches or pull requests

5 participants