Skip to content

Result location semantics provide footgun for recursive data structures #6216

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

Open
ikskuh opened this issue Aug 31, 2020 · 1 comment
Open
Labels
use case Describes a real use case that is difficult or impossible, but does not propose a solution.
Milestone

Comments

@ikskuh
Copy link
Contributor

ikskuh commented Aug 31, 2020

When writing and reading a structure at the same time, result semantics will destroy information:

const std = @import("std");

const Recursive = union(enum) {
    terminal: void,
    recurse: struct {
        left: *Recursive,
        right: *Recursive,
    },
};

fn moveToHeap(value: anytype) !*@TypeOf(value) {
    const T = @TypeOf(value);
    std.debug.assert(@typeInfo(T) != .Pointer);
    const ptr = try std.heap.page_allocator.create(T);
    ptr.* = value;
    return ptr;
}

pub fn main() !void {
    var root = Recursive {
        .terminal = {},
    };

    root = Recursive { // temporary
        .recurse = .{
            .left = try moveToHeap(root), // this will clone a value with the wrong value
            .right = try moveToHeap(Recursive {
                .terminal = {}
            }),
        },
    };

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

    var i: usize = 0;
    var ptr = &root;

    while(i < 10) : (i += 1) {
        std.debug.print("[{d}] => 0x{X}\n", .{
            i,
            @ptrToInt(ptr),
        });
        ptr = ptr.recurse.left;
    }
}

This example shows that the value copied by moveToHeap(root) is corrupted and is a copy of the temporary constructed, but has a different address.

https://zig.godbolt.org/z/571T4W

A simple workaround is this:

    var temp_root = Recursive { // temporary
        .recurse = .{
            .left = try moveToHeap(root), // this will clone a value with the wrong contents
            .right = try moveToHeap(Recursive {
                .terminal = {}
            }),
        },
    };
    root = temp_root;
@rohlem
Copy link
Contributor

rohlem commented Aug 31, 2020

See also: #3696 for general discussion of overwriting a composite type, and #3234 for specifically the case of overwriting a union.

I don't know whether there was some big discussion yet, but from comments up to now I believe manually creating the temporary, and/or initializing some fields after initial construction, is the "intended solution" (so these self-dependencies will hopefully be caught by a compile error at some point).
(This will probably also interact with the aliasing strategy for function arguments; see #1108 .)

@andrewrk andrewrk added this to the 0.8.0 milestone Oct 4, 2020
@andrewrk andrewrk modified the milestones: 0.8.0, 0.9.0 Nov 6, 2020
@SpexGuy SpexGuy added the use case Describes a real use case that is difficult or impossible, but does not propose a solution. label Mar 21, 2021
@andrewrk andrewrk modified the milestones: 0.9.0, 0.10.0 May 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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

4 participants