fix: method on array-index/deref receiver mutates the live place (issue 0145)
A *self method called directly on arr[i] (or a deref place) fell through to an alloca+store-of-value, so the callee mutated a throwaway copy and the live slot was never written. fixupMethodReceiver now takes the real address of .index_expr/.deref_expr receivers via lowerExprAsPtr (normalized to *T), mirroring the explicit-argument path. A comptime-pack index (xs[i] where xs is a pack) is excluded -- a pack has no runtime storage to address -- so it keeps flowing through the general copy path. Regression: examples/0188-types-method-array-index-receiver.sx
This commit is contained in:
26
examples/0188-types-method-array-index-receiver.sx
Normal file
26
examples/0188-types-method-array-index-receiver.sx
Normal file
@@ -0,0 +1,26 @@
|
|||||||
|
// A `*self` method called directly on an array-index place (`arr[i].method()`)
|
||||||
|
// must mutate the LIVE array slot, not a throwaway copy. `fixupMethodReceiver`
|
||||||
|
// now takes the real address of `.index_expr`/`.deref_expr` receivers (via
|
||||||
|
// `lowerExprAsPtr`), mirroring the explicit-argument path — instead of falling
|
||||||
|
// through to an alloca+store-of-value that the callee then mutates in vain.
|
||||||
|
//
|
||||||
|
// Regression (issue 0145).
|
||||||
|
#import "modules/std.sx";
|
||||||
|
|
||||||
|
S :: struct {
|
||||||
|
flag: bool;
|
||||||
|
set :: (self: *S) { self.flag = true; }
|
||||||
|
}
|
||||||
|
|
||||||
|
A :: struct { items: [4]S; }
|
||||||
|
|
||||||
|
main :: () -> i32 {
|
||||||
|
a : A = .{};
|
||||||
|
a.items[1].set(); // direct: must mutate the slot
|
||||||
|
print("direct = {}\n", a.items[1].flag); // true
|
||||||
|
|
||||||
|
p := @a.items[1];
|
||||||
|
p.set(); // via explicit pointer
|
||||||
|
print("ptr = {}\n", a.items[1].flag); // true
|
||||||
|
0
|
||||||
|
}
|
||||||
@@ -0,0 +1 @@
|
|||||||
|
0
|
||||||
@@ -0,0 +1 @@
|
|||||||
|
|
||||||
@@ -0,0 +1,2 @@
|
|||||||
|
direct = true
|
||||||
|
ptr = true
|
||||||
78
issues/0145-method-on-array-index-receiver-copies-element.md
Normal file
78
issues/0145-method-on-array-index-receiver-copies-element.md
Normal file
@@ -0,0 +1,78 @@
|
|||||||
|
# 0145 — method with `*self` called directly on an array-index expression operates on a COPY
|
||||||
|
|
||||||
|
> **RESOLVED.** `fixupMethodReceiver` (src/ir/lower/expr.zig) now takes the
|
||||||
|
> real address of `.index_expr` and `.deref_expr` receivers via
|
||||||
|
> `lowerExprAsPtr` (normalizing to `*T`), mirroring the explicit-argument path
|
||||||
|
> in `call.zig` — so `arr[i].method()` mutates the live slot instead of a
|
||||||
|
> throwaway copy. A comptime-pack index (`xs[i]` where `xs` is a pack) is
|
||||||
|
> explicitly excluded: a pack has no runtime storage to address, so it keeps
|
||||||
|
> flowing through the general alloca+store-of-value path. Regression test:
|
||||||
|
> `examples/0188-types-method-array-index-receiver.sx`.
|
||||||
|
|
||||||
|
## Summary
|
||||||
|
|
||||||
|
Calling a method whose receiver is `*self` (mutating) directly on a
|
||||||
|
fixed-array element expression — `arr[i].method(...)` — mutates a temporary
|
||||||
|
COPY of the element, not the live array slot. The mutation is silently lost.
|
||||||
|
Binding the element to a pointer first (`p := @arr[i]; p.method(...)`) works
|
||||||
|
correctly.
|
||||||
|
|
||||||
|
## Repro
|
||||||
|
|
||||||
|
```
|
||||||
|
S :: struct {
|
||||||
|
flag: bool;
|
||||||
|
set :: (self: *S) { self.flag = true; }
|
||||||
|
}
|
||||||
|
|
||||||
|
A :: struct { items: [4]S; }
|
||||||
|
|
||||||
|
main :: () -> i32 {
|
||||||
|
a : A = .{};
|
||||||
|
a.items[1].set(); // BUG: mutates a copy
|
||||||
|
print("direct = {}\n", a.items[1].flag); // prints false
|
||||||
|
|
||||||
|
p := @a.items[1];
|
||||||
|
p.set(); // OK: mutates the live slot
|
||||||
|
print("ptr = {}\n", a.items[1].flag); // prints true
|
||||||
|
0
|
||||||
|
}
|
||||||
|
```
|
||||||
|
|
||||||
|
Observed:
|
||||||
|
```
|
||||||
|
direct = false
|
||||||
|
ptr = true
|
||||||
|
```
|
||||||
|
|
||||||
|
Expected: both print `true` — `a.items[1].set()` takes `*self` and should bind
|
||||||
|
the receiver to the address of `a.items[1]`, exactly as the explicit-pointer
|
||||||
|
form does.
|
||||||
|
|
||||||
|
The same surfaced with a non-trivial method (`Slider.handle_event(self: *Slider,
|
||||||
|
...)`): the direct call returned `true` (so the method body ran) yet left the
|
||||||
|
element's `pressed`/`value` fields unchanged, while `@arr[i]` bound to a local
|
||||||
|
and called on that pointer mutated the element as expected.
|
||||||
|
|
||||||
|
## Impact
|
||||||
|
|
||||||
|
Any struct that holds a fixed array of widgets/records and dispatches `*self`
|
||||||
|
methods per element (a layers panel with one `Slider` per row, an entity table,
|
||||||
|
etc.) silently no-ops the mutation. It is easy to miss because the method's
|
||||||
|
return value is correct — only the in-place writes vanish.
|
||||||
|
|
||||||
|
## Workaround
|
||||||
|
|
||||||
|
Bind the element to a pointer before the call:
|
||||||
|
|
||||||
|
```
|
||||||
|
p := @arr[i];
|
||||||
|
p.method(...);
|
||||||
|
```
|
||||||
|
|
||||||
|
Field stores through the index (`arr[i].field = v`) and value assignment
|
||||||
|
(`arr[i] = v`) appear unaffected; only the implicit `&arr[i]` receiver of a
|
||||||
|
`*self` method call is materialized as a copy.
|
||||||
|
|
||||||
|
Found while implementing `ui/layers_panel.sx` in the photo editor (one opacity
|
||||||
|
`Slider` per layer row).
|
||||||
@@ -282,15 +282,33 @@ pub fn fixupMethodReceiver(self: *Lowering, method_args: *std.ArrayList(Ref), fu
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
// Field access: obj.field.method() → GEP to field, pass pointer directly.
|
// Compound lvalue receiver: obj.field.method() / arr[i].method() /
|
||||||
// This avoids copying the struct value (mutations through *T must be visible).
|
// (*p).method() → take the lvalue's real address so mutations
|
||||||
if (obj_node.data == .field_access) {
|
// through *T are visible on the original storage (not a throwaway
|
||||||
const gep_ref = self.lowerExprAsPtr(obj_node);
|
// copy). Mirrors the explicit-arg path in call.zig.
|
||||||
// GEP returns a pointer in LLVM but its IR type is the field value type.
|
//
|
||||||
// Wrap with addr_of (no-op in LLVM) to set the IR type to *T,
|
// Exclude a comptime-pack index (`xs[i]` where `xs` is a pack): a
|
||||||
|
// pack has no runtime storage to address — its element is materialized
|
||||||
|
// at comptime and can't be mutated in place — so it must keep flowing
|
||||||
|
// through the general alloca+store-of-value path below.
|
||||||
|
const is_pack_index = obj_node.data == .index_expr and
|
||||||
|
obj_node.data.index_expr.object.data == .identifier and
|
||||||
|
self.isPackName(obj_node.data.index_expr.object.data.identifier.name);
|
||||||
|
if (!is_pack_index and (obj_node.data == .field_access or obj_node.data == .index_expr or obj_node.data == .deref_expr)) {
|
||||||
|
// `lowerExprAsPtr` yields the lvalue's address, typed either as
|
||||||
|
// `*T` already (index/deref) or as the pointee `T` (a field
|
||||||
|
// "place" ref). Normalize to `*T`: if it's already the pointer
|
||||||
|
// type, pass it directly; if it's the pointee value type, wrap
|
||||||
|
// with addr_of (a no-op in LLVM) to set the IR type to *T,
|
||||||
// preventing coerceCallArgs from doing a spurious alloca+store.
|
// preventing coerceCallArgs from doing a spurious alloca+store.
|
||||||
const ptr_ty = self.module.types.ptrTo(obj_ty);
|
const ptr_ty = self.module.types.ptrTo(obj_ty);
|
||||||
method_args.items[0] = self.builder.emit(.{ .addr_of = .{ .operand = gep_ref } }, ptr_ty);
|
const place = self.lowerExprAsPtr(obj_node);
|
||||||
|
const place_ty = self.builder.getRefType(place);
|
||||||
|
if (place_ty == ptr_ty) {
|
||||||
|
method_args.items[0] = place;
|
||||||
|
} else {
|
||||||
|
method_args.items[0] = self.builder.emit(.{ .addr_of = .{ .operand = place } }, ptr_ty);
|
||||||
|
}
|
||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
// General case: alloca+store the value and pass the alloca pointer
|
// General case: alloca+store the value and pass the alloca pointer
|
||||||
|
|||||||
Reference in New Issue
Block a user