diff --git a/examples/0188-types-method-array-index-receiver.sx b/examples/0188-types-method-array-index-receiver.sx new file mode 100644 index 00000000..eab948b2 --- /dev/null +++ b/examples/0188-types-method-array-index-receiver.sx @@ -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 +} diff --git a/examples/expected/0188-types-method-array-index-receiver.exit b/examples/expected/0188-types-method-array-index-receiver.exit new file mode 100644 index 00000000..573541ac --- /dev/null +++ b/examples/expected/0188-types-method-array-index-receiver.exit @@ -0,0 +1 @@ +0 diff --git a/examples/expected/0188-types-method-array-index-receiver.stderr b/examples/expected/0188-types-method-array-index-receiver.stderr new file mode 100644 index 00000000..8b137891 --- /dev/null +++ b/examples/expected/0188-types-method-array-index-receiver.stderr @@ -0,0 +1 @@ + diff --git a/examples/expected/0188-types-method-array-index-receiver.stdout b/examples/expected/0188-types-method-array-index-receiver.stdout new file mode 100644 index 00000000..0a2814cf --- /dev/null +++ b/examples/expected/0188-types-method-array-index-receiver.stdout @@ -0,0 +1,2 @@ +direct = true +ptr = true diff --git a/issues/0145-method-on-array-index-receiver-copies-element.md b/issues/0145-method-on-array-index-receiver-copies-element.md new file mode 100644 index 00000000..f304a637 --- /dev/null +++ b/issues/0145-method-on-array-index-receiver-copies-element.md @@ -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). diff --git a/src/ir/lower/expr.zig b/src/ir/lower/expr.zig index 0ed66b43..05cc64dd 100644 --- a/src/ir/lower/expr.zig +++ b/src/ir/lower/expr.zig @@ -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. - // This avoids copying the struct value (mutations through *T must be visible). - if (obj_node.data == .field_access) { - const gep_ref = self.lowerExprAsPtr(obj_node); - // 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, + // Compound lvalue receiver: obj.field.method() / arr[i].method() / + // (*p).method() → take the lvalue's real address so mutations + // through *T are visible on the original storage (not a throwaway + // copy). Mirrors the explicit-arg path in call.zig. + // + // 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. 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; } // General case: alloca+store the value and pass the alloca pointer