Actionable comments posted: 0

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

693-795: Make the borrow optimization conservative on stack underflow and method loads.
The underflow path says “don’t optimize this block,” but the loop continues, so the analysis still runs and may convert unsafely. Also, LOAD_ATTR with method-flag pushes two values; treating it as (1,1) undercounts pushes and can mis-mark LOAD_FAST as fully consumed. Consider using AnyInstruction::stack_effect for pseudo ops and bailing out on underflow, plus a precise LOAD_ATTR case.

🔧 Suggested change

-            for (i, info) in block.instructions.iter().enumerate() {
-                let Some(instr) = info.instr.real() else {
-                    continue;
-                };
-
-                // Get stack effect
-                let effect = instr.stack_effect(info.arg);
+            let mut valid_block = true;
+            for (i, info) in block.instructions.iter().enumerate() {
+                let instr_real = info.instr.real();
+
+                // Get stack effect (includes pseudo ops)
+                let effect = info.instr.stack_effect(info.arg);
                 let num_popped = if effect < 0 { (-effect) as usize } else { 0 };
                 let num_pushed = if effect > 0 { effect as usize } else { 0 };
 
                 // More precise: calculate actual pops and pushes
                 // For most instructions: pops = max(0, -effect), pushes = max(0, effect)
                 // But some instructions have both pops and pushes
-                let (pops, pushes) = match instr {
+                let (pops, pushes) = match instr_real {
+                    Some(Instruction::LoadAttr { idx }) => {
+                        let (_, is_method) = bytecode::decode_load_attr_arg(idx.get(info.arg));
+                        if is_method { (1, 2) } else { (1, 1) }
+                    }
                     // Instructions that both pop and push
-                    Instruction::BinaryOp { .. } => (2, 1),
-                    Instruction::CompareOp { .. } => (2, 1),
-                    Instruction::ContainsOp(_) => (2, 1),
-                    Instruction::IsOp(_) => (2, 1),
-                    Instruction::UnaryInvert
-                    | Instruction::UnaryNegative
-                    | Instruction::UnaryNot
-                    | Instruction::ToBool => (1, 1),
-                    Instruction::GetIter | Instruction::GetAIter => (1, 1),
-                    Instruction::LoadAttr { .. } => (1, 1), // simplified
-                    Instruction::Call { nargs } => (nargs.get(info.arg) as usize + 2, 1),
-                    Instruction::CallKw { nargs } => (nargs.get(info.arg) as usize + 3, 1),
+                    Some(Instruction::BinaryOp { .. }) => (2, 1),
+                    Some(Instruction::CompareOp { .. }) => (2, 1),
+                    Some(Instruction::ContainsOp(_)) => (2, 1),
+                    Some(Instruction::IsOp(_)) => (2, 1),
+                    Some(Instruction::UnaryInvert)
+                    | Some(Instruction::UnaryNegative)
+                    | Some(Instruction::UnaryNot)
+                    | Some(Instruction::ToBool) => (1, 1),
+                    Some(Instruction::GetIter) | Some(Instruction::GetAIter) => (1, 1),
+                    Some(Instruction::Call { nargs }) => (nargs.get(info.arg) as usize + 2, 1),
+                    Some(Instruction::CallKw { nargs }) => (nargs.get(info.arg) as usize + 3, 1),
                     // Use stack effect for others
                     _ => (num_popped, num_pushed),
                 };
 
                 // Pop values from stack
                 for _ in 0..pops {
                     if stack.pop().is_none() {
                         // Stack underflow in simulation - block receives values from elsewhere
                         // Conservative: don't optimize this block
-                        break;
+                        valid_block = false;
+                        break;
                     }
                 }
+                if !valid_block {
+                    break;
+                }
 
                 // Push values to stack with source instruction index
-                let source = match instr {
-                    Instruction::LoadFast(_) | Instruction::LoadFastLoadFast { .. } => i,
+                let source = match instr_real {
+                    Some(Instruction::LoadFast(_))
+                    | Some(Instruction::LoadFastLoadFast { .. }) => i,
                     _ => NOT_LOCAL,
                 };
                 for _ in 0..pushes {
                     stack.push(source);
                 }
             }
 
+            if !valid_block {
+                continue;
+            }
             // Mark instructions whose values remain on stack at block end
             for &src in &stack {
                 if src != NOT_LOCAL {
                     unconsumed[src] = true;
                 }
             }

Actionable comments posted: 3

In `@crates/codegen/src/ir.rs`:
- Around line 731-747: The current special-casing in the match over instr (the
Instruction enum) is incomplete and can misinterpret the stack delta because
num_popped/num_pushed fallback is ambiguous; update the logic in this match to
be conservative: either enumerate and correctly handle all Instruction variants
with non-trivial pop/push behavior (e.g., BinaryOpInplaceAddUnicode,
LoadFromDictOrDeref, LoadFromDictOrGlobals, Swap, Copy, ForIter, all Build*
instructions) by returning explicit (pops, pushes) tuples, or change the default
branch to return (0, 0) for unknown/complex instructions to avoid incorrect
assumptions; adjust the Call/CallKw handling to still use nargs.get(info.arg)
and keep using info.arg where needed, and add a short comment above the match
noting the conservative assumption so future readers of ir.rs and the match over
instr know why unknown cases return (0,0).

In `@extra_tests/snippets/stdlib_io.py`:
- Around line 17-21: The assertions inside the FileIO context are checking the
wrong variable: change the three assertions that reference result to use the
local variable res (read via FileIO.read()) so they validate the bytes returned
by fio.read(); update the checks near the FileIO block (the assertions currently
using result) to assert len(res) <= 16 * 1024, len(res) >= 0, and
isinstance(res, bytes) respectively.

crates/vm/src/frame.rs (1)

1341-1391: Consider extracting common error-handling logic.

The LoadFastBorrow and LoadFastBorrowLoadFastBorrow handlers duplicate the unbound-local error creation pattern from LoadFast and LoadFastLoadFast. While the TODO comments indicate future divergence for the actual load semantics, the error-handling logic will likely remain the same.

You could extract a helper to reduce duplication:

fn unbound_local_error(varname: &'static PyStrInterned, vm: &VirtualMachine) -> PyBaseExceptionRef {
    vm.new_exception_msg(
        vm.ctx.exceptions.unbound_local_error.to_owned(),
        format!("local variable '{varname}' referenced before assignment"),
    )
}

This is optional since the current implementation is correct and the duplication may be intentional for future optimization work.

Actionable comments posted: 1

In `@crates/codegen/src/ir.rs`:
- Around line 758-760: The stack-effect mapping for Instruction::LoadAttr is
wrong for method-form attribute access: update the match arm handling
Instruction::LoadAttr in the stack-effect calculation in ir.rs so that it
returns (1, 2) when the LoadAttr instance has its method flag set (pop 1, push
method+self_or_null = 2) and remains (1, 1) otherwise; locate the match on
Instruction::LoadAttr and branch on the method flag to adjust the push count to
avoid underestimating the simulated stack and unsafe LOAD_FAST_BORROW
conversions.

.cspell.dict/rust-more.txt (1)

92-93: bitvec/Bitvec entries break alphabetical ordering.

The dictionary appears to be alphabetically sorted, but bitvec and Bitvec are appended at the end after winsock. They should be placed between bitor and bstr to maintain consistency.

📋 Suggested placement

 bitflags
 bitor
+bitvec
+Bitvec
 bitxor
 bstr

And remove from end of file:

 winresource
 winsock
-bitvec
-Bitvec

Actionable comments posted: 1

In `@crates/codegen/src/ir.rs`:
- Around line 724-727: The borrow-analysis in optimize_load_fast_borrow
currently ignores pseudo-ops by using info.instr.real() and continuing, which
can mis-model the stack (pseudo ops like LoadClosure/StoreFastMaybeNull affect
push/pop); change the logic to conservatively bail out for the whole block when
a pseudo-op is encountered: inside the loop over block.instructions detect when
info.instr.real() is None and skip further processing of that block (e.g., set a
flag and break/continue to the next block) so the pass does not make unsafe
assumptions about LOAD_FAST liveness, or alternatively move
optimize_load_fast_borrow to run after pseudo lowering if feasible.

crates/vm/src/frame.rs (1)

1341-1391: Consider tracking the borrow-optimization TODO.

If this is intentionally a placeholder, linking it to an issue will make the follow‑up easier. I can help draft the issue or a concrete implementation plan if you want.