fix: bindingless if/while/and/or over optional reads has_value (issue 0164)
lowerIfExpr emitted optional_has_value only for the binding form; a bare
'if opt' passed the raw {T,i1} aggregate to condBr, where emitCondBr's
catch-all struct arm silently folded it to 'i1 true' (structs always
truthy) — a silent miscompile that took the present-branch for null
optionals. while / and / or shared the same defect.
Reduce bindingless optional conditions to optional_has_value in
lowerIfExpr/lowerWhile and via a new lowerBoolCondition helper for and/or
operands. Replace the silent-true emitCondBr arm with a lowering-time
diagnostic (checkConditionType/isValidConditionType) rejecting conditions
whose type isn't bool/integer/pointer/optional; the backend @panic is now
an unreachable tripwire.
Regressions: examples/optionals/0908..0910 + diagnostics/1194 (negative).
Verified by 3+3 adversarial reviews.
Filed adjacent bugs found during review: 0168 (array-of-optionals element
load), 0169 (optional->bool coercion), 0170 (closure-optional layout).
This commit is contained in:
@@ -2008,6 +2008,8 @@ pub const Lowering = struct {
|
||||
pub const asmResultType = lower_expr.asmResultType;
|
||||
pub const refCapturePointee = lower_expr.refCapturePointee;
|
||||
pub const lowerBinaryOp = lower_expr.lowerBinaryOp;
|
||||
pub const lowerBoolCondition = lower_expr.lowerBoolCondition;
|
||||
pub const checkConditionType = lower_expr.checkConditionType;
|
||||
pub const lowerTupleOp = lower_expr.lowerTupleOp;
|
||||
pub const lowerTupleLexCompare = lower_expr.lowerTupleLexCompare;
|
||||
pub const lowerTupleMembership = lower_expr.lowerTupleMembership;
|
||||
|
||||
@@ -66,10 +66,30 @@ pub fn lowerIfExpr(self: *Lowering, ie: *const ast.IfExpr) Ref {
|
||||
self.target_type = null;
|
||||
const opt_val = self.lowerExpr(ie.condition);
|
||||
self.target_type = saved_cond_target;
|
||||
const cond = if (ie.binding_name != null) blk: {
|
||||
// The condition is an optional — emit has_value check
|
||||
break :blk self.builder.emit(.{ .optional_has_value = .{ .operand = opt_val } }, .bool);
|
||||
} else opt_val;
|
||||
// Whenever the condition is an optional we must test its has_value flag,
|
||||
// not the optional aggregate itself. This holds with OR without a binding:
|
||||
// a bare `if opt { }` must read has_value too (else the `{T,i1}` struct
|
||||
// reaches condBr and gets folded truthy — issue 0164). `optional_has_value`
|
||||
// handles every optional repr (struct `{T,i1}`, `?Closure` {fn,env},
|
||||
// pointer-sentinel `?*T`/`?cstring`), so this is uniform across all of them.
|
||||
const cond_ty = self.inferExprType(ie.condition);
|
||||
const cond_is_optional = blk: {
|
||||
if (ie.binding_name != null) break :blk true;
|
||||
if (cond_ty.isBuiltin()) break :blk false;
|
||||
break :blk self.module.types.get(cond_ty) == .optional;
|
||||
};
|
||||
// A bare `if <expr> { }` (no binding) must have a condition type that can
|
||||
// be tested as an i1 (bool/integer/pointer/optional). Anything else — a
|
||||
// struct, float, etc. — used to be folded truthy then `@panic` in the
|
||||
// backend (issue 0164); reject it here with a located type error. With a
|
||||
// binding (`if v := opt`) the condition is required to be an optional, so
|
||||
// the optional reduction below applies and we skip the bare-cond check.
|
||||
const cond = if (cond_is_optional)
|
||||
self.builder.emit(.{ .optional_has_value = .{ .operand = opt_val } }, .bool)
|
||||
else if (self.checkConditionType(cond_ty, ie.condition.span))
|
||||
opt_val
|
||||
else
|
||||
self.builder.constBool(false);
|
||||
const has_else = ie.else_branch != null;
|
||||
// If-else produces a value when inline OR when in value position (force_block_value)
|
||||
var is_value = (ie.is_inline or self.force_block_value) and has_else;
|
||||
@@ -220,7 +240,23 @@ pub fn lowerWhile(self: *Lowering, we: *const ast.WhileExpr) Ref {
|
||||
|
||||
// Header: evaluate condition
|
||||
self.builder.switchToBlock(header_bb);
|
||||
const cond = self.lowerExpr(we.condition);
|
||||
const cond_val = self.lowerExpr(we.condition);
|
||||
// A bare optional loop condition (`while opt { }`) must test has_value,
|
||||
// exactly like `if opt { }` — otherwise the `{T,i1}` aggregate reaches
|
||||
// condBr and folds truthy (issue 0164). `optional_has_value` covers every
|
||||
// optional repr (struct / `?Closure` / pointer-sentinel). A non-condition
|
||||
// type (struct/float/...) is a located type error (same as `if`), not a
|
||||
// backend `@panic`.
|
||||
const cond = blk: {
|
||||
const cond_ty = self.inferExprType(we.condition);
|
||||
if (!cond_ty.isBuiltin() and self.module.types.get(cond_ty) == .optional) {
|
||||
break :blk self.builder.emit(.{ .optional_has_value = .{ .operand = cond_val } }, .bool);
|
||||
}
|
||||
if (!self.checkConditionType(cond_ty, we.condition.span)) {
|
||||
break :blk self.builder.constBool(false);
|
||||
}
|
||||
break :blk cond_val;
|
||||
};
|
||||
self.builder.condBr(cond, body_bb, &.{}, exit_bb, &.{});
|
||||
|
||||
// Body
|
||||
|
||||
@@ -2745,16 +2745,76 @@ pub fn refCapturePointee(self: *Lowering, node: *const Node) ?TypeId {
|
||||
return if (info == .pointer) info.pointer.pointee else null;
|
||||
}
|
||||
|
||||
/// Is `ty` a type that may be used directly as a runtime branch condition?
|
||||
/// A condBr (and the short-circuit `and`/`or` merges) ultimately tests an
|
||||
/// i1: lowering must therefore reduce the condition to something the backend
|
||||
/// can compare against zero/null. The acceptable categories all lower to an
|
||||
/// LLVM integer or pointer (or, for `.optional`, are reduced to their
|
||||
/// has_value i1 by the caller):
|
||||
/// • bool / integers (signed/unsigned/usize/isize)
|
||||
/// • integer-backed nominals: `enum` (incl. `enum flags`, e.g. `if p & .read`)
|
||||
/// and `error_set` (a u32 tag) — both reach condBr as a plain integer
|
||||
/// • pointers: `*T` / `[*]T` / `cstring` (compared against null)
|
||||
/// • `optional` — the caller emits `optional_has_value`
|
||||
/// Everything else (float, void, string, any, type_value, struct/union/tuple/
|
||||
/// array/slice/vector/function/closure/protocol/pack) reaches condBr as a
|
||||
/// non-comparable aggregate or a value with no truthiness, and previously got
|
||||
/// silently folded truthy then `@panic`d in the backend (issue 0164). Such a
|
||||
/// condition is a type error — see `checkConditionType`.
|
||||
fn isValidConditionType(self: *Lowering, ty: TypeId) bool {
|
||||
if (ty == .unresolved) return true; // already-diagnosed elsewhere; don't double-report
|
||||
return switch (self.module.types.get(ty)) {
|
||||
.bool, .signed, .unsigned, .usize, .isize => true,
|
||||
.pointer, .many_pointer, .cstring => true,
|
||||
.@"enum", .error_set => true,
|
||||
.optional => true,
|
||||
else => false,
|
||||
};
|
||||
}
|
||||
|
||||
/// Emit a located type error when `ty` cannot be used as a branch condition
|
||||
/// (see `isValidConditionType`). Returns `true` if the type is valid (caller
|
||||
/// proceeds normally), `false` if a diagnostic was emitted (caller should
|
||||
/// recover with a placeholder bool so lowering doesn't crash before the
|
||||
/// diagnostic surfaces). This is the lowering-time replacement for the
|
||||
/// backend `@panic` in `emitCondBr` (issue 0164): the type and span are both
|
||||
/// available here, so we report a clean compile-time error instead.
|
||||
pub fn checkConditionType(self: *Lowering, ty: TypeId, span: ast.Span) bool {
|
||||
if (isValidConditionType(self, ty)) return true;
|
||||
if (self.diagnostics) |d| d.addFmt(.err, span, "condition must be a bool, integer, pointer, or optional, but has type '{s}'", .{self.formatTypeName(ty)});
|
||||
return false;
|
||||
}
|
||||
|
||||
/// Lower `node` as a boolean condition. If its type is an optional, reduce
|
||||
/// it to its has_value flag (presence-as-truth) — same rule as `if opt`/
|
||||
/// `while opt`. Without this, a bare optional operand reaches a condBr/phi as
|
||||
/// a `{T,i1}` aggregate and folds truthy (issue 0164). Returns an i1/bool Ref.
|
||||
/// A non-condition-typed operand (struct/float/...) is rejected with a located
|
||||
/// type error via `checkConditionType`; on rejection a placeholder `false` is
|
||||
/// returned so lowering can continue to surface the diagnostic.
|
||||
pub fn lowerBoolCondition(self: *Lowering, node: *const Node) Ref {
|
||||
const ty = self.inferExprType(node);
|
||||
if (!self.checkConditionType(ty, node.span)) {
|
||||
_ = self.lowerExpr(node); // still lower for side effects / further diagnostics
|
||||
return self.builder.constBool(false);
|
||||
}
|
||||
const v = self.lowerExpr(node);
|
||||
if (!ty.isBuiltin() and self.module.types.get(ty) == .optional) {
|
||||
return self.builder.emit(.{ .optional_has_value = .{ .operand = v } }, .bool);
|
||||
}
|
||||
return v;
|
||||
}
|
||||
|
||||
pub fn lowerBinaryOp(self: *Lowering, bop: *const ast.BinaryOp) Ref {
|
||||
// Short-circuit: `a and b` → if a then b else false
|
||||
if (bop.op == .and_op) {
|
||||
const lhs = self.lowerExpr(bop.lhs);
|
||||
const lhs = self.lowerBoolCondition(bop.lhs);
|
||||
const rhs_bb = self.freshBlock("and.rhs");
|
||||
const merge_bb = self.freshBlockWithParams("and.merge", &.{.bool});
|
||||
const false_val = self.builder.constBool(false);
|
||||
self.builder.condBr(lhs, rhs_bb, &.{}, merge_bb, &.{false_val});
|
||||
self.builder.switchToBlock(rhs_bb);
|
||||
const rhs = self.lowerExpr(bop.rhs);
|
||||
const rhs = self.lowerBoolCondition(bop.rhs);
|
||||
self.builder.br(merge_bb, &.{rhs});
|
||||
self.builder.switchToBlock(merge_bb);
|
||||
return self.builder.blockParam(merge_bb, 0, .bool);
|
||||
@@ -2768,13 +2828,13 @@ pub fn lowerBinaryOp(self: *Lowering, bop: *const ast.BinaryOp) Ref {
|
||||
if (self.orIsFailableChain(bop)) {
|
||||
return self.lowerFailableOr(bop);
|
||||
}
|
||||
const lhs = self.lowerExpr(bop.lhs);
|
||||
const lhs = self.lowerBoolCondition(bop.lhs);
|
||||
const rhs_bb = self.freshBlock("or.rhs");
|
||||
const merge_bb = self.freshBlockWithParams("or.merge", &.{.bool});
|
||||
const true_val = self.builder.constBool(true);
|
||||
self.builder.condBr(lhs, merge_bb, &.{true_val}, rhs_bb, &.{});
|
||||
self.builder.switchToBlock(rhs_bb);
|
||||
const rhs = self.lowerExpr(bop.rhs);
|
||||
const rhs = self.lowerBoolCondition(bop.rhs);
|
||||
self.builder.br(merge_bb, &.{rhs});
|
||||
self.builder.switchToBlock(merge_bb);
|
||||
return self.builder.blockParam(merge_bb, 0, .bool);
|
||||
|
||||
Reference in New Issue
Block a user