Unify if-else branch types when the value is used - #27
Merged
Conversation
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) <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.
Follow-up to #26, from a code review of that PR.
The bug
Typing a
vardeclaration as void fixed the f32 half of #22, but it moved the i32 half rather than removing it. An if-else whose else branch ends in a declaration now fails theresult_ty == else_tytest in the Cranelift and LLVM lowerings, so the whole expression degrades to theiconst(I32, 0)placeholder and the then-branch value is thrown away:g(true)returned 1 before #26 and 0 after, with no diagnostic. The VM backend still returned 1, so the two backends disagreed —cargo fuzz run differentialshould have been able to find this.The root cause is that
Expr::Iftook its then-branch type and never looked at the else branch, so branch disagreement was left for codegen to discover, and codegen's only recourse is a placeholder — a wrong answer instead of an error.The fix
Unify the two branches in the checker, 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
ftoadoes exactly this. Somark_value_positionsrecords 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 branches are unified only where the value is actually used.Also in the checker:
letgets the same statement treatment asvar. It still leaked the binding's type, sof() -> f32 { let u = 1.0 }compiled while the byte-identicalvarform was rejected.self.errors, which accumulates across decls — one bad function disabled constraint solving and every post-check pass for all later ones.check_void_declarationsskips expressionscheck_exprnever visited instead of bailing out when any error exists. Unvisited expressions keeping theVoidfill value was the actual reason for that gate.solved_types()call instead of each running the substitution over every expression in the function.check()callscheck_declrather than repeating its body, so a new pass only has to be added in one place.In the JIT,
merge_tyisSomeexactly when the if-else produces a value, so use it directly and drop the parallelis_valueboolean.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.Expr::subexprsmoves from a nested fn insafety_checkertoimpl Expr, since the new walk needs it too.Not changed
The review also flagged that #26 rejects
let r = p(1)wherepreturns void, which used to compile. That looks intentional — it's whatchecker/var_decl_not_a_value.lyterelies on, and a void binding can't be read — so it stays. Worth knowing it's a source-breaking change for anything that bound a void call.Tests
checker/if_branch_void_else.lyte— the miscompile above, now a type errorchecker/if_branch_value_mismatch.lyte—if c { 1 } else { 2.0 }in value positionchecker/let_decl_not_a_value.lyte—letin tail positionif_stmt_branch_types.lyte— statement-position branches with differing tail types keep working, across all four backendscargo test --workspacepasses: 351 unit tests, 306 golden tests on jit/vm/asm/stack.🤖 Generated with Claude Code
https://claude.ai/code/session_01BX9w4k8qecsH9LMgBP8hXc