GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — Add InstructionMetadata::stack_effect_jump for branch stack effects by youknowone · Pull Request #7585 · RustPython/RustPython Conversation
|
📝 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 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
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.
Fix all unresolved CodeRabbit comments on this PR:
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 9633aeda-1e39-410c-879f-d780a6209f1d
crates/codegen/src/ir.rscrates/compiler-core/src/bytecode/instruction.rscrates/stdlib/src/_opcode.rs
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| /// 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.
The reason will be displayed to describe this comment to others. Learn more.
🏁 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 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 -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 --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}") 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.
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).
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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
14bf7c5 to
f401e37
Compare
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
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.
Fix all unresolved CodeRabbit comments on this PR:
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 3af32856-1019-4537-8c7c-fe5ef48c7f15
crates/stdlib/src/_opcode.rs
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| 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.
The reason will be displayed to describe this comment to others. Learn more.
🏁 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.
- 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, + };
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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| 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.
The reason will be displayed to describe this comment to others. Learn more.
If _oparg is used, why have it with an underscore prefix?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
The fix is included in #7588
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
…7585) * Add InstructionMetadata::stack_effect_jump for branch stack effects 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). * apply review
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.