crates/codegen/src/compile.rs (2)

6732-6735: ⚠️ Potential issue | 🟡 Minor

Reword the Compare fast-path comment.

Rich comparisons can return arbitrary objects. Skipping TO_BOOL here is only safe because PopJumpIf* 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 compiling body, 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.

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 - 1 here 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.

crates/codegen/src/compile.rs (2)

6745-6748: ⚠️ Potential issue | 🟡 Minor

The Compare rationale is still inaccurate.

Rich comparisons can return non-bool objects. The fast path is safe because PopJumpIf* performs truthiness handling, not because Compare itself guarantees a bool.

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 attrs from static_attributes, but this file never inserts into that set after class scopes initialize it to Some(IndexSet::default()) on Line 1120. Every class body will therefore store an empty tuple here and clobber any explicit __static_attributes__ assignment from body.

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.

scripts/dis_dump.py (1)

20-21: Consider adding a comment clarifying PRECALL is CPython-only.

PRECALL was 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 RESUME or the imported-name LOAD_ATTR / PUSH_NULL call 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.

crates/codegen/src/ir.rs (1)

1710-1718: ⚠️ Potential issue | 🟠 Major

Return-epilogue detection still matches any constant, not just None.

The function documentation at line 1694 states it duplicates LOAD_CONST None + RETURN_VALUE, but the predicate at lines 1711-1714 matches any Instruction::LoadConst { .. } without verifying the constant is None. This could incorrectly duplicate explicit non-None returns.

To match the documented behavior and CPython's semantics, verify the loaded constant is specifically None:
,

🐛 Proposed fix

     let is_return_block = last_insts.len() == 2
-        && matches!(
-            last_insts[0].instr,
-            AnyInstruction::Real(Instruction::LoadConst { .. })
-        )
+        && matches!(last_insts[0].instr, AnyInstruction::Real(Instruction::LoadConst { .. }))
+        && {
+            let const_idx = u32::from(last_insts[0].arg) as usize;
+            // Only match if loading None (implicit return epilogue)
+            // This check would require access to self.metadata.consts
+            // Consider passing consts as a parameter or restructuring
+            true // TODO: Verify constant is None
+        }
         && matches!(
             last_insts[1].instr,
             AnyInstruction::Real(Instruction::ReturnValue)
         );

Note: The function currently takes &mut [Block] and doesn't have access to the constants table. You may need to refactor to pass &CodeUnitMetadata or the consts set to properly verify the constant is ConstantData::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 1710 - 1718, The return-epilogue
detection currently treats any Instruction::LoadConst as a None-return; update
the predicate so it only matches a load of the actual None constant by checking
the LoadConst's index/value against the constants table (ConstantData::None). To
do this, refactor the enclosing function (where is_return_block is computed) to
accept the code unit's metadata or consts slice (e.g., add a &CodeUnitMetadata
or &consts parameter), then replace the loose matches on Instruction::LoadConst
with a check that the referenced constant equals ConstantData::None before
recognizing the pattern; keep references to Instruction::LoadConst,
Instruction::ReturnValue, ConstantData::None and the function that computes
is_return_block to locate and change the logic.

crates/codegen/src/compile.rs (2)

4626-4649: ⚠️ Potential issue | 🟠 Major

Guard this store until static_attributes is actually populated.

Line 1120 still initializes every class scope with Some(IndexSet::default()), and this file still never inserts into that set before reaching this block. As written, every class body gets __static_attributes__ = () here, which also overwrites any explicit assignment from the class body.

💡 Minimal 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 code currently
always emits a StoreName for __static_attributes__ even when the collected attrs
is empty; change the block so you first build the attrs Vec from
self.code_stack.last().unwrap().static_attributes, then only call
emit_load_const(ConstantData::Tuple{...}) and emit the Instruction::StoreName
for self.name("__static_attributes__") if attrs is non-empty (i.e., skip
emitting the tuple/store when attrs.is_empty()) so you don't overwrite explicit
assignments from the class body.

---

6732-6735: ⚠️ Potential issue | 🟡 Minor

The Compare fast-path comment is still misleading.

The rationale on Line 6732 still assumes ast::Expr::Compare produces a bool. Rich comparisons can return non-bool objects; skipping ToBool here is only safe because the following PopJumpIf* handles truthiness.

📝 Suggested rewording

-                // 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.

Do Python rich comparison methods like __lt__ and __eq__ have to return bool, or can they return arbitrary objects whose truthiness is only evaluated later in a conditional context?

🤖 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
misleading comment above the Compare fast-path (around the code that checks
ast::Expr::Compare and emits Instruction::ToBool) to state that Python rich
comparison methods can return non-bool objects and therefore do not necessarily
produce a boolean; clarify that the reason the ToBool is skipped for
ast::Expr::Compare is safe only because the subsequent PopJumpIf* bytecode
instructions evaluate truthiness later, not because Compare always yields a
bool. Reference the symbols expression, ast::Expr::Compare, Instruction::ToBool,
and PopJumpIf* in the comment so future readers understand the exact rationale.