Remove SUBSCRIPT, JUMP_IF_{TRUE,FALSE}_OR_POP by youknowone · Pull Request #6810 · RustPython/RustPython
📝 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
- ShaharNaveh
🐰 ✨ 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
- 📝 Generate docstrings
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 help to get the list of available commands and usage tips.