GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — Newtype `var_num` oparg by ShaharNaveh · Pull Request #7400 · RustPython/RustPython Conversation
|
📝 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 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
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.
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.
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 443a5187-8634-44ef-9123-5f743fb1aa0a
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
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| fn get_varname(&self, var_num: oparg::VarNum) -> &str { | ||
| self.metadata.varnames[var_num.as_usize()].as_ref() |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
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.
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>")
+ }
}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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| 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()); |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
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.
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
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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.