From e5b682e622b60d2de7057073108e009cc196c52c Mon Sep 17 00:00:00 2001 From: agra Date: Tue, 23 Jun 2026 02:47:51 +0300 Subject: [PATCH] fix: reject implicit ?T -> bool coercion instead of silent false (issue 0169) The Optional->Concrete unwrap classify rule treated ?i64 -> bool as unwrap+narrow (both builtin), silently yielding false for every optional (present or null). specs.md defines no implicit optional->bool conversion. Reject it: conversions.zig adds an optional_to_bool_reject plan (dst == bool, child != bool); coerce.zig emits a located diagnostic suggesting '!= null'. Covers arg/field-init/return via the shared coerceMode. The if-opt presence test (issue 0164) is a separate path, untouched. Regression: examples/diagnostics/1199-diagnostics-optional-to-bool.sx + conversions.test.zig unit test. Verified by 3 adversarial reviews, suite 789/0. Filed adjacent issue 0179 (whole implicit ?T->concrete unwrap family silently miscompiles a null optional; design-touching). --- .../1199-diagnostics-optional-to-bool.sx | 19 +++++ .../1199-diagnostics-optional-to-bool.exit | 1 + .../1199-diagnostics-optional-to-bool.stderr | 5 ++ .../1199-diagnostics-optional-to-bool.stdout | 1 + ...-optional-to-bool-coercion-silent-false.md | 17 +++++ ...ptional-unwrap-narrow-silent-miscompile.md | 71 +++++++++++++++++++ src/ir/conversions.test.zig | 7 ++ src/ir/conversions.zig | 12 ++++ src/ir/lower/coerce.zig | 10 +++ 9 files changed, 143 insertions(+) create mode 100644 examples/diagnostics/1199-diagnostics-optional-to-bool.sx create mode 100644 examples/diagnostics/expected/1199-diagnostics-optional-to-bool.exit create mode 100644 examples/diagnostics/expected/1199-diagnostics-optional-to-bool.stderr create mode 100644 examples/diagnostics/expected/1199-diagnostics-optional-to-bool.stdout create mode 100644 issues/0179-implicit-optional-unwrap-narrow-silent-miscompile.md diff --git a/examples/diagnostics/1199-diagnostics-optional-to-bool.sx b/examples/diagnostics/1199-diagnostics-optional-to-bool.sx new file mode 100644 index 00000000..f4e2cfeb --- /dev/null +++ b/examples/diagnostics/1199-diagnostics-optional-to-bool.sx @@ -0,0 +1,19 @@ +// Passing an optional (`?T`) where a `bool` is expected is a hard type error, +// not a silent miscompile. Regression (issue 0169): the coercion classifier's +// "optional → concrete (unwrap+narrow)" rule used to accept `?i64 → bool` +// (both child and dst are builtin), unwrapping the payload and narrowing it to +// `i1` — which silently produced `false` for EVERY optional, present or null +// alike. There is no implicit optional→bool coercion in the language (only +// `T → ?T` wrapping and flow-sensitive narrowing); a bool position wants an +// explicit presence test. The classifier now rejects `?T → bool` (payload not +// itself a bool) at the coercion site (exit 1) with a `!= null` fix-it. The +// `if opt {}` presence test (issue 0164) is a separate path and still works. + +#import "modules/std.sx"; + +takes_bool :: (b: bool) { if b { print("true\n"); } else { print("false\n"); } } + +main :: () { + a : ?i64 = 42; + takes_bool(a); // <- '?i64' cannot be used where 'bool' is expected +} diff --git a/examples/diagnostics/expected/1199-diagnostics-optional-to-bool.exit b/examples/diagnostics/expected/1199-diagnostics-optional-to-bool.exit new file mode 100644 index 00000000..d00491fd --- /dev/null +++ b/examples/diagnostics/expected/1199-diagnostics-optional-to-bool.exit @@ -0,0 +1 @@ +1 diff --git a/examples/diagnostics/expected/1199-diagnostics-optional-to-bool.stderr b/examples/diagnostics/expected/1199-diagnostics-optional-to-bool.stderr new file mode 100644 index 00000000..48614399 --- /dev/null +++ b/examples/diagnostics/expected/1199-diagnostics-optional-to-bool.stderr @@ -0,0 +1,5 @@ +error: cannot use a value of type '?i64' where 'bool' is expected; test presence explicitly with '!= null' + --> examples/diagnostics/1199-diagnostics-optional-to-bool.sx:18:3 + | +18 | takes_bool(a); // <- '?i64' cannot be used where 'bool' is expected + | ^^^^^^^^^^^^^ diff --git a/examples/diagnostics/expected/1199-diagnostics-optional-to-bool.stdout b/examples/diagnostics/expected/1199-diagnostics-optional-to-bool.stdout new file mode 100644 index 00000000..8b137891 --- /dev/null +++ b/examples/diagnostics/expected/1199-diagnostics-optional-to-bool.stdout @@ -0,0 +1 @@ + diff --git a/issues/0169-optional-to-bool-coercion-silent-false.md b/issues/0169-optional-to-bool-coercion-silent-false.md index 205dc60c..b72ec77a 100644 --- a/issues/0169-optional-to-bool-coercion-silent-false.md +++ b/issues/0169-optional-to-bool-coercion-silent-false.md @@ -1,5 +1,22 @@ # 0169 — optional passed where `bool` is expected silently coerces to `false` (always) +> **RESOLVED** (scoped to `?T → bool`, `T ≠ bool`). The "Optional → Concrete +> unwrap" classify rule treated `?i64 → bool` as unwrap+narrow (both i64 and bool +> are builtin), silently yielding `false`. specs.md defines no implicit +> optional→bool conversion, so the fix REJECTS it: `src/ir/conversions.zig` adds +> an `optional_to_bool_reject` plan (when `dst == bool` and `child ≠ bool`); +> `src/ir/lower/coerce.zig` emits a located diagnostic ("…use '!= null'…") +> instead of a constant false. Covers arg / field-init / return (all share +> `coerceMode`). `if opt` presence-test (issue 0164) is a separate path, +> untouched. Verified by 3 adversarial reviews; suite 789/0. Regression: +> `examples/diagnostics/1199-diagnostics-optional-to-bool.sx` + a +> `conversions.test.zig` unit test. NOTE: review surfaced a larger sibling +> surface — the whole implicit `?T → concrete` unwrap family (incl. the +> `?bool → bool` cell still allowed here) silently miscompiles a NULL optional to +> garbage — filed as **0179** (design-touching: needs the flow-narrowing +> decision). + + ## Symptom Passing an optional (`?T`) to a `bool` parameter (or any bool-typed position: diff --git a/issues/0179-implicit-optional-unwrap-narrow-silent-miscompile.md b/issues/0179-implicit-optional-unwrap-narrow-silent-miscompile.md new file mode 100644 index 00000000..64c0b390 --- /dev/null +++ b/issues/0179-implicit-optional-unwrap-narrow-silent-miscompile.md @@ -0,0 +1,71 @@ +# 0179 — implicit `?T → ` unwrap silently miscompiles a NULL optional to garbage (whole family) + +## Symptom + +Passing an optional `?T` where a concrete `T`/other builtin is expected (function +arg, field initializer, `-> T` return) — WITHOUT any explicit `!`/`??`/binding — +compiles silently and unconditionally unwraps the payload. For a PRESENT optional +it yields the value; for a NULL optional it yields the zero/garbage payload with +NO diagnostic. Silent miscompile across the whole `?T → concrete` family. (Issue +0169 fixed only the `?T → bool` cell where `child != bool`; the rest of the +family — and the `?bool → bool` cell — remain.) + +Per specs.md §Optional Types, the ONLY legal ways to extract `T` from `?T` are +`!` (force unwrap), `??` (coalesce), `if v := opt` / while-binding, pattern +match, and flow-sensitive narrowing after a `!= null` guard. There is NO implicit +unwrap-at-a-value-position. The `T → ?T` direction is the only implicit optional +conversion sanctioned. + +## Reproduction + +```sx +#import "modules/std.sx"; +takes_i32 :: (x: i32) { print("got {}\n", x); } +main :: () { + n : ?i64 = null; + takes_i32(n); // prints "got 0", exit 0 — silent miscompile (no diagnostic) +} +``` + +Confirmed silently wrong for the NULL case across: `?i64 → i32`, `?i64 → f64`, +`?f64 → i64`, `?i64 → u8`, `?i32 → i64` (widen), `?i64 → i64` (same type), and +`?bool → bool` (yields `false`). Present optionals unwrap the payload; null +optionals yield `0`/garbage. (Found during adversarial review of issue 0169.) + +## Root cause + +`src/ir/conversions.zig` (`CoercionResolver.classify`, the "Optional → Concrete +unwrap" rule, ~line 114): `if (child_ty == dst_ty or (dst_ty.isBuiltin() and +child_ty.isBuiltin()))` classifies any `?builtin → builtin` (and `?T → T`) as +`.optional_unwrap`. The emitter `emitOptionalUnwrap` +(`src/backend/llvm/ops.zig` ~2212) does an UNCONDITIONAL `ExtractValue` of the +payload field — it never reads the has_value flag — so a null optional yields its +zero payload. (The comptime VM's `optHas` path traps, so runtime and comptime +diverge.) + +## Investigation prompt — NOTE: this is design-touching, resolve the semantics first + +Per spec, implicit `?T → concrete` should be REJECTED entirely (require explicit +`!`/`??`/binding). Broadening issue 0169's `optional_to_bool_reject` to all +`?T → concrete` is the mechanical change (in `classify`), BUT it interacts with +flow-sensitive narrowing: + +1. **Flow narrowing is not actually implemented as type refinement.** The + non-binding `if x != null { use(x) }` spec example (specs.md §Flow-Sensitive + Narrowing) currently "works" only because the broken unconditional unwrap + fires everywhere, guard or not — there is no real narrowed type. The genuine + narrowing mechanism is the BINDING form `if v := opt {}` + (`src/ir/lower/control_flow.zig`), which emits an explicit `optional_unwrap` + into a fresh `inner_ty` scope binding and does NOT route through `classify`. + So a broad reject would reject the non-binding `if x != null { use(x) }` + pattern. Decide: (a) implement real flow-sensitive type refinement so `x` is + genuinely `T` inside the guarded branch (then the broad reject is safe), or + (b) require the binding form / `!` / `??` and update specs.md §Flow-Sensitive + Narrowing accordingly. +2. **Close the `?bool → bool` hole** — issue 0169's `child_ty != .bool` carve-out + still silently unwraps a null `?bool` to `false`; per spec there's no implicit + `?bool → bool` either. + +Whichever path, the NULL case must never silently yield garbage. Verify the +present cases still work via the legal explicit forms; add regressions. This +likely warrants its own focused session given the flow-narrowing decision. diff --git a/src/ir/conversions.test.zig b/src/ir/conversions.test.zig index 1e0f7bfc..1ceb2e79 100644 --- a/src/ir/conversions.test.zig +++ b/src/ir/conversions.test.zig @@ -48,6 +48,13 @@ test "conversions: classify covers the built-in coercion ladder" { try std.testing.expectEqual(Plan.optional_unwrap, cr.classify(opt_i64, .i64)); try std.testing.expectEqual(Plan.void_to_optional, cr.classify(.void, opt_i64)); + // `?T → bool` is NOT an unwrap-then-narrow presence test (issue 0169): + // it must reject, never silently produce `false`. But `?bool → bool` + // is a genuine unwrap of a bool payload. + try std.testing.expectEqual(Plan.optional_to_bool_reject, cr.classify(opt_i64, .bool)); + const opt_bool = tt.optionalOf(.bool); + try std.testing.expectEqual(Plan.optional_unwrap, cr.classify(opt_bool, .bool)); + // Tuple → tuple, same arity. const t_ss = tt.intern(.{ .tuple = .{ .fields = &[_]TypeId{ .i64, .i64 }, .names = null } }); const t_ii = tt.intern(.{ .tuple = .{ .fields = &[_]TypeId{ .i32, .i32 }, .names = null } }); diff --git a/src/ir/conversions.zig b/src/ir/conversions.zig index a8e1fa25..193c9214 100644 --- a/src/ir/conversions.zig +++ b/src/ir/conversions.zig @@ -34,6 +34,7 @@ pub const CoercionResolver = struct { closure_to_fn_reject, // closure value → bare fn-ptr (diagnostic, returns operand) tuple_elementwise, // (A,B) → (C,D), same arity optional_unwrap, // ?T → concrete (narrowing) + optional_to_bool_reject, // ?T → bool (no presence-test coercion; diagnostic) void_to_optional, // void (null literal) → ?T optional_wrap, // concrete → ?T erase_protocol, // concrete → protocol value @@ -99,6 +100,17 @@ pub const CoercionResolver = struct { const src_info = self.l.module.types.get(src_ty); if (src_info == .optional) { const child_ty = src_info.optional.child; + // `?T → bool` is NOT a presence test. The unwrap-then-narrow + // ladder below would extract the payload and narrow it to `i1`, + // which silently yields `false` for every optional (issue 0169). + // There is no implicit optional→bool coercion in the language + // (only `T → ?T` wrapping and flow-sensitive narrowing); a bool + // position wants an explicit presence test. Reject loudly unless + // the payload is itself a bool (`?bool → bool` is a genuine + // unwrap of a bool payload, handled by the same arm below). + if (dst_ty == .bool and child_ty != .bool) { + return .optional_to_bool_reject; + } if (child_ty == dst_ty or (dst_ty.isBuiltin() and child_ty.isBuiltin())) { return .optional_unwrap; } diff --git a/src/ir/lower/coerce.zig b/src/ir/lower/coerce.zig index 94832875..9654eb4c 100644 --- a/src/ir/lower/coerce.zig +++ b/src/ir/lower/coerce.zig @@ -647,6 +647,16 @@ pub fn coerceMode(self: *Lowering, val: Ref, src_ty: TypeId, dst_ty: TypeId, mod const unwrapped = self.builder.emit(.{ .optional_unwrap = .{ .operand = val } }, child_ty); return self.coerceMode(unwrapped, child_ty, dst_ty, mode); }, + // Optional → bool: there is no implicit presence-test coercion. The + // old unwrap-then-narrow ladder silently produced `false` for every + // optional (issue 0169). Reject with a fix-it pointing at `!= null`. + .optional_to_bool_reject => { + if (self.diagnostics) |d| { + const cs = self.builder.current_span; + d.addFmt(.err, ast.Span{ .start = cs.start, .end = cs.end }, "cannot use a value of type '{s}' where 'bool' is expected; test presence explicitly with '!= null'", .{self.formatTypeName(src_ty)}); + } + return val; + }, // string → cstring: ONLY a string LITERAL coerces implicitly — its // bytes are a terminated constant (Odin's literal blessing). Any // other string may be an unterminated view, so it must materialize