GeezerVison is showing github.com — Newtype for `[ir::Block]` by ShaharNaveh · Pull Request #8142 · RustPython/RustPython
Conversation
ShaharNaveh
commented
Jun 22, 2026
•
edited by coderabbitai
Bot
Loading
Uh oh!
There was an error while loading. Please reload this page.
Summary
Summary by CodeRabbit
- Refactor
- Reorganized internal compiler code generation system for improved code structure and maintainability. No user-facing functionality changes.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
coderabbitai
Bot
commented
Jun 22, 2026
•
edited
Loading
Uh oh!
There was an error while loading. Please reload this page.
|
📝 Walkthrough
Walkthrough
Introduces a Blocks newtype wrapping Vec<Block> in ir.rs, with BlockIdx-aware Index/IndexMut and standard conversion traits. CodeInfo.blocks is changed from Vec<Block> to Blocks, and every CFG construction, optimization, analysis, and transformation function that previously accepted &mut [Block] or &mut Vec<Block> is updated to accept &mut Blocks. All initialization and append sites in compile.rs are updated accordingly, as are unit tests.
Changes
Blocks Newtype Refactor
| Layer / File(s) |
Summary |
Blocks type definition and BlockIdx conversions
crates/codegen/src/ir.rs |
Adds pub struct Blocks(Vec<Block>) with try_reserve, push, Deref/DerefMut, Index/IndexMut for usize and BlockIdx, and From conversions. Reworks BlockIdx helpers (as_u32, as_usize, idx) and updates the core::ops import and ConstantPool::Index impl path. |
CodeInfo.blocks field and compile.rs callsites
crates/codegen/src/ir.rs, crates/codegen/src/compile.rs |
Changes CodeInfo.blocks from Vec<Block> to Blocks. Updates all initialization (Blocks::from([Block::default()])), reset (mem::replace), and block-append sites in compile.rs and adds the Block/Blocks imports. |
CFG builder and construction pipeline
crates/codegen/src/ir.rs |
Updates CfgBuilder.blocks, cfg_from_instruction_sequence (returns Blocks), basicblock_append_block_instructions, cfg_to_instruction_sequence, translate_jump_labels_to_targets, and dataflow passes to use Blocks and BlockIdx indexing. |
Optimization phases
crates/codegen/src/ir.rs |
Propagates Blocks through optimize_code_unit, optimize_cfg, optimized_cfg_to_instruction_sequence, unreachable-block removal, optimize_load_const, optimize_basic_block, calculate_stackdepth, insert_superinstructions, and stack-depth helpers. |
CFG analysis and transformation pipeline
crates/codegen/src/ir.rs |
Converts exception handler propagation, warm/cold marking, cold-block relocation, CFG validation, jump threading, jump normalization, redundancy removals, and traversal-stack helpers to Blocks. |
Line-number propagation and exception-stack handling
crates/codegen/src/ir.rs |
Refactors propagate_line_numbers, resolve_line_numbers, duplicate_exits_without_lineno, copy_basicblock, exception-stack helpers (except_stack_top, push/pop_except_block), label_exception_targets, and convert_pseudo_ops to &mut Blocks. |
fix_cell_offsets and tests
crates/codegen/src/ir.rs |
Updates fix_cell_offsets parameter to &mut Blocks and converts all unit test block constructors from vec![...] to Blocks::from([..]) across line-number, jump-threading, and optimization tests. |
Estimated code review effort
🎯 4 (Complex) | ⏱️ ~60 minutes
Poem
🐇 A wrapper so tidy, a newtype so neat,
Blocks hops in to keep the old Vec off its feet.
With Index<BlockIdx> and Deref in tow,
No more .idx() calls wherever we go.
The bunny refactored from builder to test—
Typed blocks, typed hops, a codegen blessed! 🌿
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
| Check name |
Status |
Explanation |
| Description Check |
✅ Passed |
Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check |
✅ Passed |
The title 'Newtype for [ir::Block]' directly and concisely describes the main change—introducing a newtype wrapper for blocks—which aligns with the primary modifications in the changeset. |
| Docstring Coverage |
✅ Passed |
Docstring coverage is 96.04% which is sufficient. The required threshold is 80.00%. |
| Linked Issues check |
✅ Passed |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check |
✅ Passed |
Check skipped because no linked issues were found for this pull request. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
🧪 Generate unit tests (beta)
- Create PR with unit tests
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.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
![coderabbitai[bot]](/api/image?u=https%3A%2F%2Favatars.githubusercontent.com%2Fin%2F347564%3Fs%3D60%26v%3D4)
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.
🧹 Nitpick comments (1)
crates/codegen/src/ir.rs (1)
757-808: 🧹 Nitpick | 🔵 Trivial | 💤 Low value
Consider consistent indexing style.
This function uses blocks[block_idx.idx()] (usize indexing via .idx()) throughout, while other functions like basicblock_append_block_instructions use blocks[block_idx] directly. Both patterns work correctly due to the dual Index implementations, but consistent use of direct BlockIdx indexing would be cleaner and leverage the newtype's purpose.
Not blocking since both patterns are functionally correct.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/codegen/src/ir.rs` around lines 757 - 808, The function
cfg_to_instruction_sequence uses inconsistent indexing style by calling .idx()
on BlockIdx values (like blocks[block_idx.idx()]) while other functions use
direct indexing with the BlockIdx newtype (like blocks[block_idx]). Replace all
instances of blocks[block_idx.idx()], blocks[target_block.idx()], and
blocks[handler.handler_block.idx()] throughout the function to use direct
BlockIdx indexing without calling .idx(), such as blocks[block_idx],
blocks[target_block], and blocks[handler.handler_block], to maintain consistency
with the codebase and properly leverage the BlockIdx newtype's Index trait
implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/codegen/src/ir.rs`:
- Around line 757-808: The function cfg_to_instruction_sequence uses
inconsistent indexing style by calling .idx() on BlockIdx values (like
blocks[block_idx.idx()]) while other functions use direct indexing with the
BlockIdx newtype (like blocks[block_idx]). Replace all instances of
blocks[block_idx.idx()], blocks[target_block.idx()], and
blocks[handler.handler_block.idx()] throughout the function to use direct
BlockIdx indexing without calling .idx(), such as blocks[block_idx],
blocks[target_block], and blocks[handler.handler_block], to maintain consistency
with the codebase and properly leverage the BlockIdx newtype's Index trait
implementation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 06947e9b-5ad8-4d27-9ce6-c20a4f72b915
📥 Commits
Reviewing files that changed from the base of the PR and between 55b96f9 and e476f3c.
📒 Files selected for processing (2)
crates/codegen/src/compile.rscrates/codegen/src/ir.rs
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assigned
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.
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.
|