GV

GeezerVison

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

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Add newtype of `CodeUnits` by ShaharNaveh · Pull Request #6241 · RustPython/RustPython

Conversation

@ShaharNaveh

@ShaharNaveh ShaharNaveh commented Nov 9, 2025 •
edited by coderabbitai Bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

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.

Sorry, something went wrong.

Uh oh!

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

All reactions
ShaharNaveh added 2 commits November 9, 2025 13:58
@ShaharNaveh
Base CodeUnits newtype
140ed86
@ShaharNaveh
Adjust code to use CodeUnits
8c7466d
@coderabbitai

coderabbitai Bot commented Nov 9, 2025 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

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
  • X
  • Mastodon
  • Reddit
  • LinkedIn

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.

youknowone
youknowone reviewed Nov 9, 2025
View reviewed changes
Comment thread compiler/core/src/marshal.rs
bytes
.chunks_exact(2)
.map(|cu| {
let op = Instruction::try_from(cu[0])?;

@youknowone youknowone Nov 9, 2025

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.

Where is this used in new code?

Sorry, something went wrong.

Uh oh!

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

All reactions

@ShaharNaveh ShaharNaveh Nov 9, 2025

Copy link
Copy Markdown
Contributor Author

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.

https://github.com/ShaharNaveh/RustPython/blob/8c7466dc00a6f2efbd97094e35f593f5a7647213/compiler/core/src/bytecode.rs#L869

Sorry, something went wrong.

Uh oh!

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

youknowone reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@ShaharNaveh
Trigger CI
7eb336d
youknowone
youknowone approved these changes Nov 9, 2025
View reviewed changes
@youknowone
youknowone merged commit 9792001 into RustPython:main Nov 10, 2025
12 checks passed

Uh oh!

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

@youknowone

youknowone commented Nov 10, 2025

Copy link
Copy Markdown
Member

👍

All reactions

Sorry, something went wrong.

Uh oh!

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

This was referenced Feb 26, 2026
Instrumented instruction #7212
Merged
Specialized ops #7322
Merged
@coderabbitai coderabbitai Bot mentioned this pull request Mar 6, 2026
Suspend Python threads before fork() #7364
Merged
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

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

@ShaharNaveh @youknowone
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.