Type var declarations as void (fixes #22) - #26
Merged
Conversation
A `var` declaration is a statement, but the checker typed it as the
variable's type. When a var declaration was the last expression in an
if-else branch, the branch's block took on that type, so `Expr::If`
decided the if produced a value and appended a merge block param typed
from it. Codegen meanwhile returns a placeholder `iconst(I32, 0)` for a
var declaration (src/jit.rs:1182), and that placeholder was jumped into
the merge block.
With f32 branches this failed Cranelift verification:
jump block7(v16): arg v16 has type i32, expected f32
With i32 branches the types lined up and the if silently evaluated to 0
on both the JIT and VM backends.
The Expr::Var arm now records the variable's type in types[id] — the
backends read it to size the stack slot — and returns Void from
check_expr, which is what the enclosing block and if-else consume. A
var-tailed branch is void, so is_value is false, no merge param is
added, and the placeholder never crosses a block boundary.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BX9w4k8qecsH9LMgBP8hXc
The merge block param type comes from decl.types; the branch values come
from codegen. Nothing checked that the two agree, so a mismatch either
reached the Cranelift verifier as an opaque failure, or — when the
placeholder happened to be the right Cranelift type — produced
well-formed IR computing the wrong answer.
The previous commit removed the one path that could construct such a
mismatch. This catches the class: any future expression kind typed
non-void by the checker but returning a placeholder from codegen now
fails at the mismatch, naming the branch and both types.
Before, with the checker fix reverted:
cranelift IR verification failed:
- inst32 (jump block7(v22) ; v22 = 0): arg v22 has type i32, expected f32
After:
JIT internal error: then branch of if-else produced a i32 value, but
the merge block expects f32. The checker and codegen disagree about
what this branch evaluates to.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BX9w4k8qecsH9LMgBP8hXc
Void isn't a value type in lyte: there's nothing to store and nothing you can do with the binding afterwards, so a void-typed declaration is always a mistake. It used to be accepted silently, with the error surfacing at whatever first tried to use the variable — or nowhere at all, if nothing did. check_void_declarations runs after solving and flags any let/var whose solved type is void. Like check_unsolved_types it only runs when 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. The parser recorded let/var expressions at cx.lex.loc *after* parsing the initializer, so diagnostics anchored to a declaration pointed at the following line. Capture the keyword's location instead. This also moves the second error in checker/not_a_struct.lyte from the closing brace onto the declaration it belongs to. Note this is stricter than Rust, where () is an ordinary inhabited type and `let y = ();` is legal. Lyte has no way to produce or consume a void value, so nothing is lost by rejecting the binding. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BX9w4k8qecsH9LMgBP8hXc
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #22.
The bug
A
vardeclaration is a statement, but the checker typed it as the variable's type. When a var declaration was the last expression in an if-else branch, the branch's block took on that type, soExpr::If(src/jit.rs:1459) decided the if produced a value and appended a merge block param typed from it. Codegen meanwhile returns a placeholdericonst(I32, 0)for a var declaration (src/jit.rs:1182), and that placeholder was jumped into the merge block.The crash is the lucky case. With i32 branches the types line up and nothing complains:
var x = 1.0tail (f32)var x = 7tail (i32)00--checkpassed clean on all of these.The fix
The
Expr::Vararm insrc/checker.rsnow records the variable's type intypes[id]and returnsVoidfromcheck_expr.The split matters: the backends read
decl.types[var_expr_id]to size the stack slot (src/jit.rs:1129,src/llvm_jit.rs:1907,src/stack_codegen.rs:823), so the arm can't simply returnVoidand let the tail assignment atchecker.rs:960record it. Writingtypes[id]directly keeps slot sizing intact while the enclosingBlock/Ifconsume theVoidreturn.A var-tailed branch is now void, so
is_valueis false, no merge param is added, and the placeholder never crosses a block boundary. The i32 case becomes a compile error instead of a silent0(reported at the declaration — see the third commit below).Tests
tests/cases/var_decl_tail_in_if.lyte— if-else with both branches ending in avardeclaration, f32 and i32 variants. Panics the JIT verifier before this change.tests/cases/checker/var_decl_not_a_value.lyte—--check, expects the error forlet y = if true { var x = 7 } else { var z = 9 }. Silently accepted before this change.Both were verified to fail against the unfixed compiler. Full suite is green: 351 lib tests, 21 LSP tests, and the golden suite across all four backends built here (jit, vm, asm, stack). One existing test's expected output changed —
checker/not_a_struct.lyte, explained below.Defense in depth
Second commit:
Expr::Ifnow checks each branch value's Cranelift type against the merge block param before jumping (check_merge_arginsrc/jit.rs). The param type comes fromdecl.types, the values from codegen, and nothing checked that the two agree.The first commit removed the one path that could construct such a mismatch; this catches the class. With the checker fix reverted, the same test now fails as:
instead of an opaque verifier dump. Note it catches type mismatches only: the i32 variant of this bug is type-consistent (placeholder and param are both i32) and would still slip through, which is why the checker fix is the load-bearing one.
No void bindings
Third commit: a
let/varwhose type resolves to void is now an error at the declaration.Void isn't a value type in lyte — nothing produces a void value you can hold, and nothing consumes one — so the binding is always a mistake. Previously it was accepted and the error surfaced at the first use, or nowhere if the variable went unused:
check_void_declarationsruns after solving, alongsidecheck_unsolved_typesand guarded the same way (only when no other errors were reported — unvisited expressions keep theVoidfill value fromcheck_fn_decl, so an early-bailing function would otherwise yield false positives).This required a parser fix:
let/varexpressions recordedcx.lex.locafter parsing the initializer, so any diagnostic anchored to a declaration pointed at the following line. They now record the keyword's location. That also moves the second error inchecker/not_a_struct.lyteoff the closing brace and onto the declaration — the one expected-output change to an existing test.Worth flagging: this is stricter than Rust, where
()is an ordinary inhabited type andlet y = ();is legal. Lyte has no way to produce or consume a void value, so the stricter rule costs nothing here.Not addressed here
🤖 Generated with Claude Code
https://claude.ai/code/session_01BX9w4k8qecsH9LMgBP8hXc