Proposal: a.foo() syntax requires foo's first argument be a pointer

[david-vanderson]

When calling obj.foo() we expect foo to operate on obj, not on a copy of obj. I propose that calling a function using the obj.foo() syntax be a compile error if foo's first argument is not a pointer.

Message could be something like error: calling function with dot syntax requires first argument of function to be pointer

Rationale

Any member function that accepts the struct by-val as the first argument is a foot-gun:

pub fn foo(self: Self) bar {
    // is self a const pointer or a copy?
    return self.internal_stuff.func();  // sometimes works, other times silently fails due to copied self
}

You want this instead:

pub fn foo(self: *const Self) bar {
    // self is pointer (with or without const as needed)
    return self.internal_stuff.func();  // always works
}

I've made this mistake myself, seen it in zig libs, and now even many places in the zig std lib. I'll send PRs for the ones I've found.

Motivation

I'm using TinyVG to render icons in a gui library. When switching zig to stage2, a few of the icons started rendering nonsense. Eventually I submitted a repo of what I thought was a stage2 bug as #13244.

@nektro and @Vexu quickly pointed out the real problem was accidentally passing a copy of self, which stage2 does more often (at least for now?).

This is a place where code can be working (due to the compiler choosing to pass a const pointer) and then break on a minor compiler upgrade or struct change (which causes the compiler to switch to passing a copy).

This is also hard to figure out because passing a copy often works by accident. If the struct contains only pointers and other copyable fields (Allocator?) then operating on a copy can either work or appear to work.

I'll try to implement this and see how it goes.

Downsides

If you have a small copyable struct like a 2d Point, you have to choose either:

• pub fn add(a: Point, b: Point) Point {} and call it like Point.add(a, b) or
• pub fn add(a: *const Point, b: Point) Point {} and call it like a.add(b) or Point.add(&a, b)

[nektro]

there's plenty of valid uses to not use *const T for structs. there's other areas of research going on to get at the base problem of debugging returning pointers to stack memory

[david-vanderson]

I was hoping that would work as well, but there are cases where you aren't returning anything, just calling functions on internal state. For example, in the std lib priority_queue.zig:

        pub fn deinit(self: Self) void {
            self.allocator.free(self.items);
        }

This only works because self.allocator only contains pointers, and self.items is a slice which also only contains pointers, so even though it's technically deinit-ing a copy of self it just happens to work.

[nektro]

if it made a difference, then .free() would fail because the allocator wouldnt have it in its list of owned references

in which case youd get a panic/trace and know exactly how to fix it

[InKryption]

This wouldn't prevent this case:

const std = @import("std");

test {
    var foo = Foo{};
    Foo.baz(foo);
}

const Foo = struct {
    // pretend this makes the address of the struct significant
    step: std.build.Step = undefined,

    fn bar(foo: *const Foo) void {
        _ = f00.step; // this should be totally safe, right?
    }

    fn baz(foo: Foo) void {
        self.bar(); // uh oh! we're passing a pointer to a copy of 'foo' now.
    }
};

I think this type of issue is better solved by something like #7769, which doesn't impact existing ergonomics.

[david-vanderson]

@nektro I agree that in this case it happens to work, but this is not a theoretical issue. My point with that example is that the intention conveyed when you write my_priority_queue.deinit() is never to call deinit on a copy.

This causes real bugs where internal state is being manipulated mistakenly on a copy.

[david-vanderson]

@InKryption Yes this proposal doesn't affect calling functions without dot syntax. I'm less concerned about that because it's less obvious that you mean to call a function on a specific object (at least to me). I haven't seen a bug myself related to that.

I think #7769 would help in some regards, but there are structs with internal state (say a fixed array buffer) that don't have any internal pointers, so pinning is somewhat orthogonal to this issue.

[rohlem]

I think it's an interesting point to consider that Zig's parameter semantics give more responsibilities to programmers than other languages do.

