fix: optional-chain getter/field correctness from 0160 adversarial review
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).
This commit is contained in:
@@ -37,5 +37,11 @@ main :: () -> i64 {
|
|||||||
// array element
|
// array element
|
||||||
arr : [2]?T = .[ .{ a = 20 }, .{ a = 21 } ];
|
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
|
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;
|
return 0;
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -1,9 +1,11 @@
|
|||||||
// A `#get` property accessor is reachable through an optional chain
|
// A `#get` property accessor is reachable through an optional chain
|
||||||
// (`obj?.getter`): the some-branch dispatches the getter and the result is
|
// (`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
|
// 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
|
// optional (`?T`), a pointer optional (`?*T`), and a generic-instance getter —
|
||||||
// (`List.len`), and types correctly without an explicit annotation. Real fields
|
// including a COLD user generic reached ONLY through the chain (its monomorph
|
||||||
// through `?.` keep working unchanged.
|
// 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).
|
// Regression (issue 0160).
|
||||||
#import "modules/std.sx";
|
#import "modules/std.sx";
|
||||||
|
|
||||||
@@ -12,6 +14,15 @@ Temp :: struct {
|
|||||||
doubled :: (self: *Temp) -> i64 #get => self.raw * 2;
|
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 {
|
main :: () -> i64 {
|
||||||
t : Temp = .{ raw = 4 };
|
t : Temp = .{ raw = 4 };
|
||||||
|
|
||||||
@@ -35,5 +46,18 @@ main :: () -> i64 {
|
|||||||
xs.append(10); xs.append(20); xs.append(30);
|
xs.append(10); xs.append(20); xs.append(30);
|
||||||
pxs : ?*List(i64) = @xs;
|
pxs : ?*List(i64) = @xs;
|
||||||
print("?*List len: {}\n", pxs?.len ?? -1); // 3
|
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;
|
return 0;
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -4,3 +4,4 @@ ret: 5 6
|
|||||||
nested: 1 2 8
|
nested: 1 2 8
|
||||||
reassign: 11 12
|
reassign: 11 12
|
||||||
arr: 20 21
|
arr: 20 21
|
||||||
|
optslice: 3 1 3
|
||||||
|
|||||||
@@ -3,3 +3,8 @@
|
|||||||
null getter: -1
|
null getter: -1
|
||||||
?T field: 4
|
?T field: 4
|
||||||
?*List len: 3
|
?*List len: 3
|
||||||
|
cold ?*Cell count: 5
|
||||||
|
cold ?*Cell head: 99
|
||||||
|
cold ?Cell count: 5
|
||||||
|
?*Cell field n: 5
|
||||||
|
?*Cell field slot: 99
|
||||||
|
|||||||
@@ -17,6 +17,19 @@
|
|||||||
> unsupported for all fields) and was never part of this bug. Regression tests:
|
> 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/0906-optionals-struct-literal-into-optional.sx` and
|
||||||
> `examples/optionals/0907-optionals-accessor-through-chain.sx`.
|
> `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
|
## Symptom
|
||||||
|
|
||||||
|
|||||||
43
issues/0161-struct-literal-against-non-aggregate.md
Normal file
43
issues/0161-struct-literal-against-non-aggregate.md
Normal file
@@ -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.
|
||||||
40
issues/0162-comptime-run-returning-optional-aggregate.md
Normal file
40
issues/0162-comptime-run-returning-optional-aggregate.md
Normal file
@@ -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.
|
||||||
43
issues/0163-untagged-union-payload-binding-match-panics.md
Normal file
43
issues/0163-untagged-union-payload-binding-match-panics.md
Normal file
@@ -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.
|
||||||
@@ -859,6 +859,16 @@ pub fn lowerOptionalChain(self: *Lowering, obj: Ref, fa: *const ast.FieldAccess,
|
|||||||
break :blk rn;
|
break :blk rn;
|
||||||
} else null;
|
} 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).
|
// 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);
|
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)
|
// 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 });
|
self.scope.?.put(getter_recv.?, .{ .ref = slot, .ty = self.module.types.ptrTo(inner_ty), .is_alloca = false });
|
||||||
}
|
}
|
||||||
break :blk self.lowerExpr(rn);
|
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);
|
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});
|
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 (deref_ty.isBuiltin()) return null;
|
||||||
if (self.getAccessorFor(deref_ty, field) == null) return null;
|
if (self.getAccessorFor(deref_ty, field) == null) return null;
|
||||||
const s = self.scope orelse 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;
|
var buf: [40]u8 = undefined;
|
||||||
const nm = std.fmt.bufPrint(&buf, "$oc_ty_{d}", .{self.block_counter}) catch "$oc_ty";
|
const nm = std.fmt.bufPrint(&buf, "$oc_ty_{d}", .{self.block_counter}) catch "$oc_ty";
|
||||||
self.block_counter += 1;
|
self.block_counter += 1;
|
||||||
|
|||||||
@@ -315,6 +315,13 @@ pub fn lowerVarDecl(self: *Lowering, vd: *const ast.VarDecl) void {
|
|||||||
if (!ty.isBuiltin()) {
|
if (!ty.isBuiltin()) {
|
||||||
const ty_info = self.module.types.get(ty);
|
const ty_info = self.module.types.get(ty);
|
||||||
if (ty_info == .optional and val.data != .null_literal and self.builder.getRefType(ref) != 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);
|
ref = self.builder.optionalWrap(ref, ty);
|
||||||
} else if (ty_info == .slice) {
|
} else if (ty_info == .slice) {
|
||||||
// Array → slice promotion: if value is an array, convert to slice
|
// Array → slice promotion: if value is an array, convert to slice
|
||||||
|
|||||||
Reference in New Issue
Block a user