Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
youknowone merged 4 commits into from
Jan 20, 2026
Merged

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

youknowone merged 4 commits into from
Jan 20, 2026
+17 −94

Conversation

@youknowone
Copy link
Member

@youknowone youknowone commented Jan 20, 2026 •
edited by coderabbitai bot
Loading

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
Copy link
Contributor

coderabbitai bot commented Jan 20, 2026 •
edited
Loading

📝 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 youknowone marked this pull request as ready for review January 20, 2026 06:28
coderabbitai[bot]
coderabbitai bot reviewed Jan 20, 2026
View reviewed changes
Copy link
Contributor

@coderabbitai coderabbitai bot left a 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);
}

@youknowone youknowone merged commit f0c3e7d into RustPython:main Jan 20, 2026
13 checks passed
@youknowone youknowone deleted the opcode branch January 20, 2026 07:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@coderabbitai 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

@youknowone