From bfd92552963ad5f5ee22a84563e064a0e187f409 Mon Sep 17 00:00:00 2001 From: Taylor Holliday Date: Fri, 7 Aug 2026 23:57:26 -0700 Subject: [PATCH] Unify if-else branch types when the value is used MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to #26. Typing a `var` declaration as void fixed the f32 half of issue #22, but it moved the i32 half rather than removing it: an if-else whose else branch ends in a declaration now fails the `result_ty == else_ty` test in the Cranelift and LLVM lowerings, so the whole expression degrades to the `iconst(I32, 0)` placeholder and the then-branch value is thrown away. g(c: bool) -> i32 { var y = if c { 1 } else { var z = 2 } y } g(true) returned 1 before #26 and 0 after, with no diagnostic — and the VM backend still returned 1, so the two backends disagreed. The root cause is that `Expr::If` took its then-branch type and never looked at the else branch, leaving branch disagreement for codegen to discover. Codegen's only recourse is a placeholder, i.e. a wrong answer. Unify the two branches in the checker instead, so the disagreement is a type error at the if-else. The catch is that lyte blocks evaluate to their last expression and an assignment is an expression, so an if-else in statement position routinely has branches with incidental, differing types (the stdlib's ftoa does exactly this). `mark_value_positions` records which expressions have their value used — discarded in a non-final block element and in a loop body, inherited through block tails and branches — and the branches are only unified where the value is actually used. Also in the checker: - `let` gets the same statement treatment as `var`. It still leaked the binding's type, so `f() -> f32 { let u = 1.0 }` compiled while the byte-identical `var` form was rejected. - Post-check passes gate on errors from the function being checked rather than on `self.errors`, which accumulates across decls — one bad function disabled constraint solving and every post-check pass for all later ones. - `check_void_declarations` skips expressions `check_expr` never visited instead of bailing out when any error exists. Unvisited expressions keeping the Void fill value was the actual reason for the gate. - Both post-solve passes share one `solved_types()` call instead of each running the substitution over every expression in the function. - `check()` calls `check_decl` rather than repeating its body, so a new pass only has to be added in one place. In the JIT, `merge_ty` is Some exactly when the if-else produces a value, so use it directly and drop the parallel `is_value` boolean. `check_merge_arg`'s doc claimed it caught the silently-wrong-answer case; it can't, since that case has matching Cranelift types. Branch unification in the checker is what rules that class out. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01BX9w4k8qecsH9LMgBP8hXc --- src/checker.rs | 159 ++++++++++++++---- src/expr.rs | 52 ++++++ src/jit.rs | 37 ++-- src/safety_checker.rs | 53 +----- .../checker/if_branch_value_mismatch.lyte | 18 ++ tests/cases/checker/if_branch_void_else.lyte | 23 +++ tests/cases/checker/let_decl_not_a_value.lyte | 15 ++ tests/cases/if_stmt_branch_types.lyte | 46 +++++ 8 files changed, 308 insertions(+), 95 deletions(-) create mode 100644 tests/cases/checker/if_branch_value_mismatch.lyte create mode 100644 tests/cases/checker/if_branch_void_else.lyte create mode 100644 tests/cases/checker/let_decl_not_a_value.lyte create mode 100644 tests/cases/if_stmt_branch_types.lyte diff --git a/src/checker.rs b/src/checker.rs index 63cfd104..2ac4eb2c 100644 --- a/src/checker.rs +++ b/src/checker.rs @@ -47,6 +47,15 @@ pub struct Checker { /// Expression types. pub types: Vec, + /// Which expressions `check_expr` actually visited in the current function. + /// Unvisited entries in `types` keep the fill value from `check_fn_decl`, + /// so post-check passes must skip them rather than trust their type. + visited: Vec, + + /// Is an expression's value used, rather than discarded? See + /// `mark_value_positions`. + value_pos: Vec, + /// Currently declared vars, as we're checking. vars: Vec, @@ -176,6 +185,8 @@ impl Checker { Self { types: vec![], + visited: vec![], + value_pos: vec![], lvalue: vec![], inst: Instance::new(), next_anon: 0, @@ -420,6 +431,7 @@ impl Checker { } fn check_expr(&mut self, id: ExprID, arena: &ExprArena, decls: &DeclTable) -> TypeID { + self.visited[id] = true; let ty = match &arena[id] { Expr::True | Expr::False => mk_type(Type::Bool), Expr::Int(_, None) => { @@ -713,7 +725,10 @@ impl Checker { mutable: false, }); - ty + // A let declaration is a statement too — see the comment on + // Expr::Var above. + self.types[id] = ty; + return mk_type(Type::Void); } Expr::Arena(block) => self.check_expr(*block, arena, decls), Expr::Return(expr) => self.check_expr(*expr, arena, decls), @@ -892,8 +907,25 @@ impl Checker { ); let then_t = self.check_expr(*then_expr, arena, decls); if let Some(else_expr) = else_expr { - self.check_expr(*else_expr, arena, decls); - // The if-else expression has the type of its then-branch. + let else_t = self.check_expr(*else_expr, arena, decls); + // When the if-else is used as a value, both branches must + // agree — it has a single type, and taking the then-branch's + // type without checking the else branch left the + // disagreement for codegen, which quietly substitutes a + // placeholder 0 (issue #22). + // + // In statement position the value is discarded, so branches + // are free to end in whatever their last statement happens + // to evaluate to (an assignment is an expression in lyte, + // so `if c { x = 1 } else { y = 2.0 }` is perfectly fine). + if self.value_pos[id] { + self.eq( + then_t, + else_t, + arena.locs[*else_expr], + "if-else branches must have the same type", + ); + } then_t } else { mk_type(Type::Void) @@ -979,6 +1011,12 @@ impl Checker { } fn check_fn_decl(&mut self, func_decl: &FuncDecl, decls: &DeclTable) { + // `self.errors` accumulates across every decl in the table, so the + // post-check passes below gate on errors from *this* function rather + // than on the accumulator — otherwise one bad function silently + // disables checking for every function after it. + let errors_before = self.errors.len(); + // Disallow borrowed types in return position. if type_contains_bad_borrow(func_decl.ret) { self.errors.push(TypeError { @@ -993,11 +1031,23 @@ impl Checker { let n = func_decl.arena.exprs.len(); self.types.resize(n, mk_type(Type::Void)); + self.visited.clear(); + self.visited.resize(n, false); + self.value_pos.clear(); + self.value_pos.resize(n, false); self.lvalue.resize(n, false); if let Some(body) = func_decl.body { // println!("🟧 checking function {:?} 🟧", *func_decl.name); + // The body's value is the return value, unless the function + // returns void — see the `self.eq(ty, func_decl.ret, ..)` below. + let body_used = func_decl.ret != mk_type(Type::Void); + mark_value_positions(body, &func_decl.arena, body_used, &mut self.value_pos); + for &req in &func_decl.requires { + mark_value_positions(req, &func_decl.arena, true, &mut self.value_pos); + } + self.inst.clear(); self.constraints.clear(); @@ -1094,7 +1144,7 @@ impl Checker { self.vars.clear(); - if self.errors.is_empty() { + if self.errors.len() == errors_before { solve_constraints( &mut self.constraints, &mut self.inst, @@ -1104,12 +1154,12 @@ impl Checker { } // Check lvalue validity now that types are solved. - if self.errors.is_empty() { + if self.errors.len() == errors_before { self.check_lvalues(func_decl, decls); } // Check that no two borrowed parameters alias (Fortran-style no-alias rule). - if self.errors.is_empty() { + if self.errors.len() == errors_before { self.check_slice_aliasing(func_decl, decls); } } @@ -1342,21 +1392,22 @@ impl Checker { pub fn check(&mut self, decls: &DeclTable) { for decl in &decls.decls { - self._check_decl(decl, decls); - if let Decl::Func(fd) | Decl::Macro(fd) = decl { - check_escape_in_func(fd, &mut self.errors); - self.check_void_declarations(fd); - self.check_unsolved_types(fd); - } + self.check_decl(decl, decls); } } pub fn check_decl(&mut self, decl: &Decl, decls: &DeclTable) { + let errors_before = self.errors.len(); self._check_decl(decl, decls); if let Decl::Func(fd) | Decl::Macro(fd) = decl { check_escape_in_func(fd, &mut self.errors); - self.check_void_declarations(fd); - self.check_unsolved_types(fd); + // Both post-solve passes read the same substituted types, so + // compute them once. + let solved_types = self.solved_types(); + self.check_void_declarations(fd, &solved_types); + if self.errors.len() == errors_before { + self.check_unsolved_types(fd, &solved_types); + } } } @@ -1367,19 +1418,24 @@ impl Checker { /// always a mistake. Reporting it here points at the declaration rather /// than at whatever first tried to use the variable. /// - /// Only runs if no other errors have been reported: unvisited expressions - /// keep the Void fill value from `check_fn_decl`, so a function that bailed - /// out early would otherwise produce false positives. - fn check_void_declarations(&mut self, func_decl: &FuncDecl) { - if !self.errors.is_empty() { - return; - } - let solved_types = self.solved_types(); + /// Skips expressions `check_expr` never visited: those keep the Void fill + /// value from `check_fn_decl` and would otherwise look like void + /// declarations. That guard is what lets this run regardless of earlier + /// errors — gating on `self.errors`, which accumulates across decls, meant + /// one error in the first function disabled the check for every later one. + /// + /// A function that reported its own error still won't produce this + /// diagnostic, since `check_fn_decl` skips constraint solving there and + /// the declaration's type is left an unsolved type variable. + fn check_void_declarations(&mut self, func_decl: &FuncDecl, solved_types: &[TypeID]) { for (i, expr) in func_decl.arena.exprs.iter().enumerate() { let name = match expr { Expr::Let(name, _, _) | Expr::Var(name, _, _) => name, _ => continue, }; + if !self.visited.get(i).copied().unwrap_or(false) { + continue; + } if i < solved_types.len() && matches!(&*solved_types[i], Type::Void) { self.errors.push(TypeError { location: func_decl.arena.locs[i], @@ -1390,13 +1446,11 @@ impl Checker { } /// Detect unsolved type variables in non-generic functions. - /// Only runs if no other errors have been reported (unsolved vars - /// are usually a symptom of an earlier type error). - fn check_unsolved_types(&mut self, func_decl: &FuncDecl) { - if !self.errors.is_empty() { - return; - } - let solved_types = self.solved_types(); + /// The caller skips this when the function itself reported an error: + /// unsolved vars are usually a symptom of an earlier type error, and a + /// function whose constraints never got solved has nothing but unsolved + /// vars. + fn check_unsolved_types(&mut self, func_decl: &FuncDecl, solved_types: &[TypeID]) { let is_generic = !func_decl.typevars.is_empty(); for (i, solved) in solved_types.iter().enumerate() { // For generic functions, Type::Var is expected (they have named @@ -1434,6 +1488,53 @@ impl Checker { } } +/// Mark which expressions have their value used, as opposed to discarded. +/// +/// Lyte blocks evaluate to their last expression, and an assignment is an +/// expression, so plenty of statements produce values nobody looks at. The +/// distinction matters for if-else: when the value is used both branches have +/// to produce the same type, and when it's discarded they don't have to +/// produce anything at all. +/// +/// A value is discarded in a non-final element of a block and in a loop body. +/// Everywhere else the parent uses it, and a block's or branch's last +/// expression inherits the position of the construct it belongs to. +fn mark_value_positions(id: ExprID, arena: &ExprArena, used: bool, value_pos: &mut [bool]) { + value_pos[id] = used; + match &arena[id] { + Expr::Block(exprs) => { + for (i, e) in exprs.iter().enumerate() { + let is_last = i + 1 == exprs.len(); + mark_value_positions(*e, arena, used && is_last, value_pos); + } + } + Expr::If(cond, then_expr, else_expr) => { + mark_value_positions(*cond, arena, true, value_pos); + mark_value_positions(*then_expr, arena, used, value_pos); + if let Some(else_expr) = else_expr { + mark_value_positions(*else_expr, arena, used, value_pos); + } + } + Expr::While(cond, body) => { + mark_value_positions(*cond, arena, true, value_pos); + mark_value_positions(*body, arena, false, value_pos); + } + Expr::For { + start, end, body, .. + } => { + mark_value_positions(*start, arena, true, value_pos); + mark_value_positions(*end, arena, true, value_pos); + mark_value_positions(*body, arena, false, value_pos); + } + Expr::Arena(inner) => mark_value_positions(*inner, arena, used, value_pos), + other => { + for child in other.subexprs() { + mark_value_positions(child, arena, true, value_pos); + } + } + } +} + /// Extract the "base path" of an expression — the chain of identifiers and field /// names that uniquely identifies the storage location. Returns None for complex /// expressions (calls, indexing, literals) where aliasing can't be determined diff --git a/src/expr.rs b/src/expr.rs index 40cab28b..306bb20e 100644 --- a/src/expr.rs +++ b/src/expr.rs @@ -116,6 +116,58 @@ pub enum Expr { } impl Expr { + /// The immediate subexpression IDs of this expression. + /// + /// Lambda yields its body: walks that treat a lambda specially still need + /// to reach the body, and they match on `Expr::Lambda` before falling back + /// to this. + pub fn subexprs(&self) -> Vec { + match self { + Expr::Id(_) + | Expr::Int(_, _) + | Expr::Real(_, _) + | Expr::String(_) + | Expr::Char(_) + | Expr::True + | Expr::False + | Expr::Break + | Expr::Continue + | Expr::Enum(_) + | Expr::TypeApp(_, _) + | Expr::Error => vec![], + Expr::Call(f, args) => { + let mut v = vec![*f]; + v.extend(args.iter().copied()); + v + } + Expr::Macro(_, args) => args.clone(), + Expr::Binop(_, l, r) => vec![*l, *r], + Expr::Unop(_, e) => vec![*e], + Expr::Lambda { body, .. } => vec![*body], + Expr::Field(base, _) => vec![*base], + Expr::Array(t, s) => vec![*t, *s], + Expr::ArrayLiteral(es) => es.clone(), + Expr::ArrayIndex(a, i) => vec![*a, *i], + Expr::AsTy(e, _) => vec![*e], + Expr::Let(_, init, _) => vec![*init], + Expr::Var(_, init, _) => init.iter().copied().collect(), + Expr::If(c, t, el) => { + let mut v = vec![*c, *t]; + if let Some(e) = el { + v.push(*e); + } + v + } + Expr::While(c, b) => vec![*c, *b], + Expr::For { + start, end, body, .. + } => vec![*start, *end, *body], + Expr::Block(es) | Expr::Tuple(es) => es.clone(), + Expr::Return(e) | Expr::Arena(e) | Expr::Assume(e) => vec![*e], + Expr::StructLit(_, fields) => fields.iter().map(|(_, e)| *e).collect(), + } + } + /// Pretty-print an expression in lyte syntax. /// /// This method formats an expression as it would appear in lyte source code. diff --git a/src/jit.rs b/src/jit.rs index bb43dd75..a90057cf 100644 --- a/src/jit.rs +++ b/src/jit.rs @@ -727,10 +727,16 @@ impl<'a> FunctionTranslator<'a> { /// value from codegen. They disagree when an expression kind is typed /// non-void by the checker but returns a placeholder here (see issue #22, /// where a `var` declaration was typed f32 but yielded `iconst(I32, 0)`). - /// Failing at the mismatch names the branch and both types; letting it - /// through yields an opaque Cranelift verifier failure, or — when the - /// placeholder happens to be the right Cranelift type — well-formed IR - /// that computes the wrong answer. + /// Failing at the mismatch names the branch and both types instead of + /// leaving an opaque Cranelift verifier failure. + /// + /// This only catches disagreements that differ in Cranelift type. When the + /// placeholder happens to have the right type — the i32 half of issue #22, + /// where `var count = 0` silently yielded 0 — well-formed IR computing the + /// wrong answer sails right through. What rules that class out is the + /// checker unifying the two branch types, so a branch that evaluates to + /// something other than the if-else's type is a type error and never + /// reaches codegen. fn check_merge_arg(&self, val: Value, expected: Type, branch: &str) { let actual = self.builder.func.dfg.value_type(val); assert_eq!( @@ -1478,18 +1484,21 @@ impl<'a> FunctionTranslator<'a> { // Determine if this if-else produces a value. Both branches must // have the same concrete (non-void) type for the result to be usable. + // `merge_ty` is Some exactly when it does, so it doubles as the + // "produces a value" flag — keeping a separate boolean around + // risks the two drifting apart. let result_ty = decl.types[expr]; - let is_value = if let Some(else_expr_id) = else_id { - let else_ty = decl.types[*else_expr_id]; - !matches!( - &*result_ty, - crate::Type::Void | crate::Type::Anon(_) | crate::Type::Var(_) - ) && result_ty == else_ty - } else { - false + let produces_value = match else_id { + Some(else_expr_id) => { + !matches!( + &*result_ty, + crate::Type::Void | crate::Type::Anon(_) | crate::Type::Var(_) + ) && result_ty == decl.types[*else_expr_id] + } + None => false, }; - let merge_ty = if is_value { + let merge_ty = if produces_value { let cl_ty = result_ty.cranelift_type(); self.builder.append_block_param(merge_block, cl_ty); Some(cl_ty) @@ -1540,7 +1549,7 @@ impl<'a> FunctionTranslator<'a> { self.builder.switch_to_block(merge_block); self.builder.seal_block(merge_block); - if is_value { + if merge_ty.is_some() { self.builder.block_params(merge_block)[0] } else { self.builder.ins().iconst(I32, 0) diff --git a/src/safety_checker.rs b/src/safety_checker.rs index 2a1716aa..930aaad4 100644 --- a/src/safety_checker.rs +++ b/src/safety_checker.rs @@ -1531,57 +1531,6 @@ impl SafetyChecker { let mut lambda_node_of: HashMap<(usize, ExprID), usize> = HashMap::new(); let mut address_taken_names: HashSet = HashSet::new(); - // Return the list of immediate subexpression IDs for a given expr. - // Lambda returns its body here because the walk needs to *start* - // at the body; the walk itself creates the lambda node and passes - // the new node as the current context before recursing. - fn subexprs(e: &Expr) -> Vec { - match e { - Expr::Id(_) - | Expr::Int(_, _) - | Expr::Real(_, _) - | Expr::String(_) - | Expr::Char(_) - | Expr::True - | Expr::False - | Expr::Enum(_) - | Expr::Break - | Expr::Continue - | Expr::TypeApp(_, _) - | Expr::Error => vec![], - Expr::Call(f, args) => { - let mut v = vec![*f]; - v.extend(args.iter().copied()); - v - } - Expr::Macro(_, args) => args.clone(), - Expr::Binop(_, l, r) => vec![*l, *r], - Expr::Unop(_, e) => vec![*e], - Expr::Lambda { body, .. } => vec![*body], - Expr::Field(base, _) => vec![*base], - Expr::Array(t, s) => vec![*t, *s], - Expr::ArrayLiteral(es) => es.clone(), - Expr::ArrayIndex(a, i) => vec![*a, *i], - Expr::AsTy(e, _) => vec![*e], - Expr::Let(_, init, _) => vec![*init], - Expr::Var(_, init, _) => init.iter().copied().collect(), - Expr::If(c, t, el) => { - let mut v = vec![*c, *t]; - if let Some(e) = el { - v.push(*e); - } - v - } - Expr::While(c, b) => vec![*c, *b], - Expr::For { - start, end, body, .. - } => vec![*start, *end, *body], - Expr::Block(es) | Expr::Tuple(es) => es.clone(), - Expr::Return(e) | Expr::Arena(e) | Expr::Assume(e) => vec![*e], - Expr::StructLit(_, fields) => fields.iter().map(|(_, e)| *e).collect(), - } - } - #[allow(clippy::too_many_arguments)] fn walk( expr: ExprID, @@ -1651,7 +1600,7 @@ impl SafetyChecker { } } _ => { - for child in subexprs(e) { + for child in e.subexprs() { walk( child, arena, diff --git a/tests/cases/checker/if_branch_value_mismatch.lyte b/tests/cases/checker/if_branch_value_mismatch.lyte new file mode 100644 index 00000000..2d92f2f7 --- /dev/null +++ b/tests/cases/checker/if_branch_value_mismatch.lyte @@ -0,0 +1,18 @@ +// Branches of an if-else used as a value must agree. Previously the if-else +// simply took its then-branch type and the else branch went unchecked, so this +// type-checked as i32 and evaluated to 0 at runtime. +// +// args: --check +// expected stdout: +// ❌ ../tests/cases/checker/if_branch_value_mismatch.lyte:12:29: if-else branches must have the same type: i32 vs f32 +// var y = if c { 1 } else { 2.0 } +// ^ + +g(c: bool) -> i32 { + var y = if c { 1 } else { 2.0 } + y +} + +main { + print(g(true)) +} diff --git a/tests/cases/checker/if_branch_void_else.lyte b/tests/cases/checker/if_branch_void_else.lyte new file mode 100644 index 00000000..e0faa77e --- /dev/null +++ b/tests/cases/checker/if_branch_void_else.lyte @@ -0,0 +1,23 @@ +// An if-else used as a value must have branches of the same type. The else +// branch here ends in a `var` declaration, which is a statement and produces +// nothing, so the if-else has no value to bind. +// +// This used to type-check as i32 (the then-branch type) and silently compile +// to a placeholder 0 in the JIT and LLVM backends, while the VM backend +// returned the then-branch value — a wrong answer and a backend divergence +// rather than an error. +// +// args: --check +// expected stdout: +// ❌ ../tests/cases/checker/if_branch_void_else.lyte:17:29: if-else branches must have the same type: i32 vs void +// var y = if c { 1 } else { var z = 2 } +// ^ + +g(c: bool) -> i32 { + var y = if c { 1 } else { var z = 2 } + y +} + +main { + print(g(true)) +} diff --git a/tests/cases/checker/let_decl_not_a_value.lyte b/tests/cases/checker/let_decl_not_a_value.lyte new file mode 100644 index 00000000..461a07d9 --- /dev/null +++ b/tests/cases/checker/let_decl_not_a_value.lyte @@ -0,0 +1,15 @@ +// A `let` declaration is a statement, just like `var` — it doesn't produce a +// value, so it can't supply the function's return value. `var` was already +// handled; `let` used to leak the binding's type and make this compile. +// +// args: --check +// expected stdout: +// ❌ ../tests/cases/checker/let_decl_not_a_value.lyte:11:12: return type must match function return type: void vs f32 +// f() -> f32 { let u = 1.0 } +// ^ + +f() -> f32 { let u = 1.0 } + +main { + print(f() as i32) +} diff --git a/tests/cases/if_stmt_branch_types.lyte b/tests/cases/if_stmt_branch_types.lyte new file mode 100644 index 00000000..9ba61272 --- /dev/null +++ b/tests/cases/if_stmt_branch_types.lyte @@ -0,0 +1,46 @@ +// An if-else in statement position discards its value, so its branches don't +// have to agree — or produce anything at all. Blocks evaluate to their last +// expression and an assignment is an expression in lyte, so branches routinely +// end up with incidental, differing types. The stdlib's ftoa does exactly this. +// +// The matching value-position rules are in checker/if_branch_void_else.lyte +// and checker/if_branch_value_mismatch.lyte. +// +// expected stdout: +// compilation successful +// assert(true) +// assert(true) +// assert(true) + +main { + var x = 0.0 + var n = 0 + + // Branch tails with different types: f32 vs i32. + if true { + x = 1.0 + } else { + n = 2 + } + assert(x == 1.0) + + // A branch ending in a declaration produces nothing at all. + if n == 0 { + n = 5 + var ignored = 1 + } else { + n = 6 + } + assert(n == 5) + + // else-if chains, as in the stdlib's ftoa. + var phase = 0 + if phase == 0 { + phase = 1 + } else if phase == 1 { + n = n + 1 + } else { + var unused = 0.0 + } + assert(phase == 1) +}