Newtype ConstIdx, Constants#7358

ShaharNaveh

Summary by CodeRabbit

Refactor
- Introduced a dedicated, typed constant index for safer, consistent constant lookups across compiler, bytecode, JIT, and VM.
- Replaced raw constant storage with a unified wrapper to standardize access and iteration.
- Updated constant-loading instruction paths and behavior, including a transient "mortal" transition after first load.

coderabbitai

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

@coderabbitai resume to resume automatic reviews.
@coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

▶️ Resume reviews
🔍 Trigger review

📝 Walkthrough

Walkthrough

Replaces raw u32 constant indices with a typed oparg::ConstIdx, adds a public Constants<C> wrapper for code-object constants, and propagates those types across bytecode, codegen, IR display, marshaling, JIT, and VM frame constant-access paths (including runtime transition to LoadConstMortal).

Changes

Cohort / File(s)	Summary
OpArg type `crates/compiler-core/src/bytecode/oparg.rs`	Adds `pub struct ConstIdx(u32)` with `from_u32`, `as_u32`, `as_usize`, `From<u32>`, `From<ConstIdx> for u32`, and `OpArgType` impl.
Constants wrapper & CodeObject `crates/compiler-core/src/bytecode.rs`	Introduces `#[derive(Clone)] pub struct Constants<C: Constant>(Box<[C]>)` with `Deref`, `Index<oparg::ConstIdx>`, `FromIterator`; exposes `pub mod oparg`; changes `CodeObject::constants` from `Box<[C]>` to `Constants<C>` and updates mapping/iteration.
Instruction representation `crates/compiler-core/src/bytecode/instruction.rs`	Changes `Instruction::LoadConst` to use `Arg<oparg::ConstIdx>` instead of `Arg<u32>`; updates formatting helpers to accept typed Arg.
Codegen & IR display `crates/codegen/src/compile.rs`, `crates/codegen/src/ir.rs`	`arg_constant` now returns `oparg::ConstIdx`; `emit_load_const` and `InstrDisplayContext::get_constant` updated to accept/use `ConstIdx`; constant lookups use `as_usize()`/`as_u32()` accordingly.
Marshaling & PyCode construction `crates/compiler-core/src/marshal.rs`, `crates/vm/src/builtins/code.rs`	Deserialization collect changed to inferred collection type; `PyCode` construction now collects constants into `Vec<Literal>` (removed explicit `into_boxed_slice()`).
Runtime & JIT `crates/vm/src/frame.rs`, `crates/jit/src/instructions.rs`, `crates/jit/tests/common.rs`	LoadConst pattern bindings and indexing updated to use `consti`/`ConstIdx` paths; indexing calls now use `consti.get(arg)` or `.as_usize()`/`.into()`; VM replaces loaded constant instruction with `LoadConstMortal` at runtime.

Sequence Diagram(s)

sequenceDiagram
    participant Codegen as Compiler/Codegen
    participant Bytecode as Bytecode/CodeObject
    participant VM as VM/Frame
    participant Constants as Constants<C>
    Codegen->>Bytecode: emit Instruction::LoadConst(Arg<ConstIdx>)
    VM->>Bytecode: fetch Instruction at pc
    VM->>Constants: index via ConstIdx (consti.get(arg))
    Constants-->>VM: return Constant
    VM->>VM: push constant on stack
    VM->>Bytecode: replace instruction -> LoadConstMortal

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Bytecode enum named oparg #7294: Modifies Instruction::LoadConst payloads and related constant-indexing codepaths (closely related changes).
Make inner oparg values private #7050: Refactors oparg/Arg internals and constructors overlapping ConstIdx additions.
Bytecode pseudo opcodes #6715: Changes to the Instruction enum and bytecode representations touching LoadConst.

Suggested reviewers

youknowone

Poem

🐇 I swapped the numbers for tidier names,

ConstIdx hopped in to stake its claims,
Wrapped constants snug in a cozy crate,
Bytes now skip with a lighter gait,
A tiny hop — the VM hums and games 🥕

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 46.15% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ 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 'Newtype ConstIdx, Constants' directly reflects the main change: introducing new wrapper types ConstIdx and Constants to replace raw u32 and Box<[C]> for type-safe constant indexing.

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

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

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

