Skip to content

Remove Reverse bytecode #6675

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 9 commits into from
Jan 10, 2026
Merged

Remove Reverse bytecode #6675

youknowone merged 9 commits into from
Jan 10, 2026

Conversation

@ShaharNaveh
Copy link
Collaborator

@ShaharNaveh ShaharNaveh commented Jan 9, 2026 •
edited by coderabbitai bot
Loading

Summary by CodeRabbit

  • Chores

    • Removed a legacy bytecode instruction and its runtime handling, simplifying the instruction set and trimming related formatting/diagnostic paths.
    • Simplified opcode metadata/name generation logic by removing redundant code paths.
    • CI validation tightened to fail when generated metadata is out of date.

  • Breaking Changes

    • Potential bytecode compatibility impact for programs or artifacts that relied on the removed instruction; affected items should be regenerated or recompiled.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 9, 2026 •
edited
Loading

📝 Walkthrough

Walkthrough

Removed the RustPython-only Reverse { amount: Arg<u32> } opcode variant from the Instruction enum, deleted its execution handling in the VM, and removed related stack-effect, disassembly/formatting, and dead-code paths; also fixed opcode-name generation and inverted a CI workflow check.

Changes

Cohort / File(s) Change Summary
Bytecode instruction definition
crates/compiler-core/src/bytecode.rs		 Removed Reverse { amount: Arg<u32> } from Instruction; removed its TryFrom<u8> mapping, stack-effect branch, and disassembly/formatting entry.
Frame execution handler
crates/vm/src/frame.rs		 Removed Reverse handling from ExecutingFrame::execute_instruction (stack slicing and in-place reversal logic deleted).
Opcode metadata & tooling
scripts/generate_opcode_metadata.py		 Removed redundant/alternative return paths in Opcode.cpython_name, consolidating name derivation to a single code path.
CI workflow
.github/workflows/ci.yaml		 Inverted the condition in the "Ensure Lib/_opcode_metadata is updated" step so the job fails when the working tree is dirty (non-empty git status --porcelain).

Sequence Diagram(s)

(omitted)

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested reviewers

  • youknowone

Poem

🐇 I nudged the opcodes, gave them a glance,

The Reverse hop has hopped its last dance.
Stacks now flow in straighter streams,
I twitch my nose and polish dreams.
Carrots for tidy, humming machines! 🎉

🚥 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 clearly and directly describes the main change—removing the Reverse bytecode operation from the codebase.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

📜 Recent review details

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ab43e94 and d879bf1.

📒 Files selected for processing (1)
  • .github/workflows/ci.yaml
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings 
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-27T14:03:49.034Z
Learning: When modifying bytecode instructions, perform a full clean build by running `rm -r target/debug/build/rustpython-* && find . | grep -E '\.pyc$' | xargs rm -r`
🔇 Additional comments (1)
.github/workflows/ci.yaml (1)

334-339: LGTM! Correct enforcement of metadata synchronization.

The inverted condition now properly fails the CI when the opcode metadata generation produces uncommitted changes, ensuring developers commit updated metadata alongside opcode definition changes. This is a standard best practice for keeping generated files synchronized with source code.

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 reviewed Jan 9, 2026
View reviewed changes
ShaharNaveh and others added 2 commits January 9, 2026 17:22
@ShaharNaveh @youknowone
Update crates/compiler-core/src/bytecode.rs
2cf54f8 
Co-authored-by: Jeong, YunWon <69878+youknowone@users.noreply.github.com>
@ShaharNaveh
Gen
ad05793
@ShaharNaveh ShaharNaveh mentioned this pull request Jan 9, 2026
Merged
@github-actions
Copy link
Contributor

github-actions bot commented Jan 10, 2026

Code has been automatically formatted

The code in this PR has been formatted using cargo fmt --all.
Please pull the latest changes before pushing again:

git pull origin bytecode-remove-reverse

@ShaharNaveh
Copy link
Collaborator Author

ShaharNaveh commented Jan 10, 2026 •
edited
Loading

Code has been automatically formatted

The code in this PR has been formatted using cargo fmt --all. Please pull the latest changes before pushing again:

git pull origin bytecode-remove-reverse

Yay #6677 works

@youknowone
Copy link
Member

youknowone commented Jan 10, 2026

Great! The message looks like need to be changed. it is not by fmt --all

@youknowone youknowone merged commit 6fe0598 into RustPython:main Jan 10, 2026
13 checks passed
ShaharNaveh added a commit to ShaharNaveh/RustPython that referenced this pull request Jan 10, 2026
@ShaharNaveh @youknowone @github-actions
Remove Reverse bytecode (RustPython#6675)
d448753 
* Remove `Reverse` bytecode

* Update crates/compiler-core/src/bytecode.rs

Co-authored-by: Jeong, YunWon <69878+youknowone@users.noreply.github.com>

* Gen

* Remove Reverse

* Auto-format: cargo fmt --all

* Revert comment

* Remove debug code

* Fix CI

---------

Co-authored-by: Jeong, YunWon <69878+youknowone@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
terryluan12 pushed a commit to terryluan12/RustPython that referenced this pull request Jan 15, 2026
@ShaharNaveh @youknowone
@github-actions @terryluan12
Remove Reverse bytecode (RustPython#6675)
dc39bd6 
* Remove `Reverse` bytecode

* Update crates/compiler-core/src/bytecode.rs

Co-authored-by: Jeong, YunWon <69878+youknowone@users.noreply.github.com>

* Gen

* Remove Reverse

* Auto-format: cargo fmt --all

* Revert comment

* Remove debug code

* Fix CI

---------

Co-authored-by: Jeong, YunWon <69878+youknowone@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@youknowone youknowone youknowone approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@ShaharNaveh @youknowone