instructions #6829
instructions #6829
Conversation
📝 WalkthroughWalkthroughThis PR aligns bytecode and VM semantics with CPython 3.14 (END_FOR/POP_ITER, JumpNoInterrupt), adds LoadSmallInt optimization, adjusts async function scope emission (RETURN_GENERATOR + POP_TOP before RESUME), and propagates async scope information through the symbol table; changes touch codegen, IR, compiler-core opcodes, VM frame, and JIT. Changes
Sequence Diagram(s)sequenceDiagram
participant Parser
participant SymbolTable
participant CodeGen
participant IR
participant VM
Parser->>SymbolTable: FunctionDef(is_async=true)
SymbolTable->>SymbolTable: enter_scope_with_parameters(is_async=true)
SymbolTable-->>Parser: AsyncFunction scope created
Parser->>CodeGen: compile AsyncFunction
CodeGen->>CodeGen: emit RETURN_GENERATOR
CodeGen->>CodeGen: emit POP_TOP
CodeGen->>CodeGen: emit RESUME
CodeGen->>IR: emit pseudo-instructions (JumpNoInterrupt, END_FOR, POP_ITER)
IR->>IR: preprocess Jump/JumpNoInterrupt
IR->>CodeGen: lower JumpNoInterrupt → JumpBackwardNoInterrupt (when backward)
CodeGen-->>VM: bytecode with END_FOR/POP_ITER/LoadSmallInt
VM->>VM: execute ReturnGenerator -> push None
VM->>VM: execute RESUME
VM->>VM: FOR_ITER/END_FOR handling, on StopIteration skip to POP_ITER
sequenceDiagram
participant Compiler
participant InstructionEmit
participant Frame
Compiler->>InstructionEmit: detect small int (0-255)
InstructionEmit->>InstructionEmit: emit `LoadSmallInt` (index)
InstructionEmit->>Frame: include constant in co_consts (compat)
Frame->>Frame: execute LoadSmallInt
Frame->>Frame: convert idx -> i64 and push small int directly
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
📦 Library DependenciesThe following Lib/ modules were modified. Here are their dependencies: Click to expand dependency informationLegend:
|
12c443f to
9b1d6d6
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/codegen/src/symboltable.rs (1)
2096-2172: Ensure nested-scope detection treatsAsyncFunctionas a function.Now that
AsyncFunctionis a distinct scope,enter_scopestill only treatsCompilerScope::Functionas “nested.” That will misclassify nested defs inside async functions. Please update the nesting predicate.🐛 Suggested fix in
enter_scope- .map(|table| table.is_nested || table.typ == CompilerScope::Function) + .map(|table| { + table.is_nested + || matches!(table.typ, CompilerScope::Function | CompilerScope::AsyncFunction) + })
🤖 Fix all issues with AI agents
In `@crates/compiler-core/src/bytecode/instruction.rs`:
- Around line 509-514: The ForIter arm in Instruction::stack_effect currently
ignores the jump parameter and always returns 1; update the ForIter handling in
crates/compiler-core/src/bytecode/instruction.rs (the ForIter match arm inside
the stack_effect implementation for Instruction) to return 1 when jump is false
and 0 when jump is true so it matches CPython 3.14 semantics (jump=False => +1,
jump=True => 0); ensure the change preserves the existing comment/context about
CPython behavior and does not affect other Instruction arms.
In `@crates/vm/src/frame.rs`:
- Around line 2724-2746: The StopIteration handling in the match arm for
bytecode::IntrinsicFunction1::StopIterationError currently uses
arg.class().is(vm.ctx.exceptions.stop_iteration) which only matches the exact
StopIteration type; change this to use
arg.fast_isinstance(vm.ctx.exceptions.stop_iteration) so subclasses of
StopIteration are also treated as StopIteration, keeping the same behavior for
converting to vm.new_runtime_error("coroutine raised StopIteration") and
preserving the existing else branch that re-raises via arg.downcast().
🧹 Nitpick comments (1)
crates/vm/src/frame.rs (1)
2313-2327: Guard the END_FOR skip with a POP_ITER check.Skipping to
target + 1assumes a valid POP_ITER follows END_FOR. Consider guarding to avoid out-of-bounds or unexpected bytecode layouts.💡 Suggested defensive guard
- let jump_target = if let Some(unit) = self.code.instructions.get(target_idx) { - if matches!(unit.op, bytecode::Instruction::EndFor) { - // Skip END_FOR, jump to next instruction - bytecode::Label(target.0 + 1) - } else { - // Legacy pattern: jump directly to target (POP_TOP/POP_ITER) - target - } - } else { - target - }; + let jump_target = if let Some(unit) = self.code.instructions.get(target_idx) { + if matches!(unit.op, bytecode::Instruction::EndFor) + && matches!( + self.code.instructions.get(target_idx + 1).map(|u| &u.op), + Some(bytecode::Instruction::PopIter) + ) + { + // Skip END_FOR, jump to POP_ITER + bytecode::Label(target.0 + 1) + } else { + // Legacy pattern: jump directly to target (POP_TOP/POP_ITER) + target + } + } else { + target + };
| Self::ForIter { .. } => { | ||
| // CPython 3.14 pattern: always +1 for stack analysis | ||
| // jump=False: push next value (+1) | ||
| // jump=True: iterator stays on stack, no change (0) | ||
| if jump { 0 } else { 1 } | ||
| // jump=True: conceptually +1 so END_FOR(-1) + POP_ITER(-1) = -1 total | ||
| // At runtime, FOR_ITER skips END_FOR and jumps directly to POP_ITER | ||
| 1 |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
CPython 3.14 FOR_ITER stack_effect jump parameter dis module
💡 Result:
Short answer: dis.stack_effect(opcode.FOR_ITER, oparg, jump=...) — jump=True = jump taken (iterator exhausted) → stack effect 0; jump=False = fall-through (next value produced) → stack effect +1; jump=None (default) → returns the maximal effect (+1). See the dis docs. [1][2]
Sources:
- dis.stack_effect documentation. [1]
- FOR_ITER opcode description (notes on popping behavior and changes across versions). [2]
🌐 Web query:
CPython 3.14 FOR_ITER opcode behavior stack semantics
💡 Result:
Short answer — semantics in CPython 3.14:
- Input stack before FOR_ITER: [..., iter] (iterator object at TOS).
- Execution:
- If next(iter) yields a value: FOR_ITER pushes that value, leaving the iterator beneath it. Resulting stack: [..., iter, value].
- If iterator is exhausted (StopIteration): FOR_ITER removes the iterator from the stack (pop) and jumps forward by the instruction’s oparg (no value pushed).
- So the two possible “after” states are [..., iter, value] (loop body runs) or [...] (loop exit — iterator removed and jump). This is the traditional FOR_ITER behaviour used by for-loops and comprehensions; 3.14 continues that semantics (with related bytecode/iterator machinery changes such as adding POP_ITER/virtual iterators). (stackoverflow.com)
If you want the exact C implementation lines in ceval.c for 3.14 I can fetch and cite them.
Citations:
🏁 Script executed:
cd crates/compiler-core && find . -type f -name "*.rs" | head -20Repository: RustPython/RustPython
Length of output: 212
🏁 Script executed:
grep -r "FOR_ITER\|ForIter" crates/compiler-core/src/bytecode/ -n | head -30Repository: RustPython/RustPython
Length of output: 1596
🏁 Script executed:
cat -n crates/compiler-core/src/bytecode/instruction.rs | sed -n '509,514p'Repository: RustPython/RustPython
Length of output: 456
🏁 Script executed:
cat -n crates/compiler-core/src/bytecode/instruction.rs | sed -n '440,520p'Repository: RustPython/RustPython
Length of output: 4188
🏁 Script executed:
grep -r "stack_effect" crates/compiler-core/src/ -n | grep -v "test" | head -20Repository: RustPython/RustPython
Length of output: 784
🏁 Script executed:
grep -r "END_FOR\|POP_ITER" crates/compiler-core/src/bytecode/instruction.rs -nRepository: RustPython/RustPython
Length of output: 336
🏁 Script executed:
cat -n crates/compiler-core/src/bytecode/instruction.rs | sed -n '575,610p'Repository: RustPython/RustPython
Length of output: 2078
🏁 Script executed:
grep -rn "\.stack_effect\|stack_effect(" crates/compiler-core/src/ --include="*.rs" | grep -v "fn stack_effect" | head -30Repository: RustPython/RustPython
Length of output: 188
🏁 Script executed:
grep -rn "stack_effect" crates/compiler/ --include="*.rs" | grep -v "test\|compiler-core" | head -20Repository: RustPython/RustPython
Length of output: 47
🏁 Script executed:
grep -rn "stack_effect" crates/ --include="*.rs" | grep -v "fn stack_effect\|test\|//.*stack_effect" | head -40Repository: RustPython/RustPython
Length of output: 565
🏁 Script executed:
cat -n crates/compiler-core/src/bytecode/instruction.rs | sed -n '1165,1175p'Repository: RustPython/RustPython
Length of output: 581
🏁 Script executed:
git log --oneline -20 -- crates/compiler-core/src/bytecode/instruction.rsRepository: RustPython/RustPython
Length of output: 120
🏁 Script executed:
cat -n crates/codegen/src/ir.rs | sed -n '475,510p'Repository: RustPython/RustPython
Length of output: 1912
🏁 Script executed:
cat -n crates/stdlib/src/opcode.rs | sed -n '185,200p'Repository: RustPython/RustPython
Length of output: 591
🏁 Script executed:
cat -n crates/codegen/src/ir.rs | sed -n '500,515p'Repository: RustPython/RustPython
Length of output: 901
🏁 Script executed:
grep -A 20 "Self::ForIter" crates/codegen/src/ir.rs | head -30Repository: RustPython/RustPython
Length of output: 47
🏁 Script executed:
cd crates/compiler-core && cat -n src/bytecode/instruction.rs | sed -n '602,603p'Repository: RustPython/RustPython
Length of output: 191
🏁 Script executed:
cat -n crates/compiler-core/src/bytecode/instruction.rs | sed -n '602,602p'Repository: RustPython/RustPython
Length of output: 146
Respect the jump parameter in ForIter stack_effect to match CPython 3.14 semantics.
The jump parameter is actively used by codegen (lines 480 and 505 in crates/codegen/src/ir.rs) to calculate correct stack depths for fall-through vs. jump paths, but the ForIter implementation ignores it and unconditionally returns +1. This causes incorrect stack depth analysis for the jump case.
In CPython 3.14, FOR_ITER stack effects are:
jump=False(next value produced):+1jump=True(iterator exhausted, jumps):0
When the jump is taken, the iterator is popped and execution jumps away; there is no net stack change. The implementation should return 0 when jump=True and 1 when jump=False.
🤖 Prompt for AI Agents
In `@crates/compiler-core/src/bytecode/instruction.rs` around lines 509 - 514, The
ForIter arm in Instruction::stack_effect currently ignores the jump parameter
and always returns 1; update the ForIter handling in
crates/compiler-core/src/bytecode/instruction.rs (the ForIter match arm inside
the stack_effect implementation for Instruction) to return 1 when jump is false
and 0 when jump is true so it matches CPython 3.14 semantics (jump=False => +1,
jump=True => 0); ensure the change preserves the existing comment/context about
CPython behavior and does not affect other Instruction arms.
Summary by CodeRabbit
Performance Improvements
Async Improvements
Bug Fixes
Compatibility
✏️ Tip: You can customize this high-level summary in your review settings.