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

new for loops don't want to give pointers to elements of an array #14734

Open
kristoff-it opened this issue Feb 26, 2023 · 18 comments
Open

new for loops don't want to give pointers to elements of an array #14734

kristoff-it opened this issue Feb 26, 2023 · 18 comments
Labels
proposal This issue suggests modifications. If it also has the "accepted" label then it is planned.
Milestone

Comments

@kristoff-it
Copy link
Member

Zig Version

0.11.0-dev-1817+f6c934677

Steps to Reproduce and Observed Behavior

var good_digits: [3]usize = .{4, 2, 0};
for (good_digits) |*d| {
   d.* = 6;
}

Causes this compile error:

ranges.zig:5:24: error: pointer capture of non pointer type '[3]usize'
    for (good_digits) |*d| {
                       ^~~
ranges.zig:5:10: note: consider using '&' here
    for (good_digits) |*d| {
         ^~~~~~~~~~~

Expected Behavior

Should compile and change the array to 666.

@kristoff-it kristoff-it added the bug Observed behavior contradicts documented or intended behavior label Feb 26, 2023
@InKryption
Copy link
Contributor

InKryption commented Feb 26, 2023

Iiuc these are the new intended semantics. If you want a pointer to the array elements, you need a pointer to the array, e.g. for (&good_digits) |*d| d.* = 6;

@mlugg
Copy link
Member

mlugg commented Feb 26, 2023

Indeed - see the second note in #14671 (comment)

@kristoff-it
Copy link
Member Author

kristoff-it commented Feb 26, 2023

Uh, thank you both.

I'm sure there are a few implementation details that motivated this choice, but setting aside my expectations (it did feel like a bug to me), there's one thing that maybe could be kept into consideration.

When I wrote Easy Interfaces with Zig 0.10.0, a point arose about how to have interface methods that can modify the underlying implementation. You can read the exchange in the comments at the bottom of the article.

In the end, @SpexGuy confirmed that this is correct code:

const Cat = struct {
    hp: usize,

    pub fn eat(self: *Cat) void {
        self.hp += 1;
    }
};

const Animal = union(enum) {
    cat: Cat,
    dog: Dog,

    pub fn eat(self: *Animal) void {
        switch (self.*) {
            inline else => |*case| case.eat(),
        }
    }
};

Note how the switch is only able to give us a mutable case pointer in the inline else branch because we dereference the self pointer first in the switch argument.

This does make sense, but it seems to be at odds with the fact that the for loop refuses to give us a pointer to the array elements unless we first take a pointer to the array.

I'll leave this issue open in case this consideration does have comparable weight to the other reasons that led to the current design.

@Srekel
Copy link

Srekel commented Feb 26, 2023

This also surprised me and think it would be interesting to find out why this choice was made.

My thinking goes, given this:
for (myarr) |*elem|

The * already indicates that I'm explicitly interested in modifying the element of the array. To me the for loop is a language construct that takes something that can be iterated over, and there would be absolutely zero confusion about what I want to do even without the &. Given that it's a language construct it could just be part of the language spec how it behaves.

In fact I'm a bit more confused because what is the problem that could happen if someone used * and didn't use &? Clearly it's fine to iterate over an array without the &, so what does that mean, is it being copied by value? Clearly not because that would be pointlessly bad for performance.

So it's not clearer, it's not more explicit, but it does add an extra, unintuitive, easily forgettable step (requiring going through another compile cycle).

But - I haven't thought it through from all possible angles, there may very well be something that hasn't crossed my mind. But this is what's going through my mind as a user of this new (great!!) feature.

@jayschwa
Copy link
Sponsor Contributor

Clearly it's fine to iterate over an array without the &, so what does that mean, is it being copied by value? Clearly not because that would be pointlessly bad for performance.

Arrays in Zig are pass-by-value, unlike C, where array identifiers are pointers. Think of it as sort of like a struct { 1: usize, 2: usize, 3: usize }. The optimizer can still choose to pass-by-reference under the hood, but that's an implementation detail. In some cases, maybe it's faster for small arrays to be copied.

@SpexGuy
Copy link
Contributor

SpexGuy commented Feb 26, 2023

Arrays in Zig are pass-by-value, unlike C, where array identifiers are pointers.

This is true, but for is not a function. For example, &((array)[idx]) has an array subexpression but may not make a copy. Similarly, if (optional) |*item| may not make a copy, even if the optional would pass by value.

I think this change is very unintuitive. While I can see how this would make the compiler simpler, I think bending the language around that particular optimization is a mistake. IMO we should instead pursue other methods to rethink the way zir treats values vs pointers, potentially fixing these problems in other places as well.

@Jarred-Sumner
Copy link
Contributor

Jarred-Sumner commented Feb 27, 2023

I think this change is very unintuitive. While I can see how this would make the compiler simpler, I think bending the language around that particular optimization is a mistake. IMO we should instead pursue other methods to rethink the way zir treats values vs pointers, potentially fixing these problems in other places as well.

It would be nice if tagged unions also did this coercion, so that you don't have to dereference self.* to determine the active tag.

Every time I read code that dereferences a tagged union pointer in a switch statement, I ask myself:

since self.* is being dereferenced, does *case refer to a stack copy of self or is it a pointer to the field in self?

The syntax makes it read like a stack copy, but real world code typically doesn't want to mutate a stack copy created in the switch (operand) (if it is a stack copy)

const Animal = union(enum) {
    cat: Cat,
    dog: Dog,

    pub fn eat(self: *Animal) void {
        switch (self.*) {
            inline else => |*case| case.eat(),
                      //    ^~~~~
                     //     is case a pointer to a stack copy of self?
        }
    }
};

Instead:

const Animal = union(enum) {
    cat: Cat,
    dog: Dog,

    pub fn eat(self: *Animal) void {
        switch (self) {
            inline else => |*case| case.eat(),
                      //    ^~~~~
                     //     no ambiguity. case is clearly &@field(self, field)
        }
    }
};

The more uncommon usecase of a switch statement on a pointer address value could still be covered via @ptrToInt(self)

@Srekel
Copy link

Srekel commented Feb 27, 2023

Arrays in Zig are pass-by-value, unlike C, where array identifiers are pointers.

This is true, but for is not a function.

Yeah 👍 this is what I mean about it being a construct. Also, for function calls it makes sense that you explicitly add an & because you can see at the call site that you're passing it in to be modified (presumably). But with the for loop it's right there next to it, in the capture.

Again it's not a huge deal but it is a bit annoying as it doesn't seem to give anything to the reader of the program. And to the writer it is in effect worse, especially coupled with the compile error when you comment out the thing that changes the capture - now you'll need to remove two characters to make it compile. And then re-add them when you comment that thing back in again.

@kristoff-it
Copy link
Member Author

It would be nice if tagged unions also did this coercion, so that you don't have to dereference self.* to determine the active tag.

The syntax makes it read like a stack copy, but real world code typically doesn't want to mutate a stack copy created in the switch (operand) (if it is a stack copy)

IMO you need to build a different mental model for this. I had a similar question a while ago and talking with Spex made it clear that constructs like for and switch must be thought of in a different way.

It would be nice if the language had semantics for this stuff that were simple enough to be intuitively understood, so probably that's a goal worth pursuing explicitly, but in the meantime we need to avoid using 1 model for everything and base our analysis of language features only from that perspective.

As an example, your proposal would mean that we would be switching on a pointer but the cases would not be pointer values, which would just move the weird quirk from one place to another (as switching over a value of type T normally means that the cases are also of type T). Deferefencing the pointer does look sus from the perspective of copies, but at least it's sound when it comes to types.

IMO moving the quirk from one place to the other wouldn't be an improvement and, more importantly, the self.* stuff looks suspicious only because we don't have a clear mental model of what that means when done in a switch. Those who do, might not even find it problematic at all.

@andrewrk andrewrk removed the bug Observed behavior contradicts documented or intended behavior label Feb 27, 2023
@andrewrk
Copy link
Member

andrewrk commented Feb 27, 2023

This is working as designed.

When a by-ref capture is present in a switch case, Zig implicitly takes a pointer to the switch condition. The reason switch (x.*) does not make a copy is that zig also has the rule that a pointer deref within a syntactic reference cancel out. In other words, &(x.*) generates the same ZIR as x. I agree with @Jarred-Sumner that switch is counter-intuitive as a result. I'm also not 100% sure that this is actually sound from a language perspective.

As for for loops, the benefit of this recent breaking change is that there is no longer any implicit address-of in for loops. In the syntax for (x), there is no implicit &x, it's just being accessed directly. I think this is ultimately a simpler mental model to grasp. However, I do recognize that having it be different than switch makes it less consistent, and thereby increases the mental overhead of keeping the language in one's head.

It is possible to make the changes suggested by @Jarred-Sumner to if and while, since the expected types there are bool, optional, and error union. The presence of a by-ref capture could instead make the expected condition *bool, *?T, or *E!T. This would make for, if, while all consistent and fundamentally simpler, leaving switch as the only odd one out.

I do think that Jarred's switch suggestion is too problematic with respect to pointers to take as-is. However, I can think of a similar idea that might finish up this hypothetical change to zig's core control flow structures. The idea would be to apply these modifications to switch:

  • If none of the captures are by-ref, then:
    • if the switch condition is a pointer, the switch case items are also pointers
    • if the switch condition is not a pointer, the switch case items are enum tags
  • If any of the captures are by-ref, then:
    • if the switch condition is a pointer, the switch case items are value, and byval captures are union payloads
    • if the switch condition is not a pointer, compile error

I think if all of my suggested changes are applied, it would result in consistency across for, if, while, and switch as well as reduce fundamental language complexity, both for humans and for the compiler. I also think that my suggested semantics are free of footguns. That is, in general, mistakes made due to not understanding the rules about when something is byref or byval, or when something needs its address taken, will always result in compile errors rather than runtime bugs.

@andrewrk andrewrk added the proposal This issue suggests modifications. If it also has the "accepted" label then it is planned. label Feb 27, 2023
@andrewrk andrewrk added this to the 0.11.0 milestone Feb 27, 2023
@SpexGuy
Copy link
Contributor

SpexGuy commented Feb 28, 2023

If none of the captures are by-ref, then:
if the switch condition is a pointer, the switch case items are also pointers
if the switch condition is not a pointer, the switch case items are enum tags
If any of the captures are by-ref, then:
if the switch condition is a pointer, the switch case items are value, and byval captures are union payloads
if the switch condition is not a pointer, compile error

This feels very deeply unintuitive to me. You absolutely should not need to look at the contents of every capture to determine how to interpret a switch. This is easy for the compiler but difficult for humans.

If we were going to do this (and for the record I don't think we should), we should instead give if and switch one implicit dereference, just like .. We should also disallow switch on pointers for clarity, it's not very useful IMO so I don't think that would be a problem.

It's worth noting that this "implicit &x" that you are worried about being complicated occurs in a number of other common places in the language. If you really think this is hard for humans, we should consider changing them too:
x.foo() with foo(self: *@This())
x[a..b] when x is an array value
To make this nicer, we might want to consider changing address-of syntax to x.&, so these become x.&.foo() and x.&[a..b] instead of needing parentheses.

@SpexGuy
Copy link
Contributor

SpexGuy commented Feb 28, 2023

This also doesn't solve the underlying problem that stage 2 doesn't know how to handle references, which still affects RLS and parameter semantics. Instead it bleeds the known-to-be-problematic implementation details of the current compiler into the design of the language. That's why I don't like it. "Simpler for humans" feels like an after-the-fact justification here to make "simpler for a compiler that uses SSA in places it shouldn't" sound ok.

@rohlem
Copy link
Contributor

rohlem commented Mar 1, 2023

As this has been tagged as a proposal, maybe wider-reaching syntax suggestions are on the table.
I'd suggest a separate byref (for by-reference) keyword. It would signal the address-of/reference operation, and be required whenever using a pointer capture.
I think the result would be quite readable:

a: *Animal = &instance;
switch byref (a.*) {
  dog => |*dog| {...},
  cat => |cat| {...}, //not taking a pointer is also ok, but error if none of the captures do.
}
var jumped_or_error = a.jump();
if byref (jumped_or_error) |*jumped| {
  jumped.height *= 2; //context: we are on the moon
} else byref |*e| { //not many use cases, but I guess for consistency
  e.* = error.DidNotJumpButAlsoOnTheMoon;
}
while byref (a.eatNext().*) |*eaten| {
  eaten.tasteLevel += 1; //food tastes better on the moon
}
for byref (a.name) |*char| {
 char.* = upperCaseAssertAscii(char.*); //makes the text a little more readable from far away (like the earth)
}

While we could also signal it by the & operator, which might be clearer semantics,
having it work differently in if &(joe) than in if (&joe)
is less clear when compared with languages that don't require parentheses and would allow if joe as well.

We could put it in front of every capture as &|*capture| instead.
My argument against this would be that whether taking a reference makes sense is more closely tied to the switch subject than to the captures,
F.e. a change from switch (a.foo()) to switch (a.bar()) should make you reconsider whether you want byref,
which is less obvious if it's only written before some of the captures.

@kuon
Copy link
Sponsor Contributor

kuon commented Mar 1, 2023

IMO switch should behave like functions.

What I mean by that is that if you do:


fn main() void {
    var original: Bar;
    fooptr(&original);
    foo(original);   
}

fn foo(bar: Bar) void {
whatever I do here I won't be able to edit "original"
}

fn fooptr(bar: *Bar) void {
here I am able to edit "original"
}

then:

switch(original) {
here we cannot modify original pointer
}
switch(&original) {
here we can
}

I think that would be more consistent and be a simpler mental model.

@haoyu234
Copy link

haoyu234 commented Mar 1, 2023

In the past article introducing soa, a data structure MultiArrayList was mentioned. We can traverse a certain field through its items method. If I need to modify the value of this field, do I need to add a new mut_items method to MultiArrayList to return a pointer?

    // Heal all monsters
    for (soa.mut_items(.hp)) |*hp| {
        hp.* = 100;
    }

@InKryption
Copy link
Contributor

Are there any real world use cases where switching on a pointer to a tagged union is actually useful? Or any pointer for that matter. I've never seen it out in the wild. I'd say that prioritizing the common case (mutating the member of a union through a switch capture) over the uncommon case would make more sense.

A really simple way to do that would be to just disallow switching on pointer values; so to switch on a pointer value, you'd need to use @ptrToInt - or we make an exception for something like *anyopaque, so you'd need to use @ptrCast to switch on a pointer value. Then from there, it's logical to just allow implicitly dereferencing a pointer to a tagged union in a switch.

@rohlem
Copy link
Contributor

rohlem commented Mar 1, 2023

@haoyu234 Method items already returns a value of type []FieldType(field), which is a slice (pointer and length) of non-const elements.
So it already returns a pointer, which in status-quo allows you to capture by pointer and modify the elements.

@kuon
Copy link
Sponsor Contributor

kuon commented Mar 1, 2023 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
proposal This issue suggests modifications. If it also has the "accepted" label then it is planned.
Projects
None yet
Development

No branches or pull requests