coderabbitai

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@crates/jit/tests/common.rs`:
- Line 197: The slice indexing uses a ConstIdx returned by consti.get(arg) which
can't index a slice; update the indexing in the LoadConst site so you convert
the ConstIdx to usize (e.g., call .as_usize() on the ConstIdx) before indexing
the constants slice; modify the expression that builds the pushed value (the
call in self.stack.push(...)) to index constants with consti.get(arg).as_usize()
(or an equivalent usize conversion) so the types match.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: 2dab4afd-3b23-4617-90df-f8e9a46b76ce

📥 Commits

Reviewing files that changed from the base of the PR and between 375b547 and 571ac6b.

📒 Files selected for processing (10)

crates/codegen/src/compile.rs
crates/codegen/src/ir.rs
crates/compiler-core/src/bytecode.rs
crates/compiler-core/src/bytecode/instruction.rs
crates/compiler-core/src/bytecode/oparg.rs
crates/compiler-core/src/marshal.rs
crates/jit/src/instructions.rs
crates/jit/tests/common.rs
crates/vm/src/builtins/code.rs
crates/vm/src/frame.rs

coderabbitai

♻️ Duplicate comments (1)

crates/jit/tests/common.rs (1)
196-196: ⚠️ Potential issue | 🔴 Critical

Fix slice indexing type mismatch at Line 196.

constants is a slice (&[ConstantData]), but this line indexes it with ConstIdx. Convert to usize before indexing (or change this API to accept the typed constants wrapper end-to-end). This matches the previously reported unresolved issue.
Proposed fix
-                self.stack.push(constants[consti.get(arg)].clone().into())
+                self.stack
+                    .push(constants[consti.get(arg).as_usize()].clone().into())
#!/bin/bash
set -euo pipefail

# Verify the local function signature and indexing site in this file
rg -n -C2 'fn process_instruction\(|constants:\s*&\[[^]]+\]|constants\[consti\.get\(arg\)\]' crates/jit/tests/common.rs

# Check whether there is any repository-level indexing support for ConstIdx on slices
rg -n 'impl\s+.*Index<\s*.*ConstIdx|impl\s+.*SliceIndex<.*ConstIdx' crates --type=rust
Expected verification outcome: first command shows constants is a slice and the current indexing expression; second command should not reveal a slice-index impl making ConstIdx directly valid for slice indexing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@crates/jit/tests/common.rs` at line 196, The slice indexing uses a ConstIdx
value: change the indexing at
self.stack.push(constants[consti.get(arg)].clone().into()) to convert the
ConstIdx to usize before indexing (e.g. use consti.get(arg).into() / .to_usize()
/ as usize depending on the ConstIdx newtype implementation) so that
constants[usize_index] is used; update the expression inside self.stack.push
accordingly to preserve the clone().into() chain.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In `@crates/jit/tests/common.rs`:
- Line 196: The slice indexing uses a ConstIdx value: change the indexing at
self.stack.push(constants[consti.get(arg)].clone().into()) to convert the
ConstIdx to usize before indexing (e.g. use consti.get(arg).into() / .to_usize()
/ as usize depending on the ConstIdx newtype implementation) so that
constants[usize_index] is used; update the expression inside self.stack.push
accordingly to preserve the clone().into() chain.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: fbdaf0af-13e7-487f-8f81-9c76e274681e

📥 Commits

Reviewing files that changed from the base of the PR and between 571ac6b and 32ac79a.

📒 Files selected for processing (4)

crates/codegen/src/compile.rs
crates/codegen/src/ir.rs
crates/jit/tests/common.rs
crates/vm/src/frame.rs

🚧 Files skipped from review as they are similar to previous changes (2)

crates/codegen/src/compile.rs
crates/codegen/src/ir.rs

coderabbitai

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@crates/codegen/src/ir.rs`:
- Around line 705-707: The indexing uses a ConstIdx directly; change it to
extract the ConstIdx value then convert to usize before indexing
self.metadata.consts: for the match arm (Instruction::LoadConst { consti },
Instruction::UnaryNot) use consti.get(curr.arg) to obtain the ConstIdx, cast or
call .into()/.0 to get a usize (same pattern used around lines 697–698), then
index self.metadata.consts[usize_index] when binding constant; update the
reference to consti/constant accordingly.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: 76bc2939-013e-4379-a0e3-bede4853d7c8

📥 Commits

Reviewing files that changed from the base of the PR and between 1d38d7d and 7bd9764.

📒 Files selected for processing (1)

crates/codegen/src/ir.rs

…type

* Newtype ConstIdx, Constants * Set generic

ShaharNaveh marked this pull request as ready for review March 5, 2026 13:51

coderabbitai Bot reviewed Mar 5, 2026

View reviewed changes

Newtype ConstIdx, Constants

aa607b2

ShaharNaveh force-pushed the bytecode-consti-newtype branch from a986d55 to aa607b2 Compare March 6, 2026 08:45

Set generic

c17bd49

youknowone approved these changes Mar 6, 2026

View reviewed changes

Merge remote-tracking branch 'upstream/main' into bytecode-consti-new… …

1139157

…type

coderabbitai Bot mentioned this pull request Mar 11, 2026

Newtype var_num oparg #7400

Merged

youknowone pushed a commit to youknowone/RustPython that referenced this pull request Mar 22, 2026

Newtype ConstIdx, Constants (RustPython#7358) …

753a026

* Newtype ConstIdx, Constants * Set generic

This was referenced Mar 24, 2026

#[newtype_oparg] derive macro #7489

Closed

Compiler parity: docstring dedent, StopIteration wrapper, constant folding #7530

Merged

Bytecode parity #7535

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Newtype ConstIdx, Constants#7358