Skip to content

Commit b60f021

Browse files
timfennisclaude
andcommitted
Fix unbalanced if as expression: always yields a value (unit when false)
Previously `if cond { val }` without an else branch was stack-neutral (net-zero), which meant `return if cond { val }` would pop the wrong stack slot. The fix has two parts: 1. `compile_if` (no-else): emit a Jump/else-path that pushes unit so the expression always leaves exactly one value on the stack. 2. Parser `block()`: wrap non-last, non-Statement block items in `to_statement()` so `Expression::Statement` handles value cleanup as designed, rather than leaking the if's result onto the stack. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
1 parent 53c6616 commit b60f021

4 files changed

Lines changed: 35 additions & 5 deletions

File tree

compiler_tests/tests/compiler.rs

Lines changed: 15 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -27,16 +27,27 @@ fn compile_with_analysis(input: &str) -> Vec<OpCode> {
2727
// if true { 1 }
2828
//
2929
// 0: Constant(0) push `true`
30-
// 1: JumpIfFalse(2) if false, skip true-branch and land on final Pop
30+
// 1: JumpIfFalse(3) if false, jump to else path (index 5)
3131
// 2: Pop pop condition (true path)
3232
// 3: Constant(1) push `1`
33-
// 4: Pop pop condition (false path, jumped here)
34-
// 5: Halt
33+
// 4: Jump(2) skip else path (jump to Halt at index 7)
34+
// 5: Pop pop condition (false path, jumped here)
35+
// 6: Constant(2) push `None` (unit, no else branch)
36+
// 7: Halt
3537
#[test]
3638
fn test_if_without_else() {
3739
assert_eq!(
3840
compile("if true { 1 }"),
39-
[Constant(0), JumpIfFalse(2), Pop, Constant(1), Pop, Halt]
41+
[
42+
Constant(0),
43+
JumpIfFalse(3),
44+
Pop,
45+
Constant(1),
46+
Jump(2),
47+
Pop,
48+
Constant(2),
49+
Halt
50+
]
4051
);
4152
}
4253

ndc_parser/src/parser.rs

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1225,6 +1225,21 @@ impl Parser {
12251225
}
12261226
};
12271227

1228+
// Non-last items in a block are in statement position: wrap bare expressions
1229+
// (e.g. `if` without else) in Statement so the compiler discards their values.
1230+
let last = statements.len().saturating_sub(1);
1231+
let statements = statements
1232+
.into_iter()
1233+
.enumerate()
1234+
.map(|(i, stmt)| {
1235+
if i < last && !matches!(stmt.expression, Expression::Statement(_)) {
1236+
stmt.to_statement()
1237+
} else {
1238+
stmt
1239+
}
1240+
})
1241+
.collect();
1242+
12281243
Ok(Expression::Block { statements }.to_location(left_curly_span.merge(loop_span)))
12291244
}
12301245

ndc_vm/src/compiler.rs

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -511,8 +511,12 @@ impl Compiler {
511511
self.compile_expr(on_false)?;
512512
self.chunk.patch_jump(jump_to_end);
513513
} else {
514+
let jump_to_end = self.chunk.write(OpCode::Jump(0), Span::new(0, 0));
514515
self.chunk.patch_jump(conditional_jump_idx);
515516
self.chunk.write(OpCode::Pop, Span::new(0, 0));
517+
let idx = self.chunk.add_constant(Value::unit());
518+
self.chunk.write(OpCode::Constant(idx), Span::new(0, 0));
519+
self.chunk.patch_jump(jump_to_end);
516520
}
517521

518522
Ok(())
@@ -898,7 +902,6 @@ fn produces_value(expr: &Expression) -> bool {
898902
..
899903
}
900904
| Expression::While { .. }
901-
| Expression::If { on_false: None, .. }
902905
| Expression::Break
903906
| Expression::Continue
904907
| Expression::Return { .. } => false,

tests/programs/998_not_desired/return_unbalanced_if.ndc

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
// vm-ready
12
// NOTE: this is definitely a weird one that should probably generate a type error in the future
23
fn is_positive(n) {
34
return if n >= 0 {

0 commit comments

Comments
 (0)