compiler/core/src/bytecode.rs (2)

36-58: Enum naming nit: consider NoLine over None.

None reads like Option::None at use sites; NoLine matches CPython docs and avoids ambiguity.

---

218-221: New fields: document invariants.

Add a brief doc that linetable/exceptiontable are CPython 3.11+ raw blobs (co_linetable/co_exceptiontable) and may be empty when unavailable.

compiler/codegen/src/ir.rs (1)

225-227: Exception table placeholder.

Leaving exceptiontable empty is fine short‑term; add a TODO pointing to the planned encoder to avoid shipping a silent stub.

vm/src/builtins/code.rs (4)

47-96: Advance logic: robust to malformed entries.

Skips payload bytes until next header (bit7 set). Consider logging at debug level when encountering invalid bytes to aid diagnostics.

---

160-176: replace(): new args are reasonable; validate inputs.

Nice to add co_nlocals consistency check. Consider validating co_firstlineno=0 maps to None (already handled) and rejecting negative via API.

---

498-569: co_lines(): eager list is fine; iterator later.

Merges adjacent ranges with same line and maps NO_LINE to None. Consider yielding a lazy iterator in a follow‑up for large functions.

---

860-921: Duplicated linetable parsing logic; consider sharing.

LineTableReader duplicates varint/signed‑varint logic from codegen. Extract a small shared helper/module to avoid drift.

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

vm/src/builtins/code.rs (1)

759-767: Don’t silently ignore co_code; reject until parsing is implemented.

This still returns the original instructions when a user supplies co_code, which is surprising. Emit a clear NotImplemented error instead.

Apply:

-        let instructions = match args.co_code {
-            OptionalArg::Present(_code_bytes) => {
-                // Convert bytes back to instructions
-                // For now, keep the original instructions
-                // TODO: Properly parse bytecode from bytes
-                self.code.instructions.clone()
-            }
-            OptionalArg::Missing => self.code.instructions.clone(),
-        };
+        let instructions = match args.co_code {
+            OptionalArg::Present(_code_bytes) => {
+                return Err(vm.new_not_implemented_error(
+                    "co_code replacement is not supported yet".to_string(),
+                ));
+            }
+            OptionalArg::Missing => self.code.instructions.clone(),
+        };

vm/src/builtins/code.rs (2)

42-46: NO_LINE detection should use decoded kind, not raw header bits.

Comparing (byte >> 3) == 0x1f relies on the top bit being set and is indirect. Use the already‑decoded code/kind.

Apply:

