From 1b0c857b917af4acaf023fed635497ba95095a48 Mon Sep 17 00:00:00 2001 From: agra Date: Mon, 22 Jun 2026 18:28:57 +0300 Subject: [PATCH] =?UTF-8?q?fix:=20struct-literal=20=E2=86=92=20optional=20?= =?UTF-8?q?coercion=20+=20#get=20through=20optional=20chain=20(issue=20016?= =?UTF-8?q?0)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two fixes for optional interactions surfaced by the #set/#get review. The original issue 0160 mis-diagnosed (A) as an optional-chain bug; the chain works fine for real fields. The actual bugs: (A) A bare struct literal `.{ ... }` against an optional target `?T` was built into the optional's {payload, has_value} layout instead of the inner T, then re-wrapped — corrupting the value (a multi-field payload's first field clobbered by the has_value flag, or a `?T` arg silently null) or failing LLVM verification. lowerStructLiteral now builds the inner T, materializes it, and wraps via coerceToType; lowerVarDecl's previously-UNCONDITIONAL optional wrap is guarded so an already-`?T` value isn't double-wrapped. Fixed across var-decl, arg, return, nested field, reassignment, and array-element contexts. (B) `#get` accessors are now reachable through an optional chain (`obj?.getter`): lowerOptionalChain dispatches the getter via a synthetic receiver, and expr_typer types `obj?.getter` through a shared getterReturnTypeOnDeref helper (handles `?T` and `?*T`, value and pointer optionals, and generic-instance getters like List.len). The `#set` write side through `?.` is intentionally left matching real-field behavior (optional-chain assignment unsupported). Regression tests: examples/optionals/0906 (struct-literal → optional) and 0907 (accessor through chain). issues/0160 marked RESOLVED with the corrected root cause. --- ...-optionals-struct-literal-into-optional.sx | 41 +++++++ .../0907-optionals-accessor-through-chain.sx | 39 +++++++ ...ptionals-struct-literal-into-optional.exit | 1 + ...ionals-struct-literal-into-optional.stderr | 1 + ...ionals-struct-literal-into-optional.stdout | 6 + ...0907-optionals-accessor-through-chain.exit | 1 + ...07-optionals-accessor-through-chain.stderr | 1 + ...07-optionals-accessor-through-chain.stdout | 5 + ...onal-chain-value-optional-and-accessors.md | 18 +++ src/ir/expr_typer.zig | 13 +++ src/ir/lower.zig | 1 + src/ir/lower/expr.zig | 103 +++++++++++++++++- src/ir/lower/stmt.zig | 6 +- 13 files changed, 232 insertions(+), 4 deletions(-) create mode 100644 examples/optionals/0906-optionals-struct-literal-into-optional.sx create mode 100644 examples/optionals/0907-optionals-accessor-through-chain.sx create mode 100644 examples/optionals/expected/0906-optionals-struct-literal-into-optional.exit create mode 100644 examples/optionals/expected/0906-optionals-struct-literal-into-optional.stderr create mode 100644 examples/optionals/expected/0906-optionals-struct-literal-into-optional.stdout create mode 100644 examples/optionals/expected/0907-optionals-accessor-through-chain.exit create mode 100644 examples/optionals/expected/0907-optionals-accessor-through-chain.stderr create mode 100644 examples/optionals/expected/0907-optionals-accessor-through-chain.stdout diff --git a/examples/optionals/0906-optionals-struct-literal-into-optional.sx b/examples/optionals/0906-optionals-struct-literal-into-optional.sx new file mode 100644 index 00000000..26571a5a --- /dev/null +++ b/examples/optionals/0906-optionals-struct-literal-into-optional.sx @@ -0,0 +1,41 @@ +// A bare struct literal `.{ ... }` against an optional target `?T` builds the +// inner `T` and wraps it once — in every caller context: var-decl, function +// argument, return value, a nested `?T` struct field, reassignment, and an +// array element. Previously this filled the optional's {payload, has_value} +// layout directly, corrupting the value (a multi-field payload's first field +// was clobbered by the has_value flag, or a `?T` arg silently read as null) or +// failing LLVM verification on a double wrap. +// Regression (issue 0160). +#import "modules/std.sx"; + +T :: struct { a: i64 = 0; b: i64 = 0; } +Outer :: struct { opt: ?T = null; tag: i64 = 0; } + +take :: (o: ?T) -> i64 { if o != null { return o!.a * 10 + o!.b; } return -1; } +make :: () -> ?T { return .{ a = 5, b = 6 }; } + +main :: () -> i64 { + // var-decl + v : ?T = .{ a = 3, b = 9 }; + if v != null { print("var: {} {}\n", v!.a, v!.b); } // 3 9 + + // function argument + print("arg: {}\n", take(.{ a = 4, b = 7 })); // 47 + + // return value + r := make(); + if r != null { print("ret: {} {}\n", r!.a, r!.b); } // 5 6 + + // nested ?T struct field + o : Outer = .{ opt = .{ a = 1, b = 2 }, tag = 8 }; + if o.opt != null { print("nested: {} {} {}\n", o.opt!.a, o.opt!.b, o.tag); } // 1 2 8 + + // reassignment + v = .{ a = 11, b = 12 }; + if v != null { print("reassign: {} {}\n", v!.a, v!.b); } // 11 12 + + // array element + arr : [2]?T = .[ .{ a = 20 }, .{ a = 21 } ]; + if arr[0] != null { if arr[1] != null { print("arr: {} {}\n", arr[0]!.a, arr[1]!.a); } } // 20 21 + return 0; +} diff --git a/examples/optionals/0907-optionals-accessor-through-chain.sx b/examples/optionals/0907-optionals-accessor-through-chain.sx new file mode 100644 index 00000000..f2f3ed0d --- /dev/null +++ b/examples/optionals/0907-optionals-accessor-through-chain.sx @@ -0,0 +1,39 @@ +// A `#get` property accessor is reachable through an optional chain +// (`obj?.getter`): the some-branch dispatches the getter and the result is +// re-wrapped as `?R`; a null receiver short-circuits to null. Works for a value +// optional (`?T`), a pointer optional (`?*T`), and a generic-instance getter +// (`List.len`), and types correctly without an explicit annotation. Real fields +// through `?.` keep working unchanged. +// Regression (issue 0160). +#import "modules/std.sx"; + +Temp :: struct { + raw: i64 = 0; + doubled :: (self: *Temp) -> i64 #get => self.raw * 2; +} + +main :: () -> i64 { + t : Temp = .{ raw = 4 }; + + // value optional ?T — getter through chain + ot : ?Temp = t; + print("?T getter: {}\n", ot?.doubled ?? -1); // 8 + + // pointer optional ?*T — getter through chain + pt : ?*Temp = @t; + print("?*T getter: {}\n", pt?.doubled ?? -1); // 8 + + // null receiver short-circuits + nope : ?Temp = null; + print("null getter: {}\n", nope?.doubled ?? -1); // -1 + + // real field through chain still works + print("?T field: {}\n", ot?.raw ?? -1); // 4 + + // generic-instance getter (List.len) through chain + xs : List(i64) = .{}; + xs.append(10); xs.append(20); xs.append(30); + pxs : ?*List(i64) = @xs; + print("?*List len: {}\n", pxs?.len ?? -1); // 3 + return 0; +} diff --git a/examples/optionals/expected/0906-optionals-struct-literal-into-optional.exit b/examples/optionals/expected/0906-optionals-struct-literal-into-optional.exit new file mode 100644 index 00000000..573541ac --- /dev/null +++ b/examples/optionals/expected/0906-optionals-struct-literal-into-optional.exit @@ -0,0 +1 @@ +0 diff --git a/examples/optionals/expected/0906-optionals-struct-literal-into-optional.stderr b/examples/optionals/expected/0906-optionals-struct-literal-into-optional.stderr new file mode 100644 index 00000000..8b137891 --- /dev/null +++ b/examples/optionals/expected/0906-optionals-struct-literal-into-optional.stderr @@ -0,0 +1 @@ + diff --git a/examples/optionals/expected/0906-optionals-struct-literal-into-optional.stdout b/examples/optionals/expected/0906-optionals-struct-literal-into-optional.stdout new file mode 100644 index 00000000..d9b34362 --- /dev/null +++ b/examples/optionals/expected/0906-optionals-struct-literal-into-optional.stdout @@ -0,0 +1,6 @@ +var: 3 9 +arg: 47 +ret: 5 6 +nested: 1 2 8 +reassign: 11 12 +arr: 20 21 diff --git a/examples/optionals/expected/0907-optionals-accessor-through-chain.exit b/examples/optionals/expected/0907-optionals-accessor-through-chain.exit new file mode 100644 index 00000000..573541ac --- /dev/null +++ b/examples/optionals/expected/0907-optionals-accessor-through-chain.exit @@ -0,0 +1 @@ +0 diff --git a/examples/optionals/expected/0907-optionals-accessor-through-chain.stderr b/examples/optionals/expected/0907-optionals-accessor-through-chain.stderr new file mode 100644 index 00000000..8b137891 --- /dev/null +++ b/examples/optionals/expected/0907-optionals-accessor-through-chain.stderr @@ -0,0 +1 @@ + diff --git a/examples/optionals/expected/0907-optionals-accessor-through-chain.stdout b/examples/optionals/expected/0907-optionals-accessor-through-chain.stdout new file mode 100644 index 00000000..0297f2e4 --- /dev/null +++ b/examples/optionals/expected/0907-optionals-accessor-through-chain.stdout @@ -0,0 +1,5 @@ +?T getter: 8 +?*T getter: 8 +null getter: -1 +?T field: 4 +?*List len: 3 diff --git a/issues/0160-optional-chain-value-optional-and-accessors.md b/issues/0160-optional-chain-value-optional-and-accessors.md index a538861e..ed737055 100644 --- a/issues/0160-optional-chain-value-optional-and-accessors.md +++ b/issues/0160-optional-chain-value-optional-and-accessors.md @@ -1,5 +1,23 @@ # 0160 — optional-chain field access: `?T` value-optional read miscompiles, and `#get`/`#set` accessors aren't reached through `?.` +> **RESOLVED.** Root cause was MIS-DIAGNOSED in the original writeup below: (A) +> was NOT an optional-chain bug — the chain works fine for real fields. The real +> bug was **struct-literal → optional coercion**: a bare `.{ ... }` against a +> `?T` target was built into the optional's `{payload, has_value}` layout +> instead of building the inner `T` and wrapping it. Fix: +> `lowerStructLiteral` (`src/ir/lower/expr.zig`) now builds the inner `T`, +> materializes it, and wraps via `coerceToType`; and `lowerVarDecl` +> (`src/ir/lower/stmt.zig`) only wraps when the value is not already the target +> optional (its previous UNCONDITIONAL wrap double-wrapped any already-`?T` +> value). (B) the `#get`-through-`?.` gap was real and is fixed: +> `lowerOptionalChain` dispatches the getter via a synthetic receiver, and +> `expr_typer` types `obj?.getter` through a shared `getterReturnTypeOnDeref` +> helper (handles `?T` and `?*T`). The `#set` write side through `?.` is +> intentionally left matching real-field behavior (optional-chain assignment is +> unsupported for all fields) and was never part of this bug. Regression tests: +> `examples/optionals/0906-optionals-struct-literal-into-optional.sx` and +> `examples/optionals/0907-optionals-accessor-through-chain.sx`. + ## Symptom Two related gaps in optional-chain field access (`obj?.field`), surfaced while diff --git a/src/ir/expr_typer.zig b/src/ir/expr_typer.zig index 39c97fec..542421e0 100644 --- a/src/ir/expr_typer.zig +++ b/src/ir/expr_typer.zig @@ -259,6 +259,19 @@ pub const ExprTyper = struct { for (fields) |f| { if (f.name == field_name_id) return if (is_opt_chain) self.l.optionalOfFlattened(f.ty) else f.ty; } + // `#get` property accessor through an optional chain + // (`obj?.getter`): resolve the getter on the dereferenced + // inner type via a synthetic `*T` receiver, so a value + // optional (`?T`) — whose receiver would otherwise be the + // optional itself — and a pointer optional (`?*T`) both type + // exactly as `lowerOptionalChain` emits (issue 0160). + if (is_opt_chain) { + var deref_inner = obj_ty; + if (!deref_inner.isBuiltin() and self.l.module.types.get(deref_inner) == .pointer) + deref_inner = self.l.module.types.get(deref_inner).pointer.pointee; + if (self.l.getterReturnTypeOnDeref(deref_inner, fa.field)) |rt| + return self.l.optionalOfFlattened(rt); + } // `#get` property accessor: type as the accessor's return // type. Resolve via the synthesized no-arg call so generic // bindings (e.g. a `List(T)` getter returning `T`) resolve diff --git a/src/ir/lower.zig b/src/ir/lower.zig index 72957b16..5415e3ea 100644 --- a/src/ir/lower.zig +++ b/src/ir/lower.zig @@ -1972,6 +1972,7 @@ pub const Lowering = struct { pub const getStructFields = lower_expr.getStructFields; pub const getAccessorFor = lower_expr.getAccessorFor; pub const getSetterFor = lower_expr.getSetterFor; + pub const getterReturnTypeOnDeref = lower_expr.getterReturnTypeOnDeref; pub const fixupMethodReceiver = lower_expr.fixupMethodReceiver; pub const getStructTypeName = lower_expr.getStructTypeName; pub const builtinTypeName = lower_expr.builtinTypeName; diff --git a/src/ir/lower/expr.zig b/src/ir/lower/expr.zig index d09d08de..dea8101f 100644 --- a/src/ir/lower/expr.zig +++ b/src/ir/lower/expr.zig @@ -95,6 +95,34 @@ pub fn lowerStructLiteral(self: *Lowering, sl: *const ast.StructLiteral, span: a return self.lowerUnionLiteral(sl, ty, span); } + // A bare struct literal against an optional target `?T` builds the INNER + // `T` and wraps it once. Without this `ty` is the optional itself, so the + // literal is lowered into the optional's `{payload, has_value}` layout and + // then re-wrapped — corrupting the value (a `?T` arg silently reads as + // null) or failing LLVM verification on the double wrap (issue 0160). + // Only the bare `.{ ... }` form reaches here with an optional `ty` (a named + // / generic literal resolves `ty` from its own type, never the target). + if (!ty.isBuiltin() and self.module.types.get(ty) == .optional) { + const child = self.module.types.get(ty).optional.child; + // Build the inner `T` (targeting it so nested literals resolve), then + // wrap to `?T`. Building into `ty` (the optional) directly would fill + // its {payload, has_value} layout and corrupt the value / fail LLVM + // verification. Wrapping the raw struct_init SSA aggregate ALSO mislays + // a multi-field payload, so round-trip through memory first — the wrap + // then sees a loaded value, the same shape the working `T -> ?T` value + // coercion wraps. Returning a fully-built `?T` makes EVERY caller + // context correct, including array/struct-literal element slots that + // don't re-coerce (issue 0160). + const saved_tt = self.target_type; + self.target_type = child; + const inner = self.lowerStructLiteral(sl, span); + self.target_type = saved_tt; + const slot = self.builder.alloca(child); + self.builder.store(slot, inner); + const reloaded = self.builder.load(slot, child); + return self.coerceToType(reloaded, child, ty); + } + // Get struct field types for coercion and ordering const struct_fields = self.getStructFields(ty); @@ -802,8 +830,37 @@ pub fn lowerOptionalChain(self: *Lowering, obj: Ref, fa: *const ast.FieldAccess, break :blk if (info == .optional) info.optional.child else obj_ty; } else obj_ty; - // Get the field type on the inner type - const field_ty = self.resolveFieldType(inner_ty, fa.field); + // `#get` accessor through `?.`: if the unwrapped (and pointer-deref'd) + // receiver has a getter for this field, the some-branch dispatches the + // getter instead of a struct-field read. A synthetic receiver local (typed + // `inner_ty`) lets the existing getter intercept in `lowerFieldAccess` do + // the deref / address-of; we bind it type-only here for the return-type + // query, then fill its ref in the some-branch (issue 0160). + var deref_inner = inner_ty; + if (!deref_inner.isBuiltin() and self.module.types.get(deref_inner) == .pointer) + deref_inner = self.module.types.get(deref_inner).pointer.pointee; + const getter_recv: ?[]const u8 = if (self.scope != null and self.getAccessorFor(deref_inner, fa.field) != null) blk: { + var buf: [40]u8 = undefined; + const nm = std.fmt.bufPrint(&buf, "$oc_recv_{d}", .{self.block_counter}) catch "$oc_recv"; + self.block_counter += 1; + const owned = self.alloc.dupe(u8, nm) catch break :blk null; + // Type-only binding for the return-type query: type it as a POINTER to + // the struct so inference routes through the (working) pointer-deref + // getter path for both `?T` and `?*T`. The some-branch re-binds it to + // the actual unwrapped receiver before lowering. + self.scope.?.put(owned, .{ .ref = Ref.none, .ty = self.module.types.ptrTo(deref_inner), .is_alloca = false }); + break :blk owned; + } else null; + const read_node: ?*Node = if (getter_recv) |nm| blk: { + const id = self.alloc.create(Node) catch break :blk null; + id.* = .{ .span = span, .data = .{ .identifier = .{ .name = nm } } }; + const rn = self.alloc.create(Node) catch break :blk null; + rn.* = .{ .span = span, .data = .{ .field_access = .{ .object = id, .field = fa.field } } }; + break :blk rn; + } else null; + + // Get the field type on the inner type (the getter's return type, if any). + const field_ty = if (read_node) |rn| self.inferExprType(rn) else self.resolveFieldType(inner_ty, fa.field); // If field is already optional, flatten (don't double-wrap) const field_already_optional = if (!field_ty.isBuiltin()) self.module.types.get(field_ty) == .optional else false; const result_ty = if (field_already_optional) field_ty else self.module.types.optionalOf(field_ty); @@ -821,7 +878,24 @@ pub fn lowerOptionalChain(self: *Lowering, obj: Ref, fa: *const ast.FieldAccess, // Some: unwrap, access field (already ?FieldType if flattened, else wrap) self.builder.switchToBlock(some_bb); const unwrapped = self.builder.emit(.{ .optional_unwrap = .{ .operand = obj } }, inner_ty); - const field_val = self.lowerFieldAccessOnType(unwrapped, inner_ty, fa.field, span); + const field_val = if (read_node) |rn| blk: { + // Re-bind the synthetic receiver to the unwrapped value, then dispatch + // the getter through the normal field-access intercept. A `?*T` unwraps + // to the pointer receiver directly; a `?T` unwraps to a value that the + // getter (`self: *T`) needs to address, so materialize it into an alloca + // and bind it like an ordinary `T` local (is_alloca). + if (!inner_ty.isBuiltin() and self.module.types.get(inner_ty) == .pointer) { + self.scope.?.put(getter_recv.?, .{ .ref = unwrapped, .ty = inner_ty, .is_alloca = false }); + } else { + // Materialize the value and bind the alloca POINTER as a `*T` value + // (not an is_alloca `T`), so the receiver path is identical to the + // `?*T` case — a plain pointer receiver the getter intercept derefs. + const slot = self.builder.alloca(inner_ty); + self.builder.store(slot, unwrapped); + self.scope.?.put(getter_recv.?, .{ .ref = slot, .ty = self.module.types.ptrTo(inner_ty), .is_alloca = false }); + } + break :blk self.lowerExpr(rn); + } else self.lowerFieldAccessOnType(unwrapped, inner_ty, fa.field, span); const some_result = if (field_already_optional) field_val else self.builder.emit(.{ .optional_wrap = .{ .operand = field_val } }, result_ty); self.builder.br(merge_bb, &.{some_result}); @@ -886,6 +960,29 @@ pub fn getAccessorFor(self: *Lowering, ty: TypeId, field: []const u8) ?*const as /// effective `field$set` name (so a same-name `#get` keeps the plain `field`), /// and a REAL field of the same name wins over it (parallels the `#get` rule). /// `ty` must be the dereferenced (non-pointer) receiver type. +/// The return type of a `#get` accessor named `field` on `deref_ty` (a +/// dereferenced struct type), or null when there is no such getter (or no scope +/// to resolve through). Resolves the type the SAME way a real read does — via a +/// synthetic `*deref_ty` receiver local routed through inference — so a generic +/// instance getter (`List(T).len`) binds its type parameter exactly as the call +/// path would. Shared by `lowerOptionalChain` and the optional-chain inference +/// in `expr_typer`, so `obj?.getter` types identically to how it lowers. +pub fn getterReturnTypeOnDeref(self: *Lowering, deref_ty: TypeId, field: []const u8) ?TypeId { + if (deref_ty.isBuiltin()) return null; + if (self.getAccessorFor(deref_ty, field) == null) return null; + const s = self.scope orelse return null; + var buf: [40]u8 = undefined; + const nm = std.fmt.bufPrint(&buf, "$oc_ty_{d}", .{self.block_counter}) catch "$oc_ty"; + self.block_counter += 1; + const owned = self.alloc.dupe(u8, nm) catch return null; + s.put(owned, .{ .ref = Ref.none, .ty = self.module.types.ptrTo(deref_ty), .is_alloca = false }); + const id = self.alloc.create(Node) catch return null; + id.* = .{ .span = .{ .start = 0, .end = 0 }, .data = .{ .identifier = .{ .name = owned } } }; + const rn = self.alloc.create(Node) catch return null; + rn.* = .{ .span = .{ .start = 0, .end = 0 }, .data = .{ .field_access = .{ .object = id, .field = field } } }; + return self.inferExprType(rn); +} + pub fn getSetterFor(self: *Lowering, ty: TypeId, field: []const u8) ?*const ast.FnDecl { if (ty.isBuiltin()) return null; // A REAL field of this name wins over a same-name `#set` (a setter must not diff --git a/src/ir/lower/stmt.zig b/src/ir/lower/stmt.zig index 4478a69e..a0e0bfed 100644 --- a/src/ir/lower/stmt.zig +++ b/src/ir/lower/stmt.zig @@ -308,9 +308,13 @@ pub fn lowerVarDecl(self: *Lowering, vd: *const ast.VarDecl) void { self.target_type = saved_target; self.force_block_value = saved_fbv; // If target is optional and value isn't null, wrap with optional_wrap + // — UNLESS the value is already that optional (e.g. a `?T`-returning + // call, or a struct literal that lowered straight to `?T`); wrapping + // again would build a `??`-shaped value typed `?T` and corrupt it / + // fail LLVM verification (issue 0160). if (!ty.isBuiltin()) { const ty_info = self.module.types.get(ty); - if (ty_info == .optional and val.data != .null_literal) { + if (ty_info == .optional and val.data != .null_literal and self.builder.getRefType(ref) != ty) { ref = self.builder.optionalWrap(ref, ty); } else if (ty_info == .slice) { // Array → slice promotion: if value is an array, convert to slice