GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — Move `OpArg` to its own file by ShaharNaveh · Pull Request #6703 · RustPython/RustPython

Conversation

ShaharNaveh commented Jan 11, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

Followup on #6693

Summary by CodeRabbit

Refactor
- Reorganized bytecode API: opcode-argument handling moved into a dedicated submodule and public surface consolidated for clearer boundaries.
New Features
- Added encoded exception-table support and utilities to locate handlers.
- Introduced strongly-typed opcode-argument helpers and improved human-readable formatting for opcode arguments.
Breaking Changes
- Public exposure of several opcode-argument types and paths changed; update call sites accordingly.

_{✏️ 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.

All reactions


          Move OpArg to its own file

42da99f

coderabbitai Bot commented Jan 11, 2026 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

📝 Walkthrough

Walkthrough

Reorganizes bytecode argument handling into a new bytecode::oparg module, adds generic Arg<T> in instruction, introduces ExceptionTableEntry plus encode_exception_table/find_exception_handler, and updates top-level public re-exports while removing legacy oparg symbols from the main module.

Changes

Cohort / File(s)	Summary
Bytecode root `crates/compiler-core/src/bytecode.rs`	Added `mod oparg`; reworked public re-exports to expose `instruction::{Arg, Instruction, decode_load_super_attr_arg, encode_load_super_attr_arg}` and to re-export oparg types under `crate::bytecode::oparg`; added `ExceptionTableEntry`, `encode_exception_table`, and `find_exception_handler`; removed legacy oparg symbols from top-level exports.
Instruction module `crates/compiler-core/src/bytecode/instruction.rs`	Added `pub struct Arg<T: OpArgType>(PhantomData<T>)` with `marker`, `new`, `new_single`, `get`, `try_get`, `get_unchecked`; added `encode_load_super_attr_arg` / `decode_load_super_attr_arg`; imports updated to use `oparg::...`.
New oparg module `crates/compiler-core/src/bytecode/oparg.rs`	New module defining `OpArgType` trait, `OpArgByte`/`OpArg`/`OpArgState`, macros for enum generation, many `OpArgType` enums/bitflags (e.g., `BinaryOperator`, `ComparisonOperator`, `RaiseKind`, `MakeFunctionFlags`, `UnpackExArgs`, `ConvertValueOparg`, `ResumeType`, intrinsics), Display/From/Into impls and helpers for op-arg encoding/decoding.

Sequence Diagram(s)

(Skipped — changes are API/module reorganization and encoding utilities without a multi-component runtime control flow requiring sequence visualization.)

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Rework compiler about exception handling #6638 — modifies bytecode exception-table encoding/decoding and introduces/uses ExceptionTableEntry / encoding helpers.
Assign opcode ids #6637 — refactors oparg-related types and updates Instruction variants to use Arg<T>/oparg types.
Super instructions #6694 — adds/uses encode_load_super_attr_arg/decode_load_super_attr_arg and related LoadSuper instruction handling.

Suggested reviewers

youknowone

Poem

🐰 I hopped through bytes with eager cheer,

I tunneled opargs to a burrow near,
Arg wears a tiny PhantomData hat,
Exceptions boxed and mapped to a spot,
Bytecode burrow now tidy — munch the carrot, that!

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Title check	⚠️ Warning	The PR title 'Move `OpArg` to its own file' does not accurately reflect the actual changes. While OpArg was moved, the PR involves extensive API reorganization, creating a new oparg module with many type re-exports, adding new public types (ExceptionTableEntry, Arg), and significantly restructuring the bytecode module's public surface (+10/-651 lines in bytecode.rs).	Revise the title to better reflect the scope of changes, such as 'Reorganize bytecode module with new oparg module and public API restructuring' or 'Extract oparg types into new module with exception table support'.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
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.}

✨ 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.}

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


          Merge remote-tracking branch 'upstream/main' into bytecode-cleanup-oparg

672a95e

coderabbitai Bot reviewed

Jan 12, 2026

View reviewed changes

coderabbitai Bot left a comment

Copy link

Copy Markdown

Contributor

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: 1

📜 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 42da99f and 672a95e.

📒 Files selected for processing (1)

crates/compiler-core/src/bytecode/instruction.rs

🧰 Additional context used

📓 Path-based instructions (1)

