Fix bytecode compiler bugs: switch default, loop continue, class methods

+179 -52

2 changed files

expand all

crates

src

bytecode.rs

compiler.rs

+6 -2

crates/js/src/bytecode.rs

··· 381 381 382 382 /// Patch a jump offset at the given position to point to the current offset. 383 383 pub fn patch_jump(&mut self, pos: usize) { 384 - let target = self.offset() as i32; 385 - let offset = target - (pos as i32 + 4); // relative to after the i32 384 + self.patch_jump_to(pos, self.offset()); 385 + } 386 + 387 + /// Patch a jump offset at the given position to point to an arbitrary target. 388 + pub fn patch_jump_to(&mut self, pos: usize, target: usize) { 389 + let offset = target as i32 - (pos as i32 + 4); // relative to after the i32 386 390 let bytes = offset.to_le_bytes(); 387 391 self.func.code[pos] = bytes[0]; 388 392 self.func.code[pos + 1] = bytes[1];

+173 -50

crates/js/src/compiler.rs

··· 28 28 struct LoopCtx { 29 29 /// Label, if this is a labeled loop. 30 30 label: Option<String>, 31 - /// Bytecode offset of the loop condition check (for `continue`). 32 - continue_target: usize, 33 31 /// Patch positions for break jumps. 34 32 break_patches: Vec<usize>, 33 + /// Patch positions for continue jumps (patched after body compilation). 34 + continue_patches: Vec<usize>, 35 35 } 36 36 37 37 impl FunctionCompiler { ··· 223 223 StmtKind::Continue(label) => { 224 224 let idx = find_loop_ctx(&fc.loop_stack, label.as_deref()) 225 225 .ok_or_else(|| JsError::SyntaxError("continue outside of loop".into()))?; 226 - let target = fc.loop_stack[idx].continue_target; 227 - fc.builder.emit_jump_to(target); 226 + let patch = fc.builder.emit_jump(Op::Jump); 227 + fc.loop_stack[idx].continue_patches.push(patch); 228 228 } 229 229 230 230 StmtKind::Labeled { label, body } => { ··· 470 470 fc.builder.emit_reg_u16(Op::CreateClosure, reg, func_idx); 471 471 } 472 472 } else { 473 - // No constructor: create an empty function. 474 - let empty = Function::new(name.clone(), 0); 475 - let func_idx = fc.builder.add_function(empty); 473 + // No constructor: create a minimal function that returns undefined. 474 + let mut empty = BytecodeBuilder::new(name.clone(), 0); 475 + let r = 0u8; 476 + empty.func.register_count = 1; 477 + empty.emit_reg(Op::LoadUndefined, r); 478 + empty.emit_reg(Op::Return, r); 479 + let func_idx = fc.builder.add_function(empty.finish()); 476 480 fc.builder.emit_reg_u16(Op::CreateClosure, reg, func_idx); 477 481 } 478 482 ··· 579 583 580 584 fc.loop_stack.push(LoopCtx { 581 585 label, 582 - continue_target: loop_start, 583 586 break_patches: Vec::new(), 587 + continue_patches: Vec::new(), 584 588 }); 585 589 586 590 compile_stmt(fc, body, result_reg)?; ··· 591 595 for patch in ctx.break_patches { 592 596 fc.builder.patch_jump(patch); 593 597 } 598 + // In a while loop, continue jumps back to the condition check (loop_start). 599 + for patch in ctx.continue_patches { 600 + fc.builder.patch_jump_to(patch, loop_start); 601 + } 594 602 Ok(()) 595 603 } 596 604 ··· 606 614 607 615 fc.loop_stack.push(LoopCtx { 608 616 label, 609 - continue_target: loop_start, 610 617 break_patches: Vec::new(), 618 + continue_patches: Vec::new(), 611 619 }); 612 620 613 621 compile_stmt(fc, body, result_reg)?; 614 622 623 + // continue in do-while should jump here (the condition check). 624 + let cond_start = fc.builder.offset(); 625 + 615 626 let cond = fc.alloc_reg(); 616 627 compile_expr(fc, test, cond)?; 617 628 fc.builder ··· 621 632 let ctx = fc.loop_stack.pop().unwrap(); 622 633 for patch in ctx.break_patches { 623 634 fc.builder.patch_jump(patch); 635 + } 636 + for patch in ctx.continue_patches { 637 + fc.builder.patch_jump_to(patch, cond_start); 624 638 } 625 639 Ok(()) 626 640 } ··· 670 684 None 671 685 }; 672 686 673 - // continue_target points to the update expression (or loop_start if no update). 674 - // We'll set this after compiling the body. 675 - let continue_placeholder = fc.builder.offset(); 676 687 fc.loop_stack.push(LoopCtx { 677 688 label, 678 - continue_target: continue_placeholder, // will be updated 679 689 break_patches: Vec::new(), 690 + continue_patches: Vec::new(), 680 691 }); 681 692 682 693 compile_stmt(fc, body, result_reg)?; 683 694 684 - // Set the continue target to the update expression position. 695 + // continue in a for-loop should jump here (the update expression). 685 696 let continue_target = fc.builder.offset(); 686 - let loop_idx = fc.loop_stack.len() - 1; 687 - fc.loop_stack[loop_idx].continue_target = continue_target; 688 697 689 698 // Update. 690 699 if let Some(update) = update { ··· 703 712 for patch in ctx.break_patches { 704 713 fc.builder.patch_jump(patch); 705 714 } 715 + for patch in ctx.continue_patches { 716 + fc.builder.patch_jump_to(patch, continue_target); 717 + } 706 718 707 719 fc.locals.truncate(saved_locals); 708 720 fc.next_reg = saved_next; ··· 722 734 // Use a loop context for break statements. 723 735 fc.loop_stack.push(LoopCtx { 724 736 label: None, 725 - continue_target: 0, // not applicable for switch 726 737 break_patches: Vec::new(), 738 + continue_patches: Vec::new(), 727 739 }); 728 740 729 - let mut case_patches = Vec::new(); 730 - let mut default_patch = None; 741 + // Phase 1: emit comparison jumps for each non-default case. 742 + // Store (case_index, patch_position) for each case with a test. 743 + let mut case_jump_patches: Vec<(usize, usize)> = Vec::new(); 744 + let mut default_index: Option<usize> = None; 731 745 732 - // Phase 1: emit comparison jumps for each case. 733 - for case in cases { 746 + for (i, case) in cases.iter().enumerate() { 734 747 if let Some(test) = &case.test { 735 748 let test_reg = fc.alloc_reg(); 736 749 compile_expr(fc, test, test_reg)?; ··· 740 753 let patch = fc.builder.emit_cond_jump(Op::JumpIfTrue, cmp_reg); 741 754 fc.free_reg(cmp_reg); 742 755 fc.free_reg(test_reg); 743 - case_patches.push(patch); 756 + case_jump_patches.push((i, patch)); 744 757 } else { 745 - // Default case: jump is emitted after all test comparisons. 746 - case_patches.push(0); // placeholder 747 - default_patch = Some(case_patches.len() - 1); 758 + default_index = Some(i); 748 759 } 749 760 } 750 761 751 - // Jump to default or end if no case matched. 752 - let end_jump = if default_patch.is_some() { 753 - None 754 - } else { 755 - Some(fc.builder.emit_jump(Op::Jump)) 756 - }; 762 + // After all comparisons: jump to default body or end. 763 + let fallthrough_patch = fc.builder.emit_jump(Op::Jump); 757 764 758 - // Phase 2: emit case bodies. 765 + // Phase 2: emit case bodies in order (fall-through semantics). 766 + let mut body_offsets: Vec<(usize, usize)> = Vec::new(); 759 767 for (i, case) in cases.iter().enumerate() { 760 - if default_patch == Some(i) { 761 - // Patch the "no match" jump to here if this is the default. 762 - if let Some(end_j) = end_jump { 763 - fc.builder.patch_jump(end_j); 764 - } 765 - // We need to update the default case patch to point here. 766 - let default_jump_target = fc.builder.offset(); 767 - // The default_patch entry is a placeholder; we don't jump TO it. 768 - // Instead, if no case matched and there's a default, we jump here. 769 - // Let's fix: emit a Jump before the first case body that jumps to default. 770 - let _ = default_jump_target; 771 - } 768 + body_offsets.push((i, fc.builder.offset())); 769 + compile_stmts(fc, &case.consequent, result_reg)?; 770 + } 771 + 772 + let end_offset = fc.builder.offset(); 772 773 773 - fc.builder.patch_jump(case_patches[i]); 774 - compile_stmts(fc, &case.consequent, result_reg)?; 774 + // Patch case test jumps to their respective body offsets. 775 + for (case_idx, patch) in &case_jump_patches { 776 + let body_offset = body_offsets 777 + .iter() 778 + .find(|(i, _)| i == case_idx) 779 + .map(|(_, off)| *off) 780 + .unwrap(); 781 + fc.builder.patch_jump_to(*patch, body_offset); 775 782 } 776 783 777 - if let Some(end_j) = end_jump { 778 - fc.builder.patch_jump(end_j); 784 + // Patch fallthrough: jump to default body if present, otherwise to end. 785 + if let Some(def_idx) = default_index { 786 + let default_offset = body_offsets 787 + .iter() 788 + .find(|(i, _)| *i == def_idx) 789 + .map(|(_, off)| *off) 790 + .unwrap(); 791 + fc.builder.patch_jump_to(fallthrough_patch, default_offset); 792 + } else { 793 + fc.builder.patch_jump_to(fallthrough_patch, end_offset); 779 794 } 780 795 781 796 fc.free_reg(disc_reg); ··· 1195 1210 fc.builder.emit_reg_u16(Op::CreateClosure, dst, func_idx); 1196 1211 } 1197 1212 } else { 1198 - let empty = Function::new(name, 0); 1199 - let func_idx = fc.builder.add_function(empty); 1213 + let mut empty = BytecodeBuilder::new(name, 0); 1214 + let r = 0u8; 1215 + empty.func.register_count = 1; 1216 + empty.emit_reg(Op::LoadUndefined, r); 1217 + empty.emit_reg(Op::Return, r); 1218 + let func_idx = fc.builder.add_function(empty.finish()); 1200 1219 fc.builder.emit_reg_u16(Op::CreateClosure, dst, func_idx); 1220 + } 1221 + 1222 + // Compile methods as properties on the constructor. 1223 + for member in &class_def.body { 1224 + match &member.kind { 1225 + ClassMemberKind::Method { 1226 + key, 1227 + value, 1228 + kind, 1229 + is_static: _, 1230 + computed: _, 1231 + } => { 1232 + if matches!(kind, MethodKind::Constructor) { 1233 + continue; 1234 + } 1235 + let method_name = match key { 1236 + PropertyKey::Identifier(s) | PropertyKey::String(s) => s.clone(), 1237 + _ => continue, 1238 + }; 1239 + let inner = compile_function_body(value)?; 1240 + let func_idx = fc.builder.add_function(inner); 1241 + let method_reg = fc.alloc_reg(); 1242 + fc.builder 1243 + .emit_reg_u16(Op::CreateClosure, method_reg, func_idx); 1244 + let name_idx = fc.builder.add_name(&method_name); 1245 + fc.builder.emit_set_prop_name(dst, name_idx, method_reg); 1246 + fc.free_reg(method_reg); 1247 + } 1248 + ClassMemberKind::Property { .. } => {} 1249 + } 1201 1250 } 1202 1251 } 1203 1252 ··· 1746 1795 let f = compile_src("for (;;) { break; }"); 1747 1796 let dis = f.disassemble(); 1748 1797 assert!(dis.contains("Jump"), "got:\n{dis}"); 1798 + } 1799 + 1800 + #[test] 1801 + fn test_for_continue_targets_update() { 1802 + // `continue` in a for-loop must jump to the update expression, not back 1803 + // to the condition check. Verify the continue jump goes to the Add (i + 1) 1804 + // rather than to the LessThan condition. 1805 + let f = compile_src("for (var i = 0; i < 10; i = i + 1) { continue; }"); 1806 + let dis = f.disassemble(); 1807 + // The for-loop should contain: LessThan (test), JumpIfFalse (exit), 1808 + // Jump (continue), Add (update), Jump (back to test). 1809 + assert!(dis.contains("LessThan"), "missing test: {dis}"); 1810 + assert!(dis.contains("Add"), "missing update: {dis}"); 1811 + // There should be at least 2 Jump instructions (continue + back-edge). 1812 + let jump_count = dis.matches("Jump ").count(); 1813 + assert!( 1814 + jump_count >= 2, 1815 + "expected >= 2 jumps for continue + back-edge, got {jump_count}: {dis}" 1816 + ); 1817 + } 1818 + 1819 + #[test] 1820 + fn test_do_while_continue_targets_condition() { 1821 + // `continue` in do-while must jump to the condition, not the body start. 1822 + let f = compile_src("var i = 0; do { i = i + 1; continue; } while (i < 5);"); 1823 + let dis = f.disassemble(); 1824 + assert!(dis.contains("LessThan"), "missing condition: {dis}"); 1825 + assert!(dis.contains("JumpIfTrue"), "missing back-edge: {dis}"); 1826 + } 1827 + 1828 + #[test] 1829 + fn test_switch_default_case() { 1830 + // Default case must not corrupt bytecode. 1831 + let f = compile_src("switch (1) { case 1: 10; break; default: 20; break; }"); 1832 + let dis = f.disassemble(); 1833 + assert!(dis.contains("StrictEq"), "missing case test: {dis}"); 1834 + // The first instruction should NOT be corrupted. 1835 + assert!( 1836 + dis.contains("LoadUndefined r0"), 1837 + "first instruction corrupted: {dis}" 1838 + ); 1839 + } 1840 + 1841 + #[test] 1842 + fn test_switch_only_default() { 1843 + // Switch with only a default case. 1844 + let f = compile_src("switch (42) { default: 99; }"); 1845 + let dis = f.disassemble(); 1846 + // Should compile without panicking and contain the default body. 1847 + assert!(dis.contains("LoadInt8"), "got:\n{dis}"); 1848 + } 1849 + 1850 + #[test] 1851 + fn test_class_empty_constructor_has_return() { 1852 + // A class without an explicit constructor should produce a function with Return. 1853 + let f = compile_src("class Foo {}"); 1854 + assert!(!f.functions.is_empty(), "should have constructor function"); 1855 + let ctor = &f.functions[0]; 1856 + let dis = ctor.disassemble(); 1857 + assert!( 1858 + dis.contains("Return"), 1859 + "empty constructor must have Return: {dis}" 1860 + ); 1861 + } 1862 + 1863 + #[test] 1864 + fn test_class_expression_compiles_methods() { 1865 + // Class expression should compile methods, not just the constructor. 1866 + let f = compile_src("var C = class { greet() { return 1; } };"); 1867 + let dis = f.disassemble(); 1868 + assert!( 1869 + dis.contains("SetPropertyByName"), 1870 + "method should be set as property: {dis}" 1871 + ); 1749 1872 } 1750 1873 }

Configure Feed

Configure Feed