From ff9e448f8cdebc8ab440487b8a5116edbf920f2d Mon Sep 17 00:00:00 2001 From: agra Date: Mon, 22 Jun 2026 18:55:41 +0300 Subject: [PATCH] fix: optional-chain getter/field correctness from 0160 adversarial review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five adversarial reviews of the issue-0160 fix surfaced three more bugs in the touched optional-chain / optional-coercion code; all fixed here: 1. A COLD generic-instance getter through `?.` (`?*Vec(i64)` `.getter`, never called directly first) panicked with "unresolved type reached LLVM emission": a cold instance method is absent from resolveFuncByName, so the getter's return type resolved to .unresolved → a ?unresolved merge type. lowerOptionalChain and getterReturnTypeOnDeref now warm the monomorph (ensureGenericInstanceMethodLowered) before querying its return type. (The 0907 test passed only by luck — List(i64) is warmed by stdlib use; 0907 now also exercises a cold user generic.) 2. A real-field read through a `?*T` chain (`op?.field`, op: ?*T) reinterpreted the pointer bits as the field (silent garbage) — the some-branch real-field path didn't load through the pointer. It now derefs `?*T` before the field access. (Pre-existing — the else-branch predates 0160 — but it's the same function and a silent miscompile, so fixed here.) 3. `?[]T = array` skipped the array→slice promotion (corrupt .len/.ptr): the lowerVarDecl optional arm wrapped the raw array. It now coerces the value to the optional's child type (array→slice) before wrapping. Regression examples 0906/0907 extended to cover all three. Distinct PRE-EXISTING bugs the reviews surfaced in untouched subsystems are filed as issues 0161 (struct-literal vs scalar), 0162 (#run returning an optional aggregate), 0163 (untagged-union payload-binding match). --- ...-optionals-struct-literal-into-optional.sx | 6 +++ .../0907-optionals-accessor-through-chain.sx | 30 +++++++++++-- ...ionals-struct-literal-into-optional.stdout | 1 + ...07-optionals-accessor-through-chain.stdout | 5 +++ ...onal-chain-value-optional-and-accessors.md | 13 ++++++ ...61-struct-literal-against-non-aggregate.md | 43 +++++++++++++++++++ ...mptime-run-returning-optional-aggregate.md | 40 +++++++++++++++++ ...gged-union-payload-binding-match-panics.md | 43 +++++++++++++++++++ src/ir/lower/expr.zig | 30 ++++++++++++- src/ir/lower/stmt.zig | 7 +++ 10 files changed, 214 insertions(+), 4 deletions(-) create mode 100644 issues/0161-struct-literal-against-non-aggregate.md create mode 100644 issues/0162-comptime-run-returning-optional-aggregate.md create mode 100644 issues/0163-untagged-union-payload-binding-match-panics.md diff --git a/examples/optionals/0906-optionals-struct-literal-into-optional.sx b/examples/optionals/0906-optionals-struct-literal-into-optional.sx index 26571a5a..60b622ad 100644 --- a/examples/optionals/0906-optionals-struct-literal-into-optional.sx +++ b/examples/optionals/0906-optionals-struct-literal-into-optional.sx @@ -37,5 +37,11 @@ main :: () -> i64 { // 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 + + // array value into an optional-of-slice `?[]T`: the array→slice promotion + // must run on the child BEFORE wrapping, or `.len`/`.ptr` are corrupted. + nums : [3]i64 = .[ 1, 2, 3 ]; + os : ?[]i64 = nums; + if os != null { s := os!; print("optslice: {} {} {}\n", s.len, s[0], s[2]); } // 3 1 3 return 0; } diff --git a/examples/optionals/0907-optionals-accessor-through-chain.sx b/examples/optionals/0907-optionals-accessor-through-chain.sx index f2f3ed0d..3810d373 100644 --- a/examples/optionals/0907-optionals-accessor-through-chain.sx +++ b/examples/optionals/0907-optionals-accessor-through-chain.sx @@ -1,9 +1,11 @@ // 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. +// optional (`?T`), a pointer optional (`?*T`), and a generic-instance getter — +// including a COLD user generic reached ONLY through the chain (its monomorph +// is warmed so its return type resolves, not `?unresolved`). Real fields read +// correctly through `?.` for both `?T` and `?*T` (the pointer is loaded, not +// reinterpreted). All type correctly without an explicit annotation. // Regression (issue 0160). #import "modules/std.sx"; @@ -12,6 +14,15 @@ Temp :: struct { doubled :: (self: *Temp) -> i64 #get => self.raw * 2; } +// A user generic with a type-parameter field, reached ONLY through `?.` (never +// a direct call first) — exercises the cold-monomorph warming. +Cell :: struct ($T: Type) { + slot: T; // type-parameter field — the cold-panic trigger + n: i64; + count :: (self: *Cell($T)) -> i64 #get => self.n; + head :: (self: *Cell($T)) -> T #get => self.slot; // returns the type param +} + main :: () -> i64 { t : Temp = .{ raw = 4 }; @@ -35,5 +46,18 @@ main :: () -> i64 { xs.append(10); xs.append(20); xs.append(30); pxs : ?*List(i64) = @xs; print("?*List len: {}\n", pxs?.len ?? -1); // 3 + + // COLD user generic getter through chain (no direct call first): concrete + // return and type-parameter return, via ?*Cell and ?Cell. + c := Cell(i64).{ slot = 99, n = 5 }; + pc : ?*Cell(i64) = @c; + print("cold ?*Cell count: {}\n", pc?.count ?? -1); // 5 + print("cold ?*Cell head: {}\n", pc?.head ?? -1); // 99 + oc : ?Cell(i64) = c; + print("cold ?Cell count: {}\n", oc?.count ?? -1); // 5 + + // real field read through ?*T (the pointer is loaded, not reinterpreted) + print("?*Cell field n: {}\n", pc?.n ?? -1); // 5 + print("?*Cell field slot: {}\n", pc?.slot ?? -1); // 99 return 0; } diff --git a/examples/optionals/expected/0906-optionals-struct-literal-into-optional.stdout b/examples/optionals/expected/0906-optionals-struct-literal-into-optional.stdout index d9b34362..69ab8687 100644 --- a/examples/optionals/expected/0906-optionals-struct-literal-into-optional.stdout +++ b/examples/optionals/expected/0906-optionals-struct-literal-into-optional.stdout @@ -4,3 +4,4 @@ ret: 5 6 nested: 1 2 8 reassign: 11 12 arr: 20 21 +optslice: 3 1 3 diff --git a/examples/optionals/expected/0907-optionals-accessor-through-chain.stdout b/examples/optionals/expected/0907-optionals-accessor-through-chain.stdout index 0297f2e4..45ed4dc3 100644 --- a/examples/optionals/expected/0907-optionals-accessor-through-chain.stdout +++ b/examples/optionals/expected/0907-optionals-accessor-through-chain.stdout @@ -3,3 +3,8 @@ null getter: -1 ?T field: 4 ?*List len: 3 +cold ?*Cell count: 5 +cold ?*Cell head: 99 +cold ?Cell count: 5 +?*Cell field n: 5 +?*Cell field slot: 99 diff --git a/issues/0160-optional-chain-value-optional-and-accessors.md b/issues/0160-optional-chain-value-optional-and-accessors.md index ed737055..1df8578c 100644 --- a/issues/0160-optional-chain-value-optional-and-accessors.md +++ b/issues/0160-optional-chain-value-optional-and-accessors.md @@ -17,6 +17,19 @@ > 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`. +> +> **Review follow-ups (5 adversarial reviews on the fix):** three further +> optional bugs in the touched code were fixed in the same pass — (1) a COLD +> generic-instance getter through `?.` panicked (`?unresolved`) because the +> monomorph wasn't lowered before its return type was queried; `lowerOptionalChain` +> + `getterReturnTypeOnDeref` now warm it. (2) a real-field read through a `?*T` +> chain reinterpreted the pointer bits as the field (silent garbage); the +> some-branch now loads through the pointer. (3) `?[]T = array` skipped +> array→slice promotion (corrupt `.len`); `lowerVarDecl`'s optional arm now +> coerces to the child before wrapping. Both regression examples were extended. +> Separately-filed PRE-EXISTING bugs the reviews surfaced (distinct subsystems, +> untouched here): [[0161]] struct-literal vs scalar, [[0162]] `#run` returning an +> optional aggregate, [[0163]] untagged-union payload-binding match. ## Symptom diff --git a/issues/0161-struct-literal-against-non-aggregate.md b/issues/0161-struct-literal-against-non-aggregate.md new file mode 100644 index 00000000..a178422e --- /dev/null +++ b/issues/0161-struct-literal-against-non-aggregate.md @@ -0,0 +1,43 @@ +# 0161 — struct literal against a non-aggregate (scalar) type crashes instead of diagnosing + +## Symptom + +A struct literal `.{ field = ... }` whose resolved target type is a scalar (or +any non-struct) reaches LLVM emission and fails verification, instead of emitting +a clean "struct literal against non-struct type" diagnostic. + +- Observed: `LLVM verification failed: Invalid InsertValueInst operands! %si = insertvalue i64 undef, i64 1, 0` (exit 1). +- Expected: a diagnostic like "cannot build a struct literal for non-struct type 'i64'". + +`.{}` (empty) against a scalar is worse — it silently produces garbage with no +diagnostic. + +This surfaced while reviewing issue 0160: `?i64 = .{...}` routes through the +struct-literal→optional path (which recurses with the child type `i64` as +target) into this same crash. But it is NOT optional-specific — a plain +`i64 = .{...}` crashes identically, so the root cause is the general +struct-literal path, not the 0160 optional handling. + +## Reproduction + +```sx +#import "modules/std.sx"; +main :: () { + x : i64 = .{ a = 1 }; // struct literal targeting a scalar + print("{}\n", x); // actual: LLVM verification failure +} +``` +Also: `y : i64 = .{}` → silent garbage; `o : ?i64 = .{ a = 1 }` → same crash. + +## Investigation prompt + +`src/ir/lower/expr.zig` `lowerStructLiteral`: after the resolved literal type +`ty` is computed (and the optional/union special-cases), the named/positional +field path calls `getStructFields(ty)` and emits `structInit`/`insertvalue` +without first checking that `ty` is actually a struct. Add an early guard: if +`ty.isBuiltin()` or `module.types.get(ty)` is not `.@"struct"` (after the +existing tagged-union / union / optional intercepts), emit a diagnostic via +`self.diagnostics.addFmt(.err, span, "...", .{...})` and return a placeholder, +rather than building `insertvalue` against a scalar LLVM type. Verify with the +repro (expect a clean error, exit 1, no LLVM panic). Add +`examples/diagnostics/11xx-...` for the negative case. diff --git a/issues/0162-comptime-run-returning-optional-aggregate.md b/issues/0162-comptime-run-returning-optional-aggregate.md new file mode 100644 index 00000000..983536b7 --- /dev/null +++ b/issues/0162-comptime-run-returning-optional-aggregate.md @@ -0,0 +1,40 @@ +# 0162 — `#run` returning an optional aggregate fails the comptime VM reg→value bridge + +## Symptom + +A `#run` (or comptime const init) whose function returns an OPTIONAL value +(`?T`, `?i64`, any optional) fails comptime evaluation with: + +`error: comptime init of 'X' failed: reg→value: aggregate shape not bridged yet` + +A non-optional return of the same type works. This is a pre-existing limitation +in the comptime VM's register→value bridge for optional-typed results; it is +orthogonal to issue 0160 (it reproduces for a value-init optional with no struct +literal anywhere, and for a scalar optional `?i64`). + +## Reproduction + +```sx +#import "modules/std.sx"; +T :: struct { a: i64 = 0; } +mk :: () -> ?T { t : T = .{ a = 7 }; return t; } +mk2 :: () -> ?i64 { return 5; } + +X :: #run mk(); // error: reg→value: aggregate shape not bridged yet +Y :: #run mk2(); // same class of failure +main :: () { print("ok\n"); } +``` +Baseline that WORKS: `Z :: #run (() -> T { return .{ a = 7 }; })();` (non-optional). + +## Investigation prompt + +`src/ir/comptime_vm.zig` — the reg→value bridge (search "aggregate shape not +bridged" / `regToValue`) handles scalars/structs/slices but bails on an +OPTIONAL-typed result. An optional is `{payload, has_value}` (or a pointer for +`?*T` / a sentinel for `?Closure`); the bridge needs to read the has_value flag +and, when set, bridge the payload as its child type (recursively), producing a +`Value` optional — and a null optional when clear. Add the `.optional` arm to +the reg→value bridge (mirror the value→reg direction, which already builds +optionals — see `makeStringList`/`writeField` optional handling). Verify with +the repro (expect `X`/`Y` to evaluate, `main` prints ok). Add a +`examples/comptime/06xx-...` regression. diff --git a/issues/0163-untagged-union-payload-binding-match-panics.md b/issues/0163-untagged-union-payload-binding-match-panics.md new file mode 100644 index 00000000..fa1b0ca7 --- /dev/null +++ b/issues/0163-untagged-union-payload-binding-match-panics.md @@ -0,0 +1,43 @@ +# 0163 — payload-binding match on a plain untagged `union` panics instead of diagnosing + +## Symptom + +A `match`-style `if x == { case .variant: (v) { ... } }` with a PAYLOAD BINDING +`(v)` on a value of a plain UNTAGGED `union` type panics in the LLVM backend +instead of producing a diagnostic. An untagged union has no discriminant, so a +case-payload binding is not a valid construct and should be rejected at +typecheck. + +- Observed: `thread panic: unresolved type reached LLVM emission` at + `src/backend/llvm/types.zig:196`, reached via `emit_llvm.zig:1289` + `declareFunction` → `toLLVMType(param.ty)` (exit 134). +- Expected: a clean diagnostic (e.g. "cannot bind a payload from an untagged + union — use a tagged enum/union with a discriminant"). + +Surfaced during the issue-0160 review (blast-radius probing). NOT caused by 0160 +— the panic path is union-match lowering → `declareFunction`, none of which the +0160 fix touches. Removing the `(v)` binding, or using a tagged `enum` instead, +both work. + +## Reproduction + +```sx +#import "modules/std.sx"; +Shape :: union { circle: i64; rect: i64; } // plain untagged union (no discriminant) +main :: () { + s : Shape = .{ circle = 5 }; + r := if s == { case .circle: (v) { v } case .rect: (v) { v * 2 } }; // panic + print("{}\n", r); +} +``` + +## Investigation prompt + +The match/case lowering binds a case payload `(v)` whose type it resolves +against the union variant — but a plain `.@"union"` (untagged) has no per-variant +discriminant, so the binding's type leaks out as `.unresolved` and reaches +`declareFunction`. In the match-arm lowering (grep the case/`match_arm` path in +`src/ir/lower/`), reject a payload-binding case when the scrutinee type is an +untagged `.@"union"` (only `.tagged_union` / `.@"enum"` payloads are bindable): +emit a diagnostic and bail, before any `.unresolved` type is produced. Verify +with the repro (expect a clean error, not a panic). Add a diagnostics example. diff --git a/src/ir/lower/expr.zig b/src/ir/lower/expr.zig index dea8101f..af292d57 100644 --- a/src/ir/lower/expr.zig +++ b/src/ir/lower/expr.zig @@ -859,6 +859,16 @@ pub fn lowerOptionalChain(self: *Lowering, obj: Ref, fa: *const ast.FieldAccess, break :blk rn; } else null; + // Warm a generic-instance getter's monomorph BEFORE typing the chain: a + // cold instance method is absent from `resolveFuncByName`, so `resultType` + // would resolve the getter to `.unresolved` → a `?unresolved` merge type + // that panics at LLVM emission. Lowering it now binds its type parameter so + // both the type query and the some-branch call see the concrete signature. + if (getter_recv != null) { + const tn = self.formatTypeName(deref_inner); + if (self.genericInstanceMethod(tn, fa.field)) |gm| _ = self.ensureGenericInstanceMethodLowered(gm); + } + // 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) @@ -895,7 +905,21 @@ pub fn lowerOptionalChain(self: *Lowering, obj: Ref, fa: *const ast.FieldAccess, 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); + } else blk: { + // Real-field read. For a `?*T` the unwrapped value is the POINTER, so + // load through it before the struct-field access — `lowerFieldAccessOnType` + // does not auto-deref, and a `structGet` on the raw pointer would read + // the pointer bits as the field (silent garbage). A `?T` value optional + // accesses the unwrapped value directly. + var fobj = unwrapped; + var fty = inner_ty; + if (!fty.isBuiltin() and self.module.types.get(fty) == .pointer) { + const pointee = self.module.types.get(fty).pointer.pointee; + fobj = self.builder.load(unwrapped, pointee); + fty = pointee; + } + break :blk self.lowerFieldAccessOnType(fobj, fty, 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}); @@ -971,6 +995,10 @@ pub fn getterReturnTypeOnDeref(self: *Lowering, deref_ty: TypeId, field: []const if (deref_ty.isBuiltin()) return null; if (self.getAccessorFor(deref_ty, field) == null) return null; const s = self.scope orelse return null; + // Warm a generic-instance getter's monomorph so its return type resolves + // (cold instance methods are absent from `resolveFuncByName` → `.unresolved`). + if (self.genericInstanceMethod(self.formatTypeName(deref_ty), field)) |gm| + _ = self.ensureGenericInstanceMethodLowered(gm); var buf: [40]u8 = undefined; const nm = std.fmt.bufPrint(&buf, "$oc_ty_{d}", .{self.block_counter}) catch "$oc_ty"; self.block_counter += 1; diff --git a/src/ir/lower/stmt.zig b/src/ir/lower/stmt.zig index a0e0bfed..fcd43b08 100644 --- a/src/ir/lower/stmt.zig +++ b/src/ir/lower/stmt.zig @@ -315,6 +315,13 @@ pub fn lowerVarDecl(self: *Lowering, vd: *const ast.VarDecl) void { if (!ty.isBuiltin()) { const ty_info = self.module.types.get(ty); if (ty_info == .optional and val.data != .null_literal and self.builder.getRefType(ref) != ty) { + // Coerce to the optional's CHILD first (e.g. an array value + // into a `?[]T` promotes array→slice), THEN wrap — wrapping + // the raw value would store e.g. array bits into the slice + // payload and corrupt `.len`/`.ptr`. + const child = ty_info.optional.child; + const rt = self.builder.getRefType(ref); + if (rt != child and rt != .void and child != .void) ref = self.coerceToType(ref, rt, child); ref = self.builder.optionalWrap(ref, ty); } else if (ty_info == .slice) { // Array → slice promotion: if value is an array, convert to slice