-    /// Check if this is a NO_LINE marker (code 15)
-    fn is_no_line_marker(byte: u8) -> bool {
-        (byte >> 3) == 0x1f
-    }
+    /// Check if this is a NO_LINE marker (kind == None)
+    fn is_no_line_marker(code: u8) -> bool {
+        matches!(PyCodeLocationInfoKind::from_code(code), Some(PyCodeLocationInfoKind::None))
+    }
...
-        // Check for NO_LINE marker
-        if Self::is_no_line_marker(first_byte) {
+        // Check for NO_LINE marker
+        if Self::is_no_line_marker(code) {
             self.ar_line = -1;
         } else {
             self.ar_line = self.computed_line;
         }

Also applies to: 62-77

---

82-95: Skip loop is unnecessary and brittle; rely on kind‑specific reads.

get_line_delta already consumes the payload for the entry. Scanning until the next 0x80 risks desync if future payloads legally contain 0x80 bytes.

Apply:

-        // Skip remaining bytes for this entry
-        while !self.reader.at_end() {
-            if let Some(b) = self.reader.peek_byte() {
-                if (b & 0x80) != 0 {
-                    break;
-                }
-                self.reader.read_byte();
-            } else {
-                break;
-            }
-        }
+        // Payload fully consumed by get_line_delta()

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

compiler/codegen/src/ir.rs (1)

398-502: One‑line encoding wrongly restricted to columns < 128; allow full 0–255.

CPython’s OneLine{0,1,2} forms use raw u8 columns. Limiting to < 128 forces Long, bloating tables and diverging from CPython.

-                } else if col < 128 && end_col < 128 {
+                } else if col >= 0 && col <= 255 && end_col >= 0 && end_col <= 255 {
@@
-                if col < 128 && end_col < 128 {
+                if col >= 0 && col <= 255 && end_col >= 0 && end_col <= 255 {

Also consider emitting NoColumns when column is unavailable in future changes rather than always going Long.

vm/src/builtins/code.rs (1)

759-767: Don’t silently ignore co_code; reject until parsing is implemented.

Returning the original instructions surprises users. Prefer a clear error.

-            OptionalArg::Present(_code_bytes) => {
-                // Convert bytes back to instructions
-                // For now, keep the original instructions
-                // TODO: Properly parse bytecode from bytes
-                self.code.instructions.clone()
-            }
+            OptionalArg::Present(_code_bytes) => {
+                return Err(vm.new_not_implemented_error(
+                    "co_code replacement is not supported yet".to_string()
+                ));
+            }

compiler/core/src/bytecode.rs (1)

36-58: Enum shape looks good; small naming nit on the “no line” variant.

Consider renaming None to NoLine to avoid repeated Option::None disambiguation and improve readability wherever both appear together. Non‑blocking.

compiler/codegen/src/ir.rs (4)

75-76: Consider capturing end locations sooner.

Tracking end_location now will let you emit column ranges (and multi‑line spans) later without reshaping the pipeline.

---

203-205: Use safe first-line conversion.

first_line_number.get() as i32 can silently truncate on very large inputs. Prefer i32::try_from(...).unwrap_or(i32::MAX) or clamp.

-        let linetable = generate_linetable(&locations, first_line_number.get() as i32);
+        let first = i32::try_from(first_line_number.get()).unwrap_or(i32::MAX);
+        let linetable = generate_linetable(&locations, first);

---

225-227: Empty exception table placeholder — gate or follow‑up issue.

Returning an empty table is fine temporarily; please either feature‑gate exposure or open a tracked issue to avoid shipping a partially inert API.

---

504-527: Varint writers match the reader and CPython’s scheme; consider reuse.

Encoding looks correct (6‑bit chunks, signed mapping). If feasible, place these helpers in a shared module to avoid drift with the VM’s reader.

vm/src/builtins/code.rs (4)

42-96: Minor: simplify NO_LINE detection.

Instead of (byte >> 3) == 0x1f, use the already-extracted code and compare to PyCodeLocationInfoKind::None for clarity.

-    fn is_no_line_marker(byte: u8) -> bool {
-        (byte >> 3) == 0x1f
-    }
+    fn is_no_line_marker(code: u8) -> bool {
+        matches!(PyCodeLocationInfoKind::from_code(code), Some(PyCodeLocationInfoKind::None))
+    }
@@
-        if Self::is_no_line_marker(first_byte) {
+        if Self::is_no_line_marker(code) {

---

486-496: co_linetable/co_exceptiontable OK — zero‑copy not required but fine.

Returning a copy is acceptable; future optimization could expose a memoryview.

---

786-795: co_nlocals validation guard is good; mirror CPython error text.

Message is clear; consider matching CPython’s exact wording if compatibility is a goal.

---

860-921: Reader mirrors writer; keep implementations in sync.

read_varint/read_signed_varint match the encoder. Consider centralizing with the encoder to avoid divergence.

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

compiler/codegen/src/ir.rs (1)

441-449: One‑line encoding must allow full 0–255 column range (not <128).

CPython’s OneLine{0,1,2} store start/end columns as raw u8. Restricting to < 128 bloats tables and diverges from CPython.

Apply:

-                } else if col < 128 && end_col < 128 {
+                } else if (0..=255).contains(&col) && (0..=255).contains(&end_col) {
...
-                if col < 128 && end_col < 128 {
+                if (0..=255).contains(&col) && (0..=255).contains(&end_col) {

Also applies to: 466-471

vm/src/builtins/code.rs (1)

759-767: Don’t silently ignore user‑provided co_code. Return a clear error until parsing is implemented.

Silently reusing existing instructions is surprising and user‑visible.

-        let instructions = match args.co_code {
-            OptionalArg::Present(_code_bytes) => {
-                // Convert bytes back to instructions
-                // For now, keep the original instructions
-                // TODO: Properly parse bytecode from bytes
-                self.code.instructions.clone()
-            }
-            OptionalArg::Missing => self.code.instructions.clone(),
-        };
+        let instructions = match args.co_code {
+            OptionalArg::Present(_code_bytes) => {
+                return Err(vm.new_not_implemented_error(
+                    "co_code replacement is not yet supported".to_string(),
+                ));
+            }
+            OptionalArg::Missing => self.code.instructions.clone(),
+        };

compiler/codegen/src/ir.rs (3)

398-405: Consider emitting NoColumns/None when appropriate to match CPython semantics.

Current encoder never uses NoColumns (13) or None/NO_LINE (15). If any instruction has “no position” or unknown columns, encoding those forms improves fidelity and downstream iteration. Not blocking, but worth tracking.

---

225-227: Exception table placeholder.

Leaving exceptiontable empty is fine short‑term; please gate consumers accordingly and add a TODO reference/issue so this isn’t forgotten.

I can open an issue with a checklist and CPython references if helpful.

---

506-514: Varint helpers: mirror reader/writer invariants.

Writers look correct; add brief asserts to guarantee payload bytes never set bit7 (MSB) to prevent accidental header confusion if code changes.

 fn write_varint(buf: &mut Vec<u8>, mut val: u32) -> usize {
     let start_len = buf.len();
     while val >= 64 {
-        buf.push(0x40 | (val & 0x3f) as u8);
+        let b = 0x40 | (val & 0x3f) as u8;
+        debug_assert_eq!(b & 0x80, 0);
+        buf.push(b);
         val >>= 6;
     }
-    buf.push(val as u8);
+    let b = val as u8;
+    debug_assert_eq!(b & 0x80, 0);
+    buf.push(b);
     buf.len() - start_len
 }

Also applies to: 518-527

vm/src/builtins/code.rs (4)

796-807: Validate replaced tables.

When co_linetable/co_exceptiontable are provided, decoding should succeed and cover the instruction stream. Consider verifying that linetable iteration spans the full code length (sum of lengths equals instructions.len()*2) and erroring otherwise.

---

42-46: Make NO_LINE detection explicit.

(byte >> 3) == 0x1f works but is opaque. Prefer extracting code = (first_byte >> 3) & 0x0f and comparing to 15 for readability.

-    fn is_no_line_marker(byte: u8) -> bool {
-        (byte >> 3) == 0x1f
-    }
+    fn is_no_line_marker(byte: u8) -> bool {
+        ((byte >> 3) & 0x0f) == 0x0f
+    }

Also applies to: 62-64

---

498-569: co_lines builds a full Vec; consider lazy iterator.

Non‑blocking. A generator/iterator like CPython’s lineiterator avoids allocating for large functions.

---

889-917: Reader matches encoder; add minimal sanity checks.

As a guard against malformed inputs, add limits to varint loop iterations (e.g., cap at 6 chunks => 36 bits) and early‑exit on EOF to avoid O(n²) in degenerate data.

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro