GeezerVison

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

2397-2431: prev_real/next_real walk includes NOPs.

find_map(|info| info.instr.real()) returns the first real() hit, but Instruction::Nop is a Real variant — so the "previous real instruction" can be a NOP and the ForIter guard silently fails if any NOP remains between ForIter and the StoreFast/LoadFast pair. This happens to be safe here because insert_superinstructions is always invoked after remove_nops / remove_redundant_nops_and_jumps, but the current expression does not encode that invariant.

Suggest filtering NOPs explicitly for clarity and to be robust to future pipeline reordering:

♻️ Suggested refactor

-                let prev_real = block.instructions[..i]
-                    .iter()
-                    .rev()
-                    .find_map(|info| info.instr.real());
-                let next_real = block.instructions[(j + 1)..]
-                    .iter()
-                    .find_map(|info| info.instr.real());
+                let prev_real = block.instructions[..i]
+                    .iter()
+                    .rev()
+                    .find_map(|info| info.instr.real())
+                    .filter(|i| !matches!(i, Instruction::Nop));
+                let next_real = block.instructions[(j + 1)..]
+                    .iter()
+                    .find_map(|info| info.instr.real())
+                    .filter(|i| !matches!(i, Instruction::Nop));

🤖 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 2397 - 2431, The current
prev_real/next_real lookup uses find_map(|info| info.instr.real()) which treats
Instruction::Nop as a "real" instruction and can make the ForIter guard misfire;
modify the searches around block.instructions[..i].iter().rev() and
block.instructions[(j + 1)..].iter() to explicitly skip Nop results (e.g. use
find_map(|info| match info.instr.real() { Some(Instruction::Nop) => None, other
=> other })) so prev_real and next_real never return Nop and the
Generator/ForIter checks behave correctly.

---

5554-5616: duplicate_named_except_cleanup_returns: inner-loop scan is O(n²).

The inner for block_idx in 0..blocks.len() runs for every named-except cleanup target, and find_layout_predecessor itself is O(n). For codebases with many try/except ... as name: patterns this can become a hot spot during compilation. Not blocking, but consider precomputing layout predecessors and a map of trailing-conditional-jump targets if compile-time regressions are observed.

🤖 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 5554 - 5616,
duplicate_named_except_cleanup_returns currently does an O(n²) scan by calling
find_layout_predecessor and iterating all blocks per target; instead precompute
data once: compute layout_predecessor for every block (cache results of
find_layout_predecessor), and build a map from target BlockIdx to a Vec of
(block_idx, instr_idx) by scanning each block once with
trailing_conditional_jump_index and next_nonempty_block to record which blocks
jump to which target; then iterate only the targets that are
named-except-cleanup and use the precomputed predecessors, layout_predecessor
and the jump-map to populate clones, avoiding repeated full scans of blocks.
Ensure you reuse the existing predecessors vector and update references to
find_layout_predecessor, trailing_conditional_jump_index, next_nonempty_block,
clones, and blocks accordingly.

---

1201-1237: Constant folding misses (negative int) * str/bytes.

n.try_into().ok()? on a negative BigInt fails and aborts folding, but Python defines -2 * "ab" == "" and -2 * b"ab" == b"" (any non-positive multiplier yields an empty sequence). Same for the existing Str * Integer branch at line 1189. Minor missed optimization, not a correctness issue.

♻️ Suggested refactor

-            (ConstantData::Integer { value: n }, ConstantData::Str { value: s })
-                if matches!(op, BinOp::Multiply) =>
-            {
-                let n: usize = n.try_into().ok()?;
-                if n > 4096 {
-                    return None;
-                }
-                let result = s.to_string().repeat(n);
-                Some(ConstantData::Str {
-                    value: result.into(),
-                })
-            }
+            (ConstantData::Integer { value: n }, ConstantData::Str { value: s })
+                if matches!(op, BinOp::Multiply) =>
+            {
+                let n: usize = n.try_into().ok().unwrap_or(0);
+                if n > 4096 {
+                    return None;
+                }
+                let result = s.to_string().repeat(n);
+                Some(ConstantData::Str {
+                    value: result.into(),
+                })
+            }

