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

Defer cannot affect return value #6186

Closed
Vexu opened this issue Aug 28, 2020 · 4 comments
Closed

Defer cannot affect return value #6186

Vexu opened this issue Aug 28, 2020 · 4 comments
Milestone

Comments

@Vexu
Copy link
Member

Vexu commented Aug 28, 2020

fn foo() u32 {
    var i: u32 = 5;
    defer i += 5;
    return i;
}

test "defer" {
    var i: u32 = foo();
    std.debug.assert(i == 10); // fails i == 5
}

I would assume the mutated i to be returned since it is possible to mutate the returned variables in other ways, for example deiniting a struct.

@Vexu Vexu added the bug Observed behavior contradicts documented or intended behavior label Aug 28, 2020
@Vexu Vexu added this to the 0.8.0 milestone Aug 28, 2020
@AssortedFantasy
Copy link

I actually thought it wouldn't change them. And I'm not entirely convinced its a great idea that they should change. Because the return in my mind acts a bit like an assignment.

const ret_value: return_type = i

Plus if you are using this it seems like the return statement is kinda a sham and you have to scroll around to see if any defers are lurking around waiting to mutate it.

Also consider this: Should it mutate here?

fn bar() u32 {
    var i: u32 = 5;
    defer i += 5;
    return i+0;
}

Or here?

fn bar() u32 {
    var i: u32 = 5;
    defer i += 5;
    return (&i).*;
}

Or here?

pub var win: bool = true;
fn bar() u32 {
    var i: u32 = 5;
    defer i += 5;
    return if (win) i else i;
}

Treating it as an assignment expression of some return value, the answer is clear in all three cases: no.

Proposal: Defer semantically happens after "return value assignment" but before control flow returns. only items returned by reference may be modified by the defer statememt.

@SpexGuy
Copy link
Contributor

SpexGuy commented Aug 28, 2020

Another example where the current behavior is desired:

const MyList = struct {
    items: []u32,
    capacity: usize,

    fn init(...) @This() { ... }
    fn deinit(self: *@This()) void {
        // ... free array ...
        self.* = undefined;
    }
};

fn foo() usize {
    var list = MyList.init(...);
    defer list.deinit();
    // do stuff
    return list.items.len;
}

If defers run before the return statement here is evaluated, foo() will unexpectedly return undefined.

@katesuyu
Copy link
Contributor

katesuyu commented Aug 28, 2020

I would assume the mutated i to be returned since it is possible to mutate the returned variables in other ways, for example deiniting a struct.

I disagree with the logic of this, primarily the reference to deiniting a struct. Deiniting a struct can only lead to mutation of a "returned" variable when pointers are involved. For example, you had a pointer to the struct and are now deiniting it, or you returned a pointer you obtained through some transitive reference that you now happen to modify in a defer. Remember: when you call ptr_to_struct.deinit(), you're not actually modifying the pointer, you're modifying what it points to. This does not change when you encapsulate the pointer within a struct (a. la. File encapsulating fd_t). Closing a File will not change the representation of the File struct, it will simply close the OS resource corresponding to the fd_t.

We should not write code that mutates a function's locally scoped variable as if it were the result location. We are not prioritizing the reader of our code if we define a local variable and expect modifying it to have external side effects. The result location is external. Currently it is simply a copy elision optimization, and relying on it like you propose is just asking for undefined behavior to bite you later. When we find a way to expose result location explicitly, as #2765 sets out to do, you'll see that it's indeed external: it's an out ptr. Mutating that after return should have external side effects. Not mutating a local variable that happens to be copy-elided into the result location.

@Vexu
Copy link
Member Author

Vexu commented Aug 28, 2020

It is possible to modify local variables via pointers. For example if SpexGuy's foo were to return the MyList then with current behavior you'd get a copy which seems correct except items is now a dangling pointer. If defers ran before the result was copied you'd get undefined bytes.

I'm OK with either way, I was just curious about the expected behavior is since I thought my example would return 10.

@Vexu Vexu closed this as completed Aug 28, 2020
@Vexu Vexu removed the bug Observed behavior contradicts documented or intended behavior label Aug 28, 2020
@Vexu Vexu modified the milestones: 0.8.0, 0.7.0 Oct 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants