Skip to content

Add InstructionMetadata::stack_effect_jump for branch stack effects#7585

Merged
youknowone merged 2 commits intoRustPython:mainfrom
youknowone:stack-effect-jump
Apr 13, 2026
Merged

Add InstructionMetadata::stack_effect_jump for branch stack effects#7585
youknowone merged 2 commits intoRustPython:mainfrom
youknowone:stack-effect-jump

Conversation

@youknowone
Copy link
Copy Markdown
Member

@youknowone youknowone commented Apr 13, 2026

Summary by CodeRabbit

  • Bug Fixes

    • More accurate stack-depth calculation for branching and targeted control-flow, fixing incorrect depth estimates for some paths.
    • Corrected branch-aware stack-effect behavior so stack-effect queries reflect taken vs fallthrough paths.
  • Refactor

    • Simplified propagation of stack depths to target blocks for clearer, more consistent control-flow analysis.
  • New Features

    • Branch-aware stack-effect query supports explicit, taken, and unknown jump semantics.

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).
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Apr 13, 2026

📝 Walkthrough

Walkthrough

Simplifies stack-depth propagation in codegen to use a new branch-aware stack-effect API. Adds stack_effect_jump to instruction metadata and pseudo-instructions, and updates the stdlib stack_effect helper to accept an optional jump argument and choose branch/fallthrough or max effect accordingly.

Changes

Cohort / File(s) Summary
Codegen IR
crates/codegen/src/ir.rs
Removed special-casing for SETUP_* pseudo-ops and SEND adjustment when computing target depths; always compute jump_effect via new API and push target_depth unconditionally.
Instruction metadata
crates/compiler-core/src/bytecode/instruction.rs
Added InstructionMetadata::stack_effect_jump(&self, oparg: u32) -> i32 with default to stack_effect; implemented dispatch for AnyInstruction; PseudoInstruction overrides stack_effect_jump for SETUP_FINALLY, SETUP_WITH, and SETUP_CLEANUP.
Stdlib opcode helper
crates/stdlib/src/_opcode.rs
stack_effect now accepts an optional jump: Option<bool>: Some(true) uses branch effect, Some(false) uses fallthrough, None returns max of both; conversion of jump uses try_to_bool.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

"I hopped through ops and depths unknown,
Pushed handler depths where once was shown,
A jump-aware wiggle in my bytecode song,
Stack effects balanced, hopping along 🐇"

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: adding a new stack_effect_jump method to InstructionMetadata for computing branch-taken stack effects, which is the core feature across all modified files.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@youknowone youknowone marked this pull request as ready for review April 13, 2026 05:26
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between 27aed85 and 89191b8.

📒 Files selected for processing (3)
  • crates/codegen/src/ir.rs
  • crates/compiler-core/src/bytecode/instruction.rs
  • crates/stdlib/src/_opcode.rs

Comment on lines +1716 to +1724
/// 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)
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# Check current state of instruction.rs implementation
fd -t f instruction.rs | head -5

Repository: 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 -60

Repository: 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 5

Repository: 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 -250

Repository: 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.rs

Repository: 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 exhausted

Repository: 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 -100

Repository: 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:


🏁 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 -30

Repository: 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/null

Repository: 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.rs

Repository: 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.rs

Repository: 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 -20

Repository: 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 1

Repository: 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.rs

Repository: 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 -5

Repository: 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 -60

Repository: 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" -i

Repository: 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")
EOF

Repository: 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
EOF

Repository: 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}")
EOF

Repository: 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 -60

Repository: 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.py

Repository: 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 -3

Repository: RustPython/RustPython

Length of output: 253


🏁 Script executed:

# Check what version of Python is being used in the test environment
python3 --version

Repository: 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}")
EOF

Repository: 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 -30

Repository: 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 -10

Repository: 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 -100

Repository: 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 -5

Repository: 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 exhausted

Repository: 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 -40

Repository: 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 -20

Repository: 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 -80

Repository: 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 -50

Repository: 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 -100

Repository: 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 -10

Repository: 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")
EOF

Repository: 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/null

Repository: 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 -3

Repository: 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.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between 14bf7c5 and f401e37.

📒 Files selected for processing (1)
  • crates/stdlib/src/_opcode.rs

Comment on lines +189 to +200
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,
};
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 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" -C2

Repository: 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,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If _oparg is used, why have it with an underscore prefix?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, didnt mean to ignore this. Let me fix it

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix is included in #7588

@youknowone youknowone merged commit 7e5e026 into RustPython:main Apr 13, 2026
20 checks passed
@youknowone youknowone deleted the stack-effect-jump branch April 13, 2026 08:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants