Skip to content

Zir: eliminate field_call_bind and field_call_bind_named #15691

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

Merged
merged 1 commit into from
May 20, 2023

Conversation

mlugg
Copy link
Member

@mlugg mlugg commented May 13, 2023

This commit removes the field_call_bind and field_call_bind_named ZIR instructions, replacing them with a field_call instruction which does the bind and call in one.

field_call_bind is an unfortunate instruction. It's tied into one very specific usage pattern - its result can only be used as a callee. This means that it creates a value of a "pseudo-type" of sorts, bound_fn - this type used to exist in Zig, but now we just hide it from the user and have AstGen ensure it's only used in one way. This is quite silly - Type and Value should, as much as possible, reflect real Zig types and values.

It makes sense to instead encode the a.b() syntax as its own ZIR instruction, so that's what we do here. This commit introduces a new instruction, field_call. It's like call, but rather than a callee ref, it contains a ref to the object pointer (&a in a.b()) and the string field name (b). This eliminates bound_fn from the language, and slightly decreases the size of generated ZIR - stats below.

This commit does remove a few usages which used to be allowed:

  • @field(a, "b")()
  • @call(.auto, a.b, .{})
  • @call(.auto, @field(a, "b"), .{})

These forms used to work just like a.b(), but are no longer allowed. I believe this is the correct choice for a few reasons:

  • a.b() is a purely syntactic form; for instance, (a.b)() is not valid. This means it is not inconsistent to not allow it in these cases; the special case here isn't "a field access as a callee", but rather this exact syntax.
  • The second argument to @call looks much more visually distinct from the callee in standard call syntax. To me, this makes it seem strange for that argument to not work like a normal expression in this context.
  • A more practical argument: it's confusing! @field and @call are used in very different contexts to standard function calls: the former normally hints at some comptime machinery, and the latter that you want more precise control over parts of a function call. In these contexts, you don't want implicit arguments adding extra confusion: you want to be very explicit about what you're doing.

Lastly, some stats. I mentioned before that this change slightly reduces the size of ZIR - this is due to two instructions (field_call_bind then call) being replaced with one (field_call). Here are some numbers:

File Before After Change
Sema.zig 4.72M 4.53M -4%
AstGen.zig 1.52M 1.48M -3%
hash_map.zig 283.9K 276.2K -3%
math.zig 312.6K 305.3K -2%

@mlugg mlugg requested a review from kristoff-it as a code owner May 13, 2023 16:22
@mlugg
Copy link
Member Author

mlugg commented May 13, 2023

@kristoff-it, I've not integrated the new instruction into Autodoc properly - I'm not sure how badly this will break things since I'm not familiar with how Autodoc works, but this form of call is faiiirly common, so maybe it's important. If so, is it alright if I leave that work to you or another contributor familiar with Autodoc?

@rohlem
Copy link
Contributor

rohlem commented May 13, 2023

Note that with one possible resolution to #3839 , splitting @field and @decl,
the usage for method calling would not cleanly fall into either category.
And in lieu of a @methodCall builtin, a userspace function meta.methodCall(s, method_name, args_tuple) does seem more appropriate to me.

@mlugg mlugg force-pushed the refactor/all-my-homies-hate-bound-fn branch 4 times, most recently from b7a4d9d to 092ff28 Compare May 19, 2023 16:39
Copy link
Member

@andrewrk andrewrk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! This looks great. I appreciate the nice writeup.

I found one issue, but it's not worth blocking the merge on. I'll resolve the conflicts myself, do a little touchup, and push this to master.

src/AstGen.zig Outdated
Comment on lines 8978 to 8981
assert(switch (callee) {
.direct => |obj| obj != .none,
.field => |field| field.obj_ptr != .none,
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be more useful with 2 asserts inside each switch prong, so that if an assertion trips it points to the one that failed

This commit removes the `field_call_bind` and `field_call_bind_named` ZIR
instructions, replacing them with a `field_call` instruction which does the bind
and call in one.

`field_call_bind` is an unfortunate instruction. It's tied into one very
specific usage pattern - its result can only be used as a callee. This means
that it creates a value of a "pseudo-type" of sorts, `bound_fn` - this type used
to exist in Zig, but now we just hide it from the user and have AstGen ensure
it's only used in one way. This is quite silly - `Type` and `Value` should, as
much as possible, reflect real Zig types and values.

It makes sense to instead encode the `a.b()` syntax as its own ZIR instruction,
so that's what we do here. This commit introduces a new instruction,
`field_call`. It's like `call`, but rather than a callee ref, it contains a ref
to the object pointer (`&a` in `a.b()`) and the string field name (`b`). This
eliminates `bound_fn` from the language, and slightly decreases the size of
generated ZIR - stats below.

This commit does remove a few usages which used to be allowed:
- `@field(a, "b")()`
- `@call(.auto, a.b, .{})`
- `@call(.auto, @field(a, "b"), .{})`

These forms used to work just like `a.b()`, but are no longer allowed. I believe
this is the correct choice for a few reasons:
- `a.b()` is a purely *syntactic* form; for instance, `(a.b)()` is not valid.
  This means it is *not* inconsistent to not allow it in these cases; the
  special case here isn't "a field access as a callee", but rather this exact
  syntactic form.
- The second argument to `@call` looks much more visually distinct from the
  callee in standard call syntax. To me, this makes it seem strange for that
  argument to not work like a normal expression in this context.
- A more practical argument: it's confusing! `@field` and `@call` are used in
  very different contexts to standard function calls: the former normally hints
  at some comptime machinery, and the latter that you want more precise control
  over parts of a function call. In these contexts, you don't want implicit
  arguments adding extra confusion: you want to be very explicit about what
  you're doing.

Lastly, some stats. I mentioned before that this change slightly reduces the
size of ZIR - this is due to two instructions (`field_call_bind` then `call`)
being replaced with one (`field_call`). Here are some numbers:

+--------------+----------+----------+--------+
| File         | Before   | After    | Change |
+--------------+----------+----------+--------+
| Sema.zig     | 4.72M    | 4.53M    | -4%    |
| AstGen.zig   | 1.52M    | 1.48M    | -3%    |
| hash_map.zig | 283.9K   | 276.2K   | -3%    |
| math.zig     | 312.6K   | 305.3K   | -2%    |
+--------------+----------+----------+--------+
@andrewrk andrewrk added the breaking Implementing this issue could cause existing code to no longer compile or have different behavior. label May 20, 2023
@andrewrk andrewrk force-pushed the refactor/all-my-homies-hate-bound-fn branch from 092ff28 to 32148b4 Compare May 20, 2023 19:27
@andrewrk andrewrk merged commit 38b83d9 into ziglang:master May 20, 2023
@mlugg mlugg deleted the refactor/all-my-homies-hate-bound-fn branch May 18, 2025 20:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Implementing this issue could cause existing code to no longer compile or have different behavior.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants