Open
Conversation
Contributor
📝 WalkthroughWalkthroughCompiler, IR, VM, symbol-table, and tooling updates: bytecode emission indices for container ops changed, closure/freevar setup emitted earlier, a CFG pass duplicates final returns, VM stack-indexing adjusted, class/module store/load ordering changed, symbol-table now marks classdict needs earlier, and two bytecode-diffing scripts were added. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
ab3cbd9 to
7ff591f
Compare
Contributor
📦 Library DependenciesThe following Lib/ modules were modified. Here are their dependencies: [ ] test: cpython/Lib/test/test_descr.py (TODO: 40) dependencies: dependent tests: (no tests depend on descr) [x] lib: cpython/Lib/dis.py dependencies:
dependent tests: (70 tests)
Legend:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/compile.rs`:
- Around line 6732-6735: The comment above the ToBool emission is misleading
because ast::Expr::Compare may produce non-bool values; update the comment near
the if !matches!(expression, ast::Expr::Compare) check to state that the Compare
fast path is safe due to the jump opcode’s truthiness handling (not because
Compare always yields a bool), and clarify that everything else needs
Instruction::ToBool before branching; reference the emit!(self,
Instruction::ToBool) call and the ast::Expr::Compare pattern in the comment.
- Around line 4626-4649: The current block always emits and stores an empty
tuple to __static_attributes__ because enter_scope() initializes
static_attributes to Some(empty) but nothing is added before this emit; update
compile.rs to skip emitting/storing __static_attributes__ when the collected
attrs vector is empty (i.e., check the
code_stack.last().unwrap().static_attributes contents and only call
emit_load_const/Instruction::StoreName/name("__static_attributes__") if attrs is
non-empty), or alternatively change enter_scope() to initialize
static_attributes as None and only create/populate it when the collector runs;
target the symbols static_attributes, enter_scope, emit_load_const,
Instruction::StoreName and name("__static_attributes__") when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: df81d4bb-8612-4e9c-a28a-4f556bc8453f
⛔ Files ignored due to path filters (4)
Lib/test/test_descr.pyis excluded by!Lib/**crates/codegen/src/snapshots/rustpython_codegen__compile__tests__if_ands.snapis excluded by!**/*.snapcrates/codegen/src/snapshots/rustpython_codegen__compile__tests__if_mixed.snapis excluded by!**/*.snapcrates/codegen/src/snapshots/rustpython_codegen__compile__tests__if_ors.snapis excluded by!**/*.snap
📒 Files selected for processing (1)
crates/codegen/src/compile.rs
Comment on lines
+4626
to
+4649
| // Emit __static_attributes__ tuple | ||
| { | ||
| let attrs: Vec<String> = self | ||
| .code_stack | ||
| .last() | ||
| .unwrap() | ||
| .static_attributes | ||
| .as_ref() | ||
| .map(|s| s.iter().cloned().collect()) | ||
| .unwrap_or_default(); | ||
| self.emit_load_const(ConstantData::Tuple { | ||
| elements: attrs | ||
| .into_iter() | ||
| .map(|s| ConstantData::Str { value: s.into() }) | ||
| .collect(), | ||
| }); | ||
| let static_attrs_name = self.name("__static_attributes__"); | ||
| emit!( | ||
| self, | ||
| Instruction::StoreName { | ||
| namei: static_attrs_name | ||
| } | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
__static_attributes__ is always serialized as () here.
enter_scope() initializes every class with Some(IndexSet::default()) on Line 1120, but this file never inserts into that set before reaching this block. The new store therefore overwrites every class body’s __static_attributes__ with an empty tuple, including any explicit assignment made in body. Either populate static_attributes before this point or skip the store until the set is non-empty.
Minimal guard to avoid clobbering classes until the collector exists
- // Emit __static_attributes__ tuple
- {
- let attrs: Vec<String> = self
- .code_stack
- .last()
- .unwrap()
- .static_attributes
- .as_ref()
- .map(|s| s.iter().cloned().collect())
- .unwrap_or_default();
- self.emit_load_const(ConstantData::Tuple {
- elements: attrs
- .into_iter()
- .map(|s| ConstantData::Str { value: s.into() })
- .collect(),
- });
- let static_attrs_name = self.name("__static_attributes__");
- emit!(
- self,
- Instruction::StoreName {
- namei: static_attrs_name
- }
- );
- }
+ if let Some(attrs) = self
+ .code_stack
+ .last()
+ .and_then(|info| info.static_attributes.as_ref())
+ .filter(|attrs| !attrs.is_empty())
+ {
+ self.emit_load_const(ConstantData::Tuple {
+ elements: attrs
+ .iter()
+ .cloned()
+ .map(|s| ConstantData::Str { value: s.into() })
+ .collect(),
+ });
+ let static_attrs_name = self.name("__static_attributes__");
+ emit!(self, Instruction::StoreName { namei: static_attrs_name });
+ }🤖 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 4626 - 4649, The current block
always emits and stores an empty tuple to __static_attributes__ because
enter_scope() initializes static_attributes to Some(empty) but nothing is added
before this emit; update compile.rs to skip emitting/storing
__static_attributes__ when the collected attrs vector is empty (i.e., check the
code_stack.last().unwrap().static_attributes contents and only call
emit_load_const/Instruction::StoreName/name("__static_attributes__") if attrs is
non-empty), or alternatively change enter_scope() to initialize
static_attributes as None and only create/populate it when the collector runs;
target the symbols static_attributes, enter_scope, emit_load_const,
Instruction::StoreName and name("__static_attributes__") when making the change.
Comment on lines
+6732
to
+6735
| // Compare already produces a bool; everything else needs TO_BOOL | ||
| if !matches!(expression, ast::Expr::Compare(_)) { | ||
| emit!(self, Instruction::ToBool); | ||
| } |
Contributor
There was a problem hiding this comment.
The Compare rationale is inaccurate.
Rich comparisons can return arbitrary objects; the reason this fast path is safe is the jump opcode’s truthiness handling, not that Compare itself always yields a bool. Please reword the comment so future refactors do not depend on the wrong invariant.
Suggested comment update
- // Compare already produces a bool; everything else needs TO_BOOL
+ // Rich comparisons may return non-bool objects; skipping TO_BOOL
+ // here is only correct because PopJumpIf* performs truthiness
+ // conversion itself.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Compare already produces a bool; everything else needs TO_BOOL | |
| if !matches!(expression, ast::Expr::Compare(_)) { | |
| emit!(self, Instruction::ToBool); | |
| } | |
| // Rich comparisons may return non-bool objects; skipping TO_BOOL | |
| // here is only correct because PopJumpIf* performs truthiness | |
| // conversion itself. | |
| if !matches!(expression, ast::Expr::Compare(_)) { | |
| emit!(self, Instruction::ToBool); | |
| } |
🤖 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 6732 - 6735, The comment above
the ToBool emission is misleading because ast::Expr::Compare may produce
non-bool values; update the comment near the if !matches!(expression,
ast::Expr::Compare) check to state that the Compare fast path is safe due to the
jump opcode’s truthiness handling (not because Compare always yields a bool),
and clarify that everything else needs Instruction::ToBool before branching;
reference the emit!(self, Instruction::ToBool) call and the ast::Expr::Compare
pattern in the comment.
- Emit TO_BOOL before POP_JUMP_IF_TRUE/FALSE in the general case of compile_jump_if (Compare expressions excluded since they already produce a bool) - Module-level __doc__: use STORE_NAME instead of STORE_GLOBAL - Class body __module__: use LOAD_NAME instead of LOAD_GLOBAL - Class body: store __firstlineno__ before __doc__
Emit MAKE_CELL for each cell variable and COPY_FREE_VARS N for free variables at the start of each code object, before RESUME. These instructions are no-ops in the VM but align the bytecode with CPython 3.14's output.
Store a tuple of attribute names (currently always empty) as __static_attributes__ in the class namespace, matching CPython 3.14's class body epilogue. Attribute name collection from self.xxx accesses is a follow-up task.
test_iter_keys, test_iter_values, test_iter_items now pass because class bodies emit __static_attributes__ and __firstlineno__, matching the expected dict key set.
Switch LIST_APPEND, LIST_EXTEND, SET_ADD, SET_UPDATE, MAP_ADD from 0-based to 1-based stack depth argument, matching CPython's PEEK(oparg) convention. Adjust the VM to subtract 1 before calling nth_value.
When the call target is an attribute of an imported name (e.g., logging.getLogger()), use plain LOAD_ATTR (method_flag=0) with a separate PUSH_NULL instead of method-mode LOAD_ATTR. This matches CPython 3.14's behavior which avoids the method call optimization for module attribute access.
When the last block in a code object is exactly LOAD_CONST None + RETURN_VALUE (the implicit return), duplicate these instructions into blocks that would otherwise fall through to it. This matches CPython 3.14's behavior of giving each code path its own explicit return instruction.
7ff591f to
904bdaf
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
crates/codegen/src/compile.rs (2)
6732-6735:⚠️ Potential issue | 🟡 MinorReword the
Comparefast-path comment.Rich comparisons can return arbitrary objects. Skipping
TO_BOOLhere is only safe becausePopJumpIf*performs truthiness conversion itself.Suggested comment update
- // Compare already produces a bool; everything else needs TO_BOOL + // Rich comparisons may return non-bool objects; skipping TO_BOOL + // here is only correct because PopJumpIf* performs truthiness + // conversion itself.🤖 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 6732 - 6735, Update the comment above the Compare fast-path to note that rich comparisons may return arbitrary objects and that we only skip emitting Instruction::ToBool for ast::Expr::Compare because the subsequent conditional jump opcodes (PopJumpIfTrue / PopJumpIfFalse / other PopJumpIf* handlers) perform the truthiness conversion themselves; reference the match on expression (ast::Expr::Compare), the emit!(self, Instruction::ToBool) call, and the PopJumpIf* jump handlers to make the reason explicit.
4626-4649:⚠️ Potential issue | 🟠 Major
__static_attributes__is still always written as().This file only initializes
static_attributes; it never populates it before this block runs. As written, every class body overwrites__static_attributes__with an empty tuple, and it does so after compilingbody, so it also clobbers an explicit class-body assignment.Suggested guard until the collector is wired up
- // Emit __static_attributes__ tuple - { - let attrs: Vec<String> = self - .code_stack - .last() - .unwrap() - .static_attributes - .as_ref() - .map(|s| s.iter().cloned().collect()) - .unwrap_or_default(); - self.emit_load_const(ConstantData::Tuple { - elements: attrs - .into_iter() - .map(|s| ConstantData::Str { value: s.into() }) - .collect(), - }); - let static_attrs_name = self.name("__static_attributes__"); - emit!( - self, - Instruction::StoreName { - namei: static_attrs_name - } - ); - } + if let Some(attrs) = self + .code_stack + .last() + .and_then(|info| info.static_attributes.as_ref()) + .filter(|attrs| !attrs.is_empty()) + { + self.emit_load_const(ConstantData::Tuple { + elements: attrs + .iter() + .cloned() + .map(|s| ConstantData::Str { value: s.into() }) + .collect(), + }); + let static_attrs_name = self.name("__static_attributes__"); + emit!(self, Instruction::StoreName { namei: static_attrs_name }); + }🤖 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 4626 - 4649, The code unconditionally emits and stores __static_attributes__ as an empty tuple because static_attributes is never populated; fix by guarding the emit: only call emit_load_const + Instruction::StoreName for __static_attributes__ when self.code_stack.last().unwrap().static_attributes is Some(vec) and the vec is non-empty (i.e., contains attributes), so it doesn't overwrite an explicit class-body assignment or write an empty tuple; use the existing name("__static_attributes__"), emit_load_const, and the Instruction::StoreName path when the Option has data.
🧹 Nitpick comments (1)
crates/vm/src/frame.rs (1)
2512-2512: Encode the new 1-based container depth in one place.These five handlers now rely on the same subtle rule: the opcode arg is 1-based,
nth_value()is 0-based, and the lookup happens after popping the transient operand(s). Repeating raw- 1here makes that contract easy to miss or accidentally revert later; a tiny helper or short comment would make the parity rule much clearer.Also applies to: 2522-2522, 2864-2864, 3195-3195, 3205-3205
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/vm/src/frame.rs` at line 2512, Create a small helper to centralize the 1-based→0-based conversion and use it from the handlers instead of repeating `- 1`; for example add a method on the same type (e.g., fn arg_to_index(&self, arg: u32) -> usize or fn nth_arg_value(&self, arg: u32) -> Value) and replace occurrences like `let obj = self.nth_value(i.get(arg) - 1);` in the handlers referenced (the sites at the current diff and the ones you noted: the uses at ~2522, ~2864, ~3195, ~3205) to call that helper so the parity rule is enforced in one place and clearly named. Ensure the helper documents that opcode args are 1-based and nth_value is 0-based.
🤖 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 1652-1660: The return-epilogue predicate (is_return_block) is too
broad — it matches any LOAD_CONST + RETURN_VALUE; narrow it by matching the
actual LoadConst payload to ensure it's specifically loading None: change the
second match clause that currently checks
AnyInstruction::Real(Instruction::LoadConst { .. }) to explicitly match the
constant variant for None (e.g. AnyInstruction::Real(Instruction::LoadConst {
value: Constant::None }) or the equivalent None representation used in this IR),
while keeping the subsequent AnyInstruction::Real(Instruction::ReturnValue)
check; use the existing symbols is_return_block, last_insts,
AnyInstruction::Real, Instruction::LoadConst and Instruction::ReturnValue to
locate and update the condition.
---
Duplicate comments:
In `@crates/codegen/src/compile.rs`:
- Around line 6732-6735: Update the comment above the Compare fast-path to note
that rich comparisons may return arbitrary objects and that we only skip
emitting Instruction::ToBool for ast::Expr::Compare because the subsequent
conditional jump opcodes (PopJumpIfTrue / PopJumpIfFalse / other PopJumpIf*
handlers) perform the truthiness conversion themselves; reference the match on
expression (ast::Expr::Compare), the emit!(self, Instruction::ToBool) call, and
the PopJumpIf* jump handlers to make the reason explicit.
- Around line 4626-4649: The code unconditionally emits and stores
__static_attributes__ as an empty tuple because static_attributes is never
populated; fix by guarding the emit: only call emit_load_const +
Instruction::StoreName for __static_attributes__ when
self.code_stack.last().unwrap().static_attributes is Some(vec) and the vec is
non-empty (i.e., contains attributes), so it doesn't overwrite an explicit
class-body assignment or write an empty tuple; use the existing
name("__static_attributes__"), emit_load_const, and the Instruction::StoreName
path when the Option has data.
---
Nitpick comments:
In `@crates/vm/src/frame.rs`:
- Line 2512: Create a small helper to centralize the 1-based→0-based conversion
and use it from the handlers instead of repeating `- 1`; for example add a
method on the same type (e.g., fn arg_to_index(&self, arg: u32) -> usize or fn
nth_arg_value(&self, arg: u32) -> Value) and replace occurrences like `let obj =
self.nth_value(i.get(arg) - 1);` in the handlers referenced (the sites at the
current diff and the ones you noted: the uses at ~2522, ~2864, ~3195, ~3205) to
call that helper so the parity rule is enforced in one place and clearly named.
Ensure the helper documents that opcode args are 1-based and nth_value is
0-based.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 7134ce0a-4869-417e-9966-1ac435da0e6a
⛔ Files ignored due to path filters (4)
Lib/test/test_descr.pyis excluded by!Lib/**crates/codegen/src/snapshots/rustpython_codegen__compile__tests__if_ands.snapis excluded by!**/*.snapcrates/codegen/src/snapshots/rustpython_codegen__compile__tests__if_mixed.snapis excluded by!**/*.snapcrates/codegen/src/snapshots/rustpython_codegen__compile__tests__if_ors.snapis excluded by!**/*.snap
📒 Files selected for processing (3)
crates/codegen/src/compile.rscrates/codegen/src/ir.rscrates/vm/src/frame.rs
Comment on lines
+1652
to
+1660
| let is_return_block = last_insts.len() == 2 | ||
| && matches!( | ||
| last_insts[0].instr, | ||
| AnyInstruction::Real(Instruction::LoadConst { .. }) | ||
| ) | ||
| && matches!( | ||
| last_insts[1].instr, | ||
| AnyInstruction::Real(Instruction::ReturnValue) | ||
| ); |
Contributor
There was a problem hiding this comment.
Return-epilogue detection is too broad for the documented behavior
Line 1652 only checks LOAD_CONST + RETURN_VALUE, but the function is documented to duplicate only LOAD_CONST None + RETURN_VALUE. This can duplicate explicit constant returns as well, which risks bytecode parity/tracing mismatches. Please tighten the predicate to verify the loaded constant is specifically None.
🤖 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 1652 - 1660, The return-epilogue
predicate (is_return_block) is too broad — it matches any LOAD_CONST +
RETURN_VALUE; narrow it by matching the actual LoadConst payload to ensure it's
specifically loading None: change the second match clause that currently checks
AnyInstruction::Real(Instruction::LoadConst { .. }) to explicitly match the
constant variant for None (e.g. AnyInstruction::Real(Instruction::LoadConst {
value: Constant::None }) or the equivalent None representation used in this IR),
while keeping the subsequent AnyInstruction::Real(Instruction::ReturnValue)
check; use the existing symbols is_return_block, last_insts,
AnyInstruction::Real, Instruction::LoadConst and Instruction::ReturnValue to
locate and update the condition.
- scripts/dis_dump.py: Dumps normalized bytecode for Python files, usable with both CPython and RustPython. Normalizes code object reprs, COMPARE_OP argrepr, and LOAD_FAST_BORROW opnames. - scripts/compare_bytecode.py: Orchestrates CPython vs RustPython bytecode comparison across Lib/, reports match rate and diffs. Current baseline: 176/1515 files match (11.6%).
Set needs_classdict=true for class scopes that contain function definitions (def/async def), matching CPython 3.14's behavior for PEP 649 deferred annotation support. Also restore the Compare expression check in compile_jump_if to skip TO_BOOL for comparison operations.
Store the __classdict__ cell reference as __classdictcell__ in the class namespace when the class has __classdict__ as a cell variable. Uses LOAD_DEREF (RustPython separates cell vars from fast locals unlike CPython's unified array).
Run basic dead code elimination (truncating instructions after RETURN_VALUE/RAISE/JUMP within blocks) at all optimization levels, not just optimize > 0. CPython always removes this dead code during assembly.
Only use plain LOAD_ATTR + PUSH_NULL for imports at module or class scope. Function-local imports use method call mode LOAD_ATTR, matching CPython 3.14's behavior.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/codegen/src/compile.rs (1)
3502-3507:⚠️ Potential issue | 🟡 MinorThe inline stack math still describes the old 0-based operand scheme.
These comments still derive
i = 2/i = 0, but the emitted instructions are nowi: 3/i: 1because the VM interprets these operands as 1-based depths. Please update the explanation so it matches the code.Also applies to: 3532-3536
🤖 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 3502 - 3507, The inline comments in the LIST_APPEND handling are computing stack index using a 0-based operand scheme while the VM uses 1-based depths; update the explanatory math around Instruction::ListAppend { i: 3 } (and the analogous block at the location covering 3532-3536) to show the 1-based depth calculation so the derivation matches the emitted i values (e.g., explain that nth_value uses 1-based depth so list is at depth 3 leading to i: 3, and adjust the other comment similarly).
♻️ Duplicate comments (2)
crates/codegen/src/compile.rs (2)
6745-6748:⚠️ Potential issue | 🟡 MinorThe
Comparerationale is still inaccurate.Rich comparisons can return non-bool objects. The fast path is safe because
PopJumpIf*performs truthiness handling, not becauseCompareitself guarantees abool.Suggested comment update
- // Compare already produces a bool; everything else needs TO_BOOL + // Rich comparisons may return non-bool objects; skipping TO_BOOL + // here is only safe because PopJumpIf* performs truthiness + // conversion itself.🤖 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 6745 - 6748, Update the inline comment above the ToBool emission to correctly state that Compare may return non-bool objects and that the reason we skip emitting Instruction::ToBool for ast::Expr::Compare(_) is not that Compare guarantees a bool but because the fast-path jump instructions (e.g., PopJumpIfTrue/PopJumpIfFalse) perform truthiness handling themselves; replace the current "Compare already produces a bool" wording with a note that Compare can return non-bool values and we rely on PopJumpIf*'s truthiness semantics rather than forcing a ToBool.
4626-4649:⚠️ Potential issue | 🟠 Major
__static_attributes__is still always overwritten with().Line 4628 builds
attrsfromstatic_attributes, but this file never inserts into that set after class scopes initialize it toSome(IndexSet::default())on Line 1120. Every class body will therefore store an empty tuple here and clobber any explicit__static_attributes__assignment frombody.Possible guard
- // Emit __static_attributes__ tuple - { - let attrs: Vec<String> = self - .code_stack - .last() - .unwrap() - .static_attributes - .as_ref() - .map(|s| s.iter().cloned().collect()) - .unwrap_or_default(); - self.emit_load_const(ConstantData::Tuple { - elements: attrs - .into_iter() - .map(|s| ConstantData::Str { value: s.into() }) - .collect(), - }); - let static_attrs_name = self.name("__static_attributes__"); - emit!( - self, - Instruction::StoreName { - namei: static_attrs_name - } - ); - } + if let Some(attrs) = self + .code_stack + .last() + .and_then(|info| info.static_attributes.as_ref()) + .filter(|attrs| !attrs.is_empty()) + { + self.emit_load_const(ConstantData::Tuple { + elements: attrs + .iter() + .cloned() + .map(|s| ConstantData::Str { value: s.into() }) + .collect(), + }); + let static_attrs_name = self.name("__static_attributes__"); + emit!(self, Instruction::StoreName { namei: static_attrs_name }); + }🤖 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 4626 - 4649, The compiler unconditionally emits and stores __static_attributes__ (via emit_load_const + Instruction::StoreName with name from name("__static_attributes__")), but code_stack.last().static_attributes is initialized to Some(empty) and never populated, so every class body is clobbered with an empty tuple; change the code in compile.rs so you only emit and StoreName for __static_attributes__ when the current code_stack.last().static_attributes is present and non-empty (i.e., check static_attributes.as_ref().map(|s| !s.is_empty()).unwrap_or(false)) so explicit assignments in the class body are not overwritten.
🧹 Nitpick comments (2)
scripts/dis_dump.py (1)
20-21: Consider adding a comment clarifying PRECALL is CPython-only.
PRECALLwas removed in CPython 3.12 and is not present in RustPython's opcode table. A brief comment would help future readers understand why it's included.📝 Suggested comment
# Non-semantic filler instructions to skip +# Note: PRECALL is CPython 3.10-3.11 only (removed in 3.12), included for compatibility SKIP_OPS = frozenset({"CACHE", "PRECALL"})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/dis_dump.py` around lines 20 - 21, Add a brief clarifying comment above the SKIP_OPS definition explaining that the PRECALL opcode is included for compatibility with older CPython versions, was removed in CPython 3.12, and does not exist in RustPython’s opcode table; reference the SKIP_OPS constant and the PRECALL entry so readers know why PRECALL is present even though some Python implementations (like RustPython) won't have it.crates/codegen/src/compile.rs (1)
1141-1157: Please add snapshot coverage for the new parity-sensitive shapes.This file already has disassembly snapshot helpers, but there is no direct coverage here for the prologue reorder before
RESUMEor the imported-nameLOAD_ATTR/PUSH_NULLcall path. A tiny snapshot for each would make parity regressions much easier to catch.Also applies to: 7517-7535
🤖 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 1141 - 1157, Add snapshot tests to cover the parity-sensitive prologue reorder before RESUME and the imported-name call path that emits LOAD_ATTR / PUSH_NULL: locate the disassembly snapshot helpers in crates/codegen/src/compile.rs and add two small snapshots — one that exercises the MAKE_CELL / CopyFreeVars emission sequence immediately before RESUME (to catch prologue reorder regressions) and another that builds an imported-name call site which emits LOAD_ATTR followed by PUSH_NULL (to catch that call path); wire these into the existing snapshot test harness so they run with other disassembly snapshots and assert stable output.
🤖 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/compile.rs`:
- Around line 4651-4662: The code stores the dereferenced dict for
"__classdict__" into the name "__classdictcell__", but it should store the cell
object to match the "__classcell__" pattern; in the block gated by
self.current_symbol_table().needs_classdict (using
get_cell_var_index("__classdict__") and name("__classdictcell__") and the
subsequent emit StoreName), replace the Instruction::LoadDeref { i:
classdict_idx } emission with the cell-loading pseudo-instruction
(PseudoInstruction::LoadClosure { i: classdict_idx.to_u32() } or the equivalent
cell loader) so the actual cell object is stored under "__classdictcell__"
rather than the dereferenced dict.
In `@crates/codegen/src/symboltable.rs`:
- Around line 295-307: The code currently sets symbol_table.needs_classdict
whenever any immediate child scope is CompilerScope::Function or
::AsyncFunction; change this so needs_classdict is set only when there is an
actual classdict demand—i.e., check each child scope for evidence of needing
__classdict__ (for example inspect the child's newfree/cell capture flags or an
annotation/type-parameter marker used for PEP 649) instead of just matching on
CompilerScope::Function/AsyncFunction; update the loop over
symbol_table.sub_tables to test the child's fields (e.g., t.newfree or a
dedicated annotation/type-param flag) and only set symbol_table.needs_classdict
= true when those indicators are present.
In `@scripts/dis_dump.py`:
- Around line 82-83: The mapping _RP_CMP_OPS currently includes an invalid key 0
duplicating "<"; remove the 0 key from _RP_CMP_OPS and ensure callers use a safe
lookup (for example, replace direct indexing with a .get on _RP_CMP_OPS when
resolving comparison strings, e.g., in the code that computes cmp_str from
inst.arg) so unknown enum values produce a clear default like "?cmp{n}" instead
of silently mapping to "<". This change references _RP_CMP_OPS and the bytecode
ComparisonOperator values (inst.arg) used when resolving cmp_str.
---
Outside diff comments:
In `@crates/codegen/src/compile.rs`:
- Around line 3502-3507: The inline comments in the LIST_APPEND handling are
computing stack index using a 0-based operand scheme while the VM uses 1-based
depths; update the explanatory math around Instruction::ListAppend { i: 3 } (and
the analogous block at the location covering 3532-3536) to show the 1-based
depth calculation so the derivation matches the emitted i values (e.g., explain
that nth_value uses 1-based depth so list is at depth 3 leading to i: 3, and
adjust the other comment similarly).
---
Duplicate comments:
In `@crates/codegen/src/compile.rs`:
- Around line 6745-6748: Update the inline comment above the ToBool emission to
correctly state that Compare may return non-bool objects and that the reason we
skip emitting Instruction::ToBool for ast::Expr::Compare(_) is not that Compare
guarantees a bool but because the fast-path jump instructions (e.g.,
PopJumpIfTrue/PopJumpIfFalse) perform truthiness handling themselves; replace
the current "Compare already produces a bool" wording with a note that Compare
can return non-bool values and we rely on PopJumpIf*'s truthiness semantics
rather than forcing a ToBool.
- Around line 4626-4649: The compiler unconditionally emits and stores
__static_attributes__ (via emit_load_const + Instruction::StoreName with name
from name("__static_attributes__")), but code_stack.last().static_attributes is
initialized to Some(empty) and never populated, so every class body is clobbered
with an empty tuple; change the code in compile.rs so you only emit and
StoreName for __static_attributes__ when the current
code_stack.last().static_attributes is present and non-empty (i.e., check
static_attributes.as_ref().map(|s| !s.is_empty()).unwrap_or(false)) so explicit
assignments in the class body are not overwritten.
---
Nitpick comments:
In `@crates/codegen/src/compile.rs`:
- Around line 1141-1157: Add snapshot tests to cover the parity-sensitive
prologue reorder before RESUME and the imported-name call path that emits
LOAD_ATTR / PUSH_NULL: locate the disassembly snapshot helpers in
crates/codegen/src/compile.rs and add two small snapshots — one that exercises
the MAKE_CELL / CopyFreeVars emission sequence immediately before RESUME (to
catch prologue reorder regressions) and another that builds an imported-name
call site which emits LOAD_ATTR followed by PUSH_NULL (to catch that call path);
wire these into the existing snapshot test harness so they run with other
disassembly snapshots and assert stable output.
In `@scripts/dis_dump.py`:
- Around line 20-21: Add a brief clarifying comment above the SKIP_OPS
definition explaining that the PRECALL opcode is included for compatibility with
older CPython versions, was removed in CPython 3.12, and does not exist in
RustPython’s opcode table; reference the SKIP_OPS constant and the PRECALL entry
so readers know why PRECALL is present even though some Python implementations
(like RustPython) won't have it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: e7a2283a-8db4-4f7f-b366-2e756e9fca89
⛔ Files ignored due to path filters (1)
Lib/test/test_dis.pyis excluded by!Lib/**
📒 Files selected for processing (5)
crates/codegen/src/compile.rscrates/codegen/src/ir.rscrates/codegen/src/symboltable.rsscripts/compare_bytecode.pyscripts/dis_dump.py
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/codegen/src/ir.rs
Comment on lines
+4651
to
+4662
| // Store __classdictcell__ if __classdict__ is a cell variable | ||
| if self.current_symbol_table().needs_classdict { | ||
| let classdict_idx = self.get_cell_var_index("__classdict__")?; | ||
| emit!(self, Instruction::LoadDeref { i: classdict_idx }); | ||
| let classdictcell = self.name("__classdictcell__"); | ||
| emit!( | ||
| self, | ||
| Instruction::StoreName { | ||
| namei: classdictcell | ||
| } | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "# Consumers of __classdictcell__ and __classcell__"
rg -n -C3 '__classdictcell__|__classcell__' .
echo
echo "# Class-body emitters in compile.rs"
rg -n -C2 '__classdictcell__|classdict_idx|__classcell__|classcell_idx' crates/codegen/src/compile.rsRepository: RustPython/RustPython
Length of output: 14796
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "# Search for __classdictcell__ comments or PEP references"
rg -n -B2 -A2 '__classdictcell__' crates/codegen/src/compile.rs
echo
echo "# Git history snippet for the __classdictcell__ block"
git log --oneline -S '__classdictcell__' -- crates/codegen/src/compile.rs | head -5
echo
echo "# Search for any mention of classdict cell design in comments"
rg -n 'classdict.*cell|needs_classdict' crates/codegen/src/compile.rs | head -20Repository: RustPython/RustPython
Length of output: 1458
Change __classdictcell__ emission to store the cell, not the dereferenced dict.
This block currently dereferences __classdict__ and stores the resulting dict under the name __classdictcell__, but the neighboring __classcell__ block (lines 4664–4673) uses PseudoInstruction::LoadClosure to store the actual cell object. The naming convention and symmetry with __classcell__ indicate that __classdictcell__ should also carry the cell object, not the dict. Change Instruction::LoadDeref { i: classdict_idx } to PseudoInstruction::LoadClosure { i: classdict_idx.to_u32() } (or equivalent cell-loading instruction) to match the pattern.
🤖 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 4651 - 4662, The code stores the
dereferenced dict for "__classdict__" into the name "__classdictcell__", but it
should store the cell object to match the "__classcell__" pattern; in the block
gated by self.current_symbol_table().needs_classdict (using
get_cell_var_index("__classdict__") and name("__classdictcell__") and the
subsequent emit StoreName), replace the Instruction::LoadDeref { i:
classdict_idx } emission with the cell-loading pseudo-instruction
(PseudoInstruction::LoadClosure { i: classdict_idx.to_u32() } or the equivalent
cell loader) so the actual cell object is stored under "__classdictcell__"
rather than the dereferenced dict.
Comment on lines
+295
to
+307
| // If the class has any function definitions, it needs __classdict__ | ||
| // for PEP 649 deferred annotation evaluation support | ||
| if !symbol_table.needs_classdict { | ||
| let has_functions = symbol_table.sub_tables.iter().any(|t| { | ||
| matches!( | ||
| t.typ, | ||
| CompilerScope::Function | CompilerScope::AsyncFunction | ||
| ) | ||
| }); | ||
| if has_functions { | ||
| symbol_table.needs_classdict = true; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# 1) Find all production/consumption sites for __classdictcell__/__classdict__.
rg -n -C3 '__classdictcell__|__classdict__'
# 2) Inspect all writes to needs_classdict and related compile-time branching.
rg -n -C3 'needs_classdict\s*=' crates/codegen/src/symboltable.rs crates/codegen/src/compile.rs
rg -n -C3 'if\s+self\.current_symbol_table\(\)\.needs_classdict|if\s+ste\.needs_classdict' crates/codegen/src/compile.rsRepository: RustPython/RustPython
Length of output: 18389
Avoid promoting needs_classdict for every class that merely has methods.
At lines 297–306, this now sets needs_classdict for any class with immediate Function/AsyncFunction children. The bytecode overhead—LoadLocals -> StoreDeref (compile.rs:4590–4594) and LoadDeref -> StoreName("__classdictcell__") (compile.rs:4651–4658)—applies unconditionally, even for ordinary methods without type parameters or annotation scope usage. The comment cites "PEP 649 deferred annotation evaluation support," which is type-parameter-specific, yet the implementation gates on method presence alone.
Gate this on actual __classdict__ demand (type parameter annotations, explicit child captures in newfree) rather than method presence.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/codegen/src/symboltable.rs` around lines 295 - 307, The code currently
sets symbol_table.needs_classdict whenever any immediate child scope is
CompilerScope::Function or ::AsyncFunction; change this so needs_classdict is
set only when there is an actual classdict demand—i.e., check each child scope
for evidence of needing __classdict__ (for example inspect the child's
newfree/cell capture flags or an annotation/type-parameter marker used for PEP
649) instead of just matching on CompilerScope::Function/AsyncFunction; update
the loop over symbol_table.sub_tables to test the child's fields (e.g.,
t.newfree or a dedicated annotation/type-param flag) and only set
symbol_table.needs_classdict = true when those indicators are present.
Comment on lines
+82
to
+83
| # RustPython's ComparisonOperator enum values → operator strings | ||
| _RP_CMP_OPS = {0: "<", 1: "<", 2: ">", 3: "!=", 4: "==", 5: "<=", 6: ">="} |
Contributor
There was a problem hiding this comment.
Key 0 in _RP_CMP_OPS appears incorrect.
According to the ComparisonOperator enum in crates/compiler-core/src/bytecode/oparg.rs, valid values start at 1 (Less = 0b001). There is no variant with value 0. The current mapping duplicates "<" for both keys 0 and 1, which could mask bugs where an invalid comparison operator is encountered.
🛠️ Suggested fix: remove key 0 or handle it explicitly
# RustPython's ComparisonOperator enum values → operator strings
-_RP_CMP_OPS = {0: "<", 1: "<", 2: ">", 3: "!=", 4: "==", 5: "<=", 6: ">="}
+_RP_CMP_OPS = {1: "<", 2: ">", 3: "!=", 4: "==", 5: "<=", 6: ">="}If a fallback is needed, use .get() with a default in the calling code instead:
cmp_str = _RP_CMP_OPS.get(inst.arg, "?cmp%d" % inst.arg)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # RustPython's ComparisonOperator enum values → operator strings | |
| _RP_CMP_OPS = {0: "<", 1: "<", 2: ">", 3: "!=", 4: "==", 5: "<=", 6: ">="} | |
| # RustPython's ComparisonOperator enum values → operator strings | |
| _RP_CMP_OPS = {1: "<", 2: ">", 3: "!=", 4: "==", 5: "<=", 6: ">="} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/dis_dump.py` around lines 82 - 83, The mapping _RP_CMP_OPS currently
includes an invalid key 0 duplicating "<"; remove the 0 key from _RP_CMP_OPS and
ensure callers use a safe lookup (for example, replace direct indexing with a
.get on _RP_CMP_OPS when resolving comparison strings, e.g., in the code that
computes cmp_str from inst.arg) so unknown enum values produce a clear default
like "?cmp{n}" instead of silently mapping to "<". This change references
_RP_CMP_OPS and the bytecode ComparisonOperator values (inst.arg) used when
resolving cmp_str.
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
1 participant
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
Bug Fixes
Refactor
New Features