ziglang / zig

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

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

Open kristoff-it opened 1 year ago

kristoff-it commented 1 year ago

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.

InKryption commented 1 year ago

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 commented 1 year ago

Indeed - see the second note in https://github.com/ziglang/zig/pull/14671#issue-1590183046

kristoff-it commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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:

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.

SpexGuy commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

I don't think switching on a tagged union pointer is a thing, that is why I propose that you have to pass a pointer to actually switch on the union value but being allowed to mutate the content. switch(&val) would actually switch on val but allow you to mutate the content.

It is less "theorically correct" as it is some kind of syntax sugar, but more natural and less error prone.

--

Nicolas Goy https://www.kuon.ch

-------- Original Message -------- From: InKryption @.> Sent: March 1, 2023 3:39:41 PM GMT+01:00 To: ziglang/zig @.> Cc: Nicolas Goy @.>, Comment @.> Subject: Re: [ziglang/zig] new for loops don't want to give pointers to elements of an array (Issue #14734)

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 @.- or we make an exception for something likeanyopaque`, so you'd need to use **@.***` 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.

mg979 commented 1 month ago

Just my 2 cents, but I find the need to write this very counter-intuitive:

for (&good_digits) |*d| {
   d.* = 6;
}

If for is meant to loop over arrays, for (good_digits) would do it, while like this, it seems I want to loop over its address, which makes no sense at all to me. Why would I want to loop over the address of an array? From the perspective of someone who is still learning the language. I hope the semantics don't get too hard to understand and the code too hard to read, I skipped on rust for this reason.

IntegratedQuantum commented 1 month ago

it seems I want to loop over its address, which makes no sense at all to me. Why would I want to loop over the address of an array?

I think the key here is understanding how arrays and slices relate to each other. A pointer to an array is basically just a slice of comptime-known length. And it makes perfect sense to iterate over a slice.

for (&good_digits) |*d| {

is equivalent to

for (good_digits[0..]) |*d| {

From the perspective of someone who is still learning the language. I hope the semantics don't get too hard to understand and the code too hard to read, I skipped on rust for this reason.

If you need further help I'd suggest to join one of the Community Spaces. From what I have seen the discord server and the ziggit forum are the most active and both are very welcoming towards newcomers who need help.

mg979 commented 1 month ago

Thanks for explaining. I still don't understand though, in this case, the need to handle the array as a slice and not just as an array.

I found this topic on Ziggit that digs more on this aspect.