**/*.rs

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.rs: Follow the default rustfmt code style by running cargo fmt to format Rust code
Always run clippy to lint code (cargo clippy) before completing tasks and fix any warnings or lints introduced by changes
Follow Rust best practices for error handling and memory management
Use the macro system (pyclass, pymodule, pyfunction, etc.) when implementing Python functionality in Rust

Files:

crates/compiler-core/src/bytecode/instruction.rs

🧬 Code graph analysis (1)

crates/compiler-core/src/bytecode/instruction.rs (1)

crates/compiler-core/src/bytecode/oparg.rs (14)

fmt (31-33)

fmt (140-150)

fmt (256-258)

fmt (466-496)

fmt (579-581)

get (81-87)

from_op_arg (8-8)

from_op_arg (155-165)

from_op_arg (185-187)

from_op_arg (197-199)

from_op_arg (245-247)

from_op_arg (335-337)

from_op_arg (532-538)

from_op_arg (567-570)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)

GitHub Check: Ensure compilation on various targets
GitHub Check: Run snippets and cpython tests (windows-2025)
GitHub Check: Run rust tests (windows-2025)
GitHub Check: Run tests under miri
GitHub Check: Check Rust code with clippy

🔇 Additional comments (2)

crates/compiler-core/src/bytecode/instruction.rs (2)

866-905: LGTM! Well-designed zero-cost type-safe abstraction.

The Arg<T> wrapper using PhantomData<T> provides compile-time type safety without runtime overhead. The API is clean with appropriate #[inline] hints for the hot-path accessors, and the unsafe get_unchecked method has proper safety documentation.

907-919: LGTM!

The trait implementations are appropriate for a zero-sized marker type. PartialEq correctly returns true for all instances since Arg<T> carries no runtime data—only type information via PhantomData. The Debug implementation using type_name::<T>() provides helpful output for diagnostics.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

crates/compiler-core/src/bytecode/instruction.rs Resolved

Uh oh!

There was an error while loading. Please reload this page.

ShaharNaveh added 2 commits

January 12, 2026 10:31


          Fix merge

65086a3


          Fix more merge

0273bc9

coderabbitai Bot reviewed

Jan 12, 2026

View reviewed changes

coderabbitai Bot left a comment

Copy link

Copy Markdown

Contributor

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

🧹 Nitpick comments (1)

crates/compiler-core/src/bytecode.rs (1)
57-71: Consider a debug assertion for entry invariants.

The saturating_sub on line 62 silently produces 0 if entry.end < entry.start, which would result in an empty range being encoded. While this is safe, it could mask logic errors in callers.
🔧 Optional: Add debug assertion
 pub fn encode_exception_table(entries: &[ExceptionTableEntry]) -> alloc::boxed::Box<[u8]> {
     let mut data = Vec::new();
     for entry in entries {
+        debug_assert!(
+            entry.end >= entry.start,
+            "ExceptionTableEntry end ({}) must be >= start ({})",
+            entry.end,
+            entry.start
+        );
         let size = entry.end.saturating_sub(entry.start);

📜 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 65086a3 and 0273bc9.

📒 Files selected for processing (2)

crates/compiler-core/src/bytecode.rs
crates/compiler-core/src/bytecode/instruction.rs

🧰 Additional context used

📓 Path-based instructions (1)

**/*.rs

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.rs: Follow the default rustfmt code style by running cargo fmt to format Rust code
Always run clippy to lint code (cargo clippy) before completing tasks and fix any warnings or lints introduced by changes
Follow Rust best practices for error handling and memory management
Use the macro system (pyclass, pymodule, pyfunction, etc.) when implementing Python functionality in Rust

Files:

crates/compiler-core/src/bytecode/instruction.rs
crates/compiler-core/src/bytecode.rs

🧬 Code graph analysis (1)

crates/compiler-core/src/bytecode.rs (2)

crates/compiler-core/src/bytecode/instruction.rs (4)

core (310-310)

core (374-374)

decode_load_super_attr_arg (929-934)

encode_load_super_attr_arg (923-925)

crates/vm/src/frame.rs (2)

core (410-410)

core (1958-1958)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)

