Newtype ConstIdx, Constants#7358
Merged
youknowone merged 3 commits intoRustPython:mainfrom Mar 9, 2026
Merged
Conversation
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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces raw u32 constant indices with a typed 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)
✅ Passed checks (2 passed)
✏️ 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
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
📒 Files selected for processing (10)
crates/codegen/src/compile.rscrates/codegen/src/ir.rscrates/compiler-core/src/bytecode.rscrates/compiler-core/src/bytecode/instruction.rscrates/compiler-core/src/bytecode/oparg.rscrates/compiler-core/src/marshal.rscrates/jit/src/instructions.rscrates/jit/tests/common.rscrates/vm/src/builtins/code.rscrates/vm/src/frame.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()) |
Contributor
There was a problem hiding this comment.
🧩 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())
}🤖 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 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.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
crates/jit/tests/common.rs (1)
196-196:⚠️ Potential issue | 🔴 CriticalFix slice indexing type mismatch at Line 196.
constantsis a slice (&[ConstantData]), but this line indexes it withConstIdx. Convert tousizebefore 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
constantsis a slice and the current indexing expression; second command should not reveal a slice-index impl makingConstIdxdirectly 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
📒 Files selected for processing (4)
crates/codegen/src/compile.rscrates/codegen/src/ir.rscrates/jit/tests/common.rscrates/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
Contributor
There was a problem hiding this comment.
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
📒 Files selected for processing (1)
crates/codegen/src/ir.rs
ShaharNaveh
force-pushed
the
bytecode-consti-newtype
branch
from
a986d55 to
aa607b2
Compare
youknowone
approved these changes
Mar 6, 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
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