diff --git a/src/checker.rs b/src/checker.rs index a0c95a2c..63cfd104 100644 --- a/src/checker.rs +++ b/src/checker.rs @@ -685,7 +685,15 @@ impl Checker { mutable: true, }); - ty + // A var declaration is a statement, not an expression: it does + // not produce a value. Record the variable's type for the + // backends (they read types[id] to size the slot), but report + // the declaration itself as Void so an enclosing block or + // if-else doesn't treat the declaration as its result. See + // issue #22: codegen emits a placeholder i32 0 here, which used + // to flow into a merge block typed from the declared type. + self.types[id] = ty; + return mk_type(Type::Void); } Expr::Let(name, init, ty) => { let ty = if let Some(ty) = ty { *ty } else { self.fresh() }; @@ -1337,6 +1345,7 @@ impl Checker { 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); } } @@ -1346,10 +1355,40 @@ impl Checker { 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); } } + /// Reject variables declared with type void. + /// + /// Void isn't a value type in lyte — there's nothing to store and nothing + /// you can later do with the binding — so a void-typed declaration is + /// 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(); + for (i, expr) in func_decl.arena.exprs.iter().enumerate() { + let name = match expr { + Expr::Let(name, _, _) | Expr::Var(name, _, _) => name, + _ => continue, + }; + if i < solved_types.len() && matches!(&*solved_types[i], Type::Void) { + self.errors.push(TypeError { + location: func_decl.arena.locs[i], + message: format!("variable '{}' cannot have type void", name), + }); + } + } + } + /// 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). diff --git a/src/jit.rs b/src/jit.rs index 08fd6a00..bb43dd75 100644 --- a/src/jit.rs +++ b/src/jit.rs @@ -720,6 +720,28 @@ impl<'a> FunctionTranslator<'a> { self.builder.seal_block(continue_block); } + /// Check that a value about to be passed to an if-else merge block matches + /// the type the merge block param was declared with. + /// + /// The two are derived independently: the param from `decl.types`, the + /// 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. + fn check_merge_arg(&self, val: Value, expected: Type, branch: &str) { + let actual = self.builder.func.dfg.value_type(val); + assert_eq!( + actual, expected, + "JIT internal error: {} branch of if-else produced a {} value, \ + but the merge block expects {}. The checker and codegen disagree \ + about what this branch evaluates to.", + branch, actual, expected + ); + } + fn translate_lvalue(&mut self, expr: ExprID, decl: &FuncDecl, decls: &DeclTable) -> Value { match &decl.arena[expr] { Expr::Id(name) => { @@ -1467,10 +1489,13 @@ impl<'a> FunctionTranslator<'a> { false }; - if is_value { + let merge_ty = if is_value { let cl_ty = result_ty.cranelift_type(); self.builder.append_block_param(merge_block, cl_ty); - } + Some(cl_ty) + } else { + None + }; // Branch based on condition. self.builder @@ -1482,7 +1507,8 @@ impl<'a> FunctionTranslator<'a> { self.builder.seal_block(then_block); let then_val = self.translate_expr(*then_id, decl, decls); if !self.builder.is_unreachable() { - if is_value { + if let Some(merge_ty) = merge_ty { + self.check_merge_arg(then_val, merge_ty, "then"); self.builder .ins() .jump(merge_block, &[codegen::ir::BlockArg::Value(then_val)]); @@ -1500,7 +1526,8 @@ impl<'a> FunctionTranslator<'a> { self.builder.ins().iconst(I32, 0) }; if !self.builder.is_unreachable() { - if is_value { + if let Some(merge_ty) = merge_ty { + self.check_merge_arg(else_val, merge_ty, "else"); self.builder .ins() .jump(merge_block, &[codegen::ir::BlockArg::Value(else_val)]); diff --git a/src/parser.rs b/src/parser.rs index 5a3cd77d..f1f84f7b 100644 --- a/src/parser.rs +++ b/src/parser.rs @@ -739,33 +739,38 @@ fn skip_reserved(cx: &mut ParseContext) { fn parse_stmt(arena: &mut ExprArena, typevars: &[Name], cx: &mut ParseContext) -> ExprID { match &cx.lex.tok { Token::Var => { + // Capture the location of the `var` keyword: cx.lex.loc has moved + // past the initializer by the time the expression is added, which + // would anchor diagnostics to the following line. + let loc = cx.lex.loc; cx.next(); let name = expect_id(cx); if cx.lex.tok == Token::Assign { cx.next(); let e = parse_lambda(arena, typevars, cx); - arena.add(Expr::Var(name, Some(e), None), cx.lex.loc) + arena.add(Expr::Var(name, Some(e), None), loc) } else if cx.lex.tok == Token::Colon { cx.next(); let t = parse_type(typevars, cx); - arena.add(Expr::Var(name, None, Some(t)), cx.lex.loc) + arena.add(Expr::Var(name, None, Some(t)), loc) } else { cx.err(String::from("expected assignment or type")); - arena.add(Expr::Var(name, None, None), cx.lex.loc) + arena.add(Expr::Var(name, None, None), loc) } } Token::Let => { + let loc = cx.lex.loc; cx.next(); let name = expect_id(cx); if cx.lex.tok == Token::Assign { cx.next(); let e = parse_lambda(arena, typevars, cx); - arena.add(Expr::Let(name, e, None), cx.lex.loc) + arena.add(Expr::Let(name, e, None), loc) } else { cx.err(String::from("expected assignment or type")); - arena.add(Expr::Error, cx.lex.loc) + arena.add(Expr::Error, loc) } } Token::Arena => { diff --git a/tests/cases/checker/not_a_struct.lyte b/tests/cases/checker/not_a_struct.lyte index c3bfe723..ba595213 100644 --- a/tests/cases/checker/not_a_struct.lyte +++ b/tests/cases/checker/not_a_struct.lyte @@ -3,9 +3,9 @@ // ❌ ../tests/cases/checker/not_a_struct.lyte:11:14: ambiguous constraint: i32.foo == ?3 // var y = x.foo // ^ -// ❌ ../tests/cases/checker/not_a_struct.lyte:12:1: ambiguous constraint: ?3 == ?3 -// } -// ^ +// ❌ ../tests/cases/checker/not_a_struct.lyte:11:5: ambiguous constraint: ?3 == ?3 +// var y = x.foo +// ^ f { var x = 42 var y = x.foo diff --git a/tests/cases/checker/var_decl_not_a_value.lyte b/tests/cases/checker/var_decl_not_a_value.lyte new file mode 100644 index 00000000..67df73b0 --- /dev/null +++ b/tests/cases/checker/var_decl_not_a_value.lyte @@ -0,0 +1,14 @@ +// A `var` declaration is a statement, not an expression, so an if-else whose +// branches end in one produces void rather than the variable's type. This used +// to type-check and silently evaluate to 0 (issue #22). +// +// Void is not a value type, so binding it is rejected at the declaration. +// +// args: --check +// expected stdout: +// ❌ ../tests/cases/checker/var_decl_not_a_value.lyte:13:5: variable 'y' cannot have type void +// let y = if true { var x = 7 } else { var z = 9 } +// ^ +main { + let y = if true { var x = 7 } else { var z = 9 } +} diff --git a/tests/cases/var_decl_tail_in_if.lyte b/tests/cases/var_decl_tail_in_if.lyte new file mode 100644 index 00000000..d0b7686e --- /dev/null +++ b/tests/cases/var_decl_tail_in_if.lyte @@ -0,0 +1,35 @@ +// Regression test for issue #22: a branch of an if-else ending in a `var` +// declaration used to make the if-else look like it produced a value of the +// variable's type, while codegen handed the merge block a placeholder i32 0. +// With f32 branches that failed Cranelift verification: +// jump block7(v16): arg v16 has type i32, expected f32 +// A var declaration is a statement, so this if-else produces no value and +// both branches simply fall through to the merge block. +// +// expected stdout: +// compilation successful +// assert(true) +// assert(true) + +main { + var acc = 0.0 + if acc < 1.0 { + acc = 2.0 + var unused = 1.0 + } else { + acc = 3.0 + var unused = 4.0 + } + assert(acc == 2.0) + + // Same shape with integer branches, which used to silently yield 0. + var count = 0 + if count == 0 { + count = 5 + var ignored = 1 + } else { + count = 6 + var ignored = 2 + } + assert(count == 5) +}