While lifetime nuances are always an additional detail to consider/ worry about, and I've written bugs like this myself, I currently wouldn't lean towards restricting code of this shape.
My go-to argument is that if you take away the obvious path, people instead may resort to workarounds.
These workarounds make the new code more difficult to write, and read/ reason about, which increases the likelihood of mistakes in those areas.

I could see a potential path restricting one method invocation syntax, if we introduce a second one for the other remaining use cases.
For example, we could have one "pinning method call" syntax, which makes the self-argument non-copyable, to ensure the original object is modified in-place (see #7769 for a related idea).
Maybe in that case it would make sense to explicitly write that into the function signature.
Then perhaps we'd also want a pinning reference-of operator for arguments in other positions.
The logical extreme would be a borrow-checking-lite for those spans of code. I'm sure there's good arguments for and against any position on such a sliding scale.

I would like to point out though that there are many useful instances (obviously non-modifying) of methods that take the first ("self"-) argument by value.
I don't think forbidding method call syntax for them, without more wide-reaching changes to accommodate for it, would have a net-positive effect overall.

[david-vanderson]

@rohlem Thanks for the feedback!

I don't understand the workarounds argument. Can you give an example of where someone would need to work around getting this error? I can't think of any time a person would want to temporarily copy obj during obj.foo() (and it's not guaranteed either way) so it just seems like we are pointing out a mistake. In this case, the obvious way of writing the code has a hidden foot-gun.

I agree that there are many examples of code that uses self by-val that work fine. I think changing them to specifically call by const pointer is not a large burden. Do you disagree?

If so, how about a slightly weaker proposal where instead of a compiler error, the compiler is just forced to always pass obj by-pointer when called with dot syntax? Maybe that will help narrow down where we disagree.

[rohlem]

@david-vanderson My response ended up very long, sorry about that.

First a motivating example for status-quo:

const Minion = struct{ pub fn render(self: A, renderer: *Renderer) !void {
    try renderer.render(...); // directly renders the Minion to the screen
} };
const Mountain = struct{ pub fn render(self: *const B, renderer: *Renderer) !void {
    try renderer.delayedRenderList.enqueue(self); // enqueues the Mountain to be rendered later
} };

Here we can document the fact that Minion.render doesn't depend on the address of the object.
This makes it perfectly safe to call the function on a temporary, the caller doesn't need to f.e. reserve memory for every single Minion object, but can generate one after the other on-the-fly, say in a loop.

We also document the fact that Mountain.render does use the address.
One can assume that this means the lifetime of B should extend to some later point, but to document the details in code would require lifetime type annotations such as in Rust.

Why do this?

As authors of the code, we provide this information both to:

• Other people reading the code - they could also get it from documentation, or from reading and understanding the code itself.

• The compiler - analyzing the lifetimes throughout code can get very complex. Giving guarantees to the compiler (and other language-based tooling) allows optimizations and safety checks.

  • Example of a safety check: The compiler could track all writes of the local variable's addresses (including arguments), and defer writing undefined in these location, to make the object inaccessible after the function returned.
    The compiler obviously couldn't do such a transformation for pointer arguments, because the address is explicitly given, so the code might expect to access the object at the given address later.
    EDIT: This is how I could see such a check implemented; official issue numbers are Add Compiler Error when returning pointer to stack variable #2646 and memory safety when use-after-free occurs of a local function stack variable #3180 .

  • I couldn't come up with a realistic optimization scenario. Everything I thought up would be based on inline-ing, which the one additional address-of shouldn't really prohibit though.

Now if we forbid the dot-syntax for calling minion.render(render_context) as-is, there are two possible consequences:

• All calls will have to be written as Minion.render(minion, render_context). For one-off operations that's fine, but I think it may get unwieldy quickly.
• The function signature is changed to take a pointer type, as a workaround to re-enable using dot-syntax. This means we're no longer communicating this information.

---

Now for direct responses:

Can you give an example of where someone would need to work around getting this error?

Your initial proposal forbids dot-syntax for method calls for methods with a non-pointer first ("self"-) argument.
That means when someone prefers the more concise syntax, they may change the function's signature.
This removes the information that the object's address is irrelevant, and that the argument's lifetime is limited to the function's execution scope.

I can't think of any time a person would want to temporarily copy obj during obj.foo() (and it's not guaranteed either way) so it just seems like we are pointing out a mistake.

To me a value type is helpful documentation that the address (and lifetime outside the function scope) of the object are not needed.
It also enables optimizations and safety checks (mentioned above, EDIT: see also #2646 and #3180), although as the safety checks aren't currently implemented, it's fair to be critical of them.

The obvious way of writing the code has a hidden foot-gun.

For me, seeing a pointer argument is a strong indication that it's relevant to understand the lifetime of the pointee.
On the other hand, a value clearly indicates that the lifetime (and address) are irrelevant.
You are correct in that choosing the latter can be done in error.
The question is whether this source of error outweighs the benefit of optimizations and safety checks that value types allow.
(Again, these safety checks aren't currently implemented, so this obviously remains to be seen.)

---

If everything I wrote so far made sense maybe it's superfluous, but as for your last point:

How about a slightly weaker proposal where instead of a compiler error, the compiler is just forced to always pass obj by-pointer when called with dot syntax?

This would mean @TypeOf(x).foo(x) and x.foo() are no longer equivalent, which is relatively "surprising" behaviour for Zig's standards.
It would also add some complexity having to generate two variants of the same function - at that point we could also always pass by reference.
Even if it's doable, I don't think I see the benefit in it.

The Zen of Zig includes the line "runtime crashes are better than bugs".
Whether this includes bugs in program correctness that don't manifest in execution is maybe a philosophical quandary.

If answered yes, then the goal were actually to add maximum friction within the bounds of simple-to-understand language semantics.
In this instance, that would instead mean to always copy parameters passed by value, because this would lead to runtime errors if their addresses are taken, which would then lead to the bugs in the code being noticed and fixed.
If answered no, then either resolution seems equally fine.

[leroycep]

Another way to make it harder to do this by accident is by forbidding automatic coercion from a T to a *const T when doing t.func()

[david-vanderson]

@rohlem This is good, because I was thinking of the very same example with a different outcome. We start with this code working:

const Minion = struct{ pub fn render(self: Minion, renderer: *Renderer) !void {
    try renderer.render(&self);  // Renders immediately
} };

This works. It gets committed. Tomorrow someone modifies it to:

const Minion = struct{ pub fn render(self: Minion, renderer: *Renderer) !void {
    try renderer.renderDelayed(&self);  // Renders later
} };

And it works! Maybe. Most of the time. Depending on the whims of the zig compiler's choice. And generally we don't get a crash, just random memory corruption. Very hard to debug.

Do you agree with this line of reasoning?

My theory is that the first version gets written because of a hidden assumption that calling m.render() always passes self by ref. I think enforcing this assumption in some way is the goal.

[david-vanderson]

@leroycep That resolves the ambiguity but I think in the opposite direction than most people expect.

[rohlem]

@david-vanderson Not sure if I missed something, but in my eyes your last example doesn't really motivate your proposal.

In your example, at step (1) it is semantically sound to call the method on a temporary or copy of the "originally intended" object. Let's say it is "address-agnostic" for short.
At step (2) the implementation was changed to disallow this, however erroneously the function signature wasn't updated to pass self as (const) pointer to reflect this.

With status-quo, the error that happens in step (2) is that the function is no longer address-agnostic, AND the interface wasn't updated accordingly.
Note that we have here formalized some of the semantics into the program code:
The language allows us to express a value type for step (1), which makes it easier to detect the error in the code of step (2).
Tooling, like the compiler, will be able to add some safety-checks to mitigate it in certain cases, although they realistically can't catch everything.
Programmers can also spot this class of error, although of course realistically we all make mistakes.

In fact, let's consider the scenario where the change in step (2) was written correctly:

• The function signature argument type is changed.
• At a couple of call sites, the type may no longer coerce naturally, which gives the programmer opportunities to spot instances of (B).
• As with any change of the function signature, the programmer may be naturally inclined to check call sites whether their invocation is still semantically sound.
  Particularly with the change from value to pointer type, I'm personally inclined to believe they'd consider the involved object's lifetime.

Note: Intentional points of friction such as these, present throughout Zig's language design, are tangible benefits (even if just minor) of the parameter type changing along with the change in lifetime semantics.
If we stipulate self to always be written as *const, we directly lose them.

From my perspective, the fact that the interface would be required to change in tandem with the semantics, is a benefit of initially writing the value type in step (1), which would be demotivated by your proposal "downgrading" functions with value-type parameters.

---

For the erroneous code of step (2) to actually result in a runtime error (such as "random memory corruption"), two things can happen:

• (A) The Compiler decides to implement the argument passing by creating a copy.
• (B) The object is copied at the call site, some time before the method call.

Keep in mind that it's a valid opinion to want to trigger errors from buggy code, so that those bugs will be discovered and fixed earlier than if we didn't.
Now how would your proposal change the situation?

If we force self to be written as a pointer, then error scenario (A) cannot happen.
If our stance is that we want to find bugs, then hiding bugs this way is counterproductive.
Even if your stance is the opposite, to try to avoid runtime errors,
Error scenario (B) can still happen further up the call chain, whether you wrote the method argument as a pointer or not.

Also note that runtime errors of scenario (B), with your proposal in place, will still only manifest at step (2) and not at step (1), because previously invoking the method on a copy of the object was semantically sound.
The actual bug lies in the fact that the code at step (2) contains incorrect lifetime semantics of the involved object.
(If you're under the impression that writing self as *const would always make programmers more aware of object lifetimes, well I don't personally agree. I think a change in parameter type is more motivating to rethink code at that point.)

---

My theory is that the first version gets written because of a hidden assumption that calling m.render() always passes self by ref.

Do you mean the second version? The first version, as far as I can tell, seems free of bugs.
Either way, I currently do (try to) closely consider object lifetimes when writing Zig code, as I would recommend to everyone else writing Zig code.
I personally don't assume m.render() to pass by reference, although maybe I'm full of myself, or just wrong.
I'm sure programmers will keep on making mistakes, and maybe my impressions of our collectives abilities is also skewed in some regard.

I think there may be value in making changes in lifetime semantics more prominent/explicit, which would make bugs easier to spot.
I personally don't think hiding semantic bugs to make code "keep working as expected", for some sets of expectations, is a foundation for a valuable language change.
(Though I don't want to speak for anyone else around here.)

P.S.:

Forgive me if I'm making an incorrect assumption, but if this particular issue personally interests you, I would highly recommend you to check out Rust if you haven't yet, a language that has been set on formalizing and modelling object lifetimes to the full extent possible/feasible.
While I believe most users of Zig feel confident in their side of the "argument" (and might not be particularly receptive to, say, attempts to introduce borrow checking methodology into Zig), I'm sure broadening one's horizons by learning and using both languages can be a worthwhile experience.

[david-vanderson]

@leroycep That resolves the ambiguity but I think in the opposite direction than most people expect.

A friend pointed out correctly that this reply itself is ambiguous, so let me try again.

const Object = struct {
    pub fn foo(self: Object) void {}
    pub fn bar(self: *const Object) void {}
};

var o = Object{};
o.foo();  // o is either copied (I think unexpectedly), or coerced to *const Object (compiler's choice)
o.bar();  // o is coerced to *const Object

If I understand, you are suggesting removing both coercions to *const Object. This proposal assumes that people expect the coercion to *const Object more than expecting the copy. That's why I wrote "opposite direction". Let me know if this is still unclear.

[rohlem]

@leroycep

At first that suggestion sounded too simplistic, because I'd want to be able to call mutating functions on declared vars.
But now that I reconsider it, with that single change I'm really fond of the idea:
Allow coercion from T to *const T only for var/const declared instances.
(=> not for arguments, temporary function call results, if/catch/loop/ switch case captures, and whatever else I'm forgetting)

example code:

const S = struct{
  fn mutate(s: *S) void {_ = s;}
  fn readLater(s: *const S) void {_ = s;}
};

fn example(mut_s: *S, arg_const_s: *const S, value_s: S) void {
  var var_s = S{};
  const const_s = S{};
  var_s.mutate(); //allowed
  mut_s.mutate(); //allowed
  //type system already disallows const_s.mutate(), arg_const_s.mutate() and value_s.mutate()

  var_s.readLater(); //allowed
  const_s.readLater(); //allowed
  mut_s.readLater(); //allowed
  arg_const_s.readLater(); //allowed
  value_s.readLater(); // NEW ERROR: taking address of contextually declared object in method invocation, use explicit syntax (&value_s)
  (&value_s).readLater(); // EXPLICIT syntax, indicating that "we thought about this"
}

Indeed, this sort of opposes the original proposal posted, but I'd be inclined to support this as a separate change proposal.
The only real "issue" is that it may be an unexpected nuance of the type system, but then again method call syntax taking the object's address is already an exception in itself.

[david-vanderson]

@rohlem Definitely helping, I appreciate your taking the time to dig into this. I think our first point of disagreement is when the bug was introduced. My claim is the bug was present in step 1, and the program only worked accidentally. This is my experience with real bugs where the code never even made it to step 2.

I'm trying to figure out a way to prevent the bug in step 1. I'm not suggesting changing the semantics of function calls in general, because it seems that programmers rarely confuse call by-ref and by-val using normal function call syntax.

I think the heart of the bug is you look at the call site minion.render(); and think "I'm calling the render function on this minion object" because almost all of the time this assumption is correct (because most objects are large enough that the compiler passes them by const pointer most of the time, or the code runs the same way with a copy of the minion object).

That's why I think of this as a foot-gun instead of a normal bug. You write code that works (by accident), and then starts failing at some later date.

[rohlem]

@david-vanderson Ah, apologies, I initially misread your example; I assumed step (1) would call the function to pass only the object's value (as I'd intended in my initial example), as renderer.render(self), not via its address &self.

But even then, the first version only has a bug if the renderer assumes a longer lifetime of its argument's pointee, right?
So, I'm arguing we should pass the object as value both times, while you seem to say passing it by reference/pointer would be preferable?

In my opinion, if there is a lifetime bug in either step, then passing the object by value helps reveal this lifetime bug, while passing it by pointer just hides it.
Note that even if it's passed by pointer, calling the method on a copy of the object can have the same effect: const minion2 = minion; minion2.render();

I think the heart of the bug is you look at the call site minion.render(); and think "I'm calling the render function on this minion object"

While it's possible to read the code that way, in the provided example the bug in step (1) - assuming additional object lifetime is necessary - would be with the function signature, not the call site.
Therefore whether the compiler chooses to copy a particular object only controls whether the bug manifests as a runtime error; the semantic bug in the code exists either way.

That's why I think of this as a foot-gun instead of a normal bug. You write code that works (by accident), and then starts failing at some later date.

I agree that non-reproducible, spuriously-manifesting bugs are more annoying to find.
But that's why I prefer to make them trigger more errors, until the underlying bugs are fixed, instead of trying to minimize the errors they cause.

Ideally, the compiler will include safety-checks that make code with errors like this fail reliably, every time, maybe even with a hard crash and stack trace.
EDIT: adding corresponding issue numbers in post: #2646 and #3180 .
I think these types of safety-checks are much more realistic to implement when using value types, not when writing pointer arguments.

[stacktracer]

I don't yet have an opinion about the best approach, but I'm grateful to see the discussion about the foot-gun. It's an especially nasty one:

• Leads to bugs where no single line of code is wrong, but interaction among two or more lines is wrong
• Leads to "landmine" bugs that may stay hidden for an arbitrarily long time, not appearing until long after the dev working on that spot in the code has forgotten all the details
• Manifests in headscratcher issues like memory corruption

[stacktracer]

It feels to me like the root of the issue is the implicit object-to-pointer coercion. Zig is generally so skeptical of implicit magic that the coercion feels out of character here. Restricting the coercion feels like addressing the root of the issue, which I find appealing at a vague aesthetic level.

As a point of reference, in C the difference is syntactically explicit: o.foo vs. o->foo.

[david-vanderson]

But even then, the first version only has a bug if the renderer assumes a longer lifetime of its argument's pointee, right?

Yes, which means that it's easy for the bug to stay hidden until later when someone changes the renderer to defer things. The person doing that change could easily reason that the objects don't move due to the way they are allocated (and miss the hidden foot-gun copy). Then things continue to work for some time, and then later break on upgrading to a newer zig. (This is almost the exact situation where I first encountered this issue)

So, I'm arguing we should pass the object as value both times, while you seem to say passing it by reference/pointer would be preferable?

I think either position is defensible and it comes down to a practical question of what people expect when they use dot syntax method calling.

Ideally, the compiler will include safety-checks that make code with errors like this fail reliably, every time, maybe even with a hard crash and stack trace.

I agree in an ideal world, but I'm not sure this is possible or practical for zig. This proposal is my attempt at a minimal practical change to improve the situation.

[david-vanderson]

I hacked the zig compiler locally to make this an error. The error reporting stuff in the zig compiler is really good!

After almost 1000 fixups to the standard library I gave up before getting zig build test-std -Dskip-release passing. You can see how far I got at https://github.com/david-vanderson/zig/tree/dot_stynax_error

Specific Notes:

• FixedBufferStream.getWritten() is the only function there that takes by-val (seems like oversight). Could be a bug depending on passed in buffer type but passing array gives other type errors (assumes you can slice it)
• BoundedArray.slice() takes self: anytype as first argument and then constructs a return type (looks like fancy way to try to get both mutable and const slices from same function, but also has fn constSlice)
• SegmentedList.at() uses different code to do similar stuff as BoundedArray
• LinearFifo uses comptime to work around this const SliceSelfArg = if (buffer_type == .Static) *Self else Self;
• process.zig: EnvMap assumes a HashMap can be copied (is this always true?)
• dwarf.zig: FormValue.getData16() is correct because it copies the whole array but would be bug if changed to return a slice
• File.read (also File.write) had to be split into 2 functions to satisfy the io.Reader type checking
• A few places needed changing x.foo(...) to @TypeOf(x).foo(x, ...) when the function signature couldn't be changed (like hash_map.zig getAutoHashFn where it specifies the function signature)

I didn't find a smoking gun kind of bug so far. Changing the first argument to a pointer caused a lot of changes in the crypto stuff which made me worried. Also functions on enums look weirder. All around the changes were more substantial than I anticipated.

I'll try a compiler change where instead of an error we just mandate passing by pointer when using dot syntax.

[nektro]

rather than forcing *const T it would likely better for the compiler to do the automatic *const conversion for structs larger than usize. this lets the optimization stay in place and not require massive changes to user code

[david-vanderson]

I must be missing something. Isn't that how the compiler works today? It gets to choose whether to pass T or convert to *const T?

[david-vanderson]

I used my hacked compiler on a bunch of zig projects (the ones listed here: #89). I also did not find any smoking gun bugs there. They had things similar to what I found in the std lib.

I was wrong about how often this bug shows up in current code. It seems either rarer to make, or faster/easier to identify and fix, which is contrary to my own experience, so maybe I'm the outlier. #2646 will help cover some of the cases here like returning a slice to an internal buffer. So I'll close this issue.

Thank you so much to everyone who commented and thought through this with me!