Skip to content

Commit 2a823ed

Browse files
timfennisclaude
timfennis
and
claude
committed
refactor(vm): newtype JumpOffset for jump operands 🔖
Wraps the `isize` operand of jump-family opcodes in a `JumpOffset` newtype so the contract has a single definition site and future representations (label IDs, basic-block targets) can be slotted in without touching every call site. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
1 parent f8a656c commit 2a823ed

4 files changed

Lines changed: 101 additions & 39 deletions

File tree

‎ndc_vm/src/chunk.rs‎

Lines changed: 44 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,41 @@ use ndc_lexer::Span;
33
use ndc_parser::CaptureSource;
44
use std::rc::Rc;
55

6+
/// A signed displacement applied to the instruction pointer to perform a jump.
7+
///
8+
/// Wraps `isize` so the contract — "relative offset, in instructions, from the
9+
/// instruction *after* the jump opcode" — has a single definition site. Future
10+
/// stages of a compile pipeline may swap this for an `enum JumpTarget { … }`
11+
/// without touching every call site.
12+
#[derive(Copy, Clone, PartialEq, Eq)]
13+
pub struct JumpOffset(isize);
14+
15+
impl JumpOffset {
16+
pub const ZERO: Self = Self(0);
17+
18+
#[inline]
19+
pub fn new(offset: isize) -> Self {
20+
Self(offset)
21+
}
22+
23+
#[inline]
24+
pub fn raw(self) -> isize {
25+
self.0
26+
}
27+
28+
/// Advance `ip` by this offset using wrapping signed arithmetic.
29+
#[inline]
30+
pub fn apply(self, ip: usize) -> usize {
31+
ip.wrapping_add_signed(self.0)
32+
}
33+
}
34+
35+
impl std::fmt::Debug for JumpOffset {
36+
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
37+
std::fmt::Debug::fmt(&self.0, f)
38+
}
39+
}
40+
641
/// A single bytecode instruction.
742
///
843
/// ## Stack effects
@@ -57,11 +92,11 @@ pub enum OpCode {
5792
/// Pops top of stack. `[… value → …]`
5893
Pop,
5994
/// Unconditional jump. `[…] → […]`
60-
Jump(isize),
95+
Jump(JumpOffset),
6196
/// Peeks top; jumps if true. `[… bool → … bool]`
62-
JumpIfTrue(isize),
97+
JumpIfTrue(JumpOffset),
6398
/// Peeks top; jumps if false. `[… bool → … bool]`
64-
JumpIfFalse(isize),
99+
JumpIfFalse(JumpOffset),
65100
/// Pushes a constant. `[… → … value]`
66101
Constant(usize),
67102
/// Copies local slot onto stack. `[… → … value]`
@@ -88,7 +123,7 @@ pub enum OpCode {
88123
/// Pops value, pushes iterator. No-op if already an iterator. `[… value → … iter]`
89124
GetIterator,
90125
/// Peeks iterator; pushes next value or jumps if exhausted. `[… iter → … iter value]`
91-
IterNext(isize),
126+
IterNext(JumpOffset),
92127
/// Pops value, appends to list at local slot. `[… value → …]`
93128
ListPush(usize),
94129
/// Pops value then key, inserts into map at local slot. `[… key value → …]`
@@ -113,9 +148,9 @@ impl std::fmt::Debug for OpCode {
113148
Self::Call(n) => write!(f, "Call({n})"),
114149
Self::CallVec(n) => write!(f, "CallVec({n})"),
115150
Self::Pop => write!(f, "Pop"),
116-
Self::Jump(n) => write!(f, "Jump({n})"),
117-
Self::JumpIfTrue(n) => write!(f, "JumpIfTrue({n})"),
118-
Self::JumpIfFalse(n) => write!(f, "JumpIfFalse({n})"),
151+
Self::Jump(n) => write!(f, "Jump({n:?})"),
152+
Self::JumpIfTrue(n) => write!(f, "JumpIfTrue({n:?})"),
153+
Self::JumpIfFalse(n) => write!(f, "JumpIfFalse({n:?})"),
119154
Self::Constant(n) => write!(f, "Constant({n})"),
120155
Self::GetLocal(n) => write!(f, "GetLocal({n})"),
121156
Self::SetLocal(n) => write!(f, "SetLocal({n})"),
@@ -141,7 +176,7 @@ impl std::fmt::Debug for OpCode {
141176
write!(f, ")")
142177
}
143178
Self::GetIterator => write!(f, "GetIterator"),
144-
Self::IterNext(n) => write!(f, "IterNext({n})"),
179+
Self::IterNext(n) => write!(f, "IterNext({n:?})"),
145180
Self::ListPush(n) => write!(f, "ListPush({n})"),
146181
Self::MapInsert(n) => write!(f, "MapInsert({n})"),
147182
Self::MakeRange { inclusive, bounded } => {
@@ -182,7 +217,7 @@ impl Chunk {
182217

183218
/// Overwrites the jump operand of a `Jump`, `JumpIfTrue`, `JumpIfFalse`, or
184219
/// `IterNext` already written at `idx`. Panics on any other opcode.
185-
pub fn set_jump_offset(&mut self, idx: usize, offset: isize) {
220+
pub fn set_jump_offset(&mut self, idx: usize, offset: JumpOffset) {
186221
match self.code.get_mut(idx) {
187222
Some(
188223
OpCode::JumpIfFalse(n)

‎ndc_vm/src/compiler.rs‎

Lines changed: 28 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
use crate::chunk::{Chunk, OpCode};
1+
use crate::chunk::{Chunk, JumpOffset, OpCode};
22
use crate::value::{CompiledFunction, Function};
33
use crate::{Object, Value};
44
use ndc_core::{StaticType, TypeSignature};
@@ -139,13 +139,17 @@ impl Compiler {
139139
self.compile_expr(*left)?;
140140
match operator {
141141
LogicalOperator::And => {
142-
let end_jump = self.chunk.write(OpCode::JumpIfFalse(0), left_span);
142+
let end_jump = self
143+
.chunk
144+
.write(OpCode::JumpIfFalse(JumpOffset::ZERO), left_span);
143145
self.chunk.write(OpCode::Pop, Span::synthetic());
144146
self.compile_expr(*right)?;
145147
self.patch_jump(end_jump);
146148
}
147149
LogicalOperator::Or => {
148-
let end_jump = self.chunk.write(OpCode::JumpIfTrue(0), left_span);
150+
let end_jump = self
151+
.chunk
152+
.write(OpCode::JumpIfTrue(JumpOffset::ZERO), left_span);
149153
self.chunk.write(OpCode::Pop, Span::synthetic());
150154
self.compile_expr(*right)?;
151155
self.patch_jump(end_jump);
@@ -420,7 +424,7 @@ impl Compiler {
420424
self.chunk.write(OpCode::Return, span);
421425
}
422426
Expression::Break => {
423-
let idx = self.chunk.write(OpCode::Jump(0), span); // will be backpatched
427+
let idx = self.chunk.write(OpCode::Jump(JumpOffset::ZERO), span); // will be backpatched
424428
self.current_loop_context_mut()
425429
.ok_or(CompileError::unexpected_break(span))?
426430
.break_instructions
@@ -589,14 +593,15 @@ impl Compiler {
589593
fn patch_jump(&mut self, op_idx: usize) {
590594
let offset =
591595
isize::try_from(self.chunk.len() - op_idx - 1).expect("jump too large to patch");
592-
self.chunk.set_jump_offset(op_idx, offset);
596+
self.chunk.set_jump_offset(op_idx, JumpOffset::new(offset));
593597
}
594598

595599
/// Emits a `Jump` that goes back to `target` (a previously recorded chunk offset).
596600
fn write_jump_back(&mut self, target: usize, span: Span) -> usize {
597601
let offset =
598602
-isize::try_from(self.chunk.len() - target + 1).expect("loop too large to jump back");
599-
self.chunk.write(OpCode::Jump(offset), span)
603+
self.chunk
604+
.write(OpCode::Jump(JumpOffset::new(offset)), span)
600605
}
601606

602607
fn compile_block(
@@ -633,18 +638,24 @@ impl Compiler {
633638
) -> Result<(), CompileError> {
634639
let condition_span = condition.span;
635640
self.compile_expr(condition)?;
636-
let conditional_jump_idx = self.chunk.write(OpCode::JumpIfFalse(0), condition_span);
641+
let conditional_jump_idx = self
642+
.chunk
643+
.write(OpCode::JumpIfFalse(JumpOffset::ZERO), condition_span);
637644
self.chunk.write(OpCode::Pop, Span::synthetic());
638645
self.compile_expr(on_true)?;
639646
if let Some(on_false) = on_false {
640-
let jump_to_end = self.chunk.write(OpCode::Jump(0), Span::synthetic());
647+
let jump_to_end = self
648+
.chunk
649+
.write(OpCode::Jump(JumpOffset::ZERO), Span::synthetic());
641650
self.patch_jump(conditional_jump_idx);
642651
self.chunk.write(OpCode::Pop, Span::synthetic());
643652
self.compile_expr(on_false)?;
644653
self.patch_jump(jump_to_end);
645654
} else {
646655
// No else branch — push unit so the if-expression always produces a value.
647-
let jump_to_end = self.chunk.write(OpCode::Jump(0), Span::synthetic());
656+
let jump_to_end = self
657+
.chunk
658+
.write(OpCode::Jump(JumpOffset::ZERO), Span::synthetic());
648659
self.patch_jump(conditional_jump_idx);
649660
self.chunk.write(OpCode::Pop, Span::synthetic());
650661
let idx = self.chunk.add_constant(Value::unit());
@@ -664,7 +675,9 @@ impl Compiler {
664675
let condition_span = condition.span;
665676
let loop_start = self.new_loop_context();
666677
self.compile_expr(condition)?;
667-
let conditional_jump_idx = self.chunk.write(OpCode::JumpIfFalse(0), condition_span);
678+
let conditional_jump_idx = self
679+
.chunk
680+
.write(OpCode::JumpIfFalse(JumpOffset::ZERO), condition_span);
668681
self.chunk.write(OpCode::Pop, Span::synthetic());
669682
self.compile_expr(loop_body)?;
670683
self.chunk.write(OpCode::Pop, Span::synthetic());
@@ -848,7 +861,7 @@ impl Compiler {
848861
self.chunk.write(OpCode::GetIterator, sequence.span);
849862

850863
let loop_start = self.new_loop_context();
851-
let iter_next = self.chunk.write(OpCode::IterNext(0), span);
864+
let iter_next = self.chunk.write(OpCode::IterNext(JumpOffset::ZERO), span);
852865
self.compile_declare_lvalue(l_value.clone(), span)?;
853866

854867
self.compile_for_iterations(rest, span, compile_leaf)?;
@@ -876,10 +889,12 @@ impl Compiler {
876889
}
877890
ForIteration::Guard(condition) => {
878891
self.compile_expr(condition.clone())?;
879-
let skip_jump = self.chunk.write(OpCode::JumpIfFalse(0), span);
892+
let skip_jump = self
893+
.chunk
894+
.write(OpCode::JumpIfFalse(JumpOffset::ZERO), span);
880895
self.chunk.write(OpCode::Pop, Span::synthetic());
881896
self.compile_for_iterations(rest, span, compile_leaf)?;
882-
let end_jump = self.chunk.write(OpCode::Jump(0), span);
897+
let end_jump = self.chunk.write(OpCode::Jump(JumpOffset::ZERO), span);
883898
self.patch_jump(skip_jump);
884899
self.chunk.write(OpCode::Pop, Span::synthetic());
885900
self.patch_jump(end_jump);

‎ndc_vm/src/vm.rs‎

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -217,7 +217,7 @@ impl Vm {
217217
let top = self.stack.last().expect("stack underflow");
218218
match top {
219219
Value::Bool(false) => {
220-
frame.ip = frame.ip.wrapping_add_signed(offset);
220+
frame.ip = offset.apply(frame.ip);
221221
}
222222
Value::Bool(true) => {}
223223
value => {
@@ -234,7 +234,7 @@ impl Vm {
234234
let top = self.stack.last().expect("stack underflow");
235235
match top {
236236
Value::Bool(true) => {
237-
frame.ip = frame.ip.wrapping_add_signed(offset);
237+
frame.ip = offset.apply(frame.ip);
238238
}
239239
Value::Bool(false) => {}
240240
value => {
@@ -247,8 +247,7 @@ impl Vm {
247247
}
248248
}
249249
OpCode::Jump(offset) => {
250-
let offset = *offset;
251-
frame.ip = frame.ip.wrapping_add_signed(offset);
250+
frame.ip = offset.apply(frame.ip);
252251
}
253252
OpCode::Pop => {
254253
self.stack.pop();
@@ -434,7 +433,7 @@ impl Vm {
434433
self.stack.push(value);
435434
}
436435
None => {
437-
frame.ip = frame.ip.wrapping_add_signed(offset);
436+
frame.ip = offset.apply(frame.ip);
438437
}
439438
}
440439
}

‎tests/compiler/tests/compiler.rs‎

Lines changed: 25 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
use ndc_lexer::{Lexer, SourceId};
22
use ndc_parser::Parser;
3+
use ndc_vm::chunk::JumpOffset;
34
use ndc_vm::chunk::OpCode;
45
use ndc_vm::chunk::OpCode::*;
56
use ndc_vm::compiler::Compiler;
@@ -53,10 +54,10 @@ fn test_if_without_else() {
5354
compile("if true { 1 }"),
5455
[
5556
Constant(0),
56-
JumpIfFalse(3),
57+
JumpIfFalse(JumpOffset::new(3)),
5758
Pop,
5859
Constant(1),
59-
Jump(2),
60+
Jump(JumpOffset::new(2)),
6061
Pop,
6162
Constant(2),
6263
Halt
@@ -80,10 +81,10 @@ fn test_if_with_else() {
8081
compile("if true { 1 } else { 2 }"),
8182
[
8283
Constant(0),
83-
JumpIfFalse(3),
84+
JumpIfFalse(JumpOffset::new(3)),
8485
Pop,
8586
Constant(1),
86-
Jump(2),
87+
Jump(JumpOffset::new(2)),
8788
Pop,
8889
Constant(2),
8990
Halt
@@ -104,7 +105,13 @@ fn test_if_with_else() {
104105
fn test_and() {
105106
assert_eq!(
106107
compile("true and false"),
107-
[Constant(0), JumpIfFalse(2), Pop, Constant(1), Halt]
108+
[
109+
Constant(0),
110+
JumpIfFalse(JumpOffset::new(2)),
111+
Pop,
112+
Constant(1),
113+
Halt
114+
]
108115
);
109116
}
110117

@@ -121,7 +128,13 @@ fn test_and() {
121128
fn test_or() {
122129
assert_eq!(
123130
compile("true or false"),
124-
[Constant(0), JumpIfTrue(2), Pop, Constant(1), Halt]
131+
[
132+
Constant(0),
133+
JumpIfTrue(JumpOffset::new(2)),
134+
Pop,
135+
Constant(1),
136+
Halt
137+
]
125138
);
126139
}
127140

@@ -186,10 +199,10 @@ fn test_if_with_statement_else() {
186199
compile("if true { 3 } else { 3; }"),
187200
[
188201
Constant(0),
189-
JumpIfFalse(3),
202+
JumpIfFalse(JumpOffset::new(3)),
190203
Pop,
191204
Constant(1),
192-
Jump(4),
205+
Jump(JumpOffset::new(4)),
193206
Pop,
194207
Constant(2),
195208
Pop,
@@ -221,12 +234,12 @@ fn test_if_with_statement_branches() {
221234
compile("if true { 3; } else { 3; }"),
222235
[
223236
Constant(0),
224-
JumpIfFalse(5),
237+
JumpIfFalse(JumpOffset::new(5)),
225238
Pop,
226239
Constant(1),
227240
Pop,
228241
Constant(2),
229-
Jump(4),
242+
Jump(JumpOffset::new(4)),
230243
Pop,
231244
Constant(3),
232245
Pop,
@@ -252,11 +265,11 @@ fn test_while() {
252265
compile("while true { 1 }"),
253266
[
254267
Constant(0),
255-
JumpIfFalse(4),
268+
JumpIfFalse(JumpOffset::new(4)),
256269
Pop,
257270
Constant(1),
258271
Pop,
259-
Jump(-6),
272+
Jump(JumpOffset::new(-6)),
260273
Pop,
261274
Halt
262275
]

0 commit comments

Comments
 (0)