GeezerVison is showing github.com — Remove SUBSCRIPT, JUMP_IF_{TRUE,FALSE}_OR_POP by youknowone · Pull Request #6810 · RustPython/RustPython
Conversation
youknowone
commented
Jan 20, 2026
•
edited by coderabbitai
Bot
Loading
Uh oh!
There was an error while loading. Please reload this page.
Summary by CodeRabbit
Release Notes
- Refactor
- Optimized compiler instruction handling and bytecode generation for improved efficiency and reduced code complexity.
- Enhanced virtual machine stack management and iterator cleanup behavior, particularly during loop execution.
- Streamlined the instruction set by removing obsolete instruction types.
- Consolidated conditional jump and subscript operation handling logic for better maintainability.
✏️ Tip: You can customize this high-level summary in your review settings.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
coderabbitai
Bot
commented
Jan 20, 2026
•
edited
Loading
Uh oh!
There was an error while loading. Please reload this page.
📝 Walkthrough
Walkthrough
This PR refactors the instruction set by removing four RustPython-specific opcodes (JumpIfFalseOrPop, JumpIfTrueOrPop, LoadAssertionError, and Subscript) and replacing Subscript with BinarySubscr throughout the codegen layer. It also adjusts for-loop cleanup semantics to emit PopTop for non-async iteration paths.
Changes
| Cohort / File(s) |
Summary |
Instruction Set Definition
crates/compiler-core/src/bytecode/instruction.rs |
Removed four enum variants (JumpIfFalseOrPop, JumpIfTrueOrPop, LoadAssertionError, Subscript) and updated TryFrom, label_arg, stack_effect, and fmt_dis implementations to drop references to removed opcodes. |
Codegen Refactoring
crates/codegen/src/compile.rs |
Replaced all usages of Instruction::Subscript with Instruction::BinarySubscr; added PopTop emissions after non-async for loops for proper iterator cleanup; adjusted two-element slice handling in Load context. |
Opcode Classification
crates/stdlib/src/opcode.rs |
Removed JumpIfFalseOrPop and JumpIfTrueOrPop from the has_jump function's jump instruction set. |
VM Execution Cleanup
crates/vm/src/frame.rs |
Deleted execution paths for JumpIfFalseOrPop, JumpIfTrueOrPop, and Subscript instructions; updated for_iter StopIteration handling to align with CPython 3.14 semantics (iterator cleanup handled by POP_ITER). |
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~22 minutes
Possibly related PRs
- RustPython/RustPython#6805: Directly introduces BinarySubscr and realigns Subscript opcode changes that this PR builds upon.
- RustPython/RustPython#6322: Removes and modifies the same Instruction enum variants (JumpIfFalseOrPop, JumpIfTrueOrPop, Subscript).
- RustPython/RustPython#6524: Refactors compile-time bytecode emission and removes JumpIfTrueOrPop/JumpIfFalseOrPop handling in similar ways.
Suggested reviewers
🐰 ✨ Old opcodes fade away,
BinarySubscr saves the day,
Loops now cleanup with a pop,
Stack discipline won't stop! 🎉
🚥 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 directly and concisely summarizes the main objective of the changeset: removing three instruction opcodes (SUBSCRIPT, JUMP_IF_TRUE_OR_POP, JUMP_IF_FALSE_OR_POP) across multiple files. |
| Docstring Coverage |
✅ Passed |
No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
❤️ Share
Comment @coderabbitai help to get the list of available commands and usage tips.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Caution Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/codegen/src/compile.rs (1)
4928-4935: Add PopTop after EndAsyncFor to clean up the async iterator.
The EndAsyncFor instruction only pops exception handling items (awaitable, exc), not the iterator itself. The iterator remains on the stack after the instruction completes. A PopTop must follow to clean it up, matching the pattern used in comprehension loops at lines 7363-7368 and 7564-7568.
if is_async {
emit!(self, Instruction::EndAsyncFor);
emit!(self, Instruction::PopTop);
} else {
// Pop the iterator after loop ends
emit!(self, Instruction::PopTop);
}
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
This was referenced Jan 22, 2026
This was referenced Jan 30, 2026
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assigned
Milestone
No milestone
Development
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
|