GitHub Check: Run snippets and cpython tests (macos-latest)
GitHub Check: Run rust tests (ubuntu-latest)
GitHub Check: Run rust tests (macos-latest)
GitHub Check: Run snippets and cpython tests (ubuntu-latest)
GitHub Check: Check the WASM package and demo
GitHub Check: Run rust tests (windows-2025)
GitHub Check: Run snippets and cpython tests on wasm-wasi
GitHub Check: Run snippets and cpython tests (windows-2025)
GitHub Check: Run tests under miri
GitHub Check: Ensure compilation on various targets
GitHub Check: Check Rust code with clippy

🔇 Additional comments (7)

crates/compiler-core/src/bytecode/instruction.rs (3)

1-13: LGTM!

The import reorganization correctly pulls in PhantomData for the new Arg<T> type and restructures the oparg-related imports to use the new oparg module path. This aligns with the PR objective of moving OpArg to its own file.

865-919: Clean zero-sized type marker pattern for Arg<T>.

The implementation correctly uses PhantomData<T> to create a zero-sized marker type that carries type information at compile time without runtime overhead. A few observations:

PartialEq returning true for all instances is correct since the marker carries no runtime data.

get() panics on invalid opargs via unwrap(), which is appropriate as invalid bytecode is a bug.

The unsafe get_unchecked properly forwards safety requirements to callers.

921-934: Bit-packing for LOAD_SUPER_ATTR looks correct.

The encoding packs load_method in bit 0, has_class in bit 1, and name_idx in bits 2+. The decoding correctly extracts these values. This matches the usage in stack_effect (line 614) and fmt_dis (line 802).

crates/compiler-core/src/bytecode.rs (4)

11-11: LGTM!

Addition of mem import is necessary for mem::size_of and mem::discriminant usage in this file.

17-27: Clean module reorganization with maintained public API.

The re-exports from instruction and the new oparg module preserve the existing public API surface while implementing the modular separation described in the PR objectives.

73-97: LGTM!

The find_exception_handler function correctly implements linear search through the varint-encoded exception table, returning the first matching entry for the given offset. The range check offset >= start && offset < end correctly implements half-open interval semantics.

811-879: Good test coverage for exception table functionality.

The tests cover:

Round-trip encode/decode with multiple entries

Empty table handling

Single entry with boundary conditions (inclusive start, exclusive end)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

ShaharNaveh added 2 commits

January 13, 2026 13:41


          Merge remote-tracking branch 'upstream/main' into bytecode-cleanup-oparg

1e93968


          Merge remote-tracking branch 'upstream/main' into bytecode-cleanup-oparg

d0ba323

ShaharNaveh requested a review from youknowone

January 13, 2026 15:11

youknowone approved these changes

Jan 13, 2026

View reviewed changes

youknowone left a comment

Copy link

Copy Markdown

Member

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.

Thanks!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

youknowone merged commit 9eeea92 into RustPython:main

Jan 13, 2026

13 checks passed

Uh oh!

There was an error while loading. Please reload this page.

terryluan12 pushed a commit to terryluan12/RustPython that referenced this pull request

Jan 15, 2026


          Move OpArg to its own file (RustPython#6703)

f0d4ae8

* Move OpArg to its own file

* Fix merge

* Fix more merge

This was referenced Jan 31, 2026

Rework exception compiler #6909

Merged

Make OpArgType::from_op_arg to return a Result<Self, MarshalError> #6914

Merged

Use num_enum crate for oparg types #6980

Merged

This was referenced Feb 8, 2026

Newtype LoadAttr oparg #7047

Merged

Make inner oparg values private #7050

Merged

coderabbitai Bot mentioned this pull request

Mar 2, 2026

Bytecode enum named oparg #7294

Merged

This was referenced Mar 11, 2026

Newtype var_num oparg #7400

Merged

Newtype oparg align methods #7403

Merged

Adapt bitflagset to use enum #7419

Merged

This was referenced Mar 22, 2026

marshal #7467

Merged

#[newtype_oparg] derive macro #7489

Closed

Reviewers

coderabbitai[bot]

coderabbitai[bot] left review comments

youknowone

youknowone approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

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.

2 participants

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.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

Conversation

ShaharNaveh commented Jan 11, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Jan 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Possibly related PRs

Suggested reviewers

Poem

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

youknowone left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ShaharNaveh commented Jan 11, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

coderabbitai Bot commented Jan 11, 2026 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.