Skip to content

Unify if-else branch types when the value is used #27

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
wtholliday merged 1 commit into from
Aug 8, 2026
Merged

Unify if-else branch types when the value is used #27

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
159 changes: 130 additions & 29 deletions src/checker.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,15 @@ pub struct Checker {
/// Expression types.
pub types: Vec<TypeID>,

/// 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<bool>,

/// Is an expression's value used, rather than discarded? See
/// `mark_value_positions`.
value_pos: Vec<bool>,

/// Currently declared vars, as we're checking.
vars: Vec<Var>,

Expand Down Expand Up @@ -176,6 +185,8 @@ impl Checker {

Self {
types: vec![],
visited: vec![],
value_pos: vec![],
lvalue: vec![],
inst: Instance::new(),
next_anon: 0,
Expand Down Expand Up @@ -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) => {
Expand Down Expand Up @@ -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),
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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 {
Expand All @@ -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();

Expand Down Expand Up @@ -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,
Expand All @@ -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);
}
}
Expand Down Expand Up @@ -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);
}
}
}

Expand All @@ -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],
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down
52 changes: 52 additions & 0 deletions src/expr.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<ExprID> {
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.
Expand Down
37 changes: 23 additions & 14 deletions src/jit.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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!(
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down
Loading
Loading