Add InstructionMetadata::stack_effect_jump for branch stack effects#7585
Add InstructionMetadata::stack_effect_jump for branch stack effects#7585youknowone merged 2 commits intoRustPython:mainfrom
Conversation
CPython's compile.c provides stack_effect(opcode, oparg, jump) where the jump parameter selects between fallthrough and branch effects. The existing stack_effect() only returns the fallthrough effect. Add stack_effect_jump() that returns the branch effect. Most instructions have identical fallthrough/branch effects; ForIter and Send are the exceptions (ForIter: fallthrough=+1, branch=-1; Send: fallthrough=0, branch=-1).
📝 WalkthroughWalkthroughSimplifies stack-depth propagation in codegen to use a new branch-aware stack-effect API. Adds Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/compiler-core/src/bytecode/instruction.rs`:
- Around line 1716-1724: Implement branch-sensitive stack effects by overriding
stack_effect_jump for the opcodes ForIter and Send: in the Instruction trait's
stack_effect_jump implementation, match on self (or the instruction variant) and
return -1 for ForIter and -1 for Send (regardless of oparg), otherwise fall back
to calling self.stack_effect(oparg); this ensures branch edges use the correct
values used by max_stackdepth and the exposed stack_effect(jump=True). Also
update the unit test expectation in test__opcode.py to assert
stack_effect(FOR_ITER, 0, jump=True) == -1 instead of 1.
In `@crates/stdlib/src/_opcode.rs`:
- Around line 209-213: The match on the Option jump currently treats None as the
taken-branch only; change handling so None returns the maximum of both paths:
compute both opcode.stack_effect(oparg) and opcode.stack_effect_jump(oparg) and
use the larger value when jump is None, while keeping Some(false) ->
opcode.stack_effect(oparg) and Some(true) -> opcode.stack_effect_jump(oparg);
update the match using the existing jump variable and the opcode.stack_effect /
opcode.stack_effect_jump calls to locate the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 9633aeda-1e39-410c-879f-d780a6209f1d
📒 Files selected for processing (3)
crates/codegen/src/ir.rscrates/compiler-core/src/bytecode/instruction.rscrates/stdlib/src/_opcode.rs
| /// Stack effect when the instruction takes its branch (jump=true). | ||
| /// | ||
| /// CPython equivalent: `stack_effect(opcode, oparg, jump=True)`. | ||
| /// For most instructions this equals the fallthrough effect. | ||
| /// Override for instructions where branch and fallthrough differ | ||
| /// (e.g. `FOR_ITER`: fallthrough = +1, branch = −1). | ||
| fn stack_effect_jump(&self, oparg: u32) -> i32 { | ||
| self.stack_effect(oparg) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check current state of instruction.rs implementation
fd -t f instruction.rs | head -5Repository: RustPython/RustPython
Length of output: 113
🏁 Script executed:
# Check the full context around line 1716-1724
head -1750 crates/compiler-core/src/bytecode/instruction.rs | tail -60Repository: RustPython/RustPython
Length of output: 1824
🏁 Script executed:
# Search for all stack_effect_jump implementations in the codebase
rg "stack_effect_jump" -B 2 -A 5Repository: RustPython/RustPython
Length of output: 3852
🏁 Script executed:
# Check what branch-sensitive opcodes exist in Instruction enum
rg "enum Instruction" -A 200 crates/compiler-core/src/bytecode/instruction.rs | head -250Repository: RustPython/RustPython
Length of output: 4520
🏁 Script executed:
# Search for ForIter stack_effect implementation
rg "ForIter" -B 3 -A 8 crates/compiler-core/src/bytecode/instruction.rs | grep -A 20 "stack_effect"Repository: RustPython/RustPython
Length of output: 47
🏁 Script executed:
# Look for all branch target instructions (those with Label arguments)
rg "Arg<Label>" crates/compiler-core/src/bytecode/instruction.rsRepository: RustPython/RustPython
Length of output: 822
🏁 Script executed:
# Check the CPython bytecode documentation for FOR_ITER
web_search
FOR_ITER CPython 3.14 stack effect branch exhaustedRepository: RustPython/RustPython
Length of output: 162
🏁 Script executed:
# Search for stack_effect_info implementations to understand current semantics
rg "fn stack_effect_info" -B 2 -A 15 crates/compiler-core/src/bytecode/instruction.rs | head -100Repository: RustPython/RustPython
Length of output: 3107
🏁 Script executed:
# Search specifically for ForIter stack effect
rg "ForIter" -A 2 crates/compiler-core/src/bytecode/instruction.rs | grep -A 50 "match self"Repository: RustPython/RustPython
Length of output: 47
🏁 Script executed:
# Get a larger context of the stack_effect_info match statement to find ForIter
sed -n '/fn stack_effect_info/,/^ }/p' crates/compiler-core/src/bytecode/instruction.rs | grep -A 2 "ForIter"Repository: RustPython/RustPython
Length of output: 518
🏁 Script executed:
# Check complete Instruction stack_effect_info implementation
sed -n '1200,1650p' crates/compiler-core/src/bytecode/instruction.rs | grep -B 5 -A 5 "ForIter"Repository: RustPython/RustPython
Length of output: 399
🌐 Web query:
CPython 3.14 FOR_ITER stack effect jump branch exhausted bytecode semantics
💡 Result:
In CPython 3.14, the FOR_ITER bytecode instruction handles iteration in for loops. It assumes an iterator on top of the stack (STACK[-1]). Stack effect: - If the iterator's next succeeds (yields a value): pushes the value on the stack, resulting in [..., iterator, value]. Stack +1. - If the iterator is exhausted (raises StopIteration): pops the iterator (STACK_SHRINK(1); Py_DECREF(iter);), jumps forward by delta bytes (JUMPBY(oparg)), leaving stack as before FOR_ITER. Stack -1, with branch taken. This behavior changed in Python 3.12: prior to 3.12, FOR_ITER always popped the iterator on exhaustion. From 3.12 (including 3.14), it does not pop on exhaustion in the non-branch case, but the implementation still pops it explicitly when jumping (exhausted branch). In 3.14, POP_ITER is added as a separate cleanup instruction (pops iterator), and NOT_TAKEN for monitoring, but core FOR_ITER semantics match 3.12+ docs. No major changes to FOR_ITER semantics in 3.14 per official docs or changelogs; docs note some 3.14 opcodes like POP_ITER are added but FOR_ITER description unchanged from 3.12. To compute exact stack effects programmatically: dis.stack_effect(FOR_ITER, oparg=..., jump=True) for branch (exhausted, -1); jump=False for value case (+1); jump=None for max (+1). Example for loop bytecode: GET_ITER >> FOR_ITER delta (to end) STORE_FAST (loop var) ... body ... JUMP_BACKWARD (to FOR_ITER) END_FOR or POP_ITER (cleanup, pops iterator)
Citations:
- 1: https://docs.python.org/3.14/library/dis.html
- 2: https://docs.python.org/3/library/dis.html
- 3: https://github.com/python/cpython/blob/main/Python/bytecodes.c
- 4: dis: FOR_ITER says it no longer pops the stack in 3.12 but it still does when the iterator ended normally python/cpython#121399
🏁 Script executed:
# Search for all match arms in stack_effect_info for instructions with Label/conditional behavior
sed -n '/fn stack_effect_info/,/^ }/p' crates/compiler-core/src/bytecode/instruction.rs | grep -E "(Instrumented|Jump|Iter|Await|Except)" | head -30Repository: RustPython/RustPython
Length of output: 1559
🏁 Script executed:
# Check the test file to see if there are any test cases for stack_effect_jump with FOR_ITER
find . -name "*.py" -o -name "*.rs" | xargs grep -l "test_stack_effect_jump" 2>/dev/nullRepository: RustPython/RustPython
Length of output: 91
🏁 Script executed:
# Look for InstrumentedForIter definition and understand it
rg "InstrumentedForIter" -B 2 -A 5 crates/compiler-core/src/bytecode/instruction.rsRepository: RustPython/RustPython
Length of output: 1650
🏁 Script executed:
# Check if there are any other instructions documented as having different branch/fallthrough effects
rg "jump=True" -B 3 -A 3 crates/compiler-core/src/bytecode/instruction.rsRepository: RustPython/RustPython
Length of output: 409
🏁 Script executed:
# Check how Instruction is used in the match statement - does it include InstrumentedForIter?
rg "InstrumentedForIter" crates/compiler-core/src/bytecode/instruction.rs | head -20Repository: RustPython/RustPython
Length of output: 264
🏁 Script executed:
# Look at the Instruction enum definition to understand the hierarchy
sed -n '/pub enum Instruction/,/^}/p' crates/compiler-core/src/bytecode/instruction.rs | grep -E "(InstrumentedForIter|ForIter)" -B 1 -A 1Repository: RustPython/RustPython
Length of output: 608
🏁 Script executed:
# Check if there's a deoptimize() method mentioned in the proposed fix
rg "fn deoptimize" -B 2 -A 10 crates/compiler-core/src/bytecode/instruction.rsRepository: RustPython/RustPython
Length of output: 484
🏁 Script executed:
# Verify the actual test expectations in test__opcode.py
cat Lib/test/test__opcode.py | grep -A 20 "def test_stack_effect_jump"Repository: RustPython/RustPython
Length of output: 1137
🏁 Script executed:
# Let me check the CPython version more carefully
grep -r "3.14\|3.13" crates/compiler-core/src/bytecode/instruction.rs | head -5Repository: RustPython/RustPython
Length of output: 336
🏁 Script executed:
# Check the full test_stack_effect_jump function to understand better
sed -n '/def test_stack_effect_jump/,/def test_/p' Lib/test/test__opcode.py | head -60Repository: RustPython/RustPython
Length of output: 1673
🏁 Script executed:
# Check CPython reference - let's look for any documentation in the codebase
rg "stack_effect.*jump.*FOR_ITER\|FOR_ITER.*jump.*-1" -iRepository: RustPython/RustPython
Length of output: 47
🏁 Script executed:
# Get more context on the semantics - check what branch (jump=True) actually means in CPython
python3 << 'EOF'
import dis
# Check FOR_ITER semantics
FOR_ITER = dis.opmap.get('FOR_ITER')
if FOR_ITER:
print(f"FOR_ITER opcode: {FOR_ITER}")
print(f"stack_effect(FOR_ITER, 0): {dis.stack_effect(FOR_ITER, 0)}")
print(f"stack_effect(FOR_ITER, 0, jump=True): {dis.stack_effect(FOR_ITER, 0, jump=True)}")
print(f"stack_effect(FOR_ITER, 0, jump=False): {dis.stack_effect(FOR_ITER, 0, jump=False)}")
else:
print("FOR_ITER not found")
EOFRepository: RustPython/RustPython
Length of output: 195
🏁 Script executed:
# Check if RustPython's test expectations are correct - this is critical
cat Lib/test/test__opcode.py | grep -B 5 -A 8 "def test_stack_effect_jump"Repository: RustPython/RustPython
Length of output: 823
🏁 Script executed:
# Now check what other opcodes might have branch-sensitive semantics in CPython 3.14
python3 << 'EOF'
import dis
import opcode
# Get all opcodes
for name in sorted(dis.opmap.keys()):
code = dis.opmap[name]
# Skip instrumented opcodes
if code >= opcode.MIN_INSTRUMENTED_OPCODE:
continue
try:
if code not in dis.hasarg:
jump_true = dis.stack_effect(code, jump=True)
jump_false = dis.stack_effect(code, jump=False)
else:
jump_true = dis.stack_effect(code, 0, jump=True)
jump_false = dis.stack_effect(code, 0, jump=False)
# Check if this is a branch-sensitive opcode (jump effects differ)
if jump_true != jump_false:
print(f"{name:30} jump=True: {jump_true:3}, jump=False: {jump_false:3}")
except:
pass
EOFRepository: RustPython/RustPython
Length of output: 212
🏁 Script executed:
# Check the current state of the Instruction match in stack_effect_jump - does it have any overrides?
sed -n '/impl InstructionMetadata for Instruction/,/^}/p' crates/compiler-core/src/bytecode/instruction.rs | grep -A 10 "fn stack_effect_jump"Repository: RustPython/RustPython
Length of output: 47
🏁 Script executed:
# Let me try a simpler approach to find all branch-sensitive opcodes
python3 << 'EOF'
import dis
# Get all opcodes and check which ones have different jump effects
branch_sensitive = []
for name in sorted(dis.opmap.keys()):
code = dis.opmap[name]
try:
if code not in dis.hasarg:
jump_true = dis.stack_effect(code, jump=True)
jump_false = dis.stack_effect(code, jump=False)
else:
jump_true = dis.stack_effect(code, 0, jump=True)
jump_false = dis.stack_effect(code, 0, jump=False)
# Check if this is a branch-sensitive opcode (jump effects differ)
if jump_true != jump_false:
branch_sensitive.append((name, jump_true, jump_false))
except:
pass
print("Branch-sensitive opcodes (jump=True vs jump=False differ):")
for name, jump_true, jump_false in branch_sensitive:
print(f" {name:30} jump=True: {jump_true:3}, jump=False: {jump_false:3}")
EOFRepository: RustPython/RustPython
Length of output: 123
🏁 Script executed:
# Now let's verify the current state - does Instruction override stack_effect_jump?
rg "impl InstructionMetadata for Instruction" -A 50 crates/compiler-core/src/bytecode/instruction.rs | head -60Repository: RustPython/RustPython
Length of output: 2032
🏁 Script executed:
# Double-check what the review test expectations actually are in RustPython
grep -A 5 "FOR_ITER.*jump=True" Lib/test/test__opcode.pyRepository: RustPython/RustPython
Length of output: 446
🏁 Script executed:
# This is critical - let me verify the exact CPython version RustPython is targeting
grep -i "cpython.*3\." crates/compiler-core/src/bytecode/instruction.rs | head -3Repository: RustPython/RustPython
Length of output: 253
🏁 Script executed:
# Check what version of Python is being used in the test environment
python3 --versionRepository: RustPython/RustPython
Length of output: 78
🏁 Script executed:
# The critical finding: verify CPython 3.14 FOR_ITER semantics one more time with explicit checks
python3 << 'EOF'
import sys
import dis
print(f"Python version: {sys.version}")
print()
# Check FOR_ITER explicitly
FOR_ITER = dis.opmap.get('FOR_ITER')
if FOR_ITER is not None:
print(f"FOR_ITER opcode number: {FOR_ITER}")
try:
effect_default = dis.stack_effect(FOR_ITER, 0)
effect_jump_true = dis.stack_effect(FOR_ITER, 0, jump=True)
effect_jump_false = dis.stack_effect(FOR_ITER, 0, jump=False)
print(f" stack_effect(FOR_ITER, 0): {effect_default}")
print(f" stack_effect(FOR_ITER, 0, jump=True): {effect_jump_true}")
print(f" stack_effect(FOR_ITER, 0, jump=False): {effect_jump_false}")
print()
if effect_jump_true != effect_jump_false:
print(f" ✓ FOR_ITER is BRANCH-SENSITIVE (jump effects differ)")
else:
print(f" ✗ FOR_ITER is NOT branch-sensitive (jump effects are same)")
except Exception as e:
print(f" Error: {e}")
EOFRepository: RustPython/RustPython
Length of output: 331
🏁 Script executed:
# Check if there's a comment or documentation in RustPython test about what the semantics SHOULD be
grep -B 10 -A 10 "test_stack_effect_jump" Lib/test/test__opcode.py | head -30Repository: RustPython/RustPython
Length of output: 1058
🏁 Script executed:
# This is a critical discovery - let me check if the test in RustPython is known to be wrong
git log --oneline -20 Lib/test/test__opcode.py 2>/dev/null | head -10Repository: RustPython/RustPython
Length of output: 141
🏁 Script executed:
# Check the git history of the test to see if it was recently changed
git log -p --follow -S "stack_effect_jump" -- Lib/test/test__opcode.py 2>/dev/null | head -100Repository: RustPython/RustPython
Length of output: 47
🏁 Script executed:
# Check when this test was added and what it's supposed to test
git log --all --oneline -- Lib/test/test__opcode.py 2>/dev/null | head -5Repository: RustPython/RustPython
Length of output: 308
🏁 Script executed:
# Check the issue - is FOR_ITER truly branch-sensitive in CPython 3.14?
# Let me check the actual CPython bytecodes.c for v3.14.2
web_search
CPython v3.14.2 FOR_ITER bytecode stack effect jump exhaustedRepository: RustPython/RustPython
Length of output: 161
🏁 Script executed:
# Let me examine the commit that added stack_effect_jump to understand the intent
git show 89191b8f --stat 2>/dev/null | head -40Repository: RustPython/RustPython
Length of output: 2282
🏁 Script executed:
# Check if there are any issues or PRs related to this
git log --all --grep="stack_effect_jump\|FOR_ITER" --oneline 2>/dev/null | head -20Repository: RustPython/RustPython
Length of output: 317
🏁 Script executed:
# Most importantly - let's verify the semantics by looking at the actual implementation expectations
# Check what max_stackdepth expects from stack_effect_jump
rg "max_stackdepth\|stack_effect_jump" -B 3 -A 3 crates/codegen/src/ir.rs | head -80Repository: RustPython/RustPython
Length of output: 47
🏁 Script executed:
# Verify the retrieved learning is still relevant and accurate
rg "is_conditional_jump\|ForIter\|NOT_TAKEN" -B 2 -A 5 crates/codegen/src/ir.rs | head -50Repository: RustPython/RustPython
Length of output: 47
🏁 Script executed:
# Now let's see if the actual implementation was done in that commit
git show 89191b8f -- crates/compiler-core/src/bytecode/instruction.rs 2>/dev/null | grep -A 30 "stack_effect_jump"Repository: RustPython/RustPython
Length of output: 3959
🏁 Script executed:
# Check what the commit actually changed for Instruction::stack_effect_jump
git show 89191b8f -- crates/compiler-core/src/bytecode/instruction.rs 2>/dev/null | grep -B 5 -A 20 "impl InstructionMetadata for Instruction"Repository: RustPython/RustPython
Length of output: 839
🏁 Script executed:
# Most importantly - verify if the test was also updated in that commit
git show 89191b8f -- Lib/test/test__opcode.py 2>/dev/null | head -100Repository: RustPython/RustPython
Length of output: 4228
🏁 Script executed:
# This is very revealing - the commit message says ForIter branch=-1, but the impl doesn't override
# Let's check if there's a follow-up commit that implements the overrides
git log --oneline --all 89191b8f..HEAD -- crates/compiler-core/src/bytecode/instruction.rs 2>/dev/null | head -10Repository: RustPython/RustPython
Length of output: 570
🏁 Script executed:
# Check if Instruction has any stack_effect_jump override currently
git show HEAD:crates/compiler-core/src/bytecode/instruction.rs 2>/dev/null | grep -A 30 "impl InstructionMetadata for Instruction" | grep -A 10 "stack_effect_jump"Repository: RustPython/RustPython
Length of output: 47
🏁 Script executed:
# Check what the test file expects right now
git show HEAD:Lib/test/test__opcode.py 2>/dev/null | grep -A 5 "def test_stack_effect_jump"Repository: RustPython/RustPython
Length of output: 380
🏁 Script executed:
# Verify if Send also needs an override
python3 << 'EOF'
import dis
SEND = dis.opmap.get('SEND')
if SEND:
print(f"SEND opcode: {SEND}")
print(f" stack_effect(SEND, 0): {dis.stack_effect(SEND, 0)}")
print(f" stack_effect(SEND, 0, jump=True): {dis.stack_effect(SEND, 0, jump=True)}")
print(f" stack_effect(SEND, 0, jump=False): {dis.stack_effect(SEND, 0, jump=False)}")
else:
print("SEND not found")
EOFRepository: RustPython/RustPython
Length of output: 186
🏁 Script executed:
# Verify the final state - check if there are any recent commits fixing this
git log --oneline -5 2>/dev/nullRepository: RustPython/RustPython
Length of output: 141
🏁 Script executed:
# Final check: verify the Send instruction exists in Instruction enum
rg "Send\s*\{" crates/compiler-core/src/bytecode/instruction.rs | head -3Repository: RustPython/RustPython
Length of output: 149
Override stack_effect_jump() for both ForIter and Send instructions—the only real opcodes with branch-sensitive stack effects.
The trait's default implementation returns the fallthrough effect for all instructions. However, CPython 3.14 semantics differ for these two:
FOR_ITER: fallthrough = +1, branch = −1 (pops iterator on exhaustion)SEND: fallthrough = 0, branch = −1 (pops on awaitable-ready branch)
Without these overrides, max_stackdepth() in codegen/src/ir.rs and stack_effect(jump=True) exposed in _opcode.rs will incorrectly use fallthrough effects for jump edges, skewing loop-exit and async-suspend target depths.
Proposed fix
impl InstructionMetadata for Instruction {
+ fn stack_effect_jump(&self, oparg: u32) -> i32 {
+ match self {
+ Self::ForIter { .. } | Self::Send { .. } => -1,
+ _ => self.stack_effect(oparg),
+ }
+ }
+
#[inline]
fn label_arg(&self) -> Option<Arg<Label>> {Also update Lib/test/test__opcode.py line 117: change self.assertEqual(stack_effect(FOR_ITER, 0, jump=True), 1) to self.assertEqual(stack_effect(FOR_ITER, 0, jump=True), -1).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/compiler-core/src/bytecode/instruction.rs` around lines 1716 - 1724,
Implement branch-sensitive stack effects by overriding stack_effect_jump for the
opcodes ForIter and Send: in the Instruction trait's stack_effect_jump
implementation, match on self (or the instruction variant) and return -1 for
ForIter and -1 for Send (regardless of oparg), otherwise fall back to calling
self.stack_effect(oparg); this ensures branch edges use the correct values used
by max_stackdepth and the exposed stack_effect(jump=True). Also update the unit
test expectation in test__opcode.py to assert stack_effect(FOR_ITER, 0,
jump=True) == -1 instead of 1.
14bf7c5 to
f401e37
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/stdlib/src/_opcode.rs`:
- Around line 189-200: The current parsing of args.jump uses try_to_bool which
accepts truthy/falsy objects; instead, in the stack_effect implementation ensure
args.jump only accepts identity True, False or None: when handling args.jump
(the variable jump / match arm using vm.is_none and v), check explicitly for
None, then check identity for True and False (use the VM/context identity checks
available) and return Some(true) or Some(false) accordingly; for any other
object return vm.new_value_error("stack_effect: jump must be False, True or
None") so non-boolean truthy/falsy values raise ValueError.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 3af32856-1019-4537-8c7c-fe5ef48c7f15
📒 Files selected for processing (1)
crates/stdlib/src/_opcode.rs
| let jump: Option<bool> = match args.jump { | ||
| Some(v) => { | ||
| if vm.is_none(&v) { | ||
| None | ||
| } else { | ||
| Some(v.try_to_bool(vm).map_err(|_| { | ||
| vm.new_value_error("stack_effect: jump must be False, True or None") | ||
| })?) | ||
| } | ||
| } | ||
| None => None, | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Local RustPython implementation (changed block):"
sed -n '189,200p' crates/stdlib/src/_opcode.rs
echo
echo "CPython reference check (v3.14.2):"
curl -sL https://raw.githubusercontent.com/python/cpython/v3.14.2/Modules/_opcode.c \
| rg -n "jump == Py_None|jump == Py_True|jump == Py_False|jump must be False, True or None" -C2Repository: RustPython/RustPython
Length of output: 932
jump argument accepts non-bool truthy/falsy objects, diverging from CPython.
At lines 194–196, try_to_bool() allows integers, strings, and other truthy/falsy values. CPython's _opcode.stack_effect() only accepts False, True, or None and raises ValueError for any other object, using strict identity checks.
Suggested fix
- let jump: Option<bool> = match args.jump {
- Some(v) => {
- if vm.is_none(&v) {
- None
- } else {
- Some(v.try_to_bool(vm).map_err(|_| {
- vm.new_value_error("stack_effect: jump must be False, True or None")
- })?)
- }
- }
- None => None,
- };
+ let jump: Option<bool> = match args.jump {
+ Some(v) if vm.is_none(&v) => None,
+ Some(v) if v.fast_isinstance(vm.ctx.types.bool_type) => Some(v.try_to_bool(vm)?),
+ Some(_) => {
+ return Err(vm.new_value_error(
+ "stack_effect: jump must be False, True or None",
+ ));
+ }
+ None => None,
+ };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/stdlib/src/_opcode.rs` around lines 189 - 200, The current parsing of
args.jump uses try_to_bool which accepts truthy/falsy objects; instead, in the
stack_effect implementation ensure args.jump only accepts identity True, False
or None: when handling args.jump (the variable jump / match arm using vm.is_none
and v), check explicitly for None, then check identity for True and False (use
the VM/context identity checks available) and return Some(true) or Some(false)
accordingly; for any other object return vm.new_value_error("stack_effect: jump
must be False, True or None") so non-boolean truthy/falsy values raise
ValueError.
| fn stack_effect_jump(&self, _oparg: u32) -> i32 { | ||
| match self { | ||
| Self::SetupFinally { .. } | Self::SetupWith { .. } => 1, | ||
| Self::SetupCleanup { .. } => 2, |
There was a problem hiding this comment.
If _oparg is used, why have it with an underscore prefix?
There was a problem hiding this comment.
Sorry, didnt mean to ignore this. Let me fix it
Summary by CodeRabbit
Bug Fixes
Refactor
New Features