Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR introduces a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
youknowone
approved these changes
Mar 11, 2026
![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: 2
🧹 Nitpick comments (1)
crates/codegen/src/compile.rs (1)
4140-4142: Look up the synthetic fast locals by name here.This loop ties the emitted
LoadFasts to.defaults/.kwdefaultsliving in slots0..num_typeparam_args, but those names are appended tometadata.varnamesafterpush_output()has already seeded the scope from the symbol table. Resolving the slots by name keeps this wrapper robust if that scope ever carries any other locals.🛠️ Proposed refactor
- // Load defaults/kwdefaults with LOAD_FAST - for i in 0..num_typeparam_args { - let var_num = oparg::VarNum::from(i as u32); - emit!(self, Instruction::LoadFast { var_num }); - } + // Load defaults/kwdefaults with LOAD_FAST + if funcflags.contains(bytecode::MakeFunctionFlags::DEFAULTS) { + let var_num = self.get_local_var_index(".defaults")?; + emit!(self, Instruction::LoadFast { var_num }); + } + if funcflags.contains(bytecode::MakeFunctionFlags::KW_ONLY_DEFAULTS) { + let var_num = self.get_local_var_index(".kwdefaults")?; + emit!(self, Instruction::LoadFast { var_num }); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/codegen/src/compile.rs` around lines 4140 - 4142, The current loop emits LoadFast by assuming the synthetic typeparam defaults live in slots 0..num_typeparam_args; instead, resolve each synthetic fast local by name using metadata.varnames so the correct var slot is used even if other locals exist. For each index i in 0..num_typeparam_args, compute the synthetic name you appended (the same name used when push_output() seeded the scope), find its position in self.metadata.varnames (e.g., .iter().position(|n| n == &name)), convert that position to oparg::VarNum and then emit Instruction::LoadFast with that VarNum; handle the case where the name is not found with an appropriate error or fallback.
🤖 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 1112-1113: get_varname currently indexes only metadata.varnames
and will panic for VarNum values that refer into the "locals-plus" table
(cellvars/freevars created for LOAD_CLOSURE); update get_varname to resolve
VarNum against the combined locals-plus layout: if var_num.as_usize() <
self.metadata.varnames.len() return that entry, otherwise subtract
varnames.len() and if the remaining index is < self.metadata.cellvars.len()
return from metadata.cellvars, else subtract cellvars.len() and return from
metadata.freevars; ensure indexes use as_usize() and do bounds-safe lookups so
it never panics when encountering LOAD_CLOSURE-created slots.
In `@crates/jit/src/instructions.rs`:
- Around line 111-115: The code directly indexes self.variables with a VarNum
(typed u32) which can panic on malformed bytecode; change both occurrences
(e.g., in store_variable and the other site where self.variables[idx] is used)
to first convert the VarNum to usize and use checked access
(self.variables.get_mut(idx_usize) or self.variables.get(idx_usize)), returning
Err(JitCompileError::BadBytecode) when None. Inside the Some(slot) branch, keep
the existing logic (declare_var via builder and set the slot) but perform it via
the checked slot reference instead of indexing to avoid panics.
---
Nitpick comments:
In `@crates/codegen/src/compile.rs`:
- Around line 4140-4142: The current loop emits LoadFast by assuming the
synthetic typeparam defaults live in slots 0..num_typeparam_args; instead,
resolve each synthetic fast local by name using metadata.varnames so the correct
var slot is used even if other locals exist. For each index i in
0..num_typeparam_args, compute the synthetic name you appended (the same name
used when push_output() seeded the scope), find its position in
self.metadata.varnames (e.g., .iter().position(|n| n == &name)), convert that
position to oparg::VarNum and then emit Instruction::LoadFast with that VarNum;
handle the case where the name is not found with an appropriate error or
fallback.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 443a5187-8634-44ef-9123-5f743fb1aa0a
📒 Files selected for processing (7)
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/jit/src/instructions.rscrates/vm/src/frame.rs
Comment on lines
+1112
to
+1113
| fn get_varname(&self, var_num: oparg::VarNum) -> &str { | ||
| self.metadata.varnames[var_num.as_usize()].as_ref() |
Contributor
There was a problem hiding this comment.
Resolve VarNum against the full locals-plus table.
Line 1113 still assumes every VarNum lives in metadata.varnames, but Line 1797 creates LOAD_FAST slots as varnames_len + ... when lowering LOAD_CLOSURE. That means displaying valid closure bytecode can walk past varnames and panic. Please fall back into cellvars/freevars here instead of indexing only varnames.
🛠️ Suggested fix
fn get_varname(&self, var_num: oparg::VarNum) -> &str {
- self.metadata.varnames[var_num.as_usize()].as_ref()
+ let idx = var_num.as_usize();
+ if let Some(name) = self.metadata.varnames.get_index(idx) {
+ name.as_ref()
+ } else {
+ let cell_idx = idx - self.metadata.varnames.len();
+ self.metadata
+ .cellvars
+ .get_index(cell_idx)
+ .or_else(|| self.metadata.freevars.get_index(cell_idx - self.metadata.cellvars.len()))
+ .map(String::as_ref)
+ .unwrap_or("<invalid var num>")
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/codegen/src/ir.rs` around lines 1112 - 1113, get_varname currently
indexes only metadata.varnames and will panic for VarNum values that refer into
the "locals-plus" table (cellvars/freevars created for LOAD_CLOSURE); update
get_varname to resolve VarNum against the combined locals-plus layout: if
var_num.as_usize() < self.metadata.varnames.len() return that entry, otherwise
subtract varnames.len() and if the remaining index is <
self.metadata.cellvars.len() return from metadata.cellvars, else subtract
cellvars.len() and return from metadata.freevars; ensure indexes use as_usize()
and do bounds-safe lookups so it never panics when encountering
LOAD_CLOSURE-created slots.
Comment on lines
+111
to
115
| fn store_variable(&mut self, idx: oparg::VarNum, val: JitValue) -> Result<(), JitCompileError> { | ||
| let builder = &mut self.builder; | ||
| let ty = val.to_jit_type().ok_or(JitCompileError::NotSupported)?; | ||
| let local = self.variables[idx as usize].get_or_insert_with(|| { | ||
| let local = self.variables[idx].get_or_insert_with(|| { | ||
| let var = builder.declare_var(ty.to_cranelift()); |
Contributor
There was a problem hiding this comment.
Use checked local-slot access instead of panicking on bad bytecode.
Line 114 and Line 651 index self.variables directly, but VarNum is only a typed u32, not a proof that the slot exists. A malformed or stale code object will panic the JIT here instead of taking the existing JitCompileError::BadBytecode path.
🛠️ Suggested fix
fn store_variable(&mut self, idx: oparg::VarNum, val: JitValue) -> Result<(), JitCompileError> {
let builder = &mut self.builder;
let ty = val.to_jit_type().ok_or(JitCompileError::NotSupported)?;
- let local = self.variables[idx].get_or_insert_with(|| {
+ let slot = self
+ .variables
+ .get_mut(idx.as_usize())
+ .ok_or(JitCompileError::BadBytecode)?;
+ let local = slot.get_or_insert_with(|| {
let var = builder.declare_var(ty.to_cranelift());
Local {
var,
ty: ty.clone(),
}
@@
Instruction::LoadFast { var_num } | Instruction::LoadFastBorrow { var_num } => {
- let local = self.variables[var_num.get(arg)]
- .as_ref()
+ let local = self
+ .variables
+ .get(var_num.get(arg).as_usize())
+ .and_then(Option::as_ref)
.ok_or(JitCompileError::BadBytecode)?;
self.stack.push(JitValue::from_type_and_value(
local.ty.clone(),
self.builder.use_var(local.var),
));Also applies to: 650-652
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/jit/src/instructions.rs` around lines 111 - 115, The code directly
indexes self.variables with a VarNum (typed u32) which can panic on malformed
bytecode; change both occurrences (e.g., in store_variable and the other site
where self.variables[idx] is used) to first convert the VarNum to usize and use
checked access (self.variables.get_mut(idx_usize) or
self.variables.get(idx_usize)), returning Err(JitCompileError::BadBytecode) when
None. Inside the Some(slot) branch, keep the existing logic (declare_var via
builder and set the slot) but perform it via the checked slot reference instead
of indexing to avoid panics.
youknowone
approved these changes
Mar 12, 2026
This was referenced Mar 12, 2026
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