FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Remove SUBSCRIPT, JUMP_IF_{TRUE,FALSE}_OR_POP by youknowone · Pull Request #6810 · RustPython/RustPython · GitHub

/ RustPython Public

Remove SUBSCRIPT, JUMP_IF_{TRUE,FALSE}_OR_POP - #6810

Merged
youknowone merged 4 commits into
RustPython:mainfrom
youknowone:opcode
Jan 20, 2026
ConversationCommits (4)ChecksFiles changed
Merged

Remove SUBSCRIPT, JUMP_IF_{TRUE,FALSE}_OR_POP#6810
youknowone merged 4 commits into
RustPython:mainfrom
youknowone:opcode

Conversation

youknowone commented Jan 20, 2026 •
edited by coderabbitai Bot
Loading

Copy link
Copy Markdown
Member

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.

coderabbitai Bot commented Jan 20, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor
📝 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.

❤️ Share

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

youknowone marked this pull request as ready for review January 20, 2026 06:28
coderabbitai Bot reviewed Jan 20, 2026
View reviewed changes

coderabbitai Bot left a comment

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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);
}

youknowone merged commit f0c3e7d into RustPython:main Jan 20, 2026
13 checks passed
youknowone deleted the opcode branch January 20, 2026 07:21
This was referenced Jan 22, 2026
Merged
Merged
This was referenced Jan 30, 2026
Merged
Merged
Merged
coderabbitai Bot mentioned this pull request Mar 14, 2026
Merged
7 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

coderabbitai[bot] coderabbitai[bot] left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

Back | FazBrowse Home | New Git URL