Add has_* methods to opcode enums#7818
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds opcode capability query helpers to the bytecode instruction layer. New predicates ( ChangesBytecode Instruction Opcode Helpers
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/compiler-core/src/bytecode/instruction.rs`:
- Around line 1051-1074: The has_jump() method is missing two specialized opcode
variants; update the matches! list inside pub const fn has_jump(self) -> bool
(in instruction.rs) to also include ForIterGen and SendGen so they map
consistently to the base opcodes ForIter and Send (similar to the existing
ForIterList/ForIterRange/ForIterTuple and JumpBackwardJit/JumpBackwardNoJit
entries); simply add ForIterGen and SendGen to the list of matched variants in
has_jump().
- Around line 1042-1049: The has_free() method currently matches DeleteDeref,
LoadFromDictOrDeref, MakeCell, and StoreDeref; update it to match CPython's
HAS_FREE_FLAG by including LoadDeref and removing LoadFromDictOrDeref and
MakeCell. Edit the matches expression inside the pub const fn has_free(&self) to
only include Self::DeleteDeref | Self::LoadDeref | Self::StoreDeref, ensuring
the function name has_free and the opcode variants DeleteDeref, LoadDeref,
StoreDeref are the referenced symbols to 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: 2a215c26-cd0a-4398-8445-1384ce64a21e
📒 Files selected for processing (2)
crates/compiler-core/src/bytecode/instruction.rscrates/stdlib/src/_opcode.rs
| /// Does this opcode have 'HAS_FREE_FLAG' set. | ||
| #[must_use] | ||
| pub const fn has_free(self) -> bool { | ||
| matches!( | ||
| self, | ||
| Self::DeleteDeref | Self::LoadFromDictOrDeref | Self::MakeCell | Self::StoreDeref | ||
| ) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
CPython 3.14 HAS_FREE_FLAG opcode classification which opcodes should be included
💡 Result:
For CPython 3.14, the opcodes marked with the internal opcode-metadata flag HAS_FREE_FLAG are the following: 1) DELETE_DEREF 2) LOAD_CLOSURE 3) LOAD_DEREF 4) STORE_DEREF Evidence: CPython defines HAS_FREE_FLAG in its internal opcode metadata and associates it with the above opcode entries (e.g., DELETE_DEREF is shown with HAS_ARG_FLAG | HAS_FREE_FLAG... in the opcode metadata table) [1]. Related context: the disassembler’s public dis.hasfree set corresponds to “bytecodes that access a free (closure) variable” [2][3], i.e., the closure/free-variable access opcodes above.
Citations:
- 1: https://github.com/python/cpython/blob/main/Include/internal/pycore_opcode_metadata.h
- 2: https://docs.python.org/tr/3.14/library/dis.html
- 3: https://docs.python.org/3/library/dis.html
🏁 Script executed:
cd crates/compiler-core/src/bytecode && grep -n "enum Opcode\|pub const fn has_free\|LoadDeref\|LoadFromDictOrDeref\|MakeCell\|CopyFreeVars" instruction.rs | head -50Repository: RustPython/RustPython
Length of output: 688
🏁 Script executed:
# Get the specific lines mentioned in the review
sed -n '1042,1049p' crates/compiler-core/src/bytecode/instruction.rsRepository: RustPython/RustPython
Length of output: 320
🏁 Script executed:
# Check for LoadDeref definition
sed -n '280,300p' crates/compiler-core/src/bytecode/instruction.rsRepository: RustPython/RustPython
Length of output: 743
🏁 Script executed:
# Check for CopyFreeVars definition
sed -n '220,230p' crates/compiler-core/src/bytecode/instruction.rsRepository: RustPython/RustPython
Length of output: 359
🏁 Script executed:
# Search for all occurrences of these opcodes in the file
rg -n "LoadDeref|LoadFromDictOrDeref|MakeCell|CopyFreeVars|DeleteDeref|StoreDeref" crates/compiler-core/src/bytecode/instruction.rs | head -30Repository: RustPython/RustPython
Length of output: 677
🏁 Script executed:
sed -n '1080,1090p' crates/compiler-core/src/bytecode/instruction.rsRepository: RustPython/RustPython
Length of output: 475
🏁 Script executed:
# Check for docstrings/comments on LoadFromDictOrDeref and MakeCell
sed -n '308,315p' crates/compiler-core/src/bytecode/instruction.rsRepository: RustPython/RustPython
Length of output: 367
🏁 Script executed:
sed -n '329,340p' crates/compiler-core/src/bytecode/instruction.rsRepository: RustPython/RustPython
Length of output: 405
🏁 Script executed:
# Search for any usage or discussion of has_free
rg -B2 -A2 "has_free" crates/compiler-core/src/bytecode/instruction.rs | head -40Repository: RustPython/RustPython
Length of output: 435
🏁 Script executed:
# Get full context around line 1044 to understand the structure
sed -n '1040,1055p' crates/compiler-core/src/bytecode/instruction.rsRepository: RustPython/RustPython
Length of output: 473
🏁 Script executed:
# Let's also check if there are any comments or documentation about why these opcodes are in has_free
rg -B5 "LoadFromDictOrDeref|MakeCell" crates/compiler-core/src/bytecode/instruction.rs | head -50Repository: RustPython/RustPython
Length of output: 1358
Add LoadDeref to has_free implementation and remove non-standard opcodes.
The has_free method is missing LoadDeref (line 289), which should be included per CPython's HAS_FREE_FLAG classification. CPython marks DELETE_DEREF, LOAD_DEREF, and STORE_DEREF as having this flag; rustpython correctly includes the first and third but omits the second.
Additionally, LoadFromDictOrDeref and MakeCell should not be in this implementation, as they do not have HAS_FREE_FLAG in CPython's opcode metadata.
Update the matches expression to:
Self::DeleteDeref | Self::LoadDeref | Self::StoreDeref
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/compiler-core/src/bytecode/instruction.rs` around lines 1042 - 1049,
The has_free() method currently matches DeleteDeref, LoadFromDictOrDeref,
MakeCell, and StoreDeref; update it to match CPython's HAS_FREE_FLAG by
including LoadDeref and removing LoadFromDictOrDeref and MakeCell. Edit the
matches expression inside the pub const fn has_free(&self) to only include
Self::DeleteDeref | Self::LoadDeref | Self::StoreDeref, ensuring the function
name has_free and the opcode variants DeleteDeref, LoadDeref, StoreDeref are the
referenced symbols to change.
| /// Does this opcode have 'HAS_JUMP_FLAG' set. | ||
| #[must_use] | ||
| pub const fn has_jump(self) -> bool { | ||
| matches!( | ||
| self, | ||
| Self::EndAsyncFor | ||
| | Self::ForIter | ||
| | Self::JumpBackward | ||
| | Self::JumpBackwardNoInterrupt | ||
| | Self::JumpForward | ||
| | Self::PopJumpIfFalse | ||
| | Self::PopJumpIfNone | ||
| | Self::PopJumpIfNotNone | ||
| | Self::PopJumpIfTrue | ||
| | Self::Send | ||
| | Self::ForIterList | ||
| | Self::ForIterRange | ||
| | Self::ForIterTuple | ||
| | Self::JumpBackwardJit | ||
| | Self::JumpBackwardNoJit | ||
| | Self::InstrumentedForIter | ||
| | Self::InstrumentedEndAsyncFor | ||
| ) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for all ForIter* and Send* opcode definitions to verify completeness
echo "=== ForIter variants ==="
rg -n "^\s+(ForIter\w*)\s*=" crates/compiler-core/src/bytecode/instruction.rs
echo -e "\n=== Send variants ==="
rg -n "^\s+(Send\w*)\s*=" crates/compiler-core/src/bytecode/instruction.rs
echo -e "\n=== Opcodes in has_jump ==="
rg -A50 "pub const fn has_jump\(self\) -> bool" crates/compiler-core/src/bytecode/instruction.rs | rg "Self::" | head -20Repository: RustPython/RustPython
Length of output: 1150
🏁 Script executed:
# Check CPython's opcode definitions to see if ForIterGen and SendGen have HAS_JUMP_FLAG
curl -s https://raw.githubusercontent.com/python/cpython/main/Include/opcode.h | grep -A 2 -B 2 "FOR_ITER_GEN\|SEND_GEN" | head -30Repository: RustPython/RustPython
Length of output: 47
🏁 Script executed:
# Also check if there's any documentation in RustPython about this
rg -B5 -A5 "ForIterGen|SendGen" crates/compiler-core/src/bytecode/instruction.rs | head -40Repository: RustPython/RustPython
Length of output: 2090
🏁 Script executed:
# Check if there's a pattern or comment explaining why Gen variants are excluded
rg -B10 "pub const fn has_jump" crates/compiler-core/src/bytecode/instruction.rs | head -20Repository: RustPython/RustPython
Length of output: 519
🏁 Script executed:
# Let's check CPython's Python/opcode_ids.h directly for the actual opcode definitions
python3 <<'EOF'
import urllib.request
import json
# Fetch CPython's opcode definitions
url = "https://raw.githubusercontent.com/python/cpython/main/Python/opcode_ids.h"
try:
with urllib.request.urlopen(url, timeout=5) as response:
content = response.read().decode('utf-8')
# Search for FOR_ITER_GEN and SEND_GEN definitions
lines = content.split('\n')
for i, line in enumerate(lines):
if 'FOR_ITER_GEN' in line or 'SEND_GEN' in line:
# Print context around the definition
start = max(0, i - 2)
end = min(len(lines), i + 3)
for j in range(start, end):
print(f"{j}: {lines[j]}")
print("---")
except Exception as e:
print(f"Error fetching CPython opcode definitions: {e}")
# Also check core/ceval.c for HAS_JUMP_FLAG definitions
url2 = "https://raw.githubusercontent.com/python/cpython/main/Python/ceval.c"
try:
with urllib.request.urlopen(url2, timeout=5) as response:
content = response.read().decode('utf-8')
lines = content.split('\n')
for i, line in enumerate(lines):
if 'FOR_ITER_GEN' in line or 'SEND_GEN' in line:
# Print context
start = max(0, i - 2)
end = min(len(lines), i + 3)
for j in range(start, end):
print(f"{j}: {lines[j]}")
print("---")
except Exception as e:
print(f"Error fetching CPython ceval.c: {e}")
EOFRepository: RustPython/RustPython
Length of output: 396
🏁 Script executed:
# Let's check if there are any comments in the RustPython code about jump flags or specialized variants
rg -B10 -A10 "has_jump" crates/compiler-core/src/bytecode/instruction.rs | grep -A30 "pub const fn has_jump"Repository: RustPython/RustPython
Length of output: 1418
🏁 Script executed:
# Check if there's any test or documentation about which opcodes should have jump flag
fd instruction tests test | head -5Repository: RustPython/RustPython
Length of output: 209
🏁 Script executed:
# Look for any git history or comments about ForIterGen/SendGen jump behavior
rg "ForIterGen|SendGen" crates/compiler-core/src -B3 -A3Repository: RustPython/RustPython
Length of output: 4344
Include ForIterGen and SendGen in the has_jump method.
The implementation includes specialized variants like ForIterList, ForIterRange, ForIterTuple, JumpBackwardJit, and JumpBackwardNoJit, but omits:
ForIterGen(line 448) — maps to base opcodeForIterSendGen(line 197) — maps to base opcodeSend
Since their base opcodes (ForIter and Send) are included, and other specialized variants are also present, these should be added for consistency.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/compiler-core/src/bytecode/instruction.rs` around lines 1051 - 1074,
The has_jump() method is missing two specialized opcode variants; update the
matches! list inside pub const fn has_jump(self) -> bool (in instruction.rs) to
also include ForIterGen and SendGen so they map consistently to the base opcodes
ForIter and Send (similar to the existing ForIterList/ForIterRange/ForIterTuple
and JumpBackwardJit/JumpBackwardNoJit entries); simply add ForIterGen and
SendGen to the list of matched variants in has_jump().
|
closed by #7797 |
1 participant
Extracted from #7797
Summary by CodeRabbit