(and similar for the Bytes branches)

🤖 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 1201 - 1237, The integer multiplier
branches for ConstantData::Str/Bytes currently call n.try_into().ok()? which
aborts folding for negative BigInt multipliers; change each BinOp::Multiply
branch handling (the patterns with ConstantData::Integer + ConstantData::Str and
both Bytes+Integer / Integer+Bytes and the existing Str*Integer branch) to first
check if the BigInt n is <= 0 and in that case return an empty ConstantData::Str
or ConstantData::Bytes respectively, otherwise convert n to usize (try_into) and
proceed with the existing 4096 cap and repeat logic; reference the
ConstantData::Integer, ConstantData::Str, ConstantData::Bytes patterns and
BinOp::Multiply in ir.rs to locate and update the branches.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

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

1269-1309: Deduplicate the __conditional_annotations__ registration.

The future and non-future paths now share the same two register_name calls; compute the condition first and perform the common registration once.

♻️ Proposed refactor

         let current_scope = self.tables.last().map(|t| t.typ);
+        let is_annotation_bookkeeping_scope = matches!(
+            current_scope,
+            Some(CompilerScope::Module | CompilerScope::Class)
+        );
         let needs_future_annotation_bookkeeping = is_ann_assign
             && self.future_annotations
-            && matches!(
-                current_scope,
-                Some(CompilerScope::Module | CompilerScope::Class)
-            );
+            && is_annotation_bookkeeping_scope;
+        let mut needs_conditional_annotation_bookkeeping =
+            needs_future_annotation_bookkeeping;
 
         // PEP 649: Only AnnAssign annotations can be conditional.
         // Function parameter/return annotations are never conditional.
         if is_ann_assign && !self.future_annotations {
             let is_conditional = matches!(current_scope, Some(CompilerScope::Module))
                 || (matches!(current_scope, Some(CompilerScope::Class))
                     && self.in_conditional_block);
 
             if is_conditional && !self.tables.last().unwrap().has_conditional_annotations {
                 self.tables.last_mut().unwrap().has_conditional_annotations = true;
-                self.register_name(
-                    "__conditional_annotations__",
-                    SymbolUsage::Assigned,
-                    annotation.range(),
-                )?;
-                self.register_name(
-                    "__conditional_annotations__",
-                    SymbolUsage::Used,
-                    annotation.range(),
-                )?;
+                needs_conditional_annotation_bookkeeping = true;
             }
         }
 
-        if needs_future_annotation_bookkeeping {
+        if needs_conditional_annotation_bookkeeping {
             self.register_name(
                 "__conditional_annotations__",
                 SymbolUsage::Assigned,
                 annotation.range(),

As per coding guidelines, “When branches differ only in a value but share common logic, extract the differing value first, then call the common logic once to avoid duplicate code.”

🤖 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 1269 - 1309, The two branches
duplicate the same register_name calls for "__conditional_annotations__";
compute a single boolean (e.g., should_register_conditional_annotations) by
combining the existing conditions (is_ann_assign && self.future_annotations &&
matches!(current_scope, Some(CompilerScope::Module|CompilerScope::Class)) OR the
non-future path: is_ann_assign && !self.future_annotations &&
(matches!(current_scope, Some(CompilerScope::Module)) ||
(matches!(current_scope, Some(CompilerScope::Class)) &&
self.in_conditional_block)) and also ensure the table flag
(self.tables.last().unwrap().has_conditional_annotations) is respected for the
non-future path; then if should_register_conditional_annotations is true, set
the table flag as needed and call the two
register_name("__conditional_annotations__", SymbolUsage::Assigned,
annotation.range()) and register_name("__conditional_annotations__",
SymbolUsage::Used, annotation.range()) once.