Newtype ConstIdx, Constants - #7358
Merged
youknowone merged 3 commits intoMar 9, 2026
Merged
Conversation
Copy link
Copy Markdown
Contributor
Sorry, something went wrong.
Copy link
Copy Markdown
Contributor
|
Note Reviews pausedIt 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:
Use the checkboxes below for quick actions:
WalkthroughReplaces 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
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
Suggested reviewers
Poem🚥 Pre-merge checks | ✅ 2 | ❌ 1 ❌ Failed checks (1 warning)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches 🧪 Generate unit tests (beta)
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. ❤️ ShareComment @coderabbitai help to get the list of available commands and usage tips. |
Sorry, something went wrong.
ShaharNaveh
marked this pull request as ready for review
March 5, 2026 13:51
Copy link
Copy Markdown
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agentsVerify 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
📥 CommitsReviewing 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
Sorry, something went wrong.
Comment thread
crates/jit/tests/common.rs
| Instruction::LoadConst { consti } => { | ||
| let idx = consti.get(arg); | ||
| self.stack.push(constants[idx as usize].clone().into()) | ||
| self.stack.push(constants[consti.get(arg)].clone().into()) |
Copy link
Copy Markdown
Contributor
There was a problem hiding this comment.
⚠️ Potential issue | 🔴 Critical
🧩 Analysis chain🏁 Script executed:
#!/bin/bash
set -euo pipefail
# First, let's look at the exact line and surrounding context
echo "=== Line 197 and surrounding context ==="
sed -n '190,205p' crates/jit/tests/common.rs
# Show the function signature
echo -e "\n=== Function containing this code ==="
rg -n -B10 'Instruction::LoadConst' crates/jit/tests/common.rs | head -40
# Check type of consti and what .get() returns
echo -e "\n=== Looking for ConstIdx definition and methods ==="
fd -e rs | xargs rg -l 'struct ConstIdx\|type ConstIdx' 2>/dev/null | head -5Repository: RustPython/RustPython
Length of output: 1918
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find where Instruction enum is defined
echo "=== Finding Instruction enum definition ==="
fd -e rs | xargs rg -l 'enum Instruction' 2>/dev/null | head -3
# Find ConstIdx type
echo -e "\n=== Finding ConstIdx type ==="
fd -e rs | xargs rg 'struct ConstIdx|type ConstIdx' 2>/dev/null | head -10
# Look at the Instruction enum definition for LoadConst
echo -e "\n=== LoadConst variant definition ==="
fd -e rs | xargs rg -A5 'LoadConst.*consti' 2>/dev/null | head -20Repository: RustPython/RustPython
Length of output: 1694
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find ConstIdx and its methods
echo "=== ConstIdx definition and methods ==="
cat crates/compiler-core/src/bytecode/oparg.rs | head -100
# Look for IndexOpArg trait or similar
echo -e "\n=== Looking for IndexOpArg trait/implementations ==="
rg -n 'trait.*OpArg|trait.*Index' crates/compiler-core/src/bytecode/oparg.rs | head -20
</script>Repository: RustPython/RustPython
Length of output: 2597
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find ConstIdx struct definition
echo "=== ConstIdx struct definition ==="
rg -A 30 'pub struct ConstIdx' crates/compiler-core/src/bytecode/oparg.rs
# Find all methods on ConstIdx
echo -e "\n=== ConstIdx impl blocks ==="
rg -A 20 'impl.*ConstIdx' crates/compiler-core/src/bytecode/oparg.rsRepository: RustPython/RustPython
Length of output: 1328
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the .get() method - it might be in IndexOpArg trait or similar
echo "=== Searching for .get() method on index types ==="
rg -B5 -A10 'fn get\(' crates/compiler-core/src/bytecode/oparg.rs
# Check what trait provides the .get() method
echo -e "\n=== IndexOpArg or similar trait ==="
rg 'trait.*Index|IndexOpArg' crates/compiler-core/src/bytecode/ -r
</script>Repository: RustPython/RustPython
Length of output: 1008
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for IndexOpArg trait definition
echo "=== Finding IndexOpArg trait ==="
rg 'trait IndexOpArg' crates/compiler-core/src/bytecode/
# Search for the .get() method definition more broadly
echo -e "\n=== Searching for .get() method with OpArg parameter ==="
rg 'fn get\(.*OpArg' crates/compiler-core/src/
# Look at the actual Instruction enum to understand the field types
echo -e "\n=== Looking at Instruction enum LoadConst definition ==="
rg -A5 'LoadConst.*{' crates/compiler-core/src/bytecode/instruction.rs | head -20Repository: RustPython/RustPython
Length of output: 97
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Let's look at what .get() is being called on consti
# First, let's see the full context around line 197 in common.rs
echo "=== Full context around line 197 ==="
sed -n '175,210p' crates/jit/tests/common.rs
# Search for the trait that provides .get() method
echo -e "\n=== Search for trait defining .get() method ==="
rg 'trait.*get|fn get.*OpArg' crates/compiler-core/src/bytecode/
# Let's check the Instruction type definition
echo -e "\n=== Instruction enum with LoadConst ==="
rg -B2 -A2 'LoadConst' crates/compiler-core/src/bytecode/instruction.rs | head -30Repository: RustPython/RustPython
Length of output: 3191
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Now let's look at the Arg type and its .get() method
echo "=== Arg type definition and .get() method ==="
rg -B5 -A15 'impl.*Arg.*{' crates/compiler-core/src/bytecode/instruction.rs | grep -A15 'pub fn get'
# Let's see the full Arg definition
echo -e "\n=== Complete Arg struct definition ==="
rg -B10 -A30 'pub struct Arg' crates/compiler-core/src/bytecode/instruction.rs | head -50Repository: RustPython/RustPython
Length of output: 215
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look at Arg type and its implementation
echo "=== Arg struct and impl ==="
rg -B5 -A20 'pub struct Arg' crates/compiler-core/src/bytecode/instruction.rs
# Find the .get method on Arg
echo -e "\n=== .get method on Arg ==="
rg -B2 -A10 'pub fn get\(self, arg: OpArg\)' crates/compiler-core/src/bytecode/instruction.rsRepository: RustPython/RustPython
Length of output: 998
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if ConstIdx implements Index trait for slices
echo "=== Check for Index impl with ConstIdx ==="
rg 'impl.*Index.*ConstIdx|impl.*Index.*Arg' crates/compiler-core/src/bytecode/
# Check what type self.code.constants is in vm/src/frame.rs
echo -e "\n=== Type of code.constants in frame.rs ==="
rg -B10 'self.code.constants\[consti' crates/vm/src/frame.rs | head -20
# Look at CodeObject to see the type of its constants field
echo -e "\n=== CodeObject definition ==="
rg -A10 'pub struct CodeObject' crates/compiler-core/src/Repository: RustPython/RustPython
Length of output: 107
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Let's verify if the code actually compiles
echo "=== Attempting to compile crates/jit/tests/common.rs ==="
cd crates/jit && cargo check 2>&1 | grep -A20 "error\|warning" | head -40
# Also check what the actual error is on line 197
echo -e "\n=== Checking line 197 compilation directly ==="
cd - > /dev/null
rg -B3 -A3 'constants\[consti.get' crates/jit/tests/common.rs
# Check if there's a custom Index implementation anywhere
echo -e "\n=== All Index implementations in codebase ==="
rg 'impl.*Index.*for' crates/ --type rs | grep -v node_modulesRepository: RustPython/RustPython
Length of output: 151
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Simply check what type .get() returns and if it can index a slice
echo "=== Return type of Arg<ConstIdx>::get() ==="
echo "From the code we've seen:"
echo "- Arg<T>::get(self, arg: OpArg) -> T"
echo "- For Arg<ConstIdx>, this means .get() returns ConstIdx"
echo ""
echo "- ConstIdx does NOT implement Deref, Index, or any trait that would let it index a slice"
echo "- ConstIdx only has: as_u32(), as_usize(), and From<u32>"
echo ""
# Verify this by checking if ConstIdx can be used directly as slice index
echo "=== Looking at the two usages in crates/jit/tests/common.rs ==="
rg -n 'consti.get' crates/jit/tests/common.rs
# Check if constants in codebase is always a slice or Vec
echo -e "\n=== How constants is typed in function signature ==="
rg -B2 'constants:.*\[ConstantData\]' crates/jit/tests/common.rsRepository: RustPython/RustPython
Length of output: 729
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify the exact current state of line 197
echo "=== Exact lines 195-200 from common.rs ==="
sed -n '195,200p' crates/jit/tests/common.rs
# Check git history to see if this is a recent change
echo -e "\n=== Recent changes to this file ==="
cd crates/jit/tests && git log --oneline -20 common.rs 2>/dev/null || echo "Git not available"
cd - > /dev/null
# Summary of type facts
echo -e "\n=== Type Analysis Summary ==="
echo "ConstIdx::get(arg: OpArg) returns: ConstIdx"
echo "ConstIdx has NO Index trait implementation"
echo "Rust slices require usize for indexing"
echo "Line 80 correctly uses: consti.get(arg).as_usize()"
echo "Line 197 incorrectly uses: constants[consti.get(arg)]"
echo ""
echo "This WILL NOT COMPILE because ConstIdx cannot be used to index a slice."Repository: RustPython/RustPython
Length of output: 794
Fix type mismatch in LoadConst slice indexing.
At line 197, consti.get(arg) returns ConstIdx, which cannot directly index a slice. Slices require usize indexing. Use .as_usize() to convert:
Suggested fix Instruction::LoadConst { consti } => {
- self.stack.push(constants[consti.get(arg)].clone().into())
+ self.stack.push(constants[consti.get(arg).as_usize()].clone().into())
}Verify each finding against the current code and only fix it if needed. In `@crates/jit/tests/common.rs` at 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.
Sorry, something went wrong.
Copy link
Copy Markdown
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
ℹ️ Review info ⚙️ Run configuration
crates/jit/tests/common.rs (1)🤖 Prompt for all review comments with AI agents196-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=rustExpected 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 AgentsVerify 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.
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
📥 CommitsReviewing 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
- crates/codegen/src/compile.rs
- crates/codegen/src/ir.rs
Sorry, something went wrong.
Copy link
Copy Markdown
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agentsVerify 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
📥 CommitsReviewing files that changed from the base of the PR and between 1d38d7d and 7bd9764.
📒 Files selected for processing (1)- crates/codegen/src/ir.rs
Sorry, something went wrong.
Comment thread
crates/codegen/src/ir.rs
ShaharNaveh
force-pushed
the
bytecode-consti-newtype
branch
from
a986d55 to
aa607b2
Compare
March 6, 2026 08:45
youknowone
approved these changes
Mar 6, 2026
Merged
youknowone
pushed a commit
to youknowone/RustPython
that referenced
this pull request
Mar 22, 2026
* Newtype ConstIdx, Constants * Set generic
This was referenced Mar 24, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Reviewers
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.
Summary by CodeRabbit