Skip to content

Conversation

@ShaharNaveh
Copy link
Collaborator

@ShaharNaveh ShaharNaveh commented Nov 9, 2025

Summary by CodeRabbit

Release Notes

  • Refactor
    • Improved internal bytecode instruction handling and storage mechanisms for better code organization and maintainability.
    • Streamlined bytecode parsing and conversion processes across compiler components.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 9, 2025

Walkthrough

A new CodeUnits wrapper type consolidates bytecode instruction storage, replacing direct Box<[CodeUnit]> usage across the compiler and VM. The public parse_instructions_from_bytes function is removed and parsing logic is centralized into CodeUnits conversion implementations.

Changes

Cohort / File(s) Summary
Core Type Definition
compiler/core/src/bytecode.rs
Introduced public CodeUnits wrapper type containing Box<[CodeUnit]>. Implemented From, TryFrom, and Deref traits for flexible construction from arrays, vectors, iterators, and byte slices. Updated CodeObject.instructions field type from Box<[CodeUnit]> to CodeUnits. Added From<u8> for OpArgByte. Refactored error types to use MarshalError alias in TryFrom<u8> implementations.
Codegen Integration
compiler/codegen/src/ir.rs
Updated CodeObject.instructions field initialization to use CodeUnits::from(instructions) conversion. Adjusted imports to include new CodeUnits type.
Marshaling / Deserialization
compiler/core/src/marshal.rs
Removed public parse_instructions_from_bytes function. Updated deserialize_code to parse raw bytecode instructions via CodeUnits::try_from(raw_instructions) instead of manual per-chunk parsing. Centralized instruction parsing into a single conversion call.
VM Builtin Code
vm/src/builtins/code.rs
Replaced parse_bytecode helper function and its call sites with CodeUnits::try_from(bytecode_bytes) conversions. Updated imports to use CodeUnits directly instead of CodeUnit and marshal-related items.

Sequence Diagram

sequenceDiagram
    autonumber
    participant Old as Old Flow
    participant New as New Flow
    
    rect rgba(200, 220, 240, 0.3)
        Note over Old,New: Instruction Deserialization
        Old->>Old: parse_instructions_from_bytes()
        Old->>Old: Iterate chunks (2 bytes)
        Old->>Old: Instruction::try_from() per chunk
        Old->>Old: Collect to Box<[CodeUnit]>
        Old-->>Old: Return Result<Box<[CodeUnit]>>
    end
    
    rect rgba(220, 240, 200, 0.3)
        Note over New: (New Flow)
        New->>New: CodeUnits::try_from(&[u8])
        New->>New: Parse all bytes at once
        New-->>New: Return Result<CodeUnits>
    end
    
    rect rgba(240, 230, 200, 0.3)
        Note over Old,New: Usage
        Old->>Old: CodeObject { instructions: Box<[...]> }
        New->>New: CodeObject { instructions: CodeUnits(...) }
    end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

  • Areas requiring attention:
    • compiler/core/src/bytecode.rs: New CodeUnits type and its trait implementations (especially TryFrom<&[u8]> logic for byte parsing)
    • compiler/core/src/marshal.rs: Verification that instruction deserialization behavior is preserved with the new CodeUnits::try_from approach
    • Cross-module consistency: Ensure all call sites correctly use the new CodeUnits API and handle errors appropriately

Possibly related PRs

  • code object linetable #6150: Modifies CodeObject representation and related serialization/deserialization paths in tandem with bytecode structure changes.
  • Fix PyCode constructor/replace #6193: Refactors bytecode parsing and CodeUnit/CodeObject construction, replacing previous parsing helpers with new centralized conversion logic.

Poem

🐰 A wrapper hops onto the bytecode stack,
CodeUnits bundles instructions back-to-back,
Where parsing once scattered, now unified stands,
The bytecode flows smoothly through compiler's hands. ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 12.50% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: introducing a new CodeUnits newtype wrapper that replaces the previous Box<[CodeUnit]> representation throughout the codebase.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 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 ed9a61d and 8c7466d.

📒 Files selected for processing (4)
  • compiler/codegen/src/ir.rs (2 hunks)
  • compiler/core/src/bytecode.rs (5 hunks)
  • compiler/core/src/marshal.rs (1 hunks)
  • vm/src/builtins/code.rs (4 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs

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

**/*.rs: Format Rust code with the default rustfmt style (run cargo fmt)
Run clippy and fix any warnings or lints introduced by your changes
Follow Rust best practices for error handling and memory management

Files:

  • compiler/core/src/marshal.rs
  • compiler/codegen/src/ir.rs
  • vm/src/builtins/code.rs
  • compiler/core/src/bytecode.rs
{vm,stdlib}/**/*.rs

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

Use RustPython macros (pyclass, pymodule, pyfunction, etc.) when implementing Python functionality in Rust

Files:

  • vm/src/builtins/code.rs
🧠 Learnings (2)
📚 Learning: 2025-09-11T05:06:59.971Z
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-11T05:06:59.971Z
Learning: Applies to {vm,stdlib}/**/*.rs : Use RustPython macros (`pyclass`, `pymodule`, `pyfunction`, etc.) when implementing Python functionality in Rust

Applied to files:

  • compiler/codegen/src/ir.rs
  • compiler/core/src/bytecode.rs
📚 Learning: 2025-08-26T05:20:54.540Z
Learnt from: youknowone
Repo: RustPython/RustPython PR: 6110
File: vm/src/frame.rs:1311-1316
Timestamp: 2025-08-26T05:20:54.540Z
Learning: In the RustPython codebase, only certain builtin types should be marked with the SEQUENCE flag for pattern matching. List and tuple are sequences, but bytes, bytearray, and range are not considered sequences in this context, even though they may implement sequence-like protocols.

Applied to files:

  • compiler/codegen/src/ir.rs
🧬 Code graph analysis (4)
compiler/core/src/marshal.rs (1)
compiler/core/src/bytecode.rs (3)
  • try_from (823-829)
  • try_from (850-855)
  • try_from (864-870)
compiler/codegen/src/ir.rs (1)
compiler/core/src/bytecode.rs (5)
  • from (264-266)
  • from (307-309)
  • from (813-816)
  • from (874-876)
  • from (880-882)
vm/src/builtins/code.rs (1)
compiler/core/src/bytecode.rs (3)
  • try_from (823-829)
  • try_from (850-855)
  • try_from (864-870)
compiler/core/src/bytecode.rs (1)
compiler/core/src/marshal.rs (4)
  • fmt (24-32)
  • from (36-38)
  • from (494-507)
  • try_from (79-111)
⏰ 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). (10)
  • GitHub Check: Ensure compilation on various targets
  • GitHub Check: Run rust tests (windows-latest)
  • GitHub Check: Run rust tests (macos-latest)
  • GitHub Check: Run rust tests (ubuntu-latest)
  • GitHub Check: Run snippets and cpython tests on wasm-wasi
  • GitHub Check: Run snippets and cpython tests (windows-latest)
  • GitHub Check: Run snippets and cpython tests (macos-latest)
  • GitHub Check: Run snippets and cpython tests (ubuntu-latest)
  • GitHub Check: Check Rust code with rustfmt and clippy
  • GitHub Check: Check the WASM package and demo

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.

bytes
.chunks_exact(2)
.map(|cu| {
let op = Instruction::try_from(cu[0])?;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is this used in new code?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@youknowone youknowone merged commit 9792001 into RustPython:main Nov 10, 2025
12 checks passed
@youknowone
Copy link